Sign in to follow this  
RonnieCassinello

C++ / Putting a class that has an SDL_Surface* member in a vector

Recommended Posts

Hello all,

For nearly two days now I've googled and tried so many different approaches to getting this to work, but still with no luck. Basically, I would like to have a very simple class (or a struct if I really have to) that contains an SDL_Surface*, and then have a std::vector to put them in. The problem comes that no matter how 'safe' I think my approach or class is, as soon as I try to put it in a vector, I get a seg fault.

Background:
In making a simple 2D game, I prefer to have a limited number of SDL_Surface's, each containing multiple sprites, with a vector of SDL_Rect's to define each particular sprite clip. This worked great when I was taking a structured approach, I would simply use something like this as global variables:

[code]vector<SDL_Surface*> Graphics; // Vector of my graphics
vector< vector<SDL_Rect> > Clipping; // 2D vector of clipping information
vector< vector<bool> > ClipRegister; // 2D vector of bools to tell me if a particular member of Clipping had be defined[/code]

The downside is that the game itself had to handle the loading and unloading of graphics, and I wanted to get away from this and put all the graphics functions in a class, and let the game get on with just being a game. It would obviously also have the advantage of being reusable in for other games. This all part of a larger project to make a single class that encapsulates all my graphic needs safely.

So step one towards this was to make a simple class (I'm learning as I go and didn't want to over-stretch myself):

[code]class GrfImage
{
public:
SDL_Surface* Graphics;
vector<SDL_Rect> Clipping;
vector<bool> ClipRegister;
};[/code]

This obviously goes against all the principles of encapsulation, but my plan is to implement this as my confidence grows.

But here I got my first problem. Using a one-off 'GrfImage', things worked fine with this temporary hack, however as soon as I made a vector of them (e.g., vector<GrfImage> Images;) I got seg faults whenever I used SDL_FreeSurface(Image[i].Graphics);, even having ensured that an image had been loaded and that the index into the vector was legal. So I thought "Well I'm clearly not managing the memory properly", and pushed on to an encapsulated version:

Header file:
[code]class GrfImage
{
public:
GrfImage();
GrfImage(const GrfImage &Original);
GrfImage& operator= (GrfImage const &Original);
~GrfImage();
bool ImgLoadBMP(string FName, SDL_Color Key);
bool ImgLoad(string FName, SDL_Color Key);
bool ImgBlitBMP(short x, short y, SDL_Surface* Dest);
bool ImgBlit(short x, short y, unsigned short i, SDL_Surface* Dest);

private:
SDL_Surface* Grf;
bool Loaded;
vector<SDL_Rect> GrfClip;
vector<bool> GrfClipReg;
};[/code]

Source file:
[code]#include "grfio.h"

using namespace std;


GrfImage::GrfImage()
{
Grf = NULL;
Loaded = false;
GrfClip.resize(0);
GrfClipReg.resize(0);
}



GrfImage::GrfImage(const GrfImage &Original)
{
*this = Original;
};



GrfImage& GrfImage::operator= (GrfImage const &Original)
{
if (Loaded) SDL_FreeSurface(Grf);
if (Original.Loaded)
{
Grf = SDL_DisplayFormat(Original.Grf);
Loaded = true;
}
else
{
Grf = NULL;
Loaded = false;
}
GrfClip = Original.GrfClip;
GrfClipReg = Original.GrfClipReg;
return *this;
}



GrfImage::~GrfImage()
{
if (Loaded) SDL_FreeSurface(Grf);
};



bool GrfImage::ImgLoadBMP(string FName, SDL_Color Key)
{
if (Loaded)
{
SDL_FreeSurface(Grf);
Grf = NULL;
}
SDL_Surface* TempGrf = NULL;
TempGrf = SDL_LoadBMP(FName.c_str());
if(TempGrf == NULL ) return true;
Grf = SDL_DisplayFormat(TempGrf);
SDL_FreeSurface(TempGrf);
if(Grf == NULL ) return true;
SDL_SetColorKey(Grf, SDL_RLEACCEL | SDL_SRCCOLORKEY, SDL_MapRGB(Grf->format, Key.r, Key.g, Key.b));
Loaded = true;
return false;
}


bool GrfImage::ImgLoad(string FName, SDL_Color Key)
{
... EXTPANDED VERSION OF ABOVE THAT LOADS CLIPPING TOO...
}



bool GrfImage::ImgBlitBMP(short x, short y, SDL_Surface* Dest)
{
if (Loaded)
{
SDL_Rect Offset;
Offset.x = x;
Offset.y = y;
if (SDL_BlitSurface(Grf, NULL, Dest, &Offset) == 0) return false;
}
return true;
}



