If you see any other ways I could improve this wreck, I'll accept suggestions...
Challenge accepted! =]
I've prepared an example of how to improve your code. It is written in C++, but only really uses features of C++ in a few places. I'm not sure if you are (or intend to be) using C at the moment, but if so there are only a few places that would need to be changed.
// If you are using C++, you should include the C headers like so
// Note the "c" prefix and the lack of a ".h" extension
// If you are actually using C, then some of my later points won't apply directly
// (but there are generally similar techniques available in C).
#include <cmath>
#include <cstdio>
#include <cstdlib>
#include <cassert>
// For dynamic player input
#include <string>
// This header appears not to be used
// Try avoid including system headers unless 100% necessary
// It will make your code harder to port.
// #include <unistd.h>
// SDL recommends not assuming that SDL headers are in a SDL subdirectory
// See http://wiki.libsdl.org/moin.cgi/FAQDevelopment, point #10
#include "SDL.h"
#include "SDL_image.h"
#include "SDL_mixer.h"
#include "SDL_thread.h"
#include "SDL_ttf.h"
// Do not use absolute include paths.
// One option is to use include paths relative to the project directory.
// You may need to add a particular directory to your compiler's default include path.
#include "/home/mdeans/Fiendlight/plyr.h"
// Don't abuse the preprocessor to create constants.
const int WINDOW_WIDTH = 640;
const int WINDOW_HEIGHT = 480;
// There is no need for these global values.
#if 0
SDL_Surface *surface[34]; ///0 = window, 1 = frame, 2-10 = lower box's text, 11-20 = upper box's text
SDL_Event event;
SDL_Rect src,dest;
SDL_Color font_color;
TTF_Font* mt_font;
char tr_panel[40];
char br_panel[20];
char user_input[9];
int input_slot;
#endif
// Instead of globals, we can make a data structure out of the common data
// we need in other functions. We can then pass this structure as an argument
struct GameData {
SDL_Surface *screen;
SDL_Surface *frame;
TTF_Font *font;
};
// Declare functions on separate lines for readability
// int game_start(),game_load(),game_credits(),game_field(),game_battle();
bool game_start(const GameData &gameData);
bool game_load(const GameData &gameData);
bool game_credits(const GameData &gameData);
// TODO: these functions aren't used...
#if 0
void plyr_talk();
void plyr_attk();
void plyr_ztat();
void plyr_get();
void plyr_look();
void plyr_warp(int how,int where[3]);
#endif
// Helper function to make it easier to draw a surface
void apply_surface(SDL_Surface *source, int x, int y, SDL_Surface *destination) {
SDL_Rect rect = { x, y };
int result = SDL_BlitSurface(source, NULL, destination, &rect);
// This might be considered overkill, but sometimes it can be handy to know this is happening.
// E.g. a frustrated user emails you saying they only get a black screen!
if(result != 0) {
printf("Warning: failed to blit surface %s\n", SDL_GetError());
}
}
// Helper function to make it easier to clear a surface to black
void clear_screen(SDL_Surface *screen)
{
// Don't specify the alpha unless it is interesting
Uint32 colour = SDL_MapRGB(screen->format, 0, 0, 0);
SDL_FillRect(screen, NULL, colour);
}
int main() {
// If you return early, there is no need to nest the rest of
// the function in an "else" block
if(SDL_Init(SDL_INIT_VIDEO) != 0) {
printf("Unable to initialise SDL: %s\n", SDL_GetError());
return 1;
}
// You should call SDL_Quit() when you cleanup.
// This is a simple approach:
atexit(&SDL_Quit);
// You were initialising SDL_TTF twice in your old code.
if(TTF_Init() != 0) {
// You were using SDL_GetError() rather than TTF_GetError() here.
printf("Unable to initialise SDL_TTF: %s\n", TTF_GetError());
return 1;
}
// You should also call TTF_Quit() during cleanup
atexit(&TTF_Quit);
// Your old code passed "mt_font" to printf() as a string.
// See note later.
const char *fontname = "gfx/FreeMono.ttf";
// I don't know what "mt_" meant, so I dropped it from the variable name.
TTF_Font *font = TTF_OpenFont(fontname, 16);
if(font == NULL) {
// Here is where "mt_font" was being erroneously passed to printf()
// This could result in printf() crashing where you need it most, error logging!
// Remember to always test error handling too (e.g. you also forgot a newline here).
printf("Unable to load font: %s: %s\n", fontname, TTF_GetError());
// Can your program handle this eventuality without crashing?
// If not, you should return here...
return 1;
}
// I moved this down a bit, trying to follow a logical progression.
SDL_EnableUNICODE(SDL_ENABLE);
SDL_WM_SetCaption("Fiendlight",NULL);
// Don't put unrelated data in the same array
// The screen surface is special, it deserves it's own variable.
SDL_Surface *screen = SDL_SetVideoMode(WINDOW_WIDTH, WINDOW_HEIGHT, 32, SDL_DOUBLEBUF);
if(!screen) {
printf("Unable to set video mode: %s\n", SDL_GetError());
return 1;
}
SDL_Surface *frame = IMG_Load("gfx/frame.png");
if(frame == NULL) {
// You appear to have omitted error handling if frame fails to load?
printf("Failed to load frame image: %s\n", IMG_GetError());
// Another option might be to fall back, e.g. use a default surface
// Perhaps procedurally generated.
return 1;
}
// There is a clever
const int NUM_STRINGS = 3;
// Here is a place where it does make sense to use an array
// These surfaces are related! Also, we can cut down on duplicated code!
const char *textStrings[NUM_STRINGS] = {
"[S]tart new quest",
"[C]ontinue your journey",
"[V]iew the credits"
};
SDL_Surface *textSurfaces[NUM_STRINGS] = {};
SDL_Color font_color = { 180, 180, 180 };
for(int i = 0 ; i < NUM_STRINGS ; ++i) {
// Whoops, it looks like my earlier warning came true, your code doesn't seem
// to gracefully handle a NULL font.
textSurfaces[i] = TTF_RenderText_Solid(font, textStrings[i], font_color);
}
// Prepare our data structure for passing to other functions
GameData gameData = {};
gameData.screen = screen;
gameData.frame = frame;
gameData.font = font;
// Note that the "event" does not need to be global,
// nor does it need to be passed in as GameData. It is transient,
// and thus makes the most sense as a local variable.
SDL_Event event;
while (SDL_WaitEvent(&event) != 0) {
switch (event.type) {
case SDL_QUIT:
exit(0);
// It is a good habit to include "break" statements
// Even if the code currently unconditionally terminates the process
// Later you might change that (e.g. warn the user if they want to save)
break;
case SDL_KEYDOWN:
// There is no point giving functions return values if you
// aren't going to use them.
// This is a simple way to use them, if the functions return
// false, then the program knows an error occurred.
bool success = true;
if(event.key.keysym.sym == SDLK_s) {
success = game_start(gameData);
} else if(event.key.keysym.sym == SDLK_c) {
success = game_load(gameData);
} else if(event.key.keysym.sym == SDLK_v) {
success = game_credits(gameData);
}
if(!success) {
// TODO: error logging, handling, cleanup, etc.
}
// Again, include a break statement.
// What if you were to add a new "case" clause?
break;
}
clear_screen(screen);
apply_surface(frame, 0, 0, screen);
for(int i = 0 ; i < NUM_STRINGS ; ++i) {
int x = 410;
// Another neat thing about an array, we can *calculate* where each
// element needs to be drawn!
// Work it out on paper, convince yourself this is the same as your old code!
int y = 280 + (16 * i);
apply_surface(textSurfaces[i], x, y, screen);
}
SDL_Flip(screen);
}
}
// This is a function I've used for a long time to handle English keyboard input.
// You should do some research on some of the points Wooh raised about character encodings
// ---------------------------------------------------------------------------------------
// Note: SDL_EnableUNICODE(1); needs to be called before the event is received by SDL
// This returns zero to indicate error/not useful key
// otherwise returns the character that this key represents
char getUnicodeValue( const SDL_KeyboardEvent &key )
{
assert( SDL_EnableUNICODE(SDL_QUERY) == SDL_ENABLE );
// magic numbers courtesy of SDL docs <img src='http://public.gamedev.net/public/style_emoticons/<#EMO_DIR#>/smile.gif' class='bbc_emoticon' alt=':)' />
const int INTERNATIONAL_MASK = 0xFF80, UNICODE_MASK = 0x7F;
int uni = key.keysym.unicode;
if( uni == 0 ) // not translatable key (like up or down arrows)
{
// probably not useful as string input
// we could optionally use this to get some value
// for it: SDL_GetKeyName( key );
return 0;
}
else if( ( uni & INTERNATIONAL_MASK ) == 0 )
{
if( SDL_GetModState() & KMOD_SHIFT )
{
return static_cast<char>(toupper(uni & UNICODE_MASK));
}
else
{
return static_cast<char>(uni & UNICODE_MASK);
}
}
else // we have a funky international character. one we can't read <img src='http://public.gamedev.net/public/style_emoticons/<#EMO_DIR#>/sad.gif' class='bbc_emoticon' alt=':(' />
{
// we could do nothing, or we can just show some sign of input, like so:
// return '?';
return 0;
}
}
// Always declare functions with a return type! If a function
// doesn't return anything useful, use "void".
bool game_start(const GameData &gameData) {
// No need to re-load the frame, it is in gameData!
// surface[1] = IMG_Load("gfx/frame.png");
// For convenience, we'll move these into local variables
SDL_Surface *screen = gameData.screen;
SDL_Surface *frame = gameData.frame;
TTF_Font *font = gameData.font;
SDL_Color font_color = { 180, 180, 180 };
const int NUM_PROMPTS = 2;
const char *promptStrings[] = {
"By what name shalt",
"thou be known?"
};
SDL_Surface *promptSurfaces[2] = {};
for(int i = 0 ; i < NUM_PROMPTS ; ++i) {
promptSurfaces[0] = TTF_RenderText_Solid(font, promptStrings[i], font_color);
// Error handling?
}
// Don't use magic numbers, use named constants instead
const int MAX_INPUT_LENGTH = 8;
// If you are using C, we'll need to have a completely different discussion about
// how this might work.
std::string user_input;
// I've removed the outer while loop from this function.
// If SDL_WaitEvent() fails, then repeating this action in a loop will likely only
// cause your program to hang
SDL_Event event;
while(SDL_WaitEvent(&event) != 0) {
switch(event.type) {
// Hmm, here is some code duplication.
// Eliminating this would require a bit more architectural work on your
// program, beyond the scope of this post.
case SDL_QUIT:
exit(0);
// See notes above about including the "break" statement.
break;
case SDL_KEYDOWN:
// Your previous conditional block was a little convoluted.
// Don't write if(condition) { ... } else if(!condition) { ... }
// Use write if(condition) { ... } else { ... }
// Also note that by handling the special cases first, we can naturally fall
// into an "else" block for the general case.
if(event.key.keysym.sym == SDLK_RETURN) {
// Never, ever, pass a user input as the first parameter to printf()!
// What if the user typed "My name is %s %d %s..."
// Your program could crash, or worse (!), based on special characters included
// accidentally or maliciously in such input. Always include a format string.
printf("%s\n", user_input);
} else if(event.key.keysym.sym == SDLK_BACKSPACE){
if(!user_input.empty()) {
user_input.pop_back();
}
} else {
if(user_input.size() < MAX_INPUT_LENGTH) {
char c = getUnicodeValue(event.key);
if(c) {
user_input.push_back(c);
}
}
// Whoops, it would have been too late to call SDL_EnableUNICODE here. It must be called before the
// the event is handled by SDL. Luckily, you've enabled it at startup.
// SDL_EnableUNICODE(SDL_ENABLE);
}
// See notes above about including the "break" statement.
break;
}
// You used to have a memory leak here...
// You kept re-loading the surfaces every frame, without deallocating them.
clear_screen(screen);
apply_surface(frame, 0, 0, screen);
int x = 414;
for(int i = 0 ; i < NUM_PROMPTS ; ++i) {
int y = 280 + (i * 16);
apply_surface(promptSurfaces[i], x, y, screen);
}
SDL_Surface *user_surface = TTF_RenderText_Solid(font, user_input.c_str(), font_color);
if(!user_surface) {
// Error handling?
}
int y = 280 + (NUM_PROMPTS * 16);
apply_surface(user_surface, x, y, screen);
// Note here how we avoid a memory leak by cleaning up the new surface
SDL_FreeSurface(user_surface);
SDL_Flip(screen);
}
// You should clean up surfaces loaded in this function here before returning to the caller
// We unconditionally return false here, because right now the only way that can happen is
// if SDL_WaitEvent fails - an error state.
return false;
}
Note I have neither compiled nor tested the code, so do not assume that it is correct. If you have any questions, feel free to ask.
1
