6
\$\begingroup\$

I'm new to C++ and SDL, and I've written a Texture manager class whose purpose is to help me manage sprites and other textures. I have a dispose method which unloads all the textures from a map container. It works correctly but I'd like to ask if I am not causing any memory leaks, or other pointer related issues.

TextureManager.h

class TextureManager
{
    public:

        bool load(std::string fileName, std::string id, SDL_Renderer* pRenderer);
        void draw(std::string id, int x, int y, int width, int height, int scale, SDL_Renderer* pRenderer,
                    SDL_RendererFlip flip = SDL_FLIP_NONE);
        void drawFrame(std::string id, int x, int y, int width, int height, int scale, int currentRow, int currentFrame, SDL_Renderer* pRenderer,
                    SDL_RendererFlip flip = SDL_FLIP_NONE);
        void dispose();

        std::map<std::string, SDL_Texture*> m_textureMap;

        static TextureManager* Instance()
        {
            if(s_pInstance == 0)
            {
                std::cout << "Created TextureManager singleton\n";

                s_pInstance = new TextureManager();
                return s_pInstance;
            }
            //point to same object (singleton pattern)
            return s_pInstance;
        }

        private:
            TextureManager(){}
            static TextureManager* s_pInstance;

            bool hasDisposed = false;
};

 typedef TextureManager TheTextureManager; //should be used when calling singleton

#endif // TEXTUREMANAGER_H

TextureManager.cpp

#include "TextureManager.h"

TextureManager* TextureManager::s_pInstance = 0;

bool TextureManager::load(std::string fileName, std::string id, SDL_Renderer* pRenderer){

    std::cout << "\n\t\\\\\\\\\TextureManager :\n";

    SDL_Surface* pTempSurface = IMG_Load(fileName.c_str());

    if(pTempSurface == 0){
        std::cerr << "ERROR : Couldn't load Texture from file : " <<
        fileName.c_str() << std::endl;
        return false;
    }
    SDL_Texture* pTexture = SDL_CreateTextureFromSurface(pRenderer, pTempSurface);
    //dispose Surface (not used)
    SDL_FreeSurface(pTempSurface);
    //everything OK, add Texture to map
    if(pTexture != 0){
        std::cout << "Loaded Texture from file : " << fileName << " successfully\n";
        m_textureMap[id] = pTexture;
        std::cout << "Allocated memory for : " << id << " at : " << pTexture << "\n";

        return true;
    }

    //something wen't wrong...
    std::cerr << "ERROR : couldn't register Texture from file : " <<
    fileName.c_str() << std::endl;
    return false;
}

void TextureManager::draw(std::string id, int x, int y, int width, int height, int scale, SDL_Renderer* pRenderer, SDL_RendererFlip flip){
    SDL_Rect srcRect;
    SDL_Rect destRect;

    srcRect.x = 0;
    srcRect.y = 0;
    srcRect.w = width;
    srcRect.h = height;
    destRect.w = width * scale;
    destRect.h = height * scale;
    destRect.x = x;
    destRect.y = y;

    SDL_RenderCopyEx(pRenderer, m_textureMap[id],
                     &srcRect, &destRect, 0, 0, flip);

}

void TextureManager::drawFrame(std::string id, int x, int y, int width, int height, int scale, int currentRow, int currentFrame, SDL_Renderer* pRenderer, SDL_RendererFlip flip){
    SDL_Rect srcRect;
    SDL_Rect destRect;

    srcRect.x = width * currentFrame;
    srcRect.y = height * (currentRow - 1);
    srcRect.w = width;
    destRect.w = width * scale;
    srcRect.h = height;
    destRect.h = height * scale;
    destRect.x = x;
    destRect.y = y;

    SDL_RenderCopyEx(pRenderer, m_textureMap[id],
                     &srcRect, &destRect, 0, 0, flip);
}

