Making STL Vectors and SDL_Surfaces place nicely

[treyrust]

Forget about everything I said previously... Thanks to the other members for pointing this out.

If you want to use an SDL_Surface in a vector, then it's simple:

Code: Select all

std::vector<SDL_Surface*> myVectorOfSurfaces; // horrible variable name

The complications (and my reason for the thread) is when you start putting them into a class.

Code: Select all

class Entity
{
 private:
   std::vector<SDL_Surface*> mySurfaces;
   //--
   --//
pubic:
  void addSurface(SDL_Surface*);
}

void Entity::addSurface(SDL_Surface* S)
{
  mySurfaces.push_back(S);
}

//Destructor:

void Entity::~Entity()
{
   int i = 0;
  while(i != mySurfaces.size())
  {
     SDL_FreeSurface(mySurfaces[i]);
  }
}

It's all fine and good until somewhere else you do something like this in you're main (or anywhere for that matter)

Code: Select all

   SDL_Surface* image = SDL_LoadBMP("myBmp.bmp");
   Entity myEntity;
   myEntity.addSurface(image);
   SDL_FreeSurface(image); /* SEGFAULT! Once you pass the pointer to the function, let it handle it! it's the same piece of data and you don't want to interfere with the destructor or it WILL segfault because it's already removed.*/

Never do "image->refcount++", all this does is trick SDL_FreeSurface into not freeing that surface. Unless your specific situation calls for it, don't do it.

Another method is to use SDL_ConvertSurface, but I haven't messed with it.

~Trey.

[TheBuzzSaw]

This is a terrible solution. Want to make it work right? Don't free the same surface pointer twice! Why are you so insistent on freeing it once as a tempSurface and then once in the vector? Just drop your call to SDL_FreeSurface(tempSurface); and the SDL_FreeSurface(mySurfaces[0]); will work fine. They're just pointers, after all. Just make sure they are allocated and freed only once.

Also, it appears that you forgot to make tempSurface a pointer.

[treyrust]

This is a terrible solution. Want to make it work right? Don't free the same surface pointer twice! Why are you so insistent on freeing it once as a tempSurface and then once in the vector? Just drop your call to SDL_FreeSurface(tempSurface); and the SDL_FreeSurface(mySurfaces[0]); will work fine. They're just pointers, after all. Just make sure they are allocated and freed only once.

Also, it appears that you forgot to make tempSurface a pointer.

Made it a pointer.

You have a point though... If it's just copying the pointer and the not actual surface then it doesn't matter.

So why does it say in here to increase the refcount if you want a copy? Why doesn't it say just free the first one? That would make more sense... I only wrote this based on that: http://www.libsdl.org/cgi/docwiki.cgi/SDL_Surface

Oh well... Thanks for the info, I've updated the original accordingly.

[treyrust]

I've put it back because, it gave me an unhandled exception still if I don't increase the refcount.

The problem on my end is, I pass a surface pointer into a function, and then after that function call I clear that surface. That gives an unhandled exception because it's trying to free it twice... So, either increase the refcount and probably mess of optimization a bit with calls to SDL_FreeSurface or just remember not to free the one you pass into the function...

I'll rewrite the first post in a bit...

EDIT: Still doesn't even if I don't free it after the function call... So I hold to my original post that you do have to do this. I welcome to a better solution.

[TheBuzzSaw]

My question stands: why are you trying to free the surface so often? Just free it when you're done!

[treyrust]

My question stands: why are you trying to free the surface so often? Just free it when you're done!

If I don't increase the refcount, and I only free it once (in a destructor that loops through the vector) it still gives me a segfault. So, I free it just in case... call me crazy but since I have to increase it I like to throw salt over my shoulder to make sure I don't get a memory leak...

[TheBuzzSaw]

My question stands: why are you trying to free the surface so often? Just free it when you're done!

If I don't increase the refcount, and I only free it once (in a destructor that loops through the vector) it still gives me a segfault. So, I free it just in case... call me crazy but since I have to increase it I like to throw salt over my shoulder to make sure I don't get a memory leak...

