Resource manager with SFML

Question

I decided to write a 2D game with SFML, and for the beginning of the game I wrote classes to manage resources like sounds, musics, textures... I would like to get reviews of my code: how would you have write the program, how can I improve it?

I also wrote a class "Logger" to make the debug of the game easier. I would like to get reviews of this class too.

ResourceManager.hpp

#ifndef RESOURCE_MANAGER_HPP
#define RESOURCE_MANAGER_HPP

#include "FontManager.hpp"
#include "MusicManager.hpp"
#include "TextureManager.hpp"
#include "SoundManager.hpp"

class ResourceManager
{
public:
    static ResourceManager& getInstance();

    void loadResources();

private:
    ResourceManager();

    ~ResourceManager();

    FontManager& fontManager = FontManager::getInstance();
    MusicManager& musicManager = MusicManager::getInstance();
    TextureManager& textureManager = TextureManager::getInstance();
    SoundManager& soundManager = SoundManager::getInstance();
};

#endif // RESOURCE_MANAGER_HPP

ResourceManager.cpp

#include "ResourceManager.hpp"

ResourceManager::ResourceManager()
{

}

ResourceManager::~ResourceManager()
{

}

ResourceManager& ResourceManager::getInstance()
{
    static ResourceManager instance;
    return instance;
}

void ResourceManager::loadResources()
{
    musicManager.addMusic("music1", "music1.wav");
    musicManager.addMusic("music2", "music2.wav");
    fontManager.addFont("font1", "font1.ttf");
    fontManager.addFont("font2", "font2.ttf");
    textureManager.addTexture("texture1", "texture1.png");
    soundManager.addSound("sound1", "sound1.wav");
}

SoundManager.hpp

#ifndef SOUND_MANAGER_HPP
#define SOUND_MANAGER_HPP

#include "Logger.hpp"
#include <unordered_map>
#include <SFML/Audio.hpp>

class SoundManager
{
public:
    static SoundManager& getInstance();

    int addSound(const std::string& id, const std::string& soundPath);

    std::unique_ptr<sf::Sound> getSound(const std::string &id);

private:
    SoundManager();

    ~SoundManager();

    std::unordered_map<std::string, std::unique_ptr<sf::Sound>> soundMap_;
    std::vector<sf::SoundBuffer> soundBufferVec_;

};

#endif // SOUND_MANAGER_HPP

SoundManager.cpp

#include "SoundManager.hpp"

SoundManager::SoundManager()
{

}

SoundManager::~SoundManager()
{

}

SoundManager& SoundManager::getInstance()
{
    static SoundManager instance;
    return instance;
}

int SoundManager::addSound(const std::string& id, const std::string& soundPath)
{
    auto it = soundMap_.find(id);
    if(it != soundMap_.end())
    {
        Logger::getInstance().log(LogLevel::ERROR, "Unable to add sound: '"
            + id + "' already exists");
        return -1;
    }

    std::unique_ptr<sf::SoundBuffer> soundBuffer = 
        std::make_unique<sf::SoundBuffer>();

    if(!soundBuffer->loadFromFile(soundPath))
    {
        Logger::getInstance().log(LogLevel::ERROR, "Unable to open sound '" + 
            soundPath + "'");
        return -1;
    }
    soundBufferVec_.push_back(std::move(*(soundBuffer)));
    unsigned int soundBufferVecSize = soundBufferVec_.size();
    std::unique_ptr<sf::Sound> sound = std::make_unique<sf::Sound>
        (std::move(soundBufferVec_[soundBufferVecSize - 1]));

    soundMap_.emplace(id, std::move(sound));

    Logger::getInstance().log(LogLevel::INFO, "'" + soundPath + 
        "' has been added");
    return 0;
}

std::unique_ptr<sf::Sound> SoundManager::getSound(const std::string &id)
{
    auto it = soundMap_.find(id);
    if(it == soundMap_.end())
    {
        Logger::getInstance().log(LogLevel::ERROR, "Unable to load sound: " +
            id + " doesn't exist");
        return nullptr;
    }

    return std::move(it->second);
}

TextureManager.hpp

#ifndef TEXTURE_MANAGER_HPP
#define TEXTURE_MANAGER_HPP

#include "Logger.hpp"
#include <unordered_map>
#include <SFML/Graphics.hpp>

class TextureManager
{
public:
    static TextureManager& getInstance();

    int addTexture(const std::string& id, const std::string& texturePath);

    std::unique_ptr<sf::Texture> getTexture(const std::string &id);

private:
    TextureManager();

    ~TextureManager();

    std::unordered_map<std::string, std::unique_ptr<sf::Texture>> textureMap_;
};

