Allegro.cc - Online Community

Allegro.cc Forums » Programming Questions » Wrapping up code in functions

This thread is locked; no one can reply to it. rss feed Print
Wrapping up code in functions
Battyhal
Member #15,775
October 2014

Hi everybody!.

I´m in the process of making a clone of Asteroids using Allegro 5. Everything was working fine, i could rotate the ship, i could move it with the characteristic acceleration of this game and i could fire...until i decided to "compress" the code in the main function (which is reaching 300 hundred lines). I wrote a Game class as seen in others posts in this forum, with the classic methods : init Allegro, input, update and render. The "init Allegro" function seems to work fine, the display appears with the expected size. But i can´t move the ship and i don´t understand why. I have only split up the code in some functions...

This is the code in the input function :

#SelectExpand
1// Nostromo is the ship 2void Game::input(){ 3 al_wait_for_event(event_queue, &ev); 4 std::cout << "Ev.type = " << ev.type << std::endl; 5 6 if(ev.type == ALLEGRO_EVENT_TIMER){ 7 if(key[KEY_UP]) 8 Nostromo->accelerate(); 9 else if(key[KEY_LEFT]) 10 Nostromo->rotateLeft(); 11 else if(key[KEY_RIGHT]) 12 Nostromo->rotateRight(); 13 } 14 else if(ev.type == ALLEGRO_EVENT_DISPLAY_CLOSE){ 15 playing = false; 16 } 17 else if(ev.type == ALLEGRO_EVENT_KEY_DOWN){ 18 switch(ev.keyboard.keycode){ 19 case ALLEGRO_KEY_UP:{ 20 key[KEY_UP] = true; 21 break; 22 } 23 case ALLEGRO_KEY_LEFT:{ 24 key[KEY_LEFT] = true; 25 break; 26 } 27 case ALLEGRO_KEY_RIGHT:{ 28 key[KEY_RIGHT] = true; 29 break; 30 } 31 case ALLEGRO_KEY_SPACE:{ 32 key[KEY_SPACE] = true; 33 // TODO bullet creation 34 break; 35 } 36 } 37 } 38 else if(ev.type == ALLEGRO_EVENT_KEY_UP){ 39 switch(ev.keyboard.keycode){ 40 case ALLEGRO_KEY_ESCAPE: 41 playing = false; 42 break; 43 case ALLEGRO_KEY_UP: 44 key[KEY_UP] = false; 45 break; 46 case ALLEGRO_KEY_LEFT: 47 key[KEY_LEFT] = false; 48 break; 49 case ALLEGRO_KEY_RIGHT: 50 key[KEY_RIGHT] = false; 51 break; 52 case ALLEGRO_KEY_SPACE: 53 key[KEY_SPACE] = false; 54 break; 55 } 56 } 57}

I suppose it is not so easy to split up the code and wrap it in functions. If someone can take a look it would be great!. Thanks !! :D

David Couzelis
Member #10,079
August 2008
avatar

I don't think it's possible to know what the issue is without seeing more (or all) of the code.

I'm not sure how you went about refactoring the code, but in the future, the safest way to refactor is to make a single change (for example, move one piece of code into a new function), ensure that it still compiles, and test to make sure that the functionality that the function performs still works.

The alternative is doing a ton of cleanup all at once, finding out that something is now broken, and having no idea what change you made that caused it. :P

Battyhal
Member #15,775
October 2014

Yes, you are right David. Here it goes :

