[SDL] Flashing window
justin
kokotusu at hotmail.com
Mon Aug 20 14:34:04 PDT 2007
Jason <jesus_freak <at> picknowl.com.au> writes:
>
> justin <kokotusu <at> hotmail.com> writes:
>
> > <snipped>
>
> Hi Justin,
>
> I've taken a look at your code. I hate to be a bit harsh, but your code has a
> number of C++ programming and logic errors. Throughout your code here, I've
put
> a number of comments. I'd suggest you try fixing them and seeing how it goes.
>
> >
> > #include "SDL/SDL.h"
> > #include "SDL/SDL_image.h"
> > #include<iostream>
> > #include<string>
> > using namespace std;
> >
> > #define SCREENWIDTH 160
> >
> > #define SCREENHEIGHT 240
> >
> > #ifdef main
> > #undef main
> > #endif
> >
> First thing, I'd scrub this #undef main stuff. You aren't doing anything
tricky
> with stdout or stderr, so I'd leave it alone.
>
> > int const Size = 16;
> >
> > class Rocket
> > {
> > SDL_Surface *Picture;
> > SDL_Surface *Optimize;
> > int dx;
> > int dy;
> > public:
> > Rocket();
> > ~Rocket();
> > SDL_Surface * LoadPicture(std::string filename);
> > void move( int x, int y);
> > int GetX();
> > int GetY();
> > };
>
> This class seems a bit pointless. Internally, you are storing a pointer to
the
> SDL_Surface which is the optimised picture, but then you use an external
pointer
> to render that surface anyway. Plus, the Picture member doesn't need to be
there
> as it is only temporary to one function. Here's a better class for you:
>
> class Rocket {
> SDL_Surface * _picture;
> int _dx;
> int _dy;
> public:
> Rocket();
> virtual ~Rocket();
> SDL_Surface * Picture();
> bool LoadPicture(std::string & filename);
> void Move(int x, int y);
> int GetX();
> int GetY();
> };
>
> A few things:
> 1. declaring the constructor virtual means you can subclass Rocket.
> 2. Instead of using SDL_Surface * user = rocket.LoadPicture(<whatever>) and
then
> using user, just do a rocket.LoadPicture(<whatever>) and *check the return
> value*. Then, use rocket.Picture() instead of user.
> 3. when passing std::string objects as arguments, always pass by reference,
> otherwise you could be passing a massive string and causing a stack overflow.
> 4. use a consistent naming method of functions and data members. If you are
> giving function names initial capitals, then be consistent. Trust me, it
helps.
>
> > <snipped>
> >
> > SDL_Surface * Rocket::LoadPicture(std::string filename)
> The argument here needs to be changed to match the class (i.e. std::string &
> filename)
> > {
> > Picture = IMG_Load(filename.c_str());
> Picture should be a temporary, so the above line needs to be:
> SDL_Surface * Picture - IMG_Load(filename.c_str());
>
> > <snipped>
>
> > int main( int argc, char* args[] )
> > {
> > GameInit();
>
> *Check your return value and exit if appropriate!!!!!*
>
> > bool quit = false;
> >
> > while( quit != true)
> > {
> > GameLoop();
> > while( SDL_PollEvent( &event ) )
>
> I'd be careful with this kind of nested loop. You could end up in the
situation
> where all the game does is keep handling events off the queue without
actually
> updating the screen. I'd be thinking of adding a limiter so you only process
a
> certain number of events per frame. Others on the group may have a different
> opinion, but I'll let them speak for themselves...
>
> > { switch(event.type)
> > {case SDL_KEYDOWN:
> > { switch(event.key.keysym.sym)
> > {
> > case SDLK_UP:
> > {
> These braces here are redundant. You only need to add braces in a case's
> statement if you are declaring variables. OK, this is just a style issue,
not a
> problem!
> > rocket.move(0,-2);
> > position = 0;
> > }
> > break;
> > case SDLK_DOWN:
> > {
> > rocket.move(2,0);
> > position = 1;
> > }
> > break;
> > case SDLK_RIGHT:
> > {
> > rocket.move(2,0);
> > position = 2;
> > }
> > break;
> > case SDLK_LEFT:
> > {
> > rocket.move(-2,0);
> > position = 3;
> > }
> > break;
> > }// end of key switch
> > }break;
> >
> > case SDL_QUIT:
> > quit = true;
> > break;
> > }// end of event switch
> >
> > }
> >
> > }
> > GameDone();
> > SDL_Quit();
> > return 0;
> > }//end of main
> >
> > void DrawMap( int sprite)
> > {
> > //takes our previous movement's rect
> > if( GAMESTARTED == true)
> > {
> > SDL_Rect PreRect; //destination postioning
> > SDL_Rect PreSRCRect; //which part of our picture to put into the game
> > PreRect.x = pre_x;
> > PreRect.y = pre_y;
> > PreRect.w = Size;
> > PreRect.h = Size;
> > PreSRCRect.x = 0;
> > PreSRCRect.y = 16 * 4;;
> > PreSRCRect.w = Size;
> > PreSRCRect.h = Size;
> > SDL_BlitSurface(user,&PreSRCRect,background,&PreRect);
> > }
>
> Is there meant to be an else in here? Otherwise what comes below will
> potentially override what you've got above.
>
> Something that will make your code much faster: declare your rectangles as
> static, and initialise them once. Then, all you need to do is update them.
>
> Even better, make your rectangles members of the Rocket class, and have a
Draw
> function in which you pass in a pointer to an SDL_Surface - which is the
> destination you want to blit to.
>
> > //plots current position
> > SDL_Rect Rect;//destination postioning
> > SDL_Rect SRCRect;//which part of our picture to put into the game
> > Rect.x = X;
> > Rect.y = Y;
> > Rect.w = Size;
> > Rect.h = Size;
> > SRCRect.x = sprite * Size;
> > SRCRect.y = sprite * Size;
> > SRCRect.w = Size;
> > SRCRect.h = Size;
> >
> > SDL_BlitSurface(user,&SRCRect,background,&Rect);
>
> You do realise that this permanently modifies your background object? I'd
> suggest you blit your background to the screen buffer, then blit the rocket
over
> the top, also to the screen buffer. That way, you don't have to re-load your
> background image every frame.
>
> > SDL_BlitSurface(background,NULL,screen,NULL);
> > }
> >
> > int GameInit()
> > {
> > if( SDL_Init(SDL_INIT_EVERYTHING) != 0 );
>
> Remove the trailing ;.
>
> > return 1;
> >
> > <snipped>
> >
> > void Collision()
> > {
> > if( X > SCREENWIDTH)
> > XMovement =-8 * 2;
> > position = 3;
>
> These need to have braces, otherwise position *always* ends up as 2 at the
end
> of this function.
>
> > if(X < 0 )
> > XMovement = 8;
> > position = 2;
> > if( Y > SCREENHEIGHT)
> > YMovement = -8 * 2;
> > position = 3;
> > if(Y < 0 )
> > YMovement = 8;
> > position = 2;
> > }
> >
> > void GameDone()
> > {
> >
> >
> >
> > SDL_FreeSurface(background);
> >
> > SDL_FreeSurface(user);
> >
> > SDL_Surface * background = NULL;
>
> This and all the following statements in this function do nothing. They
declare
> local variables which actually hide your globals. Besides which, setting a
> pointer to 0 when it is never going to be used again is, if you'll pardon the
> pun, pointless. Especially when the value of the pointer is never checked...
>
> > SDL_Surface * user=NULL;
> >
> > int X=0;
> >
> > int Y=0;
> >
> > int pre_x=0;
> >
> > int pre_y=0;
> >
> > int position=0;
> >
> > Uint32 StartTime=0;
> >
> > Uint32 Timediffer=0;
> >
> > int XMovement=0;
> >
> > int YMovement=0;
> > }
> >
> > i suspect it has something to do with my gameInit but i would liek advice
in
> > this thought....
> >
> May I ask how long you've been using C++? If it isn't very long, can I
strongly
> suggest to you that you find yourself a good book on C++ basics. If you've
got
> questions about my suggestions, you'll find the answers in any of them.
> Unfortunately, I can't think of any books offhand, but a Google search or a
look
> on Amazon.com amongst other things will turn something up.
>
> -J
>
thanks for the advice, yet im still clusmy in C++ yes but i rewrote the
program a before your quote soo it looks like this
#ifndef GAMEFUNCTIONS
#define GAMEFUNCTIONS
#include "SDL/SDL.h"
#include "SDL/SDL_image.h"
#include<string>
class DemoGame
{
int X;
int Y;
SDL_Surface * screen;
SDL_Surface * background;
SDL_Surface * user;
SDL_Event event;
Uint32 timerstart;
Uint32 timerdiff;
int movement;
int position;
public:
DemoGame();
~DemoGame();
SDL_Surface * LoadPicture(std::string filename );
void DrawMap();
void Collision();
void GameInit();
void GameLoop();
void GameDone();
};
#endif
DemoGame::DemoGame()
{
X =0;
Y =0;
screen=NULL;
background=NULL;
user=NULL;
movement=2;
position = 0;
}
DemoGame::~DemoGame()
{
GameDone();
}
SDL_Surface * DemoGame::LoadPicture(std::string filename )
{
SDL_Surface *Picture;
SDL_Surface *Optimized;
Picture = IMG_Load(filename.c_str());
Optimized = SDL_DisplayFormat(Picture);
return Optimized;
}
void DemoGame::DrawMap()
{
SDL_BlitSurface(background,NULL,screen,NULL);
//plots current position
SDL_Rect Rect;//destination postioning
SDL_Rect SRCRect;//which part of our picture to put into the game
Rect.x = X;
Rect.y = Y;
Rect.w = 16;
Rect.h = 16;
SRCRect.x = position * 16;
SRCRect.y = position * 16;
SRCRect.w = 16;
SRCRect.h = 16;
SDL_BlitSurface(user,&SRCRect,screen,&Rect);
SDL_Flip( screen );
}
void DemoGame::Collision()
{
if( X > 160 - 16 )
{
X = 160 - 16;
position = 3;
}
if( X < 0 )
{
X = 0;
position = 2;
}
if(Y > 240 - 16);
{
Y = 240 - 16;
position = 0;
}
if(Y < 0 )
{
Y = 0;
position = 1;
}
}
void DemoGame::GameInit()
{
if(SDL_Init(SDL_INIT_VIDEO) < 0)
return;
screen = SDL_SetVideoMode(160,240,32,SDL_SWSURFACE );
if(screen ==NULL)
return;
timerstart = SDL_GetTicks();
SDL_ShowCursor(SDL_DISABLE);
X = 160 / 2;
Y = 240 / 2;
SDL_WM_SetCaption("DemoGame",NULL);
background = LoadPicture("background.bmp");
user = LoadPicture("ship.bmp");
DrawMap();
}
void DemoGame::GameLoop()
{
bool quit = false;
while(quit ==false)
{
float Spf;
timerdiff = SDL_GetTicks() - timerstart;
Spf = (float)(timerdiff/1000.0f);
movement = (int)( 2/ (1/Spf ));
Uint8 *keystates = SDL_GetKeyState( NULL );
while( SDL_PollEvent( &event ) )
{
switch(event.type)
{
case SDL_KEYDOWN:
if( keystates[ SDLK_UP ] )
{
Y += movement;
position = 0;
}
if( keystates[ SDLK_DOWN ] )
{
Y -= movement;
position = 1;
}
if( keystates[ SDLK_LEFT ] )
{
X -=movement;
position = 2;
}
if( keystates[ SDLK_RIGHT ] )
{
X +=movement;
position = 3;
}
break;
case SDL_QUIT:
quit = true;
break;
}
}
if(timerdiff > 10000)
timerstart = SDL_GetTicks();
Collision();
DrawMap();
}
}
void DemoGame::GameDone()
{
X =0;
Y =0;
SDL_FreeSurface(user);
SDL_FreeSurface(background);
user=NULL;
background=NULL;
movement=0;
position = 0;
SDL_Quit();
}
A new problem now is that it does show the exact position of each picture i
intent on, and it onl move left or right..The only real thing i can argue
about is that loadpicture is use to load for two Surfaces, and ur suggestion
would work for one.
More information about the SDL
mailing list