void TextureManager::dispose(){
    if(m_textureMap.empty())//no textures were added
    {
        std::cout << "no need to unload textures : no textures loaded !\n";
        return;
    }
    if(hasDisposed)
    {
        std::cout << "Textures have already been disposed !\n";
        return;
    }

    //delete textures
    std::cout << "\nDeleting textures : \n\n";

    typedef std::map<std::string, SDL_Texture*>::iterator TextureIterator;

    for(TextureIterator it = m_textureMap.begin(); it != m_textureMap.end(); ++it){
        std::cout << "\ttextureMap  [ID==" << it->first << "]" << " [allocation address==" << it->second << "] ";
        SDL_DestroyTexture(it->second);//destroy texture
        std::cout << "--> destroyed !\n";
    }

    hasDisposed = true;//cannot dispose more then once!
}
\$\endgroup\$

4 Answers 4

6
\$\begingroup\$

Resource Managers:

The "resource manager" pattern is super common in games to manage shareable resources, such as textures, 3D models, sounds, you name it. Searching for "resource manager" on GameDev.SE yields a lot of results, you should check some of them out. Reading the chapters on Singletons and Service Locator at the online book Game Programming Patterns is also worth your time.

Now one thing I didn't quite like about your implementation is how you have implemented the resource (or texture) handle. The "handle" for a texture in your implementation is the std::string id that the users of TextureManager have to pass around. Not only a std::string is a large object to keep around, but also, whenever someone needs to access a texture, you must lookup the map of textures to find the pointer associated with that id. The lookup on a std::map has logarithmic complexity. This is not such a big deal you might think, but there is no need to do a map lookup in this case if you provide the user with a different type of handle, other than a string. That would not be a premature optimization in my book, just about choosing the best setup.

What I would do in your place would be to wrap the SDL_Texture object inside a C++ class. Then store smart pointers to this class inside the map and return smart pointers directly to the users of TextureManager. This way, whenever a texture must be used to draw something, no need for a map lookup, the user can just pass the pointer to the draw function (or call it in the object). The map might still be kept to ensure the same image file is never loaded twice. To put this into perspective, here is some pseudo-C++-code:

class Texture
{
    // I would put methods like 
    // `draw()`, `drawFrame()` and `dispose()` in here.
    // Makes more sense since they operate on a texture.
    //
private:
    SDL_Texture * sdlTexureObj;
};

// Our texture "handle"
typedef std::shared_ptr<Texture> TexturePtr;

class TextureManager
{
public:
    // Delete operator == and copy constructor 
    // to ensure single instance!

    static TextureManager & getInstance() 
    { /* as explained by @Loki in his answer */ }

    TexturePtr load(const std::string & filename)
    {
        // First lookup if the texture with that filename
        // is already loaded, if yes, return it instead
        // so that it can be shared.
        // 
        auto texIterator = loadedTextures.find(filename);
        if (texIterator != loadedTextures.end())
        {
            return texIterator.second;
        }

        // Texture with that filename is not loaded yet.
        // Load it and save it into the map cache.
        //
        // All the SDL loading routine you already know
        // + allocating a new Texture object. Take a look at `std::make_shared()`.
        TexturePtr newTexture = ...

        // Add to the cache map so that next time if someone
        // tries to load() it again, we don't duplicate any data.
        loadedTextures.insert(std::make_pair(filename, newTexture));

        return newTexture;
    }

private:

    // Texture filename + Texture object.
    // Ensures each texture/image in only loaded once.
    std::map<std::string, TexturePtr> loadedTextures;
};

Client code that uses this hypothetical TextureManger would look something like this:

void loadTextures()
{
    TextureManager & texMgr = TextureManager::getInstance();

    TexturePtr hero   = texMgr.load("assets/hero.png");
    TexturePtr badie1 = texMgr.load("assets/badie1.png");
    TexturePtr badie2 = texMgr.load("assets/badie2.png");
    TexturePtr badie3 = texMgr.load("assets/badie3.png");

    // Use the textures. I.e.: Call `hero->drawFrame()`, etc...
    //
    // If you need to, for instance, call 
    //  `texMgr.load("assets/badie1.png");` in the constructor
    // of several objects, like inside the constructor of an `Enemy` class,
    // There would be no redundant loads of "badie1.png". The first object
    // to reference that texture would load the image; other references would 
    // simply return the instance cached in the TextureManager's map.
}

