Need some advice here

Roger_D_Vargas · January 4, 2003, 5:53pm

I want to write an small article about game programming, of course using
sdl. I need some revision to the following code and some opinion. Can it
be smaller or more compact? Can it be smaller or more compact or clear
in Direct X? And other libs? The code is not finished yet, but any
optimization is welcome.

#include <stdio.h>
#include <stdlib.h>
#include “SDL.h”

#define UP 1
#define RIGTH 2
#define DOWN 3
#define LEFT 4

SDL_Surface *sp, *screen, *back;

void loadimg()
{
SDL_Surface *tmp;
//Load background from a bitmap
tmp=SDL_LoadBMP(“backg.bmp”);
back=SDL_DisplayFormat(tmp);
//SDL_BlitSurface(back, NULL, screen, NULL);
//now load the image to move
SDL_UpdateRect(screen,0,0,0,0);
tmp=SDL_LoadBMP(“sail.bmp”);

/* Set transparent pixel as the pixel at (0,0) */
if ( tmp->format->palette ) {
SDL_SetColorKey(tmp, (SDL_SRCCOLORKEY|SDL_RLEACCEL),
*(Uint8 *)tmp->pixels);
}

// Convert image to display format
sp = SDL_DisplayFormat(tmp);
SDL_FreeSurface(tmp);
}

int main(int argc, char *argv[])
{
SDL_Rect area;
SDL_Event event;
int direc; //direccion
int end;

/* Inicializar SDL */
if ( SDL_Init(SDL_INIT_VIDEO) < 0 ) {
//Si falla, mostrar el mensaje de error y abortar
fprintf(stderr, “No se puede inicializar SDL: %s\n”,SDL_GetError());
exit(1);
}
atexit(SDL_Quit);

if ((screen=SDL_SetVideoMode(640,480,24,SDL_SWSURFACE)) == NULL ) {
fprintf(stderr, “Couldn’t set 640x480 video mode: %s\n”,SDL_GetError());
exit(2);
}
//Activar repeticion de teclas
SDL_EnableKeyRepeat(SDL_DEFAULT_REPEAT_DELAY,SDL_DEFAULT_REPEAT_INTERVAL);
//Cargamos la imagen
loadimg();
end=0;
direc=0;
area.x=310;
area.y=400;
//Lazo principal
while (end!=1) {
if (SDL_PollEvent(&event)>0) {
//hay un evento pendiente
if (event.type==SDL_KEYDOWN) {
// fue presionada una tecla
//SDL_BlitSurface(saved, NULL, screen, &area);
//Ver que tecla fue
switch ((int)event.key.keysym.sym) {
case SDLK_LEFT:
direc=LEFT;

         break;
      case SDLK_RIGHT:
     direc=RIGTH;
         break;
      case SDLK_UP:
         direc=UP;
         break;   	
      case SDLK_DOWN:
     direc=DOWN;
         break;
      case SDLK_ESCAPE:
  	     //nos vamos
         end=1;
         break;
     }
    } else if (event.type==SDL_MOUSEBUTTONDOWN)
     //set stop flag
     end=1;

 } //if EventPoll

//Ahora movemos la imagen de acuerdo a la direccion
switch (direc){
case UP:
if (area.y>1) area.y–;
break;
case RIGTH:
if (area.x<640) area.x++;
break;
case DOWN:
if (area.y<480) area.y++;
break;
case LEFT:
if (area.x>1) area.x–;
break;
}
SDL_BlitSurface(back, NULL, screen, NULL);
SDL_BlitSurface(sp, NULL, screen, &area);
if ((screen->flags & SDL_DOUBLEBUF) != SDL_DOUBLEBUF) SDL_UpdateRect(screen,0,0,0,0);
else SDL_Flip(screen);

}
return(0);
}–
Roger D. Vargas | El sistema se apagara en 5 segundos.
ICQ: 117641572 | Salvese el que pueda!
Linux User: 180787 |

Jesse_David_Andrews · January 4, 2003, 5:56pm

pygame is more compact and nice in my opinion (it is a python wrapper for SDL)

www.pygame.org> ----- Original Message -----

From: roger@eht.scu.tur.cu (Roger D. Vargas)
To: SDL at libsdl.org
Date: Fri, 3 Jan 2003 15:54:15 -0500 (CST)
Subject: [SDL] Need some advice here

I want to write an small article about game programming, of course using
sdl. I need some revision to the following code and some opinion. Can it
be smaller or more compact? Can it be smaller or more compact or clear
in Direct X? And other libs? The code is not finished yet, but any
optimization is welcome.

#include <stdio.h>
#include <stdlib.h>
#include “SDL.h”

#define UP 1
#define RIGTH 2
#define DOWN 3
#define LEFT 4

SDL_Surface *sp, *screen, *back;