#endif // TEXTURE_MANAGER_HPP

TextureManager.cpp

#include "TextureManager.hpp"

TextureManager::TextureManager()
{

}

TextureManager::~TextureManager()
{

}

TextureManager& TextureManager::getInstance()
{
    static TextureManager instance;
    return instance;
}

int TextureManager::addTexture(const std::string& id, 
    const std::string& texturePath)
{
    auto it = textureMap_.find(id);
    if(it != textureMap_.end())
    {
        Logger::getInstance().log(LogLevel::ERROR, "Unable to add texture: '"
            + id + "' already exists");
        return -1;
    }

    std::unique_ptr<sf::Texture> texture = std::make_unique<sf::Texture>();

    if(!texture->loadFromFile(texturePath))
    {
        Logger::getInstance().log(LogLevel::ERROR, "Unable to open texture '"
            + texturePath + "'");
        return -1;
    }

    textureMap_.emplace(id, std::move(texture));

    Logger::getInstance().log(LogLevel::INFO, "'" + texturePath + 
        "' has been added");
    return 0;
}

std::unique_ptr<sf::Texture> TextureManager::getTexture(const std::string &id)
{
    auto it = textureMap_.find(id);
    if(it == textureMap_.end())
    {
        Logger::getInstance().log(LogLevel::ERROR, "Unable to load texture: " +
            id + " doesn't exist");
        return nullptr;
    }

    return std::move(it->second);
}

Logger.hpp

#ifndef LOGGER_HPP
#define LOGGER_HPP

#include <sstream>
#include <iostream>

enum class LogLevel
{
    NO = 0,
    ERROR = 1,
    INFO = 2,
    DEBUG = 3
};

class Logger
{
public:
    static Logger& getInstance();

    void setLogLevel(const LogLevel& logLevel);

    void log(const LogLevel& messageLevel, const std::string& message);


private:
    Logger();

    ~Logger();

    void setPrefix(const LogLevel& messageLevel);

    std::string logPrefix_;
    LogLevel currentLogLevel_;
    LogLevel messageLevel_;

};

#endif // LOGGER_HPP

Logger.cpp

#include "Logger.hpp"

Logger::Logger()
{

}

Logger::~Logger()
{

}

void Logger::setPrefix(const LogLevel& messageLevel)
{
    switch(messageLevel)
    {
        case LogLevel::DEBUG:
            logPrefix_ = "[DEBUG] ";
            break;
        case LogLevel::INFO:
            logPrefix_ = "[INFO] ";
            break;
        case LogLevel::ERROR:
            logPrefix_ = "[ERROR] ";
            break;
        default:
            break;
    }
}

Logger& Logger::getInstance()
{
    static Logger instance;
    return instance;
}

void Logger::setLogLevel(const LogLevel& logLevel)
{
    currentLogLevel_ = logLevel;
}


void Logger::log(const LogLevel& messageLevel, const std::string& message)
{
    if(messageLevel == LogLevel::NO)
        return;
    if(currentLogLevel_ != LogLevel::DEBUG)
    {
        if(messageLevel == LogLevel::ERROR && currentLogLevel_ !=
            LogLevel::ERROR)
            return;
        if(messageLevel == LogLevel::INFO && currentLogLevel_ !=
            LogLevel::INFO)
            return;
    }

    setPrefix(messageLevel);

    std::cout << logPrefix_ << message << std::endl;
}

How to use this classes

main.cpp

#include <iostream>
#include "Logger.hpp"
#include "ResourceManager.hpp"

int main()
{
    Logger::getInstance().setLogLevel(LogLevel::DEBUG);
    ResourceManager& resourceManager = ResourceManager::getInstance();
    resourceManager.loadResources();

    std::unique_ptr<sf::Texture> texture = TextureManager::getInstance().
        getTexture("texture1");
    if(texture == nullptr)
        return -1;

    sf::RenderWindow window(sf::VideoMode(800, 600), "Window");
    sf::Sprite sprite;
    sprite.setTexture(*(texture));

    std::unique_ptr<sf::Sound> sound = SoundManager::getInstance().
        getSound("sound1");
    if(sound == nullptr)
        return -1;

    while(window.isOpen())
    {
        sf::Event event;
        while(window.pollEvent(event))
        {
            if(event.type == sf::Event::Closed)
                window.close();
            else if(event.type == sf::Event::KeyPressed)
                sound->play();

        }

        window.clear(sf::Color::Black);
        window.draw(sprite);
        window.display();
    }

    return 0;
}

I don't post the two manager classes (FontManager and MusicManager) because the code is fundamentally the same as SoundManager and TextureManager.

