Sign in to follow this  

[SDL] Runtime error when having greater than 1 objects in vector

This topic is 3587 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Hi, Everything in the code below works fine as it is.
std::vector <Fruit> fruits;
    fruits.push_back( Fruit(400, 40, APPLE) );
    
    // create a player
    Player player( SCREEN_WIDTH / 2 - player.ReturnWidth() / 2, SCREEN_HEIGHT / 2 - player.ReturnHeight() / 2 ); 

    SDL_Delay( 1000 );
    
    // a variable that is increased in the main loop to make objects that are falling fall faster
    int acceleration = 0;
    
    // THE MAIN LOOP
    bool exit = false;
    while( exit == false )
    {
         // if an event has occured
         if( SDL_PollEvent( &event ) ) 
         {
             if( event.type == SDL_KEYDOWN )
             {
                 switch( event.key.keysym.sym )
                 {
                 case SDLK_ESCAPE: 
                      exit = true; 
                      break;
                 case SDLK_LEFT:
                      player.MoveLeft(10);
                      break;
                 case SDLK_RIGHT:
                      player.MoveRight(10);
                      break;
                 case SDLK_UP:
                      player.MoveUp(10);
                      break;
                 case SDLK_DOWN:
                      player.MoveDown(10);
                      break;
                 }
                 applySurface( player.x, player.y, player.ReturnImage(), screen );
             }
         }
         else
         {
             int i = 1;
             int lastInc = 0;
             bool stopLooping = false;
             
             while( (i < 200) && (!stopLooping) )
             {
                 acceleration ++;
                 
                 for( int j = 0; j < fruits.size(); j++ )
                 {    
                      if( fruits[j].y < SCREEN_HEIGHT )   
                      {
                          fruits[j].y += acceleration;
                      }
                      
                      if( j == fruits.size() - 1 ) // if we are checking the last element
                      {
                          stopLooping = true; // the 'while' loop will end when the scope finishes
                      }
                 }
                     
                 applySurface( 0, 0, background, screen );
                 applySurface( player.x, player.y, player.ReturnImage(), screen );
                 drawFruits( fruits );
                 
                 if( SDL_Flip( screen ) == -1 )
                     return 1;
             
                 if(clock() - lastInc > 1)
                 {
                     i++;
                     lastInc = clock();
                 }
             }
             
              if( SDL_Flip( screen ) == -1)
                  return 1;
         }
    } //END OF THE MAIN LOOP  


There is more code, but that's where I think the problem is. So, as you can see, I'm adding a Fruit object in the vector and enter the main loop. The object will fall down the screen. This is what it's intended to do. But if I have two objects in the vector, fruits.push_back( Fruit(400, 40, APPLE) ); fruits.push_back( Fruit(100, 40, APPLE) ); Only one image will be drawn (which has the coordinates of the first), the colorkey doesn't work (the background of the image goes yellow (it was turqoise)) and I get this runtime error. http://img81.imageshack.us/img81/8409/errordc3.jpg This is the Fruit class:
class Fruit
{
public:
   
   Fruit( int x = 0, int y = 0, const int type = 0 );
   ~Fruit();
   Fruit( const Fruit& fruit);

   SDL_Surface* ReturnImage() const;
   int ReturnWidth() const;
   int ReturnHeight() const;
   
   int x, y;
   
protected:
   SDL_Surface* m_Image;
    
   int m_Width, m_Height;
   
private:
   int m_Type;
};

//in another file
#include "SDL/SDL.h"

#include "declarations.h"

Fruit::Fruit( int x, int y, const int type ): x(x), y(y), m_Type(type)
{
     m_Image = new SDL_Surface;
     
     if( type == APPLE )
     {
         m_Image = loadImage( "apple.png" );
         m_Width = 37;
         m_Height = 48;
     }
     /*else if( type == ORANGE ) not ready yet
     {
          m_Image = loadImage( "orange.bmp" );
     }*/
}

Fruit::~Fruit()
{
     SDL_FreeSurface(m_Image);
}

Fruit::Fruit( const Fruit& fruit )
{
     m_Image = new SDL_Surface( *(fruit.m_Image) );
     
     x = fruit.x;
     y = fruit.y;
     m_Width = fruit.m_Width;
     m_Height = fruit.m_Height;
     
     m_Type = fruit.m_Type;
}

SDL_Surface* Fruit::ReturnImage() const
{
    return m_Image;
}

int Fruit::ReturnWidth() const
{
    return m_Width;
}

int Fruit::ReturnHeight() const
{
    return m_Height;
}


and the function used to apply 'fruits' on the screen
void drawFruits( const std::vector <Fruit>& fruits )
{
     if( !fruits.empty() )
    {
        for( std::vector <Fruit>::const_iterator iter = fruits.begin(); iter < fruits.end(); ++iter )
             applySurface( iter->x, iter->y, iter->ReturnImage(), SDL_GetVideoSurface() );
    }
}