Other details regarding the current code you have:

  • dispose() is a weak name for that method. It doesn't make it clear that it disposes all textures, regardless if they are still in use or not. I would give it a "stronger" name, such as purgeAllTextures() or forceDisposeAll(). By the way, you don't seem to clear() m_textureMap inside dispose()! That is probably a mistake as it will leave the map full of invalid SDL_Texture pointers!

  • m_textureMap is public. This is a bad idea. Outsiders of TextureManager shouldn't be able to modify the internal cache. This data member should be made private.

  • Can you use / have a compiler with C++11? If so, take a look at std::unordered_map. This container is exactly like std::map, but instead uses a hash-table underneath the hood, which makes lookups even faster.

  • Also, if using C++11, then prefer the nullptr literal for pointers, rather than 0.

  • You have correctly used cout for debug output and cerr for error log. One thing to note though is that eventually the output of your program might become quite verbose and annoying. When that happens, it would be a pain to have to go hunting down for cout calls through your code. You can make you life easier in the future if you replace the raw cout/cerr calls with a simple wrapping macro that can be later disabled or changed if output becomes too verbose:

    #define LOG_COMMENT(msg) do { std::cout << msg << "\n"; } while (0)
    #define LOG_ERROR(msg)   do { std::cerr << "ERROR: " << msg << "\n"; } while (0)
    
\$\endgroup\$
0
8
\$\begingroup\$

Over Complex Singelton

This is over complex because you are using pointers.
It also leaks resources at the end (luckily no destructor) but there is a principle holds. You should make sure you objects are correctly destroyed.

    static TextureManager* Instance()
    {
        if(s_pInstance == 0)
        {
            std::cout << "Created TextureManager singleton\n";

            s_pInstance = new TextureManager();
            return s_pInstance;
        }
        //point to same object (singleton pattern)
        return s_pInstance;
    }
    static TextureManager* s_pInstance;

The classic Singelton pattern looks like this:

    static TextureManager& instance()
    {
        // Notice this is a function static member.
        // This means it is created the first time instance() is called
        // and correctly destroyed at the end of the application.
        static TextureManager instance;
        return instance;
    }

Singeltons and copying

You don't want your singelton copied so disable the copy and assignment operators.

class TextureManager
{
    // In C++11
    TextureManager(TextureManager const&) = delete;
    TextureManager& operator=(TextureManager const&) = delete;

    // In C++03
    private:
    TextureManager(TextureManager const&); /* Don't define */
    TextureManager& operator=(TextureManager const&); /* Don't define */

Singelton is an anti-pattern

The singelton is a bit of an anti-pattern. They make testing hard.

It is often best to combine a singelton with a creation pattern (to allow yourself the luxury of creating different types of singelton for different things (ie testing)).

But even better is not to use a singelton. Pass the Texture manager around by reference.

Prefer not to pass pointers.

Pointers do not convey ownership information. So the user of your class does not know if they are passing ownership of the object to your methods (or even if the object should be dynamically allocated).

 // This interface
 bool load(std::string fileName, std::string id, SDL_Renderer* pRenderer);

Is the pRenderer object allocated dynamically? Is your function taking ownership and will release the object when it is finished. Is your object going to take shared ownership of the object (if so when does it release its claim on ownership) how do I know its safe to delete this resource if you are keeping a copy?

Look up Smart pointers and ownership symantics.

\$\endgroup\$
6
  • \$\begingroup\$ pRenderer is actually a pointer to a SDL fonstion : SDL_Renderer pRenderer = SDL_ CreateRenderer(p_Window); But I think you're right about the singleton : I should've overloaded the = & the copy constructor. \$\endgroup\$
    – MattMatt
    Apr 19, 2015 at 21:52
  • \$\begingroup\$ Does not matter where the pointer comes from. You have no ownership semantics defines. Thus using your function/methods becomes error prone. \$\endgroup\$ Apr 19, 2015 at 23:13
  • \$\begingroup\$ Thanks :), I agree with the fact that you must overload the copy constructor, and the =operator; \$\endgroup\$
    – MattMatt
    Apr 20, 2015 at 16:57
  • \$\begingroup\$ If that's the only bit you agree about then you are missing the whole point of C++ :-( You may as well just use Java :-( \$\endgroup\$ Apr 20, 2015 at 17:26
  • \$\begingroup\$ Sorry, I agree with the rest of what you said, but the problem is I've already implemented this class in my program, and it would be hard to change the return type of the method ! Can you come up with a solution with which the return type is still a pointer ? \$\endgroup\$
    – MattMatt
    Apr 21, 2015 at 17:51
