Well... This is lame. I can't quite get this input thing working properly (second time having tried input, if I still had my first source I'd only have the backspace and uppercase to worry about). Basically, text won't appear unless you hit backspace several times (didn't act like that the first time), and due to the unicode stuff, I don't have access to shift-uppercase. What I'm trying to accomplish is a backspace that works (instead of turning things into t's or other letters) and to have the user's input always be uppercase.
If you see any other ways I could improve this wreck, I'll accept suggestions, but I really just want to get backspace and caps working. I hope someone can and will help. Any assistance is greatly appreciated and will be (likely) included in my credits.
[EDIT: I notice that I kinda accidentally enabled unicode twice. Wasn't intentional.]
I'm a bit surprised it even compile because you forgot to specify the return type of game_start and line 138 should probably be user_input[input_slot] = '_'; because user_input[input_slot] is a char.
This will not work as you wantuser_input[input_slot] = event.key.keysym.unicode; event.key.keysym.unicode is 2 bytes while user_input[input_slot] is 1 byte so it will only work properly for the ASCII characters. You need to decide what encoding to use.
If you change char user_input[9]; to Uint16 user_input[9]; you can use TTF_RenderUNICODE_Solid to render the text (note that the text should be null terminated).
Another possibility is to convert the unicode characters to UTF-8 and use TTF_RenderUTF8_Solid. This is a bit more work but use whatever you prefer.
Well, now the following text is giving me an error: br_panel[0]={"[S]tart new quest"}; (The error being "expected expression before '{' token" which I've gotten before in this context and still dont 'get')
There will be mistakes and problems, as this is a learning project as much as it is an important one to me. If someone can shed some light on this error, he or she has tripled my chances of getting this finished successfully. :3
Oh, THANK you! I seriously and completely appreciate this! That's one of my problems resolved, so that's a good start for today... Now, to put this to use.
If you see any other ways I could improve this wreck, I'll accept suggestions...
[/quote]
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();
// 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 = TTF_RenderText_Solid(font, textStrings, 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, 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
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
{
// 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?"
};
// 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, 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.
For starters, rip-off, I am indeed using C. Allow me to comment on some of your comments:
1. The preprocessor usage was only to test it. I've changed it to just hardcoded values within SDL_SetVideoMode().
2. String.h, which I'm guessing (because it isn't an hpp) works with C, will be tested out at a later time. I'm rather unfamiliar with it, in all honesty. Things can be adapted.
3. unistd was included out of recent habit, thanks for pointing that out, lol
4. text input doesn't need to be very complicated for this game or the next two titles I've considered. Name-entry for up to 9 letters (used 'alexander' as a reference for name length possibilities, so as not to cheat people out of using favourite names) and NPC conversation input won't examine anymore than the first four letters of input (exactly the way Ultima did it, because I'm a retrojunkie for things I like).
5. SDL, in my Linux Mint Debian Edition as well as all other distros I've used, never compiles for me (gcc with codeblocks) unless I use the headers as though they're in a subdirectory. I do what works for me, and others will be welcome to change that aspect of the code if they would prefer when I release it with the game.
----------------------------------------------------------------------------------------------------------------
Before my list becomes even more ridiculously long, I'll cut it with this: I'm aware of many of the things you've pointed out, and I really should have made it clear with better commenting or something. You've supplied some helpful information, though, and what I can use will be used. @_@;
Thanks for all this feedback, I'll definitely make a copy of these posts in a file for reference and will work on correcting/revising some things before I continue with my work (to make sure future work won't need to be rewritten along with what I already have). You've been an amazing help, where your advice actually helped. Admittedly, no one can string together a reply like that and have all of their suggestions be of benefit, but you've given me quite a heap to work with!
For #1, named constants are preferred to magic numbers in general. In this case, I would admit that it is of marginal benefit.
#2 & #4 Could be approached with a fixed size character array. However, when dealing with C strings you must be very careful to properly NUL terminate the string.
For example, you might use something like this for the main part of the function:
// 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.
char user_input[MAX_INPUT_LENGTH + 1] = {};
// 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) {
int length = strlen(user_input);
if(length > 0) {
user_input[length - 1] = '\0';
}
} else {
int length = strlen(user_input);
if(length < MAX_INPUT_LENGTH) {
char c = getUnicodeValue(event.key);
if(c) {
user_input[length] = c;
user_input[length + 1] = '\0';
}
}
// 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 would need to include <string.h> to access strlen().
For #5, trying using the "sdl-config" program. The output of sdl-config --cflags should be passed to the compiler, and the output of sdl-config --libs should be passed to the linker. I'm not familiar with configuring code::blocks.
On the command line, you can use
$ gcc *.c `sdl-config --cflags --libs`
In any case, it should be as simple as adding /usr/include/SDL to your compiler's include path.
Firstly, let me just thank you for the idea to add /usr/include/SDL to the include path! It took me a few moments to figure out I had to add it in the 'compiler' page of Code::Blocks' options and not the 'linker' one, but your choice of words helped me out there.
As for user_input being included in a printf, that was only for my own testing purposes. I feel a need to stress that this source is still, sometimes, going to have my testing doodats in it and is far (very, very, very far) from being anything release-worthy. As long as I don't do those things (and I know not to), things will be fine. As for all advice, in general, it is very much appreciated and will be taken into consideration as I continue. I seriously can't thank you two enough for what I've already been given!
EDIT: I nearly forgot what else I was going to ask! How might I force-uppercase input? I would prefer not to use an all-caps font, so is there a way to coax a program into treating input as though shift or capslock is in effect? @_@;