Answer 1

Design - Singletons

A singleton may be a reasonable choice for the logger (it's used everywhere and there should only be one of it).

But for the other systems, it's best to stick with ordinary class instances, and pass references to them around as necessary. This may seem like more work, but it makes dependencies clearer. It also makes the lifetimes and order of initialisation and destruction of the systems well defined.

The various managers could simply be public members of the ResourceManager.

---

Code - SoundBuffer

The SoundBuffer loading could be simplified a bit:

sf::SoundBuffer soundBuffer;

if(!soundBuffer.loadFromFile(soundPath))
{
    Logger::getInstance().log(LogLevel::ERROR, "Unable to open sound '" + 
        soundPath + "'");
    return -1;
}

soundBufferVec_.push_back(std::move(soundBuffer));

std::unique_ptr<sf::Sound> sound = std::make_unique<sf::Sound>(soundBufferVec_.back());

However, from the SFML docs it looks like the SoundManager should actually be storing sf::SoundBuffers, not sf::Sounds. The sf::SoundBuffer is the heavyweight object that's time consuming to load, whereas an sf::Sound can have a shorter lifetime, and be bound to a buffer whenever needed.

---

Code - Error Values

int add*(id, path);

Don't return integers as an indicator of whether something worked or not. A boolean would be better as it has only two possible values, indicating success or failure (it looks like SFML does it this way).

But even then it's not explicit in the code as to what true or false mean, so an enum class LoadResult { LoadSuccess, LoadFailure }; might be a further improvement.

Or... since ResourceManager::loadResources() ignores the return codes, we could just return void!

---

Design - Removing Duplicate Code

If the SoundManager is changed to store sf::SoundBuffers, as mentioned above, then the only significant difference between the manager classes is the add* function.

The obvious fix for this is to use one templated AssetManager (or similarly named) class.

Rather than defining different add member functions for different types (which could be done with template specialization), it's simpler to move the actual loading logic out of the AssetManager class, leaving it as a simple wrapper around a map:

template<class AssetT>
class AssetManager
{
public:

    void addAsset(const std::string& id, std::unique_ptr<AssetT> asset);
    std::unique_ptr<AssetT> getAsset(const std::string& id);

private:

    std::unordered_map<std::string, std::unique_ptr<AssetT>> assetMap_;
};

template<class AssetT>
void AssetManager<AssetT>::addAsset(const std::string& id, std::unique_ptr<AssetT> asset)
{
    // check it's not already there, then add it to the map
}

template<class AssetT>
std::unique_ptr<AssetT> AssetManager<AssetT>::getAsset(const std::string& id)
{
    // find and return / log error
}

Instances of each type of asset manager can be members of ResourceManager:

    AssetManager<sf::Texture> textureManager;
    AssetManager<sf::SoundBuffer> soundManager;
    // ...

Then we can define member functions in ResourceManager to do the actual asset loading and call addAsset() on the relevant manager.

void ResourceManager::loadTexture(const std::string& id, const std::string& path)
{
    // load as before
    // add texture to the relevant asset manager
}

Note that the code using the asset always knows what type of asset it expects, so it can call getAsset() with the correct type:

auto texture = resourceManager.textureManager.getAsset<sf::Texture>("id");

---

Design - Pointer Type

What if you want to use the same texture (or whatever) in more than one place? At the moment, the asset is stored in a std::unique_ptr, and in getFoo() the ownership of it is passed out of the FooManager.

Even though the entry is still present in the map, if you ask for that same asset a second time, it'll give you an empty pointer.

To fix this, the manager should either return a non-owning raw pointer (and keep ownership of the std::unique_ptr), or we can use a std::shared_ptr instead. With std::shared_ptr, the number of shared pointers to the asset is counted, and the object only destroyed when the very last one (probably the one still in the asset manager) expires. e.g.:

template<class AssetT>
std::shared_ptr<AssetT> AssetManager<AssetT>::getAsset(const std::string& id)
{
    // find or error...

    return it->second; // note no move... we create another shared pointer pointing to the same object
}

---

Design - C++ Fun

The above removes the code duplication in the managers, and there's no real need to go any further. Feel free to ignore everything from this point on!

std::shared_ptr has another feature we can take advantage of. Creating a std::shared_ptr<void> is similar to using a void* in C. It can store an object of any type.

This means that instead of creating several different AssetManager instances (one for each type of asset) we can alter the code to have only one AssetManager instance that can store assets of any type.

The snippet below is an example of such a class from one of my own projects:

namespace Assets
{

    class Cache
    {
    public:

        Cache() = default;

        PIC_NOT_COPYABLE(Cache); // deletes copy constructor and copy assignment operator
        PIC_NOT_MOVEABLE(Cache); // deletes move constructor and move assignment operator

        template<class T>
        void Add(ID const& id, std::shared_ptr<T> const& t);

        template<class T>
        std::shared_ptr<T> Find(ID const& id);

        void Clear();

    private:

        struct TypeID
        {
            std::type_index TypeIndex;
            ID ID;
        };

        struct TypeIDHash
        {
            std::size_t operator()(TypeID const& typeID) const;
        };

        struct TypeIDEquality
        {
            bool operator()(TypeID const& a, TypeID const& b) const;
        };

        using DataT = std::unordered_map<TypeID, std::shared_ptr<void>, TypeIDHash, TypeIDEquality>;
        DataT m_data;
    };

    template<class T>
    void Cache::Add(ID const& id, std::shared_ptr<T> const& t)
    {
        PIC_ASSERT(t != nullptr);

        auto entry = m_data.emplace(TypeID{ std::type_index(typeid(T)), id }, t);

        PIC_ASSERT(entry.second);
    }

    template<class T>
    std::shared_ptr<T> Cache::Find(ID const& id)
    {
        auto entry = m_data.find({ std::type_index(typeid(T)), id });

        if (entry == m_data.end())
            return std::shared_ptr<T>();

        return std::static_pointer_cast<T>(entry->second);
    }

} // Assets

The main difference is that we pair the asset id string with a std::type_index in the key of the map. This is necessary to find the right type of asset later, as we've discarded type information by choosing to use std::shared_ptr<void>. It also allows assets of different types to have the same id and keep separate entries in the map.

The load functions remain similar to the ones above, but they all put the loaded asset into the same cache instance.

Answer 2

No singletons

The singleton pattern tries to solve exactly one problem and that is to make sure you always just have one single instance of an object. The issue with your code is, that this requirement isn't needed. Why should there only ever be one resource manager? Are there any critical issues when there are more than one?

As such the singleton pattern here is misused to provide single global access to the managers, which leads directly into the next section.

No global access

While it's a neat idea to have global access to the managers, as you can access them everywhere you need them, in the long run it will cause more issues than it solves, because you can't really track the state of the managers.

Being able to easily reason about the flow of the code and the states of the objects involved becomes more important the larger the project gets. If one disconnected piece of code can have side effects on the other piece of code, then being able to reason about the state is really hard and you may end up with bugs that take you forever to debug.

The better approach is dependency injection, meaning you pass in the managers via constructor to all the classes that need access to. This will clear up the code flow and as a benefit of doing so, you will have to rethink some of your design decisions, because you really don't want to inject the managers everywhere, but only at the places where they are really needed. Generally by decoupling classes, you'll end up with a better design.

One useful tool for not having to pass down the managers to every object is to use some form of factory pattern. Your object/entity managing class has access to the resource managers and creates the objects/entities and as such can provide every entity the needed resource directly, without having to pass down the whole manager.

std::unique_ptr means ownership

When returning the resources, you're actually returning a std::unique_ptr, but a unique_ptr really owns the object, as such by returning a unique_ptr you're ripping out the owner ship of it from the manager, making the manager not really a manager but more a "loader".

I think there is even a bug relating to this, because once you do std::move(it->second) you're really removing the resource from the manager, so if you ever tried to access the same resource again, you'll run into moved-away object and undefined behavior.

It's better to actually return a reference to the resource and leave the ownership with the manager. Alternatively, you may also use shared ownership with a std::shared_ptr. Then the object will be kept alive as long as something is still using a shared_ptr instance.

Error handling

This is a very opinionated topic, as such if you feel confident with your current solution maybe stick to it. In my book you either have functions that you know can fail in one way, in that case you use a boolean as return type. Or you have functions that shouldn't really fail, but when there's something that can go wrong that is out of your control (e.g. file doesn't exist), you throw an exception. Additionally you may want to provide checks, so the user code can actually first check if something works and doesn't have to just try and then react to it.

Examples

As most SFML users will end up writing some form of resource manager it's recommended to check what others have created in the past. The SFML Game Development book has a neat implementation of a ResourceHolder. Since the same author is also the maintainer of Thor, you'll also find a very similar implementation in Thor.

Logging

Again, singleton usage doesn't really make sense here, because why should you not be able to have multiple instance of a logger?

Personally I think it's better to just go with an existing logging library, as they provide needed optimizations and good approaches to logging (file, console, pipe, etc.). In one of my latest project I used spdlog. It's easy to setup and seems to work nicely.