Resource management classes in C++ game

Question

This is a part of simple sfml C++ game, but I think neither the library nor the language is that much crucial here. I am mainly worried about the design.

I have a class template ResourceHolder<Key, Resource> to hold one type of Resource such as texture or sound, inside a map. A key is usually an enum but can be anything.

I also have a class ResourceManager that keeps all available ResourceHolders in one place.

A simplified class diagram:

I am providing the full code for those classes:

ResourceHolder.h:

template <typename Key, typename Resource>
class ResourceHolder {
public:
    explicit ResourceHolder(std::string resourcesDir = "../resources/")
            : resourcesDir{std::move(resourcesDir)}
    {}

    template <typename... Args>
    void insert(const Key& key, Args&&... args) {
        auto resPtr = std::make_unique<Resource>();
        if (!resPtr->loadFromFile(resourcesDir + std::forward<Args>(args)...)) {
            msgErrorLoading(std::forward<Args>(args)...);
            ///* todo: should I e.g. "throw ErrorLoadingResource" here? */
        }
        resources.emplace(key, std::move(resPtr));
    }

    Resource& get(const Key& key) const {
        if (auto resource = resources.find(key); resource != std::end(resources)) {
            return *(resource->second);
        } throw std::invalid_argument{"No such resource id."};
    }

    void erase(const Key& key) noexcept {
        if (auto found = resources.find(key); found != std::end(resources)) {
            resources.erase(key);
        }
    }

    void eraseAll() {
        resources.clear();
    }

private:
    std::string resourcesDir;
    std::unordered_map<Key, std::unique_ptr<Resource>> resources;

public:
    template <typename... Args>
    ResourceHolder& operator+=(const ResourceInserter<Key, Args...>& inserter) {
        insert(std::move(inserter.key), std::move(std::get<Args>(inserter.args)...));
        return *this;
    }

    inline const Resource& operator[](const Key& key) const {
        return get(std::move(key));
    }

    inline Resource& operator[](const Key& key) {
        return get(std::move(key));
    }

    auto& getResources() const {
        return resources;
    }

    auto& getResourcesDir() const {
        return resourcesDir;
    }

    void setResourcesDir(std::string newPath) {
        resourcesDir = std::move(newPath);
    }

private:
    template <typename... Args>
    void msgErrorLoading(const Args... args) {
         std::cerr << "Failed loading resource: { Type: \"" << typeid(Resource).name()<< "\", File name: \"";
        (std::cerr << ... << args) << "\" }" << std::endl;
    }
};

ResourceManager.h:

class ResourceManager {
public:
    ResourceManager();

private:
    ResourceHolder<res::Texture, sf::Texture> textures;
    ResourceHolder<res::Sound, sf::SoundBuffer> sounds{"../resources/sound/"};

    void loadTextures();
    void loadSounds();

public:
    auto& getTextures() {
        return textures;
    }

    auto& getSounds() {
        return sounds;
    }
};

ResourceManager.cpp:

ResourceManager::ResourceManager() {
    loadTextures();
    loadSounds();
}

void ResourceManager::loadTextures() {
    textures.insert(res::Texture::Wizard, "wizard.png");
    textures.insert(res::Texture::Gray,   "gray.png");
    textures.insert(res::Texture::Orange, "orange.png");
}

void ResourceManager::loadSounds() {
    sounds += ResourceInserter(res::Sound::Bullet, "boing.wav");
    sounds += ResourceInserter(res::Sound::Bing,   "boing_long.wav");
    sounds += ResourceInserter(res::Sound::Poof,   "poof.wav");
}

ResourceInserter.h:

/** Operator += must take one argument
 *  This is a proxy class for operator+=
 *  You can use operator+= instead of insert() as an alternative insertion method
 */

template <typename Key, typename... Args>
class ResourceInserter {
public:
    explicit ResourceInserter(Key&& key, Args&&... args)
            : key{std::forward<Key>(key)}
            , args{std::forward<Args>(args)...}
    {}

