2
\$\begingroup\$

I need to write a TextureManager class which stores Texture objects; the problem is I'm using an std::map<Texture *, const char*> map to store Texture objects using a key.

Searching elements through a map is not ideal because of performance issues (I'm writing a C++ library, so performance is important).

I've had the idea to use an std::vector<Texture*>. In this case, I need to use an unsigned int. But after several tries, I still cannot implement it.

Here is the .h:

class CTextureManager
{
public:
    CTextureManager(SDL_Renderer *pRenderer);
    void SetRenderer(SDL_Renderer *pRenderer);

    ~CTextureManager();

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

    bool UnLoad(std::string name);

    CTexture *GetTexture(std::string name);

    void ClearAll();


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

};

and the .cpp:

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

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

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

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::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;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Too many pointers. Learn "Ownership Semantics" otherwise this is just Java written like C++ and thus a fail. \$\endgroup\$ – Martin York Apr 23 '15 at 17:16
  • \$\begingroup\$ I agree, but how should I take pRenderer as an argument? By reference ? It's true that I'm also a Java programmer, and I know that in Java every Object is considered as a pointer to an object... \$\endgroup\$ – MattMatt Apr 23 '15 at 19:02
  • \$\begingroup\$ In C++ there are 4 different types of variable. Dynamic, Automatic, Static and Thread. Static/Thread are less common. Java objects map (approximately) to C++ Dynamic. But it is much more common to use Automatic objects in C++. Dynamic object (those created by new) are usually wrapped inside an Automatic object (Smart-Pointer or container) so that memory management is done correctly (and automatically). \$\endgroup\$ – Martin York Apr 23 '15 at 19:59
  • \$\begingroup\$ how should I take pRenderer as an argument? That depends. Are you passing ownership? If not then pass by reference. If you are passing ownership wrap the render object in a smart pointer (std::unique_ptr) to indicate the ownership transfer. \$\endgroup\$ – Martin York Apr 23 '15 at 20:01
  • \$\begingroup\$ Actually, pRenderer* is a pointer to an SDL function, so I want m_pRenderer* to point where the pRenderer* is. Is & enough ? \$\endgroup\$ – MattMatt Apr 24 '15 at 7:09
3
\$\begingroup\$

The interface is badly designed:

  • Are you allowed to pass nullptr here?
  • Is the object taking ownership of the pointer?

    CTextureManager(SDL_Renderer *pRenderer);
    
  • Are you allowed to change the renderer?

  • Are you allowed to replace it with nullptr?
  • Is the ownership being passed to the CTextureManager object?

    void SetRenderer(SDL_Renderer *pRenderer);
    
  • Passing name and filename by value. This is causing a copy. This is inefficient and not required. Pass by const reference.

  • The returned value:
  • Is it allowed to be nullptr?
  • Who owns the pointer?

    CTexture *LoadFromFile(std::string name, std::string filename);
    
  • Another pass by value.

  • What does the return value indicate?

    bool UnLoad(std::string name);
    
  • Pass by value again.

  • Who owns the returned object?

    CTexture *GetTexture(std::string name);
    

You are storing pointers.

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

Which means your class is doing both resource management and business logic. A class should do one or the other (search for Separation of Concerns). Split your resource management into its own class. Then use this inside your texture manager.

Ownership semantics.

Your Major problem is you are not correctly indicating the ownership semantics of the objects.

Ownership is all about who is responsible for deleting a dynamically allocated object and is one of the core principles of C++ (the one that raises C++ above C in terms of memory management). If you don't define these semantics well in the interface then people have to actually dig into the implementation to understand how the ownership is working so that they can correctly write their code.

If people are digging into your code to understand the ownership semantics they are now depending on implementation details of your code. This makes your code very brittle to any future changes and tightly couples any code using your class.

  • Return by reference to indicate that ownership is not being transferred.
  • Pass by reference to indicate that you are not accepting ownership.
  • Use smart pointers to pass unique/shared ownership objects around.
  • Never pass by pointer across a public interface
    • its fine to use internally but it should not be part of your public interface to other code.

Prefer to use the constructors initialization list.

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

// When you have members that have constructors.
// These will be called before the body of the constructor is
// entered.
//
// By initializing in the body of the code you
// are first constructing the object into the default state
// the updating that state. It is better to call the constructor
// directly with the appropriate parameters.
//
// Also reference must be done in the initialization list.
// So prefer to do all initialization in the initialization list
// for consistency.
TextureManager::CTextureManager(SDL_Renderer *pRenderer)
    : m_pRenderer(pRenderer)
{}

Destructor

You destructor is not virtual.

CTextureManager::~CTextureManager()

This means you are not expecting to have any sub types of the object. Which is fine. But you should keep it in mind.

ERROR

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

m_pFont is not a member of the class. So I expect this would not compile.

Clear All

    for (std::map<std::string, CTexture *>::iterator it = m_mapTexture.begin(); it != m_mapTexture.end(); ++it)

   // Prefer to use `auto` for types you don't care about.
   // Also prefer to use the std::begin() and std::end() rather than
   // calling the methods directly. This will allow you to change the
   // container type in the future without having to change the code.
    for (auto it = std::begin(m_mapTexture); it != std::end(m_mapTexture); ++it)

   // Even better would be to use the new range based for
   for(auto& value: m_mapTexture)

   // Or potentially an algorithm:
   std::for_each(td::begin(m_mapTexture), std::end(m_mapTexture), /* ACTION */);

Don't see any need to set this to nullptr:

        (it->second) = nullptr;

When you are finished you clear the container. So the values no longer exist.

GetTexture

On std::map the operator[] will insert a value into the container if it does not exist. I don't think that is what you actually want.

    return m_mapTexture[name];

UnLoad

Again with the insert.

    CTexture *pTempTexture = m_mapTexture[name];

Don't see the need for this test.

    if (!pTempTexture)
        return false;

Deleting a nullptr is fine.

    delete pTempTexture;

This does nothing useful (as it is about to go out of scope).

    pTempTexture = nullptr;

Since you inserted the name you may want to always delete it (even if the pointer is null).

    m_mapTexture.erase(name);

Just return the pointer here:

    return true;

The pointer will get converted to the correct bool. If you found nothing then it will be false. If you found something it will be true.

LoadFromFile

So your object is always a CTexture never anything derived from it.

        pTexture = new CTexture();

Since your texture is a single type I don't see the need to store pointers in the container. Just store the object directly.

Seems like the only reason for the texture manager to have a renderer is to make this call. Seems strange. Why not pass the renderer into the LoadFromFile() method?

        pTexture->SetRenderer(m_pRenderer);

You have just dynamically allocated and placed into your structure a CTecture. You may return a nullptr now. But a call to GetTexture() is going to return the previously created texture object (that failed to load). On a load failure you may want to remove this object from the structure.

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

This is how I would do it:

// I am guessing a bit because I have not read the SDL documentation.
struct SLDTextureDeleter
{
    void operator()(SDL_Texture* p){SDL_DestroyTexture(p);}
};
class CTextureManager;
class CTexture
{
        // Constructor is private so it can only be used by CTextureManager
        friend class CTextureManager;
        CTexture(std::string const& filename, CRenderer& renderer)
        {
            // Create Texture and load from File.
            // Failure to load leaves the texture pointer as nullptr.
            // The TextureManager tests this by calling ok() to validate
            // that the texture was loaded correctly.

        }
        bool ok() const {return texture.get();}
    public:
        // Allow texture moving
        CTexture(CTexture&& move)
            : texture(std::move(move.texture))
        {}
        CTexture& operator=(CTexture&& move)
        {
            using std::swap;
            swap(texture, move.texture);
        }
        // But disable Copy (as it is manager by CTextureManager)
        CTexture(CTexture const&)            = delete;
        CTexture& operator=(CTexture const&) = delete;
    private:
        std::auto_ptr<SDL_Texture, SLDTextureDeleter>   texture;
};
class CTextureManager
{
  public:
    bool      loadFromFile(std::string const& name, std::string const& filename, CRenderer& renderer);
    bool      hasTexture(std::string const& name) const;    
    CTexture& getTexture(std::string const& name) const;

    void clearAll();
    void unLoad(std::string const& name);    

  private:
    std::map<std::string, CTexture> mapTextures;
};

bool CTextureManager::loadFromFile(std::string const& name, std::string const& filename, CRenderer& renderer)
{
    CTexture  tmp(filename, renderer);
    bool result = tmp.ok();
    if (result)
    {
        mapTextures.emplace(name, std::move(tmp));
    }
    return result;
}
bool CTextureManager::hasTexture(std::string const& name) const
{
    auto find =   mapTextures.find(name);
    return find != mapTextures.end();
}
CTexture& CTextureManager::getTexture(std::string const& name) const;
{
    // Note: UB if loadFromFile() failed to return true.
    //          or if hasTexture() returned false.
    //          user is supposed to check before use.
    auto find =   mapTextures.find(name);
    return find->second;
}

void CTextureManager::clearAll()
{
    mapTextures.clear();
}
void CTextureManager::unLoad(std::string const& name)
{
    mapTextures.erase(name);
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I posted my previous Texture class so you can see the SDL implementation ! \$\endgroup\$ – MattMatt Apr 24 '15 at 9:35
  • \$\begingroup\$ IMPORTANT : I want the user to be able to acess the Texture class !!! As you can see, my texture class allows the user to change the Texture's alpha, RGB channels. The TextureManager class should just be a class which can help the user Manage Textures in an easier way by storing them in some sort a array ! (std::vector, std::map , ....); In some games you may want just a few Textures; in this case, it is not necessary to create a TextureManager; but in games where a 100 textures are needed, you just cannot manage without the TextureManager class !! \$\endgroup\$ – MattMatt Apr 24 '15 at 9:45
  • \$\begingroup\$ @MattMatt: You can. Use getTexture() returns a reference to a texture. From there you can call the appropriate methods. This is just to demonstrate the usage of correct ownership semantics. Fill in the appropriate methods you need. \$\endgroup\$ – Martin York Apr 24 '15 at 10:13
  • \$\begingroup\$ Just a little thing : can't we use std::vector ? It would save a really big time of texture look up !! \$\endgroup\$ – MattMatt Apr 24 '15 at 10:30
  • \$\begingroup\$ Yes , but I would like, as a user to construct a Texture object directly ! As iIsaid, I would like to keep the two class seperate, and keep the TextureManager class as an "addon" to the Texture class ! \$\endgroup\$ – MattMatt Apr 24 '15 at 10:32
2
\$\begingroup\$

I know that this is not the real problem but for more optimization, I think that you should be passing reference to some variables of your member function and not a copy~

PS: I like your coding style!

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ It's actually not mine ! We've been working on it with ReymonARG ! See this post for more information : codereview.stackexchange.com/questions/87367/texture-managing \$\endgroup\$ – MattMatt Apr 23 '15 at 16:03
  • \$\begingroup\$ This should be a comment, but I realize the issue. Here's a +1 so you may comment. Welcome to CodeReview. \$\endgroup\$ – Legato Apr 23 '15 at 16:08
  • \$\begingroup\$ If you like game programming, I recommend you check out Libgdx & SDL :-) \$\endgroup\$ – MattMatt Apr 23 '15 at 16:49
  • \$\begingroup\$ @MattMatt Thank you! I will check those out! \$\endgroup\$ – Motiron Apr 23 '15 at 16:51
  • \$\begingroup\$ I would recommend Cinder a truly C++ graphics library. \$\endgroup\$ – Martin York Apr 24 '15 at 10:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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