8
\$\begingroup\$

I'm currently writing a small game, needing some advice for things I could do better on this resource management system. Speed is not crucial, and I have no memory budget.

The goal is to load and store resources so they can be reused and accessed with a string. That way, all resources are loaded at once, and can be reused by this singleton from anywhere else in the code. This system currently works, I'm just looking for ways to improve it or better leverage STL's features.

The Resource class

class Resource
{
public:
    Resource();
    virtual ~Resource();

    virtual void loadResource() = 0;
    virtual void unloadResource() = 0;

    void setResourceId(unsigned id);
    unsigned getResourceId() const;

    void setResourcePath(const std::string& path);
    std::string getResourcePath() const;

protected:
    unsigned mResourceId;
    std::string mResourcePath;

};

It's a simple base class with getters and setters, along with pure virtual loadResource() and unloadResource() functions that each derivative will override.

The ResourceManager class

class ResourceManager : public Singleton<ResourceManager>
{
public:
    ResourceManager();
    virtual ~ResourceManager();

    void addResource(Resource* resource, const std::string& name, const std::string& path);

    template <typename T>
    T* getResource(const std::string& name) {
        return dynamic_cast<T*>(mResources.find(name)->second);
    }

    static ResourceManager& getInstance();
    static ResourceManager* getInstancePtr();

private:
    std::map<std::string, Resource*> mResources;

};

The addResource implementation

void ResourceManager::addResource(Resource* resource, const std::string& name, const std::string& path)
{
    resource->setResourcePath(path);
    resource->setResourceId(mResources.size());
    resource->loadResource();

    mResources.insert(std::pair<std::string, Resource*>(name, resource));
}

The unloadResource function is called for every Resource in ResourceManager's destructor.

Usage example: loading the resource (with a hypothetical Texture class)

mResourceManager->addResource(new Texture, "test", "Resources/test.png");

Usage example: accessing the resource

mResourceManager->getResource<Texture>("test");

Like I said earlier, speed/memory isn't crucial, so all solutions are welcome.

\$\endgroup\$
  • \$\begingroup\$ What is the reasoning behind forcing a singleton upon the user of ResourceManager? \$\endgroup\$ – zett42 Jun 10 '17 at 12:54
  • \$\begingroup\$ @zett42 How else would you approach it? I can't imagine why there would be more than one resource manager, and it makes everything easy to access. Perhaps I could create a dedicated namespace for resource loading functions, but I think the singleton pattern makes more sense. A Core class in the game controls the starting up and shutting down of these Manager classes, and they're singletons so the instances can be accessed from anywhere else in the game. \$\endgroup\$ – K.K. Slider Jun 10 '17 at 14:55
  • \$\begingroup\$ You could pass the resource managers to the objects that need them. It would be more code to write but it expresses the dependencies clearer. You could just look at the interfaces and see which classes depend on which kind of resources. It also adds more flexibility in the long term as you could simply swap out a resource manager used by one class by a different one (even if you currently can't imagine a use for that). \$\endgroup\$ – zett42 Jun 10 '17 at 21:51
7
\$\begingroup\$

The ResourceManager class

You should probably use a std::unordered_map<> (from the <unordered_map> header) instead of a std::map, since resources don't really need to be ordered as far as I can see. This will give you some speed, even if you're not concerned by that.

Consider throwing an exception from the getResource() function if the dynamic_cast<> fails. Right now, it is up the caller to check that the returned pointer is not null, it might be better if the function simply throws an exception so that client code does not have to wrap every single call with a check.

This is up to you, since now the client might have to wrap calls in try-catches. However, it certainly makes errors pop up a lot more visibly and at the call location, instead of possibly later if the client forgets to check for null.

The addResource implementation

You never check that resource is not null, which a pointer can be. Consider checking or taking a Resource& instead to ensure that you are operating on an existing value.

Your insertion into the map looks like this:

mResources.insert(std::pair<std::string, Resource*>(name, resource));

std::map<> and std::unordered_map<> both have the emplace() member function. The function takes the arguments to construct the key-value-pair internally (thus avoiding copy/move operations). You can instead write your return statement like so:

mResources.emplace(name, resource);

This looks cleaner and is more efficient.


Be careful with your map, I don't know what unload resource does, but it better call delete if that's (dynamically) how you're allocating your resources. However, a better idea might instead to use std::unique_ptr<> with a custom deleter. Here's a mock implementation:

#include <iostream>
#include <memory>

struct resource
{
    void unload_resource() { std::cout << "unloaded\n"; }
};

struct resource_free
{
    void operator()(resource* r)
    {
        r->unload_resource();
        delete r;
    }
};

int main()
{
    std::unique_ptr<resource, resource_free> pres{ new resource };
}

This will simplify the implementation of the destructor as std::unique_ptr<> takes care of properly releasing your resource. You can make an alias for ease of use: using resource_ptr = std::unique_ptr<...>. This allows you to write your map variable declaration in an easier to read way:

std::unordered_map<std::string, resource_ptr> mResources;
\$\endgroup\$
  • \$\begingroup\$ Thank you for the reply. I'll definitely take note of the emplace function. Like I mentioned in the post, the destructor of ResourceManager loops through the map, calls unloadResource(), and deletes the resources themselves, but I'll definitely consider using smart pointers in the future. \$\endgroup\$ – K.K. Slider Jun 10 '17 at 6:18

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.