|
|
|
This thread is locked; no one can reply to it.
|
| Would just like some general feedback |
|
agonvs
Member #15,917
March 2015
|
Hi all, This is just a little sample program which demonstrates scrolling and joystick functionality. I just wondered if you'd be willing to provide some feedback on my code style, cleanliness, technique etc. Keep in mind that I am just learning Allegro and this is the most progress I've made so far. 1#include<allegro5/allegro.h>
2#include<allegro5/allegro_primitives.h>
3#include<allegro5/allegro_image.h>
4#include<iostream>
5using namespace std;
6
7void render()
8{
9 cout << endl;
10}
11
12enum directions {STILL, NORTH, NORTHEAST, EAST, SOUTHEAST, SOUTH, SOUTHWEST, WEST, NORTHWEST};
13enum buttons {NONE,ZERO,ONE,EIGHT,NINE};
14
15int main()
16{
17
18 float gameTime = 0;
19 int frames = 0;
20 int gameFPS = 0;
21 if(!(al_init()))
22 cout << "Couldn't initialize Allegro." << cout;
23 if(!(al_install_joystick()))
24 cout << "Couldn't install joystick." << endl;
25 al_init_image_addon();
26 ALLEGRO_DISPLAY *display = NULL;
27 display = al_create_display(1280,720);
28 ALLEGRO_EVENT_QUEUE *event_queue = NULL;
29 event_queue = al_create_event_queue();
30 if(!event_queue)
31 cout << "al_create_event_queue failed." << endl;
32 ALLEGRO_TIMER *timer;
33 timer = al_create_timer(1.0/60);
34 al_register_event_source(event_queue, al_get_timer_event_source(timer));
35
36 gameTime = al_current_time();
37 al_register_event_source(event_queue, al_get_joystick_event_source());
38 ALLEGRO_JOYSTICK* joy = al_get_joystick(0);
39 float ballposX=0;
40 float ballposY=0;
41 ALLEGRO_JOYSTICK_STATE jst;
42 directions dir = STILL;
43 buttons button = NONE;
44 bool render = false;
45 ALLEGRO_BITMAP *image = NULL;
46 ALLEGRO_BITMAP *texture = NULL;
47 ALLEGRO_BITMAP *parallax = NULL;
48 texture = al_load_bitmap("texture.pcx");
49 image = al_load_bitmap("goldenball.png");
50 parallax = al_load_bitmap("BluePaintedTiles.png");
51 bool alive = true;
52 int shiftX=0;
53 int shiftY=0;
54 int shiftX2=0;
55 al_start_timer(timer);
56 while(alive)
57 {
58 ALLEGRO_EVENT ev;
59 al_wait_for_event(event_queue, &ev);
60 al_get_joystick_state(joy, &jst);
61 if (ev.type == ALLEGRO_EVENT_JOYSTICK_AXIS)
62 {
63 if ((jst.stick[2].axis[0] == 0) && (jst.stick[2].axis[1] == 0))
64 dir = STILL;
65 if ((jst.stick[2].axis[0] == 0) && (jst.stick[2].axis[1] == -1))
66 dir = NORTH;
67 if ((jst.stick[2].axis[0] == 1) && (jst.stick[2].axis[1] == -1))
68 dir = NORTHEAST;
69 if ((jst.stick[2].axis[0] == 1) && (jst.stick[2].axis[1] == 0))
70 dir = EAST;
71 if ((jst.stick[2].axis[0] == 1) && (jst.stick[2].axis[1] == 1))
72 dir = SOUTHEAST;
73 if ((jst.stick[2].axis[0] == 0) && (jst.stick[2].axis[1] == 1))
74 dir = SOUTH;
75 if ((jst.stick[2].axis[0] == -1) && (jst.stick[2].axis[1] == 1))
76 dir = SOUTHWEST;
77 if ((jst.stick[2].axis[0] == -1) && (jst.stick[2].axis[1] == 0))
78 dir = WEST;
79 if ((jst.stick[2].axis[0] == -1) && (jst.stick[2].axis[1] == -1))
80 dir = NORTHWEST;
81
82 }
83 if (ev.type == ALLEGRO_EVENT_JOYSTICK_BUTTON_DOWN)
84 {
85 if (jst.button[0])
86 button = ZERO;
87 if (jst.button[1])
88 button = ONE;
89 if (jst.button[8])
90 button = EIGHT;
91 if (jst.button[9])
92 button = NINE;
93 }
94 else if (ev.type == ALLEGRO_EVENT_TIMER)
95 {
96 render = true;
97 frames++;
98 if (al_current_time() - gameTime >= 1)
99 {
100 gameTime = al_current_time();
101 gameFPS = frames;
102 frames = 0;
103 }
104 }
105 if (render && al_is_event_queue_empty(event_queue))
106 {
107 render = false;
108 switch (dir)
109 {
110 case NORTH:
111 ballposY -= 3;
112 if (ballposY < 0)
113 ballposY=0;
114 break;
115 case NORTHEAST:
116 ballposY -= 2;
117 ballposX += 2;
118 if (ballposY < 0)
119 ballposY = 0;
120 if (ballposX > 1152)
121 ballposX = 1152;
122 break;
123 case EAST:
124 ballposX += 3;
125 if (ballposX > 1152)
126 ballposX = 1152;
127 break;
128 case SOUTHEAST:
129 ballposY += 2;
130 ballposX += 2;
131 if (ballposY > 592)
132 ballposY = 592;
133 if (ballposX > 1152)
134 ballposX = 1152;
135 break;
136 case SOUTH:
137 ballposY += 3;
138 if (ballposY > 592)
139 ballposY = 592;
140 break;
141 case SOUTHWEST:
142 ballposX -= 2;
143 ballposY += 2;
144 if (ballposY > 592)
145 ballposY = 592;
146 if (ballposX < 0)
147 ballposX = 0;
148 break;
149 case WEST:
150 ballposX -= 3;
151 if (ballposX < 0)
152 ballposX = 0;
153 break;
154 case NORTHWEST:
155 ballposY -= 2;
156 ballposX -= 2;
157 if (ballposY < 0)
158 ballposY = 0;
159 if (ballposX < 0)
160 ballposX = 0;
161 break;
162 }
163 switch (button)
164 {
165 case ZERO:
166 if (jst.button[0])
167 image = al_load_bitmap("goldenball.png");
168 break;
169 case ONE:
170 if (jst.button[1])
171 image = al_load_bitmap("silverball.png");
172 break;
173 case EIGHT:
174 if (jst.button[8])
175 break;
176 case NINE:
177 if (jst.button[9])
178 alive = false;
179 break;
180 }
181 // al_draw_bitmap(image, ballposX, ballposY, 0);
182 al_flip_display();
183 al_clear_to_color(al_map_rgb(0,0,0));
184 for(int i = 0; i <11; i++)
185 for (int j = 0; j<7; j++)
186 al_draw_bitmap(texture, ((i*128)+shiftX),((j*128)+shiftY),0);
187 shiftX -= 8;
188 if (shiftX == -128)
189 shiftX = 0;
190 al_draw_bitmap(image, ballposX, ballposY, 0);
191 for (int i = 0; i < 11; i++)
192 {
193 al_draw_bitmap(parallax,((i*128)+shiftX2),-64,0);
194 al_draw_bitmap(parallax,((i*128)+shiftX2),656,0);
195 }
196 shiftX2 -= 16;
197 if (shiftX2 == -128)
198 shiftX2 = 0;
199 }
200 }
201 al_destroy_bitmap(image);
202 al_destroy_timer(timer);
203 al_destroy_event_queue(event_queue);
204 return 0;
205}
|
|
Dizzy Egg
Member #10,824
March 2009
 |