0
\$\begingroup\$

You extract that code from the SDL Book, It use all Classes as Singleton, and I don't like that, but well. The code is fine. I rewrite the manager to this: I separate the texture and texture manager.

class CTexture
{
public:
    CTexture();
    ~CTexture();

    void SetRenderer(SDL_Renderer *pRenderer);
    void SetFont(TTF_Font *pFont);

    int GetWidth() const;
    int GetHeight() const;

    int GetRenderWidth() const;
    int GetRenderHeight() const;

    int Render(int x, int y, double angle = 0.0, SDL_RendererFlip flip = SDL_FLIP_NONE);
    int Render(int x, int y, int currentRow, int currentFrame, double angle = 0.0, SDL_RendererFlip flip = SDL_FLIP_NONE);

    SDL_Texture *GetTexture() const;

    bool LoadFromSurface(SDL_Surface *pSurface);
    bool LoadFromFile(std::string filename);
    bool LoadFromRenderedText(std::string textureText, SDL_Color textColor);

    void SetColor(Uint8 r, Uint8 g, Uint8 b);
    void SetBlendMode(SDL_BlendMode blending);
    void SetAlpha(Uint8 a);

    void SetScale(int scale);
    int GetScale() const;

    void SetClipSize(int width, int height);

    void Free();

private:

    SDL_Texture *m_pTexture;
    SDL_Renderer *m_pRenderer;
    TTF_Font *m_pFont;

    int m_iWidth;
    int m_iHeight;

    int m_iScale;

    int m_cWidth;
    int m_cHeight;

    bool m_isClip;
    bool m_isText;
};

// Mejorar el Formato del Texto!! YA!
class CTextureManager
{
public:
    CTextureManager(SDL_Renderer *pRenderer);
    void SetRenderer(SDL_Renderer *pRenderer);

    ~CTextureManager();

    // Load File function
    CTexture *LoadFromFile(std::string name, std::string filename);
    CTexture *LoadFromRenderedText(std::string name, std::string text, SDL_Color textColor);

    bool UnLoad(std::string name);

    CTexture *GetTexture(std::string name);

    void ClearAll();

    //TTF Font
    void SetFont(std::string fontPath, int size);


protected:
    std::map<std::string, CTexture *> m_mapTexture;
    SDL_Renderer *m_pRenderer;

    TTF_Font *m_pFont;
};

And the .cpp

#include "CTextureManager.h"


void CTexture::SetClipSize(int width, int height)
{
    m_isClip = true;

    m_cWidth = width;
    m_cHeight = height;
}

int CTexture::Render(int x, int y, int currentRow, int currentFrame, double angle /*= 0.0*/, SDL_RendererFlip flip /*= SDL_FLIP_NONE*/)
{
    if (!m_pTexture)
        return -1;

    if (!m_isClip)
        return -1;

    SDL_Rect scrRect = { m_cWidth * currentFrame, m_cHeight * (currentRow - 1), m_cWidth, m_cHeight };
    SDL_Rect desRect = { x, y, m_cWidth*m_iScale, m_cHeight*m_iScale };

    return SDL_RenderCopyEx(m_pRenderer, m_pTexture, &scrRect, &desRect, angle, 0, flip);
}

int CTexture::Render(int x, int y, double angle /*= 0.0*/, SDL_RendererFlip flip /*= SDL_FLIP_NONE*/)
{
    if (!m_pTexture)
        return -1;

    // Is necessary srcRect??
    //SDL_Rect srcRect = { 0, 0, m_width, m_height };
    SDL_Rect desRect = { x, y, m_iWidth*m_iScale, m_iHeight*m_iScale };

    return SDL_RenderCopyEx(m_pRenderer, m_pTexture, NULL, &desRect, angle, 0, flip);
}

