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):
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:
Header file: 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);
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 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?!
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).
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):
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:
Header file: 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);
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 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?!
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:
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.
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.
[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:
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.
