deleting sf::Texture* - I must be doing it really wrong

[Aval]

I suspect I'm doing something very wrong here, but I'm not very experienced with STL.

Essentially, I load all of my images as sf::Texture objects on the heap, and store the pointers in a map.

Code: [Select]


//snippets:
//I use one of these
std::map<std::string, sf::Texture*> images;

//the load image function is like this:
sf::Texture *temp = new sf::Texture;
if( temp->LoadFromFile(fileName) == false )
return false;
images[imageKey] = temp;

//then at the end I iterate through the map and use delete on each pointer in the map


This creates some sort of heap corruption though, which makes me think I'm either making a huge memory leak somewhere or I'm deleting textures twice. What's going on here? Is it the map and using delete on the pointers, or am I screwing up the heap somewhere else and this is just where the heap is used again and the program notices?

[Relic]

So far everything looks fine. Though I prefer insertion of key-value pairs into a map like this:

Code: [Select]

mMap.insert(make_pair(name, ptr));
 
Can we look at the portion of the code where you delete texture objects?

[Aval]

It's gone from confusing to strange, unfortunately. When debugging in Visual Studio, the debugger doesn't produce any errors at all, and it closes cleanly. When I run the exe outside of VS, windows detects something bad upon closing, and tells me to select a debugger to analyze it.

I took out all the image managing code, until it was just a single texture on the stack, and it still happens. This makes me think it's VS's fault, and I appear to fix it by setting the project from a console subsystem to a windows subsystem, and then changing the "int main()" to "int APIENTRY WinMain(...". The console no longer shows up, and the program appears to close without errors.

Why on earth does that make the program not close with an error? Is this really a fix or am I just hiding a memory leak? Bleh.

[unranked86]

Maybe I am wrong, but your "temp" sf::Texture goes out of scope, doesn't it ? When that happens the sf::Texture* objects in your map are pointing to non-existent objects. And then you iterate through the map and try to delete an invalid pointer.

Code: [Select]


std::map<std::string, sf::Texture> images;

//the load image function is like this:
sf::Texture temp;
if( temp.LoadFromFile(fileName) == false )
   return false;
images[imageKey] = temp;
This should work without any problem. Or if you insist on using pointers, then use a smart pointer instead of raw pointers.

[Nexus]

There is no reason to use pointers here. Do it like unranked86 proposed, then you avoid unnecessary trouble.

[Atomical]

I'm having the same problem as explained, in the visual studio 2010 ultimate debbuger it works fine.

It crashes on program exit.

Moving the declaration of sf::Texture from line 24 in the header file to just before line 59 in the cpp file, thus creating a new texture every time this component is drawn, will stop the program from crashing on exit. This makes my other components unable to get and work on the texture.

This is a temorary design thought, I'm going to do something similar to the OP, but that's unrelated to the issue.

Objsprite.h

Code: [Select]


#pragma once
#include "Component.h"
#include "Object.h"
#include <SFML/Graphics.hpp>
#include <string>
class PlayState;
class ObjSprite : public Component
{
public:
ObjSprite(Entity *entity, const std::string &name);
virtual ~ObjSprite();

static Component* Create(Entity* entity, const std::string &name) { return new ObjSprite(entity, name); }

void Init(PlayState *pState);
void Update();
void Draw(sf::RenderWindow& window);
void ExecuteCommand(int command, void* data = 0);
void ExecuteEvent(int event, void* data) {};
private:
bool compInitialized;
protected:
sf::Image spriteImage;
sf::Texture texture;
sf::Sprite sprite;

Property<std::string> imagePath;
Property<sf::Sprite*> spritePtr;

Property<float> posX;
Property<float> posY;

Property<float> imagePosX;
Property<float> imagePosY;

Property<float> spriteWidth;
Property<float> spriteHeight;

Property<float> rotation;

Property<bool> staticObject;

Object* go;

};


Objsprite.cpp

Code: [Select]



#include "ObjSprite.h"
#include <iostream>
#include <boost/bind.hpp>
#include "PlayState.h"
ObjSprite::ObjSprite(Entity *entity, const std::string &name)
: Component(entity,name), go((Object*)entity)
{
imagePath = go->AddProperty<std::string>("SpriteImage","sprites/default.bmp");

posX = go->AddProperty<float>("PosX",0);
posY = go->AddProperty<float>("PosY",0);

imagePosX = go->AddProperty<float>("ImagePosX",0);
imagePosY = go->AddProperty<float>("ImagePosY",0);

spriteWidth = go->AddProperty<float>("SpriteWidth",32);
spriteHeight = go->AddProperty<float>("SpriteHeight",32);

rotation = go->AddProperty<float>("Rotation",0);

staticObject = go->AddProperty<bool>("StaticObject",true);

compInitialized = false;
}

ObjSprite::~ObjSprite()
{

}

void ObjSprite::Init(PlayState *pState)
{
if(!compInitialized)
{
spriteImage.LoadFromFile(imagePath.Get());
spriteImage.CreateMaskFromColor(sf::Color(255,0,255));

sprite.SetSubRect(sf::IntRect(0,0,spriteWidth.Get(),spriteHeight.Get()));
sprite.SetPosition( posX.Get(),posY.Get() );

spritePtr = go->AddProperty<sf::Sprite*>("Sprite",&sprite);

compInitialized = true;
sprite.SetOrigin( spriteWidth.Get()/2,spriteHeight.Get()/2);
}
}

void ObjSprite::Update()
{
if(!staticObject.Get())
{
sprite.SetPosition( posX.Get(),posY.Get() );
sprite.SetRotation( rotation.Get() );
}
}

void ObjSprite::Draw(sf::RenderWindow &window)
{
texture.LoadFromImage(spriteImage);
sprite.SetTexture( texture,false );
window.Draw(sprite);
}

void ObjSprite::ExecuteCommand(int command, void* data )
{

}


[sbroadfoot90]

Maybe I am wrong, but your "temp" sf::Texture goes out of scope, doesn't it ? When that happens the sf::Texture* objects in your map are pointing to non-existent objects. And then you iterate through the map and try to delete an invalid pointer.

No, temp is a pointer to sf::Texture which does go out of scope, however, the actual texture it's pointing to is created on the heap and not the stack and so can't go out of scope seeing as it doesn't live in any scope in the first place.

One problem with the original code that might cause a memory leak is this

Code: [Select]

if( temp->LoadFromFile(fileName) == false )
   return false;

If LoadFromFile returns false, then you exit the current function without deleting the memory you allocated with temp. You should change it to

Code: [Select]

if( temp->LoadFromFile(fileName) == false ) {
   delete temp;
   return false;
}

Or just avoid using pointers as Nexus said

[Atomical]

RenderTexture and Texture crashed on deletion because I was using Intel HD graphics driver due to my laptop forcing it on me(Or that's my solution, worked on my home computer and my friend's laptop). I'm currently installing the ATI drivers.

EDIT: Now it works.