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);
}
Dynamic
,Automatic
,Static
andThread
. Static/Thread are less common. Java objects map (approximately) to C++ Dynamic. But it is much more common to useAutomatic
objects in C++.Dynamic
object (those created by new) are usually wrapped inside anAutomatic
object (Smart-Pointer or container) so that memory management is done correctly (and automatically). – Loki Astari Apr 23 '15 at 19:59how 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. – Loki Astari Apr 23 '15 at 20:01