I'm still not quiiiiite following. You're saying that you get a seg fault even if deleting them from that vector is the *ONLY* place you free them?

Plus, increasing the ref count won't prevent a memory leak. In fact, that would pretty much GUARANTEE a leak as your free statements would not actually be disposing the surface.

I'm still very suspicious of your methods. I would need to see more code. I still think you are accidentally freeing the surface in more than one location.

[cdaragorn]

Something is not right with your code, you need to make tempSurface a pointer.

Outside of that, your code should compile and run fine WITHOUT doing anything to refcount and only freeing the surface from the vector (not twice).

I've been using SDL_Surfaces for quite a while now, and have never had to free a surface more than once, though I will admit I wrap mine inside objects to handle all that for me.

Edit: Ok, now I understand what's going on. The guy in the comments of the documentation is cheating. Increasing refcount causes the first call to freesurface to NOT actually free the surface, it just reduces the refcount. This is stupid and does not create a copy of the surface. Read the comment after that one, it shows how to correctly make a copy of an sdl_surface.

[treyrust]

The reason my code isn't working right now is I'm passing the SAME POINTER multiple times, so when I go to free it, it's already freed so - boom

What increasing the refcount does it make it so you can make multiple calls to SDL_FreeSurface with the same surface pointer without causing a segfault. The issue with this is... well... it's hackeyass, dangerous and not the C++ way...

Although... There is a major flaw in passing a pointer into a member function of a class in the first place... You have to watch the lifetime of the data that the pointer is pointing to. If it's created in a while loop or a function and not declared static (which probably wouldn't be good if you're trying to do something dynamic), it tries to call something that isn't there. So, the best thing would be to actually copy the surface the right way and then free the surface you're passing to the function separately... This is the safest way to do it, and dare I say, the C++ way (because then the surface is encapsulated, and the data structure doesn't depend on the external lifespan of the pointer)...http://www.libsdl.org/cgi/docwiki.cgi/S ... ertSurface //I was wrong here... I was thinking of a dangling pointer and that won't happen in this case... Because all the SDL_Surface* is is a pointer to some dynamic memory, not the memory itself, the memory isn't freed until you call SDL_FreeSurface.

I'll rewrite my first post accordingly in a bit... I had a bad feeling at first about the refcount thing, but I went past my better judgement and just figured it was something I didn't understand about SDL because it was in the docs... I'll be more skeptical and stick to my convictions in the future.

[TheBuzzSaw]

I recommend putting all surfaces into an object. Here is how I went about it.

https://github.com/TheBuzzSaw/paroxysm/ ... GE/Image.h
https://github.com/TheBuzzSaw/paroxysm/ ... /Image.cpp

[Aleios]

Code: Select all

std::vector<SDL_Surface*> mySurfaces;

SDL_Surface tempSurface = NULL;
tempSurface = SDL_LoadBMP("yourimage.bmp");

tempSurface->refcount += 2; // We increase it to 2, because we don't just want to free the copy, but the temp as well. If we only increase it once here, sure, it will work but we won't be able to free tempSurfe
mySurfaces.push_back(tempSurface);

SDL_FreeSurface(tempSurface); //Sense the refcount was at 2, we have enough to free both the cop(ies) and the original!
SDL_FreeSurface(mySurfaces[0]); // Success!

Whoa lets hold up for a second! From the looks of it you are creating the Surface, then you have decided to free it, the you are freeing it again. The second time you Free the surface you are essentially freeing nothing! you can't free what does not exist. When you have a vector of pointers the address of the object is stored as a pointer NOT the object itself.

As stated by the people above, either do this: SDL_FreeSurface(mySurfaces[0]); or SDL_FreeSurface(tempSurface). Trying to free something twice is not good design, nor should it work, and if it does, its not guaranteed safe, like at all.