#SelectExpand
1// Game.h 2class Game 3{ 4 public: 5 Game(); 6 ~Game(); 7 int initAllegro(); 8 bool Playing() { return playing; } 9 void input(); 10 void update(); 11 void collisions(); 12 void render(); 13 void cleanIt(); 14 15 protected: 16 float gameTime; 17 int frames; 18 int gameFPS; 19 20 bool playing; 21 bool gameOver; 22 bool redraw; 23 24 bool key[4] = { false, false, false, false }; 25 enum KEYS { KEY_UP, KEY_LEFT, KEY_RIGHT, KEY_SPACE }; 26 27 std::list<Object* > objects; 28 Ship* Nostromo; 29 30 ALLEGRO_EVENT ev; 31 ALLEGRO_FONT* font18; 32 ALLEGRO_DISPLAY* display; 33 ALLEGRO_TIMER* timer; 34 ALLEGRO_EVENT_QUEUE* event_queue; 35} 36 37//Game.cpp 38Game::Game(){ 39 playing = true; 40 gameOver = false; 41 redraw = false; 42 gameTime = 0; 43 frames = 0; 44 gameFPS = 0; 45 srand(time(NULL)); 46 47 Nostromo = new ship(WIDTH/2, HEIGHT/2); 48 objects.push_back(Nostromo); 49} 50 51Game::~Game(){} 52 53int Game::initAllegro(){ 54 if(!al_init()){ 55 std::cout << " Failed to initialize Allegro!" << std::endl; 56 return -1; 57 } 58 display = al_create_display(WIDTH, HEIGHT); 59 if(!display){ 60 std::cout << " Failed to initialize display!" << std::endl; 61 return -1; 62 } 63 if(!al_install_keyboard()){ 64 std::cout << "Failed to initialize the keyboard!\n"; 65 return -1; 66 } 67 if(!al_init_image_addon()){ 68 std::cout << "Failed to create image addon!\n"; 69 return -1; 70 } 71 if(!al_init_font_addon()){ 72 std::cout << "Failed to create font addon!\n"; 73 return -1; 74 } 75 if(!al_init_ttf_addon()){ 76 std::cout << "Failed to create ttf addon!\n"; 77 return -1; 78 } 79 if(!al_init_primitives_addon()){ 80 std::cout << "Failed to create primitives addon!\n"; 81 return -1; 82 } 83 timer = al_create_timer(1.0 / gameFPS); 84 if(!timer) { 85 std::cout << "Failed to create timer!\n"; 86 return -1; 87 } 88 event_queue = al_create_event_queue(); 89 if(!event_queue) { 90 std::cout << "Failed to create event_queue!\n"; 91 al_destroy_display(display); 92 al_destroy_timer(timer); 93 return -1; 94 } 95 al_register_event_source(event_queue, al_get_display_event_source(display)); 96 al_register_event_source(event_queue, al_get_timer_event_source(timer)); 97 al_register_event_source(event_queue, al_get_keyboard_event_source()); 98 al_start_timer(timer); 99 100 return 0; 101} 102 103// Update 104void Game::update(){ 105 if(ev.type == ALLEGRO_EVENT_TIMER){ 106 redraw = true; 107 108 //UPDATE FPS=========== 109 frames++; 110 if(al_current_time() - gameTime >= 1){ 111 gameTime = al_current_time(); 112 gameFPS = frames; 113 frames = 0; 114 } 115 //===================== 116 117 if(key[KEY_UP]) 118 Nostromo->accelerate(); 119 if(key[KEY_LEFT]) 120 Nostromo->rotateLeft(); 121 else if(key[KEY_RIGHT]) 122 Nostromo->rotateRight(); 123 124 std::list<Objeto* >::iterator it; 125 for(it = objects.begin(); it != objects.end(); ++it){ 126 (*it)->update(); 127 } 128 } 129} 130 131// Render 132void Game::render(){ 133 if(redraw && al_is_event_queue_empty(event_queue)){ 134 redraw = false; 135 std::list<Object* >::iterator it; 136 for(it = objects.begin(); it != objects.end(); ++it){ 137 (*it)->draw(); 138 } 139 al_flip_display(); 140 al_clear_to_color(al_map_rgb(0, 0, 0)); 141 } 142 143// Clean 144void Game::cleanIt(){ 145 if(font18) 146 al_destroy_font(font18); 147 if(timer) 148 al_destroy_timer(timer); 149 if(event_queue) 150 al_destroy_event_queue(event_queue); 151 if(display) 152 al_destroy_display(display); 153} 154 155// Main Function 156#include "Object.h" 157#include "Game.h" 158#include "Globals.h" 159 160using namespace std; 161 162int main(int argc, char** argv){ 163 164 Game Asteroids; 165 Asteroids.initAllegro(); 166 167 while(Asteroids.Playing()){ 168 Asteroids.input(); 169 Asteroids.update(); 170 Asteroids.render(); 171 } 172 Asteroids.cleanIt(); 173 174 return 0; 175} 176/