bool CTexture::LoadFromRenderedText(std::string textureText, SDL_Color textColor)
{
    if (m_pFont == NULL)
        return false;


    SDL_Surface *pTempSurface = TTF_RenderText_Solid(m_pFont, textureText.c_str(), textColor);

    if (pTempSurface == NULL)
        return false;

    SDL_Texture *pNewTexture = SDL_CreateTextureFromSurface(m_pRenderer, pTempSurface);

    if (pNewTexture == NULL)
    {
        SDL_FreeSurface(pTempSurface);
        return false;
    }

    //Remove Preview Data!!
    Free();

    //Load New Texture!!
    m_pTexture = pNewTexture;
    m_iWidth = pTempSurface->w;
    m_iHeight = pTempSurface->h;
    m_isText = true;

    SDL_FreeSurface(pTempSurface);
    return true;
}

void CTexture::SetFont(TTF_Font *pFont)
{
    m_pFont = pFont;
}

void CTexture::SetRenderer(SDL_Renderer *pRenderer)
{
    m_pRenderer = pRenderer;
}

bool CTexture::LoadFromFile(std::string filename)
{
    SDL_Surface *pTempSurface = IMG_Load(filename.c_str());

    if (pTempSurface == NULL)
        return false;

    SDL_Texture *pNewTexture = SDL_CreateTextureFromSurface(m_pRenderer, pTempSurface);

    if (pNewTexture == NULL)
    {
        SDL_FreeSurface(pTempSurface);
        return false;
    }

    //Remove Preview Data!!
    Free();

    //Load New Texture!!
    m_pTexture = pNewTexture;
    m_iWidth = pTempSurface->w;
    m_iHeight = pTempSurface->h;
    m_isText = false;

    SDL_FreeSurface(pTempSurface);
    return true;
}

bool CTexture::LoadFromSurface(SDL_Surface *pSurface)
{
    SDL_Texture *pNewTexture = SDL_CreateTextureFromSurface(m_pRenderer, pSurface);

    if (pNewTexture == NULL)
        return false;

    //Remove Preview Data!!
    Free();

    //Load New Texture!!
    m_pTexture = pNewTexture;
    m_iWidth = pSurface->w;
    m_iHeight = pSurface->h;
    m_isText = false; // No sabemos, ante la duda, no es texto!!

    return true;
}

void CTexture::SetAlpha(Uint8 a)
{
    SDL_SetTextureAlphaMod(m_pTexture, a);
}

void CTexture::SetColor(Uint8 r, Uint8 g, Uint8 b)
{
    SDL_SetTextureColorMod(m_pTexture, r, g, b);
}

void CTexture::Free()
{
    if (m_pTexture != nullptr)
    {
        SDL_DestroyTexture(m_pTexture);
        m_iWidth = 0;
        m_iHeight = 0;
    }
}

SDL_Texture * CTexture::GetTexture() const
{
    return m_pTexture;
}

CTexture::~CTexture()
{
    Free();
}

CTexture::CTexture()
{
    m_iWidth = 0;
    m_iHeight = 0;

    m_iScale = 1;

    m_pTexture = nullptr;
}

int CTexture::GetWidth() const
{
    return m_iWidth;
}

int CTexture::GetRenderWidth() const
{
    return m_iWidth*m_iScale;
}


int CTexture::GetHeight() const
{
    return m_iHeight;
}

int CTexture::GetRenderHeight() const
{
    return m_iHeight*m_iScale;
}

void CTexture::SetScale(int scale)
{
    m_iScale = scale;
}

int CTexture::GetScale() const
{
    return m_iScale;
}

CFont::CFont(std::string fontpath, int size)
{

}

CFont::~CFont()
{
    if (m_pFont != nullptr)
    {
        TTF_CloseFont(m_pFont);
    }
}