Thanks for your help.

Share this post


Link to post
Share on other sites
Your copy constructor is incorrect. To make a deep copy of a SDL_Surface you would need to create a new one using SDL_CreateRGBSurface() and blit the old to the new.

Here is an alternative way of implementing your fruit class:

// helper function
#if SDL_BYTEORDER == SDL_BIG_ENDIAN
const Uint32 rmask = 0xff000000;
const Uint32 gmask = 0x00ff0000;
const Uint32 bmask = 0x0000ff00;
const Uint32 amask = 0x000000ff;
#else
const Uint32 rmask = 0x000000ff;
const Uint32 gmask = 0x0000ff00;
const Uint32 bmask = 0x00ff0000;
const Uint32 amask = 0xff000000;
#endif

SDL_Surface *createSurface(unsigned w, unsigned h)
{
SDL_Surface *temp = SDL_CreateRGBSurface(SDL_SWSURFACE,w,h,32,rmask,gmask,bmask,amask);
if(!temp)
{
throw std::runtime(SDL_GetError());
}
return temp;
}

// remove the m_Width and m_Height from the class
Fruit::Fruit( int x, int y, const int type ): x(x), y(y), m_Type(type)
{
// don't do this, it is a memory leak
// m_Image = new SDL_Surface;

if( type == APPLE )
{
m_Image = loadImage( "apple.png" );
//m_Width = 37;
//m_Height = 48;
}
/*else if( type == ORANGE ) not ready yet
{
m_Image = loadImage( "orange.bmp" );
}*/

}

Fruit::~Fruit()
{
SDL_FreeSurface(m_Image);
}

Fruit::Fruit( const Fruit& fruit )
{
m_Image = createSurface(fruit.ReturnWidth(),fruit.ReturnHeight());
SDL_BlitSurface(fruit.m_Image,NULL,m_Image,NULL);

x = fruit.x;
y = fruit.y;

m_Type = fruit.m_Type;
}

SDL_Surface* Fruit::ReturnImage() const
{
return m_Image;
}

int Fruit::ReturnWidth() const
{
return m_Image->w;
}

int Fruit::ReturnHeight() const
{
return m_Image->h;
}





You should also implement operator=, or disable it by declaring it private and not implementing it. Finally, I must question your use or "Return" as part of the function names, surely fruit.Width() is just as meaningful as fruit.ReturnWidth().

[Edited by - rip-off on February 15, 2008 11:43:17 AM]

Share this post


Link to post
Share on other sites

while( (i < 200) && (!stopLooping) )
{
acceleration ++;

for( int j = 0; j < fruits.size(); j++ )
{
if( fruits[j].y < SCREEN_HEIGHT )
{
fruits[j].y += acceleration;
}

if( j == fruits.size() - 1 ) // if we are checking the last element
{
stopLooping = true; // the 'while' loop will end when the scope finishes
}
}


What are you really trying to do here? When you nest loops, the inner loop normally runs completely for each iteration of the outer loop. The first time you enter the while loop, 'i' is 1, then you run the for loop with 'j' iterating over the entire container; of course it will eventually hit 'fruits.size() - 1', so the while loop will always only run once. The fact that you are also checking 'i' against 200 (an arbitrary value?) suggests that you are very confused.

You don't really want a loop there; the actual loop that keeps things running is the outer-outer one, checking 'while (exit == false)' (BTW, you can write that more simply as 'while (!exit)'). You want to wait for some amount of time (two clock ticks, apparently, since you check for a difference that is greater than 1) each time through... your code doesn't do that, because the check against lastInc is only made once.

You should have one main game loop, with updates occurring at specified times. This is easier to do if you just put function calls into the main game loop to represent each of those steps, and then pull the real work into those functions.

Those functions would be "update" (move stuff), "render" (draw them) and "process_input" (respond to key presses). Notice: don't directly move the player in response to a key press, because that confuses responsibilities. It also messes up timing, because we want the player's movement speed to be tied to the update speed, not the event processing speed.

So, first we write the loop:


// Our process_input needs to be able to tell the update how to move the
// player. What we'll do is imagine some kind of "input state" object, which
// process_input changes, and update checks. It needs "dx" and "dy" values
// (to indicate the player velocity), and an "exit" flag.

input_state state;
// the constructor initializes 'exit' to false and 'dx' and 'dy' to 0
int lastInc = clock();
while (!state.exit) {
// We need to accept the "state" by reference, so that we can change it.
// Alternatively, we could make process_input be a member function of
// the object :)
process_input(state);

// We use the timing variables to decide whether we will update this time.
if (clock() - lastInc > 1) {
lastInc = clock();
// Pass along everything that "update" needs to know about:
update(state, player, fruits);
}

// Finally, we draw everything.
render(player, fruits);

// The check for a failed SDL_Flip needs to stay here in order to exit
// the main loop.
if (SDL_Flip(screen) == -1)
return 1;
}
}


And now we can figure out what those functions look like. I'll do 'render' to start you off.


// Notice "const". Drawing something shouldn't change its data!
void render(const Player& player, const std::vector<Fruit>& fruits) {
applySurface(0, 0, background, screen);
// Not like this:
// applySurface(player.x, player.y, player.ReturnImage(), screen);
// But like this:
player.drawTo(screen); // Tell, don't ask. The player tells SDL where it is
// and what it looks like.
drawFruits(fruits);
}



Try to imagine how the others would look. Hint: Don't use four separate functions to move the player. Instead, have one function 'move(int dx, int dy)'. Start by creating the input_state structure.

Share this post


Link to post
Share on other sites
Quote:
Original post by ZahlmanThe fact that you are also checking 'i' against 200 (an arbitrary value?) suggests that you are very confused.


First, thank you for your help.

I have to admit that I'm confused. Are there some tutorials or books that can help me understand those concepts better? (not C++, not SDL, but what you talked about, getting input, then updating and such).

Edit: Or should I use a game engine to make things simpler?

[Edited by - sheep19 on February 15, 2008 5:03:11 PM]

Share this post


Link to post
Share on other sites
Quote:
Original post by rip-off
Your copy constructor is incorrect. To make a deep copy of a SDL_Surface you would need to create a new one using SDL_CreateRGBSurface() and blit the old to the new.

Here is an alternative way of implementing your fruit class:
*** Source Snippet Removed ***

You should also implement operator=, or disable it by declaring it private and not implementing it. Finally, I must question your use or "Return" as part of the function names, surely fruit.Width() is just as meaningful as fruit.ReturnWidth().


1.Why am I producing a memory leak? Isn't this the way to add memory on the heap?
2.If I don't add the image on the heap the program will not work because I'm using a vector to store the objects and they will go out of scope. Should I use a vector of pointers instead?

That's the vector I'm using: std::vector< Fruit > fruits;
fruits.push_back( Fruit(400, 40, APPLE) );

Share this post


Link to post
Share on other sites
1) The memory leak is because you create a new SDL_Surface instance, and never delete it. But, the point is that you should never use new on a SDL_Surface, because it is a complex C type. That means that it has a number of internal fields that need setting. This is why we ask SDL to allocate us a surface, using SDL_LoadBMP or SDL_CreateRGBSurface. SDL cannot correctly initialise a surface created by new because it is a C library, hence the SDL_Surface type lacks a constructor.


2) This problem is different, you haven't correctly followed the rule of three. As I mentioned, your copy constructor is at fault.

Consider your original fruit type. When you push an anonymous instance into the vector, the following code is run:

Fruit(int,int,int) // <-- creation of temporary to pass to vector
Fruit(const Fruit &) // <-- vector creates a copy
~Fruit() // <-- temporary is *destroyed*






Now, what happens is that Fruit constructor uses SDL_LoadBMP to obtain a surface pointer from SDL. The copy constructor does a kindof-deep yet kindof-shallow copy. However, it breaks several rules. Firstly, it allocates a SDL_Surface with new. However, the destructor does not use delete - it uses SDL_FreeSurface. Secondly, it neglects the fact that SDL_Surfaces have members which are themselves pointers. Here is the SDL_Surface structure, from SDL_video.h:

typedef struct SDL_Surface {
Uint32 flags; /* Read-only */
SDL_PixelFormat *format; /* Read-only */
int w, h; /* Read-only */
Uint16 pitch; /* Read-only */
void *pixels; /* Read-write */
int offset; /* Private */

/* Hardware-specific surface info */
struct private_hwdata *hwdata;

/* clipping information */
SDL_Rect clip_rect; /* Read-only */
Uint32 unused1; /* for binary compatibility */

/* Allow recursive locks */
Uint32 locked; /* Private */

/* info for fast blit mapping to other surfaces */
struct SDL_BlitMap *map; /* Private */

/* format version, bumped at every change to invalidate blit maps */
unsigned int format_version; /* Private */

/* Reference count -- used when freeing surface */
int refcount; /* Read-mostly */
} SDL_Surface;






Here we can see four pointer members, the pixels, the format, the private_hwdata and the map. These are all shallow copied by your code.

Note: If you were dealing with a C++ type (which had the appropriate constructor, copy constructor and destructor) your code would be 100% correct! That is how one deep copies C++ types (although in that case we would generally store the type by value, or in a smart pointer). You had the right idea, its just you are interfacing with C now so you need to think differently.

Anyway, now the temporary's destructor is called. Here is your problem, the SDL_FreeSurface function will deallocate not only the surface itself, but all those members you made shallow copies of. Disaster! Your instance of Fruit in the vector contains bad pointers!

By using SDL_CreateRGBSurface (neatly wrapped for ease of use), we guarentee that there are two independent surfaces. Then, when the temporary goes out of scope, the copy in the vector still had a good pointer.

Of course, this is inefficient. A SDL_Surface of some size takes up (compared to the rest of the Fruit type) quite a lot of RAM and is expensive to copy.

I hope this demonstrates the error, and now you can look at the sample code I gave you and see how my version avoids these errors.

Share this post


Link to post
Share on other sites
I've made the changes, but I still get the runtime error. (The image doesn't show up as well, even with one object in. I've changed the main loop by the way.)

This is what the class has become:

public:

Fruit( int x = 0, int y = 0, const int type = 0 );
~Fruit();
Fruit( const Fruit& fruit);

SDL_Surface* ReturnImage() const;
int Width() const;
int Height() const;

void Fruit::drawTo( unsigned int x = 0, unsigned int y = 0, SDL_Surface* const destination = SDL_GetVideoSurface() );

int x, y;

protected:
SDL_Surface* m_Image;

SDL_Surface* CreateSurface( unsigned int w, unsigned int h );

private:
int m_Type;
};

#include "SDL/SDL.h"

#include "declarations.h"

// helper function
#if SDL_BYTEORDER == SDL_BIG_ENDIAN
const Uint32 rmask = 0xff000000;
const Uint32 gmask = 0x00ff0000;
const Uint32 bmask = 0x0000ff00;
const Uint32 amask = 0x000000ff;
#else
const Uint32 rmask = 0x000000ff;
const Uint32 gmask = 0x0000ff00;
const Uint32 bmask = 0x00ff0000;
const Uint32 amask = 0xff000000;
#endif

Fruit::Fruit( int x, int y, const int type ): x(x), y(y), m_Type(type)
{
if( type == APPLE )
m_Image = loadImage( "apple.png" );

else if( type == ORANGE )
m_Image = loadImage( "orange.png" );
}

Fruit::~Fruit()
{
SDL_FreeSurface(m_Image);
}

Fruit::Fruit( const Fruit& fruit )
{
m_Image = CreateSurface( fruit.Width(), fruit.Height() );

x = fruit.x;
y = fruit.y;

m_Type = fruit.m_Type;
}

SDL_Surface* Fruit::CreateSurface(unsigned int w, unsigned int h)
{
SDL_Surface* temp = SDL_CreateRGBSurface( SDL_FULLSCREEN, w, h, 32, rmask, gmask, bmask, amask);

if( !temp )
fprintf( stderr, "Error creating SDL_Surface in copy constructor.\n" );

return temp;
}

SDL_Surface* Fruit::ReturnImage() const
{
return m_Image;
}

int Fruit::Width() const
{
return m_Image->w;
}

int Fruit::Height() const
{
return m_Image->h;
}

void Fruit::drawTo( unsigned int x, unsigned int y, SDL_Surface* const destination )
{
applySurface( x, y, m_Image, destination );
}



What is still wrong?

PS: I should know and avoid C.

Share this post


Link to post
Share on other sites
Quote:
Original post by sheep19
I've made the changes, but I still get the runtime error. (The image doesn't show up as well, even with one object in. I've changed the main loop by the way.)


You left out the SDL_BlitSurface line in the Fruit copy constructor from my sample code. SDL_CreateRGBSurface returns a blank surface (I think it makes it all black by default). You need to copy the pixel data across onto the new surface, that is why that line needs to be there.

Quote:

This is what the class has become:

Well, you don't need to pass the x and y positions of a Fruit to its draw function, they are member variables, so you can drop the parameters. Also, I would keep CreateSurface as a free function. It can be used potentially in lots of places. Don't pass SDL_FULLSCREEN to SDL_CreateRGBSurface, that flag only makes sense to SDL_SetVideoMode.

Quote:

What is still wrong?


The error you are getting would be pretty easy to find in a debugger. Try some of the techniques outlined in this article.

Quote:
PS: I should know and avoid C.

It isn't about avoiding C. My statement about C was supposed to indicate that C uses different idioms than C++. In C++, we can use the additional language features to write copy constructors etc to simplify what you wanted to do. In C however, member functions in general do not exist, and there are no constructors, copy constructors or destructors, so a C library that needs these operations will implement them as functions.

If you are using C++, the chances are that you will need or want to interface with C, so you need to be aware of how to do so.

Share this post


Link to post
Share on other sites

This topic is 3587 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this