Hope it is not too long...:-/

David Couzelis
Member #10,079
August 2008
avatar

Hmmm... I don't see anything that would cause the ship to not move.

Here is something I think is a problem: you call the function "Nostromo->accelerate()" in two places, once in "Game::input()" and once in "Game::update()". Why?

Do you still have a copy of the working code, before you "compressed" it?

Peter Hull
Member #1,136
March 2001

Possibly this is not all the code, but:
Where are you getting the events and updating Game::ev ?
[edit] :-X Sorry I see it in your first code fragment....!

Chris Katko
Member #1,881
January 2002
avatar

Divide and conquer.

Add a key statement next to the others that either prints to the console, or draws a box. First confirm that keyboard events are actually being handled so you can rule out whether it's an issue with input, or an issue with accessing the object.

-----sig:
“Programs should be written for people to read, and only incidentally for machines to execute.” - Structure and Interpretation of Computer Programs

Battyhal
Member #15,775
October 2014

:o it´s true David. In the "original" code was included in the "update" section but when i realized that was not working after the "compression" i tried to put it in different places just to see what happened and i have forgotten to comment it. If i put it in the "input" function i can move the ship but in a very strange way, just one time per key press and with no acceleration, just rotation. And if i put the code in the "update" function, i can not move it at all. This is the "original" code :