CTextureManager::CTextureManager(SDL_Renderer *pRenderer)
{
    m_pRenderer = pRenderer;
}

CTextureManager::~CTextureManager()
{
    ClearAll();

    if (m_pFont != nullptr)
    {
        TTF_CloseFont(m_pFont);
    }
}

void CTextureManager::SetFont(std::string fontPath, int size)
{
    if (m_pFont)
    {
        TTF_CloseFont(m_pFont);
    }

    m_pFont = TTF_OpenFont(fontPath.c_str(), size);
}

void CTextureManager::ClearAll()
{
    for (std::map<std::string, CTexture *>::iterator it = m_mapTexture.begin(); it != m_mapTexture.end(); ++it)
    {
        delete (it->second);
        (it->second) = nullptr;
    }
    m_mapTexture.clear();
}

CTexture * CTextureManager::GetTexture(std::string name)
{
    return m_mapTexture[name];
}

bool CTextureManager::UnLoad(std::string name)
{
    CTexture *pTempTexture = m_mapTexture[name];

    if (!pTempTexture)
        return false;

    delete pTempTexture;
    pTempTexture = nullptr;
    m_mapTexture.erase(name);
    return true;
}

CTexture * CTextureManager::LoadFromRenderedText(std::string name, std::string text, SDL_Color textColor)
{
    if (!m_pFont)
        return NULL;

    CTexture *pTexture = m_mapTexture[name];

    if (pTexture == nullptr)
    {
        pTexture = new CTexture();
        pTexture->SetRenderer(m_pRenderer);

        m_mapTexture[name] = pTexture;
    }

    if (!pTexture->LoadFromRenderedText(text, textColor))
        return false;

    return pTexture;
}

CTexture * CTextureManager::LoadFromFile(std::string name, std::string filename)
{
    CTexture *pTexture = m_mapTexture[name];

    if (pTexture == nullptr)
    {
        pTexture = new CTexture();
        pTexture->SetRenderer(m_pRenderer);

        m_mapTexture[name] = pTexture;
    }

    if (!pTexture->LoadFromFile(filename))
        return nullptr;

    return pTexture;
}

void CTextureManager::SetRenderer(SDL_Renderer *pRenderer)
{
    m_pRenderer = pRenderer;
}

PD: They have some error, and not finished.

\$\endgroup\$
18
  • \$\begingroup\$ Thanks a LOT !!! I've tried out your code, and it works perfectly for me ! ;I've just made little modifications I can suggest to you :) -> 1) Separate the texture "font", and "image" texture; I think it's not necessary to construct a big object like this ! Split them into two classes : "Texture" & "Font Texture !"; -> I'm not sure about the setRenderer() method : what if the renderer is changed a second time ... -> finnally, I overloaded the = and copy constructor because we are using pointers :°). \$\endgroup\$
    – MattMatt
    Apr 22, 2015 at 18:54
  • \$\begingroup\$ I have font in another class, but I don't finish. So I remove it when i post here. Yes. Text and Texture are butter in separted class. ;) \$\endgroup\$
    – ReymonARG
    Apr 23, 2015 at 0:30
  • \$\begingroup\$ Is render change, you will set that again, is like to pass the render to the SDL_RenderCopy func! \$\endgroup\$
    – ReymonARG
    Apr 23, 2015 at 0:31
  • \$\begingroup\$ Or maybe, it would be better to make a singleton called AssetManager, which could manage Fonts, level files, ... like in the libgdx book ! (if you know Libgdx ?) \$\endgroup\$
    – MattMatt
    Apr 23, 2015 at 10:04
  • \$\begingroup\$ One thing I thought of was using integer type as an index in the textureMap : it would use up less memory ! We could store them as the follwing : \$\endgroup\$
    – MattMatt
    Apr 23, 2015 at 10:18
0
\$\begingroup\$

One thing I thought of was using an unsigned integer type as an index in the textureMap as it would use up less memory. We could store them in an std::vector as the following:

const unsigned int BRICK_TEXTURE_ID = 6542016; //Id of the texture : am I right ? 

///load method

int LoadFromFile(unsigned int ID, const char* fileName);
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.