void loadimg()
{
SDL_Surface *tmp;
//Load background from a bitmap
tmp=SDL_LoadBMP(“backg.bmp”);
back=SDL_DisplayFormat(tmp);
//SDL_BlitSurface(back, NULL, screen, NULL);
//now load the image to move
SDL_UpdateRect(screen,0,0,0,0);
tmp=SDL_LoadBMP(“sail.bmp”);

/* Set transparent pixel as the pixel at (0,0) */
if ( tmp->format->palette ) {
SDL_SetColorKey(tmp, (SDL_SRCCOLORKEY|SDL_RLEACCEL),
*(Uint8 *)tmp->pixels);
}

// Convert image to display format
sp = SDL_DisplayFormat(tmp);
SDL_FreeSurface(tmp);
}

int main(int argc, char *argv[])
{
SDL_Rect area;
SDL_Event event;
int direc; //direccion
int end;

/* Inicializar SDL */
if ( SDL_Init(SDL_INIT_VIDEO) < 0 ) {
//Si falla, mostrar el mensaje de error y abortar
fprintf(stderr, “No se puede inicializar SDL: %s\n”,SDL_GetError());
exit(1);
}
atexit(SDL_Quit);

if ((screen=SDL_SetVideoMode(640,480,24,SDL_SWSURFACE)) == NULL ) {
fprintf(stderr, “Couldn’t set 640x480 video mode: %s\n”,SDL_GetError());
exit(2);
}
//Activar repeticion de teclas
SDL_EnableKeyRepeat(SDL_DEFAULT_REPEAT_DELAY,SDL_DEFAULT_REPEAT_INTERVAL);
//Cargamos la imagen
loadimg();
end=0;
direc=0;
area.x=310;
area.y=400;
//Lazo principal
while (end!=1) {
if (SDL_PollEvent(&event)>0) {
//hay un evento pendiente
if (event.type==SDL_KEYDOWN) {
// fue presionada una tecla
//SDL_BlitSurface(saved, NULL, screen, &area);
//Ver que tecla fue
switch ((int)event.key.keysym.sym) {
case SDLK_LEFT:
direc=LEFT;
         break;
      case SDLK_RIGHT:
   direc=RIGTH;
         break;
      case SDLK_UP:
         direc=UP;
         break;   	
      case SDLK_DOWN:
   direc=DOWN;
         break;
      case SDLK_ESCAPE:
  	     //nos vamos
         end=1;
         break;
     }
    } else if (event.type==SDL_MOUSEBUTTONDOWN)
     //set stop flag
     end=1;

 } //if EventPoll
//Ahora movemos la imagen de acuerdo a la direccion
switch (direc){
case UP:
if (area.y>1) area.y–;
break;
case RIGTH:
if (area.x<640) area.x++;
break;
case DOWN:
if (area.y<480) area.y++;
break;
case LEFT:
if (area.x>1) area.x–;
break;
}
SDL_BlitSurface(back, NULL, screen, NULL);
SDL_BlitSurface(sp, NULL, screen, &area);
if ((screen->flags & SDL_DOUBLEBUF) != SDL_DOUBLEBUF) SDL_UpdateRect(screen,0,0,0,0);
else SDL_Flip(screen);

}
return(0);
}

–
Roger D. Vargas | El sistema se apagara en 5 segundos.
ICQ: 117641572 | Salvese el que pueda!
Linux User: 180787 |

SDL mailing list
SDL at libsdl.org
http://www.libsdl.org/mailman/listinfo/sdl

Matt_Borkowski · January 4, 2003, 10:25pm

— “Roger D. Vargas” wrote:

I want to write an small article about game
programming, of course using
sdl. I need some revision to the following code and
some opinion. Can it
be smaller or more compact? Can it be smaller or
more compact or clear
in Direct X? And other libs? The code is not
finished yet, but any
optimization is welcome.

A couple things I noticed:

In the loadimg() function you do
tmp=SDL_LoadBMP(“sail.bmp”);
without first freeing tmp from when you loaded
"backg.bmp".

At the bottom of your while(end!=1) loop you have
this:
if ((screen->flags & SDL_DOUBLEBUF) != SDL_DOUBLEBUF)
SDL_UpdateRect(screen,0,0,0,0);
else SDL_Flip(screen);

Really, you only need to call SDL_Flip(screen) because
it already checks to see if the passed surface has the
SDL_DOUBLEBUF flag set.

Heh, actually, there’s no need to even worry about the
double buffer stuff unless you actually set
SDL_DOUBLEBUF in the first place (in
SDL_SetVideoMode).

Lastly, back up in the loadimg() function you use the
pixel color at (0,0) as the key to pass to
SDL_SetColorKey(). However, by using you get the color
from the palette and thus limit yourself to only
supporting 8bit images. Instead you could do something
more flexible like this:

Uint8 r, g, b;
SDL_GetRGB(*(Uint32 *)tmp->pixels, tmp->format, &r,
&g, &b);
SDL_SetColorKey(tmp, (SDL_SRCCOLORKEY|SDL_RLEACCEL),
SDL_MapRGB(tmp->format, r, g, b));

That will grab the red, green, and blue components of
the color of the first pixel, so it will work even
above 8bit images.

Yeah, that’s all I saw. It’s a pretty small example
anyway. Nice use of the key repeat thing, though; it
simplifies the code a bit.

Matt Borkowski__________________________________________________
Do you Yahoo!?
Yahoo! Mail Plus - Powerful. Affordable. Sign up now.
http://mailplus.yahoo.com

M_Egmond · January 5, 2003, 6:59am

At 2003-01-03 21:54, you wrote:

#define UP 1
#define RIGTH 2

RIGTH should probably be RIGHT. It’s not a problem since you make the same
spelling mistake in the code, but it might look better if you correct it.

[snip]

void loadimg()

I would opt for using more parameters instead of global variables here, but
that may be more of a style issue. However, often global variables obscure
dataflow. For example my choice might be something like:
int loadimg(SDL_Surface *screen, SDL_Surface **image1return, SDL_Surface
**image2return);
and then return an error code from the function. If you don’t do error
checking (for example because it’s meant to be a simple example) then you
can simply ‘return 0’ at the end to indicate it always succeeds (assuming 0
is your choice to return OK, and non-zero are error codes). Anyway, I
already said this is more a style issue, so I will shut up about it now