#SelectExpand
1 2#include"ship.h" 3#include"ENEMY2.h" 4#include"PROJECTILE.h" 5 6using namespace std; 7 8bool key[4] = { false, false, false, false }; 9 10// Prototypes 11int initAllegroAndAddons(); 12 13// Globals 14Object* object; 15 16list<Object* > objects; 17list<Object* >::iterator iter; 18list<Object* >::iterator iter2; 19 20// Main 21int main(int argc, char **argv){ 22 23 // SHELL VARIABLES 24 bool gameOver = false; 25 bool redraw = false; 26 27 float gameTime = 0; 28 int frames = 0; 29 int gameFPS = 0; 30 31 // PROJECT VARIABLES 32 ship* Nostromo = new ship(); 33 34 initAllegroAndAddons(); 35 36 ALLEGRO_DISPLAY* display = al_create_display(ANCHURA_DISPLAY, ALTURA_DISPLAY); // display creation 37 if(!display) 38 { 39 fprintf(stderr, "failed to create display!\n"); 40 return -1; 41 } 42 43 //PROJECT INITIALIZATION 44 ALLEGRO_FONT* font18 = al_load_font("Arcade_Classic.ttf", 28, 0); 45 46 Nostromo->Initialize(); 47 objects.push_back(Nostromo); 48 49 // TIMER AND MAIN LOOP INICIALIZATION 50 srand(time(NULL)); 51 ALLEGRO_TIMER* timer = al_create_timer(1.0 / FPS); 52 if(!timer) { 53 fprintf(stderr, "failed to create timer!\n"); 54 return -1; 55 } 56 57 ALLEGRO_EVENT_QUEUE* event_queue = al_create_event_queue(); 58 if(!event_queue) { 59 fprintf(stderr, "failed to create event_queue!\n"); 60 al_destroy_display(display); 61 al_destroy_timer(timer); 62 return -1; 63 } 64 65 al_register_event_source(event_queue, al_get_display_event_source(display)); 66 al_register_event_source(event_queue, al_get_timer_event_source(timer)); 67 al_register_event_source(event_queue, al_get_keyboard_event_source()); 68 69 al_start_timer(timer); 70 gameTime = al_current_time(); 71 72 while(!gameOver){ 73 ALLEGRO_EVENT ev; 74 al_wait_for_event(event_queue, &ev); 75 76 // INPUT 77 if(ev.type == ALLEGRO_EVENT_DISPLAY_CLOSE){ 78 break; 79 } 80 else if(ev.type == ALLEGRO_EVENT_KEY_DOWN){ 81 switch(ev.keyboard.keycode){ 82 case ALLEGRO_KEY_UP: 83 key[KEY_UP] = true; 84 break; 85 case ALLEGRO_KEY_LEFT: 86 key[KEY_LEFT] = true; 87 break; 88 case ALLEGRO_KEY_RIGHT: 89 key[KEY_RIGHT] = true; 90 break; 91 case ALLEGRO_KEY_SPACE:{ 92 key[KEY_SPACE] = true; 93 PROJECTILE* bullet = ship->Fire();//)new 94 objects.push_back(bullet); 95 break; 96 } 97 } 98 } 99 else if(ev.type == ALLEGRO_EVENT_KEY_UP){ 100 switch(ev.keyboard.keycode){ 101 case ALLEGRO_KEY_ESCAPE: 102 gameOver = true; 103 break; 104 case ALLEGRO_KEY_UP: 105 key[KEY_UP] = false; 106 break; 107 case ALLEGRO_KEY_LEFT: 108 key[KEY_LEFT] = false; 109 break; 110 case ALLEGRO_KEY_RIGHT: 111 key[KEY_RIGHT] = false; 112 break; 113 case ALLEGRO_KEY_SPACE: 114 key[KEY_SPACE] = false; 115 break; 116 } 117 } 118 119 // GAME UPDATE 120 else if(ev.type == ALLEGRO_EVENT_TIMER){ 121 redraw = true; 122 123 // UPDATE FPS=========== 124 frames++; 125 if(al_current_time() - gameTime >= 1){ 126 gameTime = al_current_time(); 127 gameFPS = frames; 128 frames = 0; 129 } 130 //===================== 131 132 if(key[KEY_UP]) 133 ship->Accelerate(); 134 if(key[KEY_LEFT]) 135 ship->MoveLeft(); 136 else if(key[KEY_RIGHT]) 137 ship->MoveRight(); 138 139 int asteroidCreation = rand() % 50; 140 if(asteroidCreation == 0){ 141 ENEMY2* big = new ENEMY2(rand() % WIDTH_DISPLAY, rand() % HEIGHT_DISPLAY, sizeBigAsteroid ,sizeENEMYX, sizeENEMYY); 142 objetos.push_back(big); 143 } 144 145 // objects update 146 for(iter = objects.begin(); iter != objects.end(); ++iter){ 147 (*iter)->update(); 148 } 149 // TODO collisions 150 } // End if(....timer) 151 // RENDER 152 if(redraw && al_is_event_queue_empty(event_queue)){ 153 redraw = false; 154 for(iter = objects.begin(); iter != objects.end(); ++iter){ 155 (*iter)->Draw(); 156 } 157 al_flip_display(); 158 al_clear_to_color(al_map_rgb(0, 0, 0)); 159 } // End if(redraw.... 160 } // End While 161 162 // DESTROY OBJECTS 163 for(iter = objects.begin(); iter != objects.end(); ++iter){ 164 (*iter)->Destroy(); 165 delete (*iter); 166 iter = objects.erase(iter); 167 } 168 169 // SHELL OBJECTS 170 al_destroy_font(font18); 171 al_destroy_timer(timer); 172 al_destroy_event_queue(event_queue); 173 al_destroy_display(display); 174 175 return 0; 176} 177 178int initAllegroAndAddons(){ 179 if(!al_init()){ 180 fprintf(stderr, "failed to initialize allegro!\n"); 181 return -1; 182 } 183 if(!al_install_keyboard()) { 184 fprintf(stderr, "failed to initialize the keyboard!\n"); 185 return -1; 186 } 187 if(!al_init_image_addon()) { 188 fprintf(stderr, "failed to create image addon!\n"); 189 return -1; 190 } 191 192 al_init_font_addon(); 193 al_init_ttf_addon(); 194 al_init_primitives_addon(); 195 al_install_audio(); 196 al_init_acodec_addon(); 197 198 return 0; 199}

