Public Group

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

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

## 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:

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

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):

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

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.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:

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; };

Source file:
#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)) { SDL_Rect Offset; Offset.x = x; Offset.y = y; if (SDL_BlitSurface(Grf, &GrfClip, Dest, &Offset) == 0) return false; } return true; }

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:

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); }

...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:

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

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

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

...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?!

Ronnie

##### Share on other sites
Your class doesn't follow the rule of three. 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 on other sites
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:

 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); } 

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:

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

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):

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

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.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:

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; };

Source file:
#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)) { SDL_Rect Offset; Offset.x = x; Offset.y = y; if (SDL_BlitSurface(Grf, &GrfClip, Dest, &Offset) == 0) return false; } return true; }

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:

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); }

...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:

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

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

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

...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?!

Ronnie

##### Share on other sites

Your class doesn't follow the rule of three. 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).

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:

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; } };

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 on other sites

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, […]

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 on other sites

[quote name='SiCrane' timestamp='1310492567' post='4834408']
Your class doesn't follow the rule of three. 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).

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:

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; } };

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.

1. 1
Rutin
25
2. 2
3. 3
4. 4
JoeJ
18
5. 5

• 14
• 14
• 11
• 11
• 9
• ### Forum Statistics

• Total Topics
631757
• Total Posts
3002140
×