This refcount thing states that its for duplicating (and i wouldn't really do that either...).

Code: Select all

SDL_Surface tempSurface = NULL;
tempSurface = SDL_LoadBMP("yourimage.bmp");

tempSurface->refcount += 2; // We increase it to 2, because we don't just want to free the copy, but the temp as well. If we only increase it once here, sure, it will work but we won't be able to free tempSurface
mySurfaces.push_back(tempSurface);

That my friend is not duplicating! It is NOT a copy (in a sense), its a pointer to the address of tempSurface, or should be) since "SDL_Surface tempSurface = NULL;", last i checked that should be a pointer.

What push_back will do is to Copy the pointer, but not the object that the pointer refers to.

Anyone correct me if im wrong, or if i said non understandable crap. But im pretty sure that trying to delete an already deleted object is against c++ and against logic.

[treyrust]

Code: Select all

std::vector<SDL_Surface*> mySurfaces;

SDL_Surface tempSurface = NULL;
tempSurface = SDL_LoadBMP("yourimage.bmp");

tempSurface->refcount += 2; // We increase it to 2, because we don't just want to free the copy, but the temp as well. If we only increase it once here, sure, it will work but we won't be able to free tempSurfe
mySurfaces.push_back(tempSurface);

SDL_FreeSurface(tempSurface); //Sense the refcount was at 2, we have enough to free both the cop(ies) and the original!
SDL_FreeSurface(mySurfaces[0]); // Success!

Whoa lets hold up for a second! From the looks of it you are creating the Surface, then you have decided to free it, the you are freeing it again. The second time you Free the surface you are essentially freeing nothing! you can't free what does not exist. When you have a vector of pointers the address of the object is stored as a pointer NOT the object itself.

As stated by the people above, either do this: SDL_FreeSurface(mySurfaces[0]); or SDL_FreeSurface(tempSurface). Trying to free something twice is not good design, nor should it work, and if it does, its not guaranteed safe, like at all.

This refcount thing states that its for duplicating (and i wouldn't really do that either...).

Code: Select all

SDL_Surface tempSurface = NULL;
tempSurface = SDL_LoadBMP("yourimage.bmp");

tempSurface->refcount += 2; // We increase it to 2, because we don't just want to free the copy, but the temp as well. If we only increase it once here, sure, it will work but we won't be able to free tempSurface
mySurfaces.push_back(tempSurface);

That my friend is not duplicating! It is NOT a copy (in a sense), its a pointer to the address of tempSurface, or should be) since "SDL_Surface tempSurface = NULL;", last i checked that should be a pointer.

What push_back will do is to Copy the pointer, but not the object that the pointer refers to.

Anyone correct me if im wrong, or if i said non understandable crap. But im pretty sure that trying to delete an already deleted object is against c++ and against logic.

Yeah, I realized that. The only reason I thought it was in the first place is some guy in the docs said to do it *shrugs* *points*. I figured it was wrong but just chalked it up to not understanding SDL enough... Like I said, gimme a sec to modify the first post...

[Aleios]

Yeah, I realized that. The only reason I thought it was in the first place is some guy in the docs said to do it *shrugs* *points*. I figured it was wrong but just chalked it up to not understanding SDL enough... Like I said, gimme a sec to modify the first post...

ahh, sorry, i did not see the edit

[treyrust]

Yeah, I realized that. The only reason I thought it was in the first place is some guy in the docs said to do it *shrugs* *points*. I figured it was wrong but just chalked it up to not understanding SDL enough... Like I said, gimme a sec to modify the first post...

ahh, sorry, i did not see the edit

It's alright, I didn't edit the first post until after yours anyway.

[Falco Girgis]

I recommend putting all surfaces into an object. Here is how I went about it.

https://github.com/TheBuzzSaw/paroxysm/ ... GE/Image.h
https://github.com/TheBuzzSaw/paroxysm/ ... /Image.cpp

That's pretty cute... Quite a bit like how we're wrapping libGyro2.0's C API within the C++ engine these days...