And Peter, you are right, this is not all the code. I haven´t include it because i thought the post was going to be very large. But finally it is :-[.

Chris, i have include it this line :

#SelectExpand
1void Game::input(){ 2 al_wait_for_event(event_queue, &ev); 3 std::cout << "Event type = " << ev.type << std::endl; <-------- this line 4 5 if(ev.type == ALLEGRO_EVENT_TIMER){........

and i received this answer : " Event type = 10
Event type = 11
Event type = 12 "
after ANY key press, not only from arrow keys or space. If i press UP the answer is 10,11,12. The same with DOWN, RIGHT..... anyone. :o

And by the way, thanks for your help guys! :D

Peter Hull
Member #1,136
March 2001

Battyhal said:

and i received this answer : " Event type = 10
Event type = 11
Event type = 12 "

Those are just ALLEGRO_EVENT_KEY_DOWN/_CHAR/_UP, as you'd expect.

I think your issue is gameFPS - it's doing 2 jobs:
1. The actual measured frames per second in Game::update()
2. The desired FPS when you set up the timer.

Unfortunately in Game::initAllegro when you set the timer:

timer = al_create_timer(1.0 / gameFPS);
  if (!timer) {
    std::cout << "Failed to create timer!\n";
    return -1;
  }

gameFPS is zero so you create a timer which never fires. Hence the ship's position is never updated.

Pete

bamccaig
Member #7,536
July 2006
avatar

What stands out to me is that Game::input() is grabbing the next event without knowing if it's an input event or not. If it is fantastically, you'll handle it as such and presumably that will work fine. If it's not you're throwing it away. What's more, what is going to call this? Presumably that is going to be eating events too, and if it is eating input events it is probably not handling them properly. What you probably want is an event dispatcher that can be registered with event handlers for specific types of events. That will do the work of getting the next event, figuring out what type it is, and passing it off to whatever should handle it. The simplest solution would be to just hard-code it with a switch statement.

Peter Hull
Member #1,136
March 2001

I don't agree; Game::input handles all the events by setting flags, and it's called in the main game loop.

  while (Asteroids.Playing()) {
    Asteroids.input();
    Asteroids.update();
    Asteroids.render();
  }
  Asteroids.cleanIt();

This seems reasonable to me.

If it were renamed to Game::handleEvents it might be more self-evident?

bamccaig
Member #7,536
July 2006
avatar

Ah, yeah, seems you are right. :-/ Hmmm... Well it looks reasonable to you, but it looks pretty silly to me to call update() and render() only to have them check if they should run. A better design would be for those methods to do their job when they're called, and only call them when it's time.

Well one can begin by trying to clean up the design. For example, an instance level event doesn't seem to make much sense. Instead track the state of the program and respond to that. It seems clear that input() is just doing too much though. It's purpose isn't clear. It seems to be doing some of the update logic, which seems duplicated...

If you want your code to work you need to keep it simple and small. Break your functions into smaller pieces, and keep their purposes simple.

For the OP: Stop posting pieces of code here and there and post the entire program in its current state so that we can try to build, run, and debug it. :)

Battyhal
Member #15,775
October 2014

Ok, i should have started this way but i have the bad habit of program in spanish and i had to translate all the code. I do apologize for this and i promise to start a fresh habit ;).

Here is the link for the whole code :

https://mega.nz/#F!gJlCERKL!48KlLndlJiSgcgu-6-vPxA

Peter : i´m not really sure to understand the problem with gameFPS. In the "original" code before the "compression" ( i quote "original" every time because is far from being mine, unfortunately, but i hope i can do something similar in a not very distant future ) it works with gameFPS initialized to zero.

bamccaig : i will break the functions in smaller ones !. :)

Peter Hull
Member #1,136
March 2001

Battyhal said:

it works with gameFPS initialized to zero.

OK, I'll leave that with you.

On the code you posted,
1. Don't call al_load_font in the render routine (that's the same problem that The_AI had just this week)
2. I don't think the accelerate function is correct.
3. Currently you can only process one event per input..update..render cycle and that's not the conventional way to do it. (see https://www.allegro.cc/forums/thread/616152/1021230#target)

Cheers,
Pete

Battyhal
Member #15,775
October 2014

Hi !.

Peter, i have to apologize ! :-/. The problem WAS gameFPS indeed !. I have taken a second (and more careful) look at your answer, and now gameFPS is initialized to 60 when i create the Game object and it works fine. Anyway i´m working in a new and improved design as you guys suggested and i´m looking for more information about how "timers" work because i think i don´t fully understand their function.

Thanks very very much for your help !!. :D

Go to: