A quick question about my sprite manager class..

[Leprosy]

Okay, First of all hey. I just started using SFML and it's great. In fact I'd just finished reading my OpenGL book, coding a whole load of Vertex Buffer Object stuff for drawing 2D, and then I found this. So yeah thanks for saving me from bug testing all that stuff!

Anyway on to my question.. I've written a quick singleton class which takes a string to an image file and returns the loaded image. It stores the images in a map, and if the string exists, returns the loaded image, and if not loads the image, adds it to the map and then returns it. Kind of what's explained in the tutorials. It works and everything, but my problem is that I can't remember for the life of me whether I've typed this out right and whether it is indeed using the version stored, or a copy. It's been a while since university now and I've been slacking (getting baked) instead of programming, so I was hoping maybe one of you could just quickly confirm whether this is valid or not.

Anyway enough blabbering.

spriteManager.h:

Code: [Select]


#ifndef _SPRITEMANAGER_H
#define _SPRITEMANAGER_H

#include <map>
#include <string>

#include "sysInc.h"

#include <SFML/Window.hpp>
#include <SFML/Graphics.hpp>

using namespace std;

class spriteManager
{
private:
static bool instanceFlag;
static spriteManager *single;
std::map <string, sf::Image*> m_Images;
sf::Image *m_Temp;
spriteManager()
{

}
public:
static spriteManager* getInstance();
void destroy();
void init();
sf::Image*& loadImage(string filename);
~spriteManager()
{

}
};
#endif


spriteManager.cpp

Code: [Select]


#include "spriteManager.h"

bool spriteManager::instanceFlag = false;
spriteManager* spriteManager::single = NULL;
spriteManager* spriteManager::getInstance()
{
if(! instanceFlag)
{
single = new spriteManager();
instanceFlag = true;
return single;
}
else
{
return single;
}
}
void spriteManager::destroy(){
LOG("Closing sprite manager");
instanceFlag = false;
delete single;
}
void spriteManager::init(){
m_Temp = new sf::Image();
}
sf::Image*& spriteManager::loadImage(string filename){
if(m_Images.find(filename) == m_Images.end()){
if (!m_Temp->LoadFromFile(filename)){
LOG("[ERROR]: Loading sprite failed");
}else{
m_Images[filename] = m_Temp;
m_Temp = NULL;
}
}
return m_Images[filename];
}


When I use the spriteManager I use something like this:

Code: [Select]


m_MainImage.SetImage(*spriteManager::getInstance()->loadImage(mainImage));


Anyway I hope someone can help me with this. Also if you spot any other dumb errors (they're probably there) that would be a great help!

Thanks for taking the time to read.

Leprosy

*edit* I realise I forgot to remove all the pointers! I have fixed this now.
*edit again* Oops just realised I posted this in the wrong place. Sorry about that.

[PeterWelzien]

Code: [Select]

sf::Image*& loadImage(string filename);
Looks a bit weird. Should it really return a reference to a pointer?

I don't see you calling init() to initialize the m_Temp pointer. Every time you load a new image from disc you need to allocate a new sf::Image to store it in.

Your destructor doesn't free the allocated memory.

And I'm not sure that an object can delete itself.

[Leprosy]

Looks a bit weird. Should it really return a reference to a pointer?

I don't see you calling init() to initialize the m_Temp pointer. Every time you load a new image from disc you need to allocate a new sf::Image to store it in.

Your destructor doesn't free the allocated memory.

And I'm not sure that an object can delete itself.

The idea was to return a reference to the pointer in order to not create a new pointer (image) every time one is requested. This allows the same image to be used multiple times without loading it again. Correct?

Init is called from the main program, it is called just not shown in the code.

Each image is stored inside the map as a pointer, m_Temp is used as a temporary pointer, which is stored in the map and then set to NULL. When an image file string is requested, it either returns a reference to the pointer of the image that is already loaded, or if it isn't loaded, loads it, stores it in the map and then returns the reference to it.

I have in fact fixed the memory in the destroy() function now, that was an error I fixed yesterday, but well spotted.

Thanks for taking a look and please correct me if any of that was incorrect.

[Xorlium]

Pointers are only addresses. Returning a reference to a pointer is like returning a pointer to a pointer.

Just return a pointer, or return the image by a const reference. It won't create a new image, it will return the address of the old one (either by reference or by pointer).

Oh, and you should read up a little on pointers.

[Leprosy]

Thanks I'll look into that and I agree I should read up on pointers. Been too long.

[PeterWelzien]

Init is called from the main program, it is called just not shown in the code.

Then you'll need to call init() everytime you call loadImage(). Why not do it like this:

Code: [Select]

sf::Image* spriteManager::loadImage(string filename){
   if(m_Images.find(filename) == m_Images.end()){
      m_Temp = new sf::Image();
      if (!m_Temp->LoadFromFile(filename)){
         delete m_Temp;
         LOG("[ERROR]: Loading sprite failed");
      }else{
         m_Images[filename] = m_Temp;
         m_Temp = NULL;
      }
   }
   return m_Images[filename];
}

[Leprosy]

Thanks a lot everyone who helped. Think I've got it sorted now.

Code: [Select]


#ifndef _SPRITEMANAGER_H
#define _SPRITEMANAGER_H

#include <map>
#include <string>

#include "sysInc.h"

#include <SFML/Graphics.hpp>

using namespace std;

class spriteManager
{
private:
static bool instanceFlag;
static spriteManager *single;
std::map <string, sf::Image*> m_Images;
sf::Image *m_Temp;
spriteManager()
{

}
public:
static spriteManager* getInstance();
void destroy();
void init();
sf::Image* loadImage(string filename);
~spriteManager()
{

}
};
#endif


Code: [Select]


#include "spriteManager.h"

bool spriteManager::instanceFlag = false;
spriteManager* spriteManager::single = NULL;
spriteManager* spriteManager::getInstance()
{
if(! instanceFlag)
{
single = new spriteManager();
instanceFlag = true;
return single;
}
else
{
return single;
}
}
void spriteManager::destroy(){
for(std::map<string, sf::Image*>::iterator it = m_Images.begin(); it != m_Images.end(); ++it)
{
sf::Image* temp = (*it).second;
delete temp;
}
m_Images.clear();
LOG("Closing sprite manager");
instanceFlag = false;
delete single;
}
void spriteManager::init(){
}
sf::Image* spriteManager::loadImage(string filename){
if(m_Images.find(filename) == m_Images.end()){
m_Temp = new sf::Image();
if (!m_Temp->LoadFromFile(filename)){
LOG("[ERROR]: Loading sprite failed");
}else{
m_Images[filename] = m_Temp;
m_Temp = NULL;
}
}
return m_Images[filename];
}


[PeterWelzien]

If you fail to load an image from file you need to delete the temporary image. Otherwise you're leaking memory.

[Leprosy]

Oops thanks Pete.

[PeterWelzien]

It's looking good. But I feel the name is a bit misleading since it doesn't actually manage sprites, but images.

And you don't really need the temporary image pointer.

Code: [Select]

  for(std::map<string, sf::Image*>::iterator it = m_Images.begin(); it != m_Images.end(); ++it)
   {
      sf::Image* temp = (*it).second;
      delete temp;
   }
You can delete it directly:

Code: [Select]

delete (*it).second;