bool GrfImage::ImgBlit(short x, short y, unsigned short i, SDL_Surface* Dest)
{
if ((Loaded) and
(i < GrfClip.size()) and
(GrfClipReg[i]))
{
SDL_Rect Offset;
Offset.x = x;
Offset.y = y;
if (SDL_BlitSurface(Grf, &GrfClip[i], Dest, &Offset) == 0) return false;
}
return true;
}[/code]

I'd thought I'd covered all bases with this - the class should comprehensively manage any access to the SDL_Surface pointer, as well as safely copying the surface rather than just copying the pointer. But now if I have in my main program source file:

[code]vector<GrfImage> MyImg;
while (MyImg.size() < 7)
{
GrfImage MyTempImg;
MyTempImg.ImgLoadBMP("graphics.bmp", C_Red); // 'C_Red' is a predefined SDL_Color
MyImg.push_back(MyTempImg);
}[/code]

...I get a seg fault (The 7 is arbritrary, and the whole routine is technically pointless as it just loads the same BMP 7 times).

At my wit's end, I tried:

[code]GrfImage MyImg[7];
int i = 0;
while (i < 7)
{
GrfImage MyTempImg;
MyTempImg.ImgLoadBMP("graphics.bmp", C_Red);
MyImg[i] = MyTempImg;
i++;
}[/code]

...And everything went fine. I also tried:

[code]GrfImage Img1;
if(1)
{
GrfImage Img2;
Img2.ImgLoadBMP("graphics.bmp", C_Red);
Img1 = Img2;
}
Img1.ImgBlitBMP(0,0,Screen);[/code]

...And everything went fine too (this was to test that operator= was working as I thought, as Img2 should destruct when it goes out of scope, shouldn't it?)

I'm metaphorically tearing my hair out over this one. Why can't I vectorise my class? What fundamental thing have I completely missed that is causing this?!

Any pointers gratefully received.

Ronnie

Share this post


Link to post
Share on other sites
SiCrane    11839
Your class doesn't follow the [url=http://en.wikipedia.org/wiki/Rule_of_three_(C%2B%2B_programming)] rule of three[/url]. The easiest way to modify your code would be to change from a simple SDL_Surface pointer to a smart pointer to an SDL_Surface. Using a boost::shared_ptr with SDL_FreeSurface as the clean up function would be one way (or std::/std::tr1:: shared_ptr if your compiler supports one of those).

Share this post


Link to post
Share on other sites
BeerNutts    4400
My guess is, when you are creating the Grf, you are doing it with a stack variable and adding it to the vector. Once that variable goes out of scope, the class with be destructed. Make sure it's a pointer, and do something like this:

[code]
vector<GrfImage *> MyImg;
while (MyImg.size() < 7)
{
GrfImage *MyTempImg;
MyTempImg = new GrfImage();
MyTempImg->ImgLoadBMP("graphics.bmp", C_Red); // 'C_Red' is a predefined SDL_Color
MyImg.push_back(MyTempImg);
}
[/code]
[quote name='RonnieCassinello' timestamp='1310492215' post='4834403']
Hello all,

For nearly two days now I've googled and tried so many different approaches to getting this to work, but still with no luck. Basically, I would like to have a very simple class (or a struct if I really have to) that contains an SDL_Surface*, and then have a std::vector to put them in. The problem comes that no matter how 'safe' I think my approach or class is, as soon as I try to put it in a vector, I get a seg fault.

Background:
In making a simple 2D game, I prefer to have a limited number of SDL_Surface's, each containing multiple sprites, with a vector of SDL_Rect's to define each particular sprite clip. This worked great when I was taking a structured approach, I would simply use something like this as global variables:

[code]vector<SDL_Surface*> Graphics; // Vector of my graphics
vector< vector<SDL_Rect> > Clipping; // 2D vector of clipping information
vector< vector<bool> > ClipRegister; // 2D vector of bools to tell me if a particular member of Clipping had be defined[/code]

The downside is that the game itself had to handle the loading and unloading of graphics, and I wanted to get away from this and put all the graphics functions in a class, and let the game get on with just being a game. It would obviously also have the advantage of being reusable in for other games. This all part of a larger project to make a single class that encapsulates all my graphic needs safely.

So step one towards this was to make a simple class (I'm learning as I go and didn't want to over-stretch myself):

[code]class GrfImage
{
public:
SDL_Surface* Graphics;
vector<SDL_Rect> Clipping;
vector<bool> ClipRegister;
};[/code]

This obviously goes against all the principles of encapsulation, but my plan is to implement this as my confidence grows.

But here I got my first problem. Using a one-off 'GrfImage', things worked fine with this temporary hack, however as soon as I made a vector of them (e.g., vector<GrfImage> Images;) I got seg faults whenever I used SDL_FreeSurface(Image[i].Graphics);, even having ensured that an image had been loaded and that the index into the vector was legal. So I thought "Well I'm clearly not managing the memory properly", and pushed on to an encapsulated version:

Header file:
[code]class GrfImage
{
public:
GrfImage();
GrfImage(const GrfImage &Original);
GrfImage& operator= (GrfImage const &Original);
~GrfImage();
bool ImgLoadBMP(string FName, SDL_Color Key);
bool ImgLoad(string FName, SDL_Color Key);
bool ImgBlitBMP(short x, short y, SDL_Surface* Dest);
bool ImgBlit(short x, short y, unsigned short i, SDL_Surface* Dest);

private:
SDL_Surface* Grf;
bool Loaded;
vector<SDL_Rect> GrfClip;
vector<bool> GrfClipReg;
};[/code]

Source file:
[code]#include "grfio.h"

using namespace std;


GrfImage::GrfImage()
{
Grf = NULL;
Loaded = false;
GrfClip.resize(0);
GrfClipReg.resize(0);
}



GrfImage::GrfImage(const GrfImage &Original)
{
*this = Original;
};



GrfImage& GrfImage::operator= (GrfImage const &Original)
{
if (Loaded) SDL_FreeSurface(Grf);
if (Original.Loaded)
{
Grf = SDL_DisplayFormat(Original.Grf);
Loaded = true;
}
else
{
Grf = NULL;
Loaded = false;
}
GrfClip = Original.GrfClip;
GrfClipReg = Original.GrfClipReg;
return *this;
}



GrfImage::~GrfImage()
{
if (Loaded) SDL_FreeSurface(Grf);
};



bool GrfImage::ImgLoadBMP(string FName, SDL_Color Key)
{
if (Loaded)
{
SDL_FreeSurface(Grf);
Grf = NULL;
}
SDL_Surface* TempGrf = NULL;
TempGrf = SDL_LoadBMP(FName.c_str());
if(TempGrf == NULL ) return true;
Grf = SDL_DisplayFormat(TempGrf);
SDL_FreeSurface(TempGrf);
if(Grf == NULL ) return true;
SDL_SetColorKey(Grf, SDL_RLEACCEL | SDL_SRCCOLORKEY, SDL_MapRGB(Grf->format, Key.r, Key.g, Key.b));
Loaded = true;
return false;
}


bool GrfImage::ImgLoad(string FName, SDL_Color Key)
{
... EXTPANDED VERSION OF ABOVE THAT LOADS CLIPPING TOO...
}



bool GrfImage::ImgBlitBMP(short x, short y, SDL_Surface* Dest)
{
if (Loaded)
{
SDL_Rect Offset;
Offset.x = x;
Offset.y = y;
if (SDL_BlitSurface(Grf, NULL, Dest, &Offset) == 0) return false;
}
return true;
}



bool GrfImage::ImgBlit(short x, short y, unsigned short i, SDL_Surface* Dest)
{
if ((Loaded) and
(i < GrfClip.size()) and
(GrfClipReg[i]))
{
SDL_Rect Offset;
Offset.x = x;
Offset.y = y;
if (SDL_BlitSurface(Grf, &GrfClip[i], Dest, &Offset) == 0) return false;
}
return true;
}[/code]

I'd thought I'd covered all bases with this - the class should comprehensively manage any access to the SDL_Surface pointer, as well as safely copying the surface rather than just copying the pointer. But now if I have in my main program source file:

[code]vector<GrfImage> MyImg;
while (MyImg.size() < 7)
{
GrfImage MyTempImg;
MyTempImg.ImgLoadBMP("graphics.bmp", C_Red); // 'C_Red' is a predefined SDL_Color
MyImg.push_back(MyTempImg);
}[/code]

...I get a seg fault (The 7 is arbritrary, and the whole routine is technically pointless as it just loads the same BMP 7 times).

At my wit's end, I tried:

[code]GrfImage MyImg[7];
int i = 0;
while (i < 7)
{
GrfImage MyTempImg;
MyTempImg.ImgLoadBMP("graphics.bmp", C_Red);
MyImg[i] = MyTempImg;
i++;
}[/code]

...And everything went fine. I also tried:

[code]GrfImage Img1;
if(1)
{
GrfImage Img2;
Img2.ImgLoadBMP("graphics.bmp", C_Red);
Img1 = Img2;
}
Img1.ImgBlitBMP(0,0,Screen);[/code]

...And everything went fine too (this was to test that operator= was working as I thought, as Img2 should destruct when it goes out of scope, shouldn't it?)

I'm metaphorically tearing my hair out over this one. Why can't I vectorise my class? What fundamental thing have I completely missed that is causing this?!

Any pointers gratefully received.

Ronnie
[/quote]

Share this post


Link to post
Share on other sites
[quote name='SiCrane' timestamp='1310492567' post='4834408']
Your class doesn't follow the [url="http://en.wikipedia.org/wiki/Rule_of_three_(C%2B%2B_programming)"]rule of three[/url]. The easiest way to modify your code would be to change from a simple SDL_Surface pointer to a smart pointer to an SDL_Surface. Using a boost::shared_ptr with SDL_FreeSurface as the clean up function would be one way (or std::/std::tr1:: shared_ptr if your compiler supports one of those).
[/quote]

Aha! Thank you very much. Being new to writing copy constructors and assignment operators, I had wrongly assumed that my copy constructor could just call the assignment operator. Explicitly writing a seperate copy constructor did the trick:

[code]GrfImage::GrfImage(const GrfImage &Original)
{
Grf = NULL;
Loaded = false;
GrfClip.resize(0);
GrfClipReg.resize(0);
if (Original.Loaded)
{
Grf = SDL_DisplayFormat(Original.Grf);
Loaded = true;
GrfClip = Original.GrfClip;
GrfClipReg = Original.GrfClipReg;
}
};[/code]

After running and closing my program several times, I haven't hit any errors and more importantly I don't appear to be leaking any memory.

I feel quite humbled as this is the first time in 25 years of programming that I've ever asked for help - a reference book or google being my weapons of choice. I suppose the important lesson learned here is that I should I RTFM more carefully sometimes - I had always thought that vector::push_back created a new empty object then assigned an existing object into the new one, whereas I'm now 99.9% sure that it calls the copy constructor instead.

@BeerNutts: Thanks also for your suggestion. I tried that first as it seemed the easier option, but alas it wouldn't compile - and I didn't think the that original object going out of scope was the issue anyway, as when I did it as C array I didn't get a problem.


Thanks again,
Ronnie

Share this post


Link to post
Share on other sites
summaky    134
[quote name='BeerNutts' timestamp='1310497516' post='4834450']
My guess is, when you are creating the Grf, you are doing it with a stack variable and adding it to the vector. Once that variable goes out of scope, the class with be destructed. Make sure it's a pointer, […]
[/quote]

No.

It doesn't matter whether the variable passed to push_back is an stack variable or not. push_back creates a new element by copying the object passed as parameter with the copy constructor. So, even if the original variable goes out of scope, the element inside the vector remains untouched.

Share this post


Link to post
Share on other sites
NightCreature83    5002
[quote name='RonnieCassinello' timestamp='1310548305' post='4834714']
[quote name='SiCrane' timestamp='1310492567' post='4834408']
Your class doesn't follow the [url="http://en.wikipedia.org/wiki/Rule_of_three_(C%2B%2B_programming)"]rule of three[/url]. The easiest way to modify your code would be to change from a simple SDL_Surface pointer to a smart pointer to an SDL_Surface. Using a boost::shared_ptr with SDL_FreeSurface as the clean up function would be one way (or std::/std::tr1:: shared_ptr if your compiler supports one of those).
[/quote]

Aha! Thank you very much. Being new to writing copy constructors and assignment operators, I had wrongly assumed that my copy constructor could just call the assignment operator. Explicitly writing a seperate copy constructor did the trick:

[code]GrfImage::GrfImage(const GrfImage &Original)
{
Grf = NULL;
Loaded = false;
GrfClip.resize(0);
GrfClipReg.resize(0);
if (Original.Loaded)
{
Grf = SDL_DisplayFormat(Original.Grf);
Loaded = true;
GrfClip = Original.GrfClip;
GrfClipReg = Original.GrfClipReg;
}
};[/code]

After running and closing my program several times, I haven't hit any errors and more importantly I don't appear to be leaking any memory.

I feel quite humbled as this is the first time in 25 years of programming that I've ever asked for help - a reference book or google being my weapons of choice. I suppose the important lesson learned here is that I should I RTFM more carefully sometimes - I had always thought that vector::push_back created a new empty object then assigned an existing object into the new one, whereas I'm now 99.9% sure that it calls the copy constructor instead.

@BeerNutts: Thanks also for your suggestion. I tried that first as it seemed the easier option, but alas it wouldn't compile - and I didn't think the that original object going out of scope was the issue anyway, as when I did it as C array I didn't get a problem.


Thanks again,
Ronnie
[/quote]

The joys of shallow copying :s. push_back always makes a new copy of the object you push in to it, the vector is trying to take control over the object, erasing from a vector or clearing it will cause the destructor of the contained object to be called as well.

Share this post


Link to post
Share on other sites

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