SFML community forums
Help => General => Topic started by: slotdev on May 23, 2011, 05:15:35 pm
-
I know that in SFML-world, using "new" is very bad practise.
I have a std::map vector of sprites/sounds/etc, held in the public section of a class, and I reference those objects by a string.
This means that, when I create the objects, I have to "new" them. Is there any other way to do this.
I'm getting heap corruption when trying to delete the objects, and memory leaks if they're not deleted. I suspect this is why I shouldn't be using new :oops:
-
I know that in SFML-world, using "new" is very bad practise.
Nop. It's more "in the C++ world, handling deallocations (and most of the allocations) manually is a very bad practice".
I have a std::map vector of sprites/sounds/etc, held in the public section of a class, and I reference those objects by a string.
This means that, when I create the objects, I have to "new" them.
Why? You can have default-constructed instances and assign/modify them later.
I'm getting heap corruption when trying to delete the objects, and memory leaks if they're not deleted. I suspect this is why I shouldn't be using new
Absolutely ;)
-
Right, that's all working. All "new" with SFML stuff is gone. Happy days. Well almost.
My sprites and other data/functions I need are contained in a "SpriteObj" class, and I create an instance of this for each sprite. Fine.
When I call the destructor, it breaks with a heap pointer error. Here's the call stack trace. Any ideas?? I know I shouldn't use it but I don't know how to do this any other way!
ntdll.dll!77ea0844()
[Frames below may be incorrect and/or missing, no symbols loaded for ntdll.dll]
ntdll.dll!77e62a74()
ntdll.dll!77e1cd87()
KernelBase.dll!7617468e()
PR1027.exe!_CrtIsValidHeapPointer(const void * pUserData=0x04335f00) Line 2072 C++
PR1027.exe!_free_dbg_nolock(void * pUserData=0x04335f00, int nBlockUse=1) Line 1279 + 0x9 bytes C++
PR1027.exe!_free_dbg(void * pUserData=0x04335f00, int nBlockUse=1) Line 1220 + 0xd bytes C++
PR1027.exe!operator delete(void * pUserData=0x04335f00) Line 54 + 0x10 bytes C++
PR1027.exe!std::allocator<std::_Tree_nod<std::_Tset_traits<sf::ResourcePtr<sf::Image> *,std::less<sf::ResourcePtr<sf::Image> *>,std::allocator<sf::ResourcePtr<sf::Image> *>,0> >::_Node>::deallocate(std::_Tree_nod<std::_Tset_traits<sf::ResourcePtr<sf::Image> *,std::less<sf::ResourcePtr<sf::Image> *>,std::allocator<sf::ResourcePtr<sf::Image> *>,0> >::_Node * _Ptr=0x04335f00, unsigned int __formal=1) Line 141 + 0x9 bytes C++
PR1027.exe!std::_Tree<std::_Tset_traits<sf::ResourcePtr<sf::Image> *,std::less<sf::ResourcePtr<sf::Image> *>,std::allocator<sf::ResourcePtr<sf::Image> *>,0> >::erase(std::_Tree<std::_Tset_traits<sf::ResourcePtr<sf::Image> *,std::less<sf::ResourcePtr<sf::Image> *>,std::allocator<sf::ResourcePtr<sf::Image> *>,0> >::iterator _Where=0x0426c7e4 {myResource=0x04267dd4 }) Line 910 C++
PR1027.exe!std::_Tree<std::_Tset_traits<sf::ResourcePtr<sf::Image> *,std::less<sf::ResourcePtr<sf::Image> *>,std::allocator<sf::ResourcePtr<sf::Image> *>,0> >::erase(std::_Tree<std::_Tset_traits<sf::ResourcePtr<sf::Image> *,std::less<sf::ResourcePtr<sf::Image> *>,std::allocator<sf::ResourcePtr<sf::Image> *>,0> >::iterator _First=0x0426c7e4 {myResource=0x04267dd4 }, std::_Tree<std::_Tset_traits<sf::ResourcePtr<sf::Image> *,std::less<sf::ResourcePtr<sf::Image> *>,std::allocator<sf::ResourcePtr<sf::Image> *>,0> >::iterator _Last=0x0426c7e4 {myResource=0x04267dd4 }) Line 926 + 0x3f bytes C++
PR1027.exe!std::_Tree<std::_Tset_traits<sf::ResourcePtr<sf::Image> *,std::less<sf::ResourcePtr<sf::Image> *>,std::allocator<sf::ResourcePtr<sf::Image> *>,0> >::erase(sf::ResourcePtr<sf::Image> * const & _Keyval=0x0426c244) Line 936 + 0x67 bytes C++
> PR1027.exe!sf::Resource<sf::Image>::Disconnect(sf::ResourcePtr<sf::Image> & Observer={...}) Line 88 C++
PR1027.exe!sf::ResourcePtr<sf::Image>::~ResourcePtr<sf::Image>() Line 69 C++
PR1027.exe!sf::Sprite::~Sprite() + 0x5b bytes C++
PR1027.exe!SpriteObj::~SpriteObj() Line 35 + 0x21 bytes C++
-
When I call the destructor
Do you mean that you do it explicitely? You mustn't. Destructors are always called automatically when the variable reaches the end of its scope.
-
No sorry, I use "delete". I get memory leaks detected for each SpriteObj instance created if I don't use "delete". But I get heap corruption if I do. :(
-
If you don't use new to allocate them, you must not use delete to destroy them. If they are members of a class, they are automatically destroyed when the owner instance is destroyed. So maybe it's this one that is not properly destroyed?
You should definitely show us some code.
-
Here's some code. I have a feeling it's being caused by the destructor in sf::Image being called twice, but that's just a hunch:
Class that holds my std::map of my "SpriteObj" objects.
class GfxEngine
{
public:
GfxEngine(void);
~GfxEngine();
std::map<string,SpriteObj*>gameSprites;
void createSprites();
};
Stuff that actually instatiates the SpriteObj's - don't worry about the xmlData stuff, I've cut that out for readability, and it all works.
void GfxEngine::createSprites()
{
// Get some data out of an XML file and create a SpriteObj
gameSprites[xmlData.gfxId] = new SpriteObj(&xmlData);
}
The SpriteObj class:
class SpriteObj
{
public:
SpriteObj(struct GfxResources::_GfxDataStruct_ * gfxData);
~SpriteObj();
// Data
Sprite spritePtr; // THE ACTUAL sf:Sprite (not a pointer to!)
private:
struct GfxResources::_GfxDataStruct_ spriteData;
};
SpriteObj's constructor & destructor:
// SpriteObj constructor
SpriteObj::SpriteObj(struct GfxResources::_GfxDataStruct_ * gfxData) : spritePtr(Sprite())
{
// Create this sprite from the data taken from the XML array
spritePtr.SetImage(gfxEngine->gfxTextures[gfxData->gfxFile]);
spritePtr.SetSubRect(sprRect);
spritePtr.SetPosition((float)gfxData->gfxDx,(float)gfxData->gfxDy);
}
// SpriteObj destructor
SpriteObj::~SpriteObj()
{
// Goes BANG here - presumably calling sf::Image destructor
}
Finally, in the destructor of GfxRngine, where I delete all instances of the SpriteObj objects, excuse the use of vectors copying the name of the sprite but I am debugging :)
// GfxEngine destructor
GfxEngine::~GfxEngine()
{
std::vector<string> gsStrings;
std::map<string,SpriteObj*>::iterator it;
for (it = gameSprites.begin(); it != gameSprites.end(); ++it)
{
gsStrings.push_back(gameSprites[it->first]->GetName());
}
std::vector<string>::const_iterator i;
for (size_t i = 0; i < gsStrings.size(); i++)
{
// This does call the destructor of each gameSprite
delete gameSprites[gsStrings[i]];
}
}
Thanks
Ed
-
Do you really need pointers to SpriteObj in your map? What is this GfxResources::_GfxDataStruct_ structure? It looks like it's not used (at least initialized) at all in SpriteObj.
By the way, gameSprites[it->first] is the same as it->second ;)
-
Do you really need pointers to SpriteObj in your map? What is this GfxResources::_GfxDataStruct_ structure? It looks like it's not used (at least initialized) at all in SpriteObj.
By the way, gameSprites[it->first] is the same as it->second ;)
Hmm, I could probably remove the pointers altogether. Would that actually solve the problem? I don't know. If the destructor to sf::Image is being called twice, then no.
-
If the destructor to sf::Image is being called twice, then no.
Where is there an sf::Image instance? Why would it be destroyed twice? Can you please answer all my questions (what is GfxResources::_GfxDataStruct_ ?)? ;)
-
This isn't your problem but this function can be greatly improved:
Blech:
// GfxEngine destructor
GfxEngine::~GfxEngine()
{
std::vector<string> gsStrings;
std::map<string,SpriteObj*>::iterator it;
for (it = gameSprites.begin(); it != gameSprites.end(); ++it)
{
gsStrings.push_back(gameSprites[it->first]->GetName());
}
std::vector<string>::const_iterator i;
for (size_t i = 0; i < gsStrings.size(); i++)
{
// This does call the destructor of each gameSprite
delete gameSprites[gsStrings[i]];
}
}
Better:
// GfxEngine destructor
GfxEngine::~GfxEngine()
{
std::map<string,SpriteObj*>::iterator it;
for (it = gameSprites.begin(); it != gameSprites.end(); ++it)
{
delete it->second;
}
gameSprites.clear(); // although even this line is superfluous, since
// it will be cleared automatically by its dtor.
}
EDIT:
actually maybe this is your problem. If GetName isn't returning the exact same thing as it->first, you'll be attempting to delete non-existing items in your map, which would cause it to explode.
-
Laurent:
GfxResources::_GfxDataStruct_ is just a structure I fill from reading in data from XML (sprite name, w/h src x/y dst x/y and so on) which I will then use to create the sprite. I didn't put that in the code I posted here as it's not relevant.
The sf::Image stuff is as in that stack trace above - I assume sf::Sprite uses sf::Image somewhere along the way?
I think I do need to get rid of these pointers. They're nothing but trouble :D
Disch:
Thanks for the modification, I have implemented it but it didn't fix the problem. I read somewhere that you shouldn't delete stuff from vectors while iterating over it, as the number of elements in the vector might not match and it all goes bang. Not sure.
-
I think I do need to get rid of these pointers. They're nothing but trouble
You need them for polymorphism (if you're using polymorphism). Otherwise, yeah you probably shouldn't be using them here.
Although this problem is likely more due to heap corruption than anything else. Are you stepping outside of an array bounds somewhere? Or dereferencing a bad pointer?
I find it very unlikely that you stumbled upon a SFML bug, as such a fundamental bug in the sf::Sprite->sf::Image relationship probably would have been exposed and corrected already. Heap corruption is seeming more and more likely.
Is it possible for you to upload the entire program somewhere so we can take a look?
I read somewhere that you shouldn't delete stuff from vectors while iterating over it
It probably was saying you shouldn't erase stuff from vectors while iterating over it (as in, actually removing the elements from the vector).
There are reasons not to do that, although you can if you do it properly. Note however that's not what I'm doing here. I'm keeping all the elements in the map, just deleting the pointers.
-
I have commented out all references to the sf::Sprite in my class and all references to it, recompiled & run, and the deleting of the SpriteObj class instances all works fine, no mem leaks/heap corruption etc.
Now, I am slowly adding things back in, just the creation of the spritePtr in the SpriteObj constructor firstly (runs/exits OK), but then adding the following call in the constructor, the crash/heap corruption occurs (when eventually deleting the instance of SpriteObj).
spritePtr.SetImage(gfxEngine->gfxTextures[gfxData->gfxFile]);
gfxTextures is an array of type sf::Image, all of which are valid. The program runs fine and displays the right graphics etc, so that's fine.
Soooo....that brings me back to this sf::Image destructor or something being at fault. I can't believe it's SFML, this would almost certainly have been found by now as Disch said.
-
Can you show us more code related to the gfxEngine->gfxTextures array?
When is your SpriteObj destroyed? Is it at global exit?
-
Hi Laurent
Sure, here's the code for where I load the textures:
int gfxNum = 0;
while (xml && xml->read())
{
if (xml->getNodeType() == EXN_ELEMENT)
{
if (!strcmp("TextureResource",xml->getNodeName()))
{
gfxEngine->gfxTextures[gfxNum].LoadFromFile(xml->getAttributeValue("file"));
gfxNum++;
}
}
The declaration of gfxTextures is:
sf::Image gfxTextures[5];
SpriteObj objects are destroyed in the destructor of the GfxEngine class.
-
It becomes too complicated, you should try to write a complete and minimal code that reproduces the problem (without all your engine stuff around).
-
Here's a VERY stripped down version (1 source file) which reproduces the problem. Please download it and try, you probably have to change the VC++ paths to the lib files. Try with your own version (SFML 1.6), but I included the debug EXE & all DLLs I use in the RAR also.
http://db.tt/1UAK2ut
It opens a small window 512x384 and displays multi colour tiles very rapidly (see testimage.png).
I have an idea of what causes the problem but if you can check, that would be great.
I think the problem is, each sprite is made from a sub-rect of testimage.png, so when the destructor gets called at the program end (automatically, not by me :)) the sf::Image is deallocated/freed/whatever but it's the same image file for all sprites, so it's being deallocated every time ?? Just an idea.
Thanks
Ed[/url]
-
Right, I have just done a test with that exact code, running SFML 2.0, and it seems to work OK. So either there is an SFML 1.6 problem or some wierd compiler issue.
Anyway I will continue with SFML 2.0, which is probably for the best!
-
Just noticed in your original code you called a function GetName() of SpriteObj when destructing the Gfx class. But no where in SpriteObj constructor do you store any information about the name. Perhaps you were returning a static name all the time??? or perhaps you left that code out in your snippet.
So this is my guess, destructor would of been calling delete on a invalid iterator returned by the subscript operator of the map. But as previous posters have said no where do you desctruct the image in that code so it can't be the image pointer.
Regarding your new code, you're keeping everything on the stack (which is good for someone of your skill) There are some issues in your code and this would explain why you are facing problems with using pointers.
There is nothing wrong with using the stack.
-
Roswell_r,
Thanks for the message. You may have guessed from the code my background is not in C++, but embedded systems (making hardware talk to one another, pretty much).
The stuff about SpriteObj - I had a separate vector of strings, and I use those to create the std::map and then reference it. It all worked fine - the only thing was the sf::Image falling over every time the destructor was called on the sf::Sprite, which I cannot fathom. This same method doesn't fall over on SFML 2.0, so I really have no idea what was going on.
I will try and get stuff off the stack where possible, but I am commercially constrained (i.e. the more time coding, the less money I make) so it has to work robustly, quickly. Such is life.
Cheers
Ed