|
|
|
This thread is locked; no one can reply to it.
|
| 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 : 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 !! |
|
David Couzelis
Member #10,079
August 2008
 |
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. |
|
Battyhal
Member #15,775
October 2014
|
Yes, you are right David. Here it goes : 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
|
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:
|
|
Chris Katko
Member #1,881
January 2002
 |
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: |
|
Battyhal
Member #15,775
October 2014
|
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 : 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 And by the way, thanks for your help guys! |
|
Peter Hull
Member #1,136
March 2001
|
Battyhal said: and i received this answer : " Event type = 10 Those are just ALLEGRO_EVENT_KEY_DOWN/_CHAR/_UP, as you'd expect. I think your issue is gameFPS - it's doing 2 jobs: 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
 |
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. -- acc.js | al4anim - Allegro 4 Animation library | Allegro 5 VS/NuGet Guide | Allegro.cc Mockup | Allegro.cc <code> Tag | Allegro 4 Timer Example (w/ Semaphores) | Allegro 5 "Winpkg" (MSVC readme) | Bambot | Blog | C++ STL Container Flowchart | Castopulence Software | Check Return Values | Derail? | Is This A Discussion? Flow Chart | Filesystem Hierarchy Standard | Clean Code Talks - Global State and Singletons | How To Use Header Files | GNU/Linux (Debian, Fedora, Gentoo) | rot (rot13, rot47, rotN) | Streaming |
|
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
|
Ah, yeah, seems you are right. 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. -- acc.js | al4anim - Allegro 4 Animation library | Allegro 5 VS/NuGet Guide | Allegro.cc Mockup | Allegro.cc <code> Tag | Allegro 4 Timer Example (w/ Semaphores) | Allegro 5 "Winpkg" (MSVC readme) | Bambot | Blog | C++ STL Container Flowchart | Castopulence Software | Check Return Values | Derail? | Is This A Discussion? Flow Chart | Filesystem Hierarchy Standard | Clean Code Talks - Global State and Singletons | How To Use Header Files | GNU/Linux (Debian, Fedora, Gentoo) | rot (rot13, rot47, rotN) | Streaming |
|
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, Cheers,
|
|
Battyhal
Member #15,775
October 2014
|
Hi !. Peter, i have to apologize ! Thanks very very much for your help !!. |
|
|
Sorry I see it in your first code fragment....!
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 :
.
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.
.