{
SDL_Surface *tmp;
//Load background from a bitmap
tmp=SDL_LoadBMP(“backg.bmp”);
back=SDL_DisplayFormat(tmp);
//SDL_BlitSurface(back, NULL, screen, NULL);

You need to free the tmp surface here to avoid memory leaks (already
mentioned by another poster). You may also consider what happens if the
’backg.bmp’ is not the same size as the screen.

[snip]

    fprintf(stderr, "No se puede inicializar SDL: %s\n",SDL_GetError());

Ah, I like printing errors to stderr instead of stdout. Good!

[snip]

SDL_EnableKeyRepeat(SDL_DEFAULT_REPEAT_DELAY,SDL_DEFAULT_REPEAT_INTERVAL);

Not sure why you need KeyRepeat. Your code sets ‘direc’ on a keypress, but
never resets it, so a keyrepeat would simply overwrite direc with the same
value. If you want to allow multiple directions to be pressed at the same
time, it might be useful.

You should also be careful. You poll only 1 event per loop. This means that
if events come in faster than your loop executes you will start lagging
behind. I would consider that especially dangerous because you have
key-repeat on…

Also, because you don’t reset ‘direc’ when you stop pressing a key, the
direc flag will remain set, and your ‘object’ will keep travelling in the
direction that was pressed last.

[snip]

//Ahora movemos la imagen de acuerdo a la direccion
switch (direc){
case UP:
if (area.y>1) area.y–;

This code is not timed, and will execute each loop. This means that if you
have a very fast videocard (or refresh rate) the ‘object’ will travel a lot
faster than on a slower machine/refresh rate. You might want to do
something like:
if (current_time-time_of_last_move>movement_interval)
{
time_of_last_move = current_time;
[insert your switch statement here]
}

[snip]

    case RIGTH:
       if (area.x<640) area.x++;

Small point: this might need to be 639, since position 640 is off-screen.
It’s interesting to note that your ‘lower’ limit is 1, even though 0 is
still on screen. Depending on what the ‘object’ actually represents this
might or might not be meaningful. If it’s a single pixel, then the correct
borders would probably be 0 and 639.

[snip]

SDL_BlitSurface(back, NULL, screen, NULL);
SDL_BlitSurface(sp, NULL, screen, &area);
if ((screen->flags & SDL_DOUBLEBUF) != SDL_DOUBLEBUF)
SDL_UpdateRect(screen,0,0,0,0);
else SDL_Flip(screen);

This piece of code was already discussed by another poster.

[snip]

Hope this helps!

Roger_D_Vargas · January 6, 2003, 7:43am

pygame is more compact and nice in my opinion (it is a python wrapper
for SDL)

But im a C programmer.On Sat, 4 Jan 2003, Jesse David Andrews wrote:

–
Roger D. Vargas | El sistema se apagara en 5 segundos.
ICQ: 117641572 | Salvese el que pueda!
Linux User: 180787 |

Roger_D_Vargas · January 6, 2003, 7:47am

In the loadimg() function you do
tmp=SDL_LoadBMP(“sail.bmp”);
without first freeing tmp from when you loaded
"backg.bmp".
Does it matters? The example runs without segfaults.

At the bottom of your while(end!=1) loop you have
this:
if ((screen->flags & SDL_DOUBLEBUF) != SDL_DOUBLEBUF)
SDL_UpdateRect(screen,0,0,0,0);
else SDL_Flip(screen);
the first time i programed this i noticed that using SDL_Flip was slightly
slower than SDL_UpdateRect when double buffering isnt supported. Maybe was
my fault because i was updating just small rectangles and flip updates
all the screen.

Lastly, back up in the loadimg() function you use the
pixel color at (0,0) as the key to pass to
SDL_SetColorKey(). However, by using you get the color
from the palette and thus limit yourself to only
supporting 8bit images. Instead you could do something
more flexible like this:

Uint8 r, g, b;
SDL_GetRGB(*(Uint32 *)tmp->pixels, tmp->format, &r,
&g, &b);
SDL_SetColorKey(tmp, (SDL_SRCCOLORKEY|SDL_RLEACCEL),
SDL_MapRGB(tmp->format, r, g, b));

That will grab the red, green, and blue components of
the color of the first pixel, so it will work even
above 8bit images.
Thanks, i will change that.On Sat, 4 Jan 2003, Matt Borkowski wrote:

–
Roger D. Vargas | El sistema se apagara en 5 segundos.
ICQ: 117641572 | Salvese el que pueda!
Linux User: 180787 |

Roger_D_Vargas · January 6, 2003, 7:55am

At 2003-01-03 21:54, you wrote:

#define UP 1
#define RIGTH 2

RIGTH should probably be RIGHT. It’s not a problem since you make the same
spelling mistake in the code, but it might look better if you correct it.
My mistake, im bad with those words.

I would opt for using more parameters instead of global variables here, but
that may be more of a style issue. However, often global variables obscure
dataflow. For example my choice might be something like:
int loadimg(SDL_Surface *screen, SDL_Surface **image1return, SDL_Surface
**image2return);
and then return an error code from the function. If you don’t do error
checking (for example because it’s meant to be a simple example) then you
can simply ‘return 0’ at the end to indicate it always succeeds (assuming 0
is your choice to return OK, and non-zero are error codes). Anyway, I
already said this is more a style issue, so I will shut up about it now
Besides, Im also bad with parameters in C. Ill take note of that.

Ah, I like printing errors to stderr instead of stdout. Good!

[snip]

SDL_EnableKeyRepeat(SDL_DEFAULT_REPEAT_DELAY,SDL_DEFAULT_REPEAT_INTERVAL);

Not sure why you need KeyRepeat. Your code sets ‘direc’ on a keypress, but
never resets it, so a keyrepeat would simply overwrite direc with the same
value. If you want to allow multiple directions to be pressed at the same
time, it might be useful.
I took it from SDL examples. Neither I knew if it was really useful in so
small example.

Also, because you don’t reset ‘direc’ when you stop pressing a key, the
direc flag will remain set, and your ‘object’ will keep travelling in the
direction that was pressed last.
Ah, what stupid! I knew i was missing something.

[snip]

//Ahora movemos la imagen de acuerdo a la direccion
switch (direc){
case UP:
if (area.y>1) area.y–;

This code is not timed, and will execute each loop. This means that if you
have a very fast videocard (or refresh rate) the ‘object’ will travel a lot
faster than on a slower machine/refresh rate. You might want to do
something like:
Yes, but fixed framerate is planned maybe for a “future” article.

Hope this helps!
It helps a lot, thanks.On Sun, 5 Jan 2003, M. Egmond wrote:

–
Roger D. Vargas | El sistema se apagara en 5 segundos.
ICQ: 117641572 | Salvese el que pueda!
Linux User: 180787 |

Nicolai_Haehnle · January 6, 2003, 8:17am

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1On Monday 06 January 2003 16:50, Roger D. Vargas wrote:

On Sat, 4 Jan 2003, Matt Borkowski wrote:

In the loadimg() function you do
tmp=SDL_LoadBMP(“sail.bmp”);
without first freeing tmp from when you loaded
"backg.bmp".

Does it matters? The example runs without segfaults.

Yes, it does matter. If you don’t free a surface, you’ll get a memory leak,
i.e. the memory used up by the earlier bitmap “backg.bmp” will remain
allocated. It will only be reclaimed by the OS once your program ends.

Memory leaks are obviously bad because you might run out of memory simply
because to many no-longer-used but still allocated blocks of memory are
around.

cu,
Nicolai
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.1 (GNU/Linux)

iD8DBQE+GayTsxPozBga0lwRAskJAJ0UYATdZz5EEEUu81eTy6HiKECMqwCdHZrN
MCDPxySF03lLSQsACMDBr2Y=
=+E8/
-----END PGP SIGNATURE-----