[SDL] Flashing window

[Jason]

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