Looks OK, although you might want to start using variables instead of hard-coding all those numbers; that way if you want to change 128 to something else, you only have to change it once and not go through your code changing them all.
---------------------------------------------------- |
|
StevenVI
Member #562
July 2000
 |
In terms of style, follow a set of rules and stick to it. It doesn't look like that's going on in here: // 1. Spaces after ifs are not consistent if(!(al_init())) if (render && al_is_event_queue_empty(event_queue)) // 2. Spaces around operators is not consistent bool alive = true; int shiftX=0; // 3. Space between function parameters is not consistent al_draw_bitmap(texture, ((i*128)+shiftX),((j*128)+shiftY),0); In terms of my personal preferences, two rules that I like to follow are: // 1. Opening braces start go on the same line if (foo) { /* do stuff */ } // 2. Single-line blocks are always enclosed in braces. This can help to prevent bugs. if (foo) { doSomething(); } // Rather than if (foo) doSomething(); For technique, I agree with Dizzy Egg, get rid of all those magic numbers and define them as constants. Structure-wise, you probably want to break up that giant main function into smaller chunks. I'd probably structure it something like this pseudocode: 1int main()
2{
3 initializeScreen();
4 loadData();
5
6 while (alive)
7 {
8 ALLEGRO_EVENT ev;
9 while (al_get_next_event(event_queue, &ev))
10 {
11 handleEvent(ev);
12 }
13 }
14
15 return 0;
16}
17
18void handleEvent(ALLEGRO_EVENT ev)
19{
20 // Do different things based on the event type
21 // Handle input events appropriately
22 // Update positions and render every timer event
23}
__________________________________________________ |
|
agonvs
Member #15,917
March 2015
|
That is some good feedback, I'll definitely take it into consideration. Thank you friends! :-) |
|
Audric
Member #907
January 2001
|
switch (button) { case ZERO: if (jst.button[0]) image = al_load_bitmap("goldenball.png"); break; case ONE: if (jst.button[1]) image = al_load_bitmap("silverball.png"); break; You keep loading from disk the same images, while the game is playing. It's needless disk access and you're slowly leaking memory. Instead you should separate the resource loading: // done one on game startup image_golden_ball = al_load_bitmap("goldenball.png"); image_silverball = al_load_bitmap("goldenball.png"); and the runtime image assignment: switch (button) { case ZERO: if (jst.button[0]) image = image_goldenball; break; case ONE: if (jst.button[1]) image = image_silverball; break;
|
|
|