    Key key;
    std::tuple<Args...> args;
};

template <typename T, typename... Args>
ResourceInserter(T&&, Args&&... args) -> ResourceInserter<T, Args...>;

Resources.h

namespace res {
    enum class Texture {
        Gray,
        Orange,
        Wizard
    };

    enum class Sound {
        Bullet,
        Poof,
        Bing
    };
}

Some basic usage (inside parent/caller/owner class):

auto wizardTexture = textures.get(res::Texture::Wizard);
auto bulletSound   = sounds[res::Sound::Bullet];

I am not asking for a deep-throughout review as I imagine it'd take too much off of your time.

---

I have few questions, answering any of them would be absolutely helpful.

1. Whatever you think of that looks smelly or problematic, please do let me know.
2. What's wrong with my design from OOP/design patterns point of view? (I am especially worried about the part where I am inserting all the resources inside ResourceManager.cpp)
3. What's wrong with my code from C++ point of view? (I am especially interested in parts where I attempted using move semantics/perfect forwarding e.g. insert method or operator+=)
4. Is there something confusing related to naming identifiers?

---

The reason why I am using enums as keys is that it works as a sort of connection interface for both insertion and then retrieval of the resource. If I insert a resource of key Enum_Type::Enum_Item, then I can also retrieve it using the same key.

I'd rather not hardcode the insertion process inside ResourceManager.cpp, and preferably keep it in a separate file, but the fact that I am using Enum as a key is kind of an obstacle here for me. Not sure how to fix it.

---

Thanks a lot!

Answer 1

template <typename... Args>
void insert(const Key& key, Args&&... args) {
    auto resPtr = std::make_unique<Resource>();
    if (!resPtr->loadFromFile(resourcesDir + std::forward<Args>(args)...)) {
        msgErrorLoading(std::forward<Args>(args)...);
        ///* todo: should I e.g. "throw ErrorLoadingResource" here? */
    }
    resources.emplace(key, std::move(resPtr));
}

The template parameter pack seems a little overcomplicated here. If all the arguments are concatenated into a std::string, it might be better to just take a single string argument and let the user do the concatenation.

It might be nice to have a Resource constructor that takes the file path so we don't need a separate function call.

---

template <typename... Args>
ResourceHolder& operator+=(const ResourceInserter<Key, Args...>& inserter) {
    insert(std::move(inserter.key), std::move(std::get<Args>(inserter.args)...));
    return *this;
}

This also seems a little unnecessary. Mathematical operators are best used for mathematical operations.

In ResourceManager it also looks like the code using += is longer than the code that calls insert directly (and we have to have that ResourceInserter).

The use of std::move looks fine here.

---

inline const Resource& operator[](const Key& key) const {
    return get(std::move(key));
}

Functions defined in the class body are already inline, so we don't need to specify it.

The argument is a const& so there's no need to std::move it.

Perhaps operator[] is also unnecessary, since we can just call get.

---

explicit ResourceInserter(Key&& key, Args&&... args)
        : key{std::forward<Key>(key)}
        , args{std::forward<Args>(args)...}
{}

Key and Args aren't actually template parameters (of the function) here, so we don't need to use std::forward. Since these are "sink arguments" (we're storing a copy of them locally), we can take them by value and then move them into place:

explicit ResourceInserter(Key key, Args... args)
        : key{std::move(key)}
        , args{std::move(args)...}
{}

---

It might be better to put the loading of resources somewhere in the game logic, rather than in ResourceManager. (We might want to separate the construction of the ResourceManager from the loading at some point).

---

I don't think that hard-coding the resource IDs (as an enum or a constant variable) is necessarily a problem for a small game.

The alternative would be to load the resource IDs from a data file (i.e. add a json or xml asset list). Then we could change the resources without recompiling (but it's more work to code and maintain).