From b4041856efe3f2a6916df42d01cae1f573beaf5b Mon Sep 17 00:00:00 2001 From: swinston Date: Wed, 19 Aug 2026 15:47:48 -0700 Subject: [PATCH 1/2] Implement reference counting in ResourceManager (#320) refCount was documented as part of the resource management design but never implemented: LoadResource never incremented it and UnloadResource unloaded and erased a resource unconditionally, even with other outstanding handles to it. Also drop the `final` on Resource and ResourceManager: as written, neither could ever be subclassed, so none of the concrete resource types the tutorial documents (Texture, Mesh, Shader) could actually exist. --- .../simple_engine/resource_manager.cpp | 2 +- attachments/simple_engine/resource_manager.h | 63 ++++++++++++++++--- 2 files changed, 54 insertions(+), 11 deletions(-) diff --git a/attachments/simple_engine/resource_manager.cpp b/attachments/simple_engine/resource_manager.cpp index 006c16b46..3e4c3575f 100644 --- a/attachments/simple_engine/resource_manager.cpp +++ b/attachments/simple_engine/resource_manager.cpp @@ -43,7 +43,7 @@ void ResourceManager::UnloadAllResources() for (auto &innerKv : val) { auto &loadedResource = innerKv.second; - loadedResource->Unload(); + loadedResource.resource->Unload(); } val.clear(); } diff --git a/attachments/simple_engine/resource_manager.h b/attachments/simple_engine/resource_manager.h index 2e0b4c539..ec4cfecd8 100644 --- a/attachments/simple_engine/resource_manager.h +++ b/attachments/simple_engine/resource_manager.h @@ -26,7 +26,7 @@ /** * @brief Base class for all resources. */ -class Resource final +class Resource { protected: std::string resourceId; @@ -157,10 +157,19 @@ class ResourceHandle * This class implements the resource management system as described in the Engine_Architecture chapter: * @see en/Building_a_Simple_Engine/Engine_Architecture/04_resource_management.adoc */ -class ResourceManager final +class ResourceManager { private: - std::unordered_map>> resources; + /** + * @brief A stored resource together with how many outstanding ResourceHandles reference it. + */ + struct ResourceData + { + std::unique_ptr resource; + int refCount = 0; + }; + + std::unordered_map> resources; public: /** @@ -191,6 +200,8 @@ class ResourceManager final auto it = typeResources.find(id); if (it != typeResources.end()) { + // Resource already loaded: another handle now references it too. + ++it->second.refCount; return ResourceHandle(id, this); } @@ -201,8 +212,8 @@ class ResourceManager final throw std::runtime_error("Failed to load resource: " + id); } - // Store the resource - typeResources[id] = std::move(resource); + // Store the resource with an initial reference count of 1 + typeResources[id] = ResourceData{std::move(resource), 1}; return ResourceHandle(id, this); } @@ -230,7 +241,34 @@ class ResourceManager final return nullptr; } - return static_cast(resourceIt->second.get()); + return static_cast(resourceIt->second.resource.get()); + } + + /** + * @brief Get the current reference count of a resource. + * @tparam T The type of resource. + * @param id The resource ID. + * @return The number of outstanding references, or 0 if the resource doesn't exist. + */ + template + int GetRefCount(const std::string &id) const + { + static_assert(std::is_base_of::value, "T must derive from Resource"); + + auto typeIt = resources.find(std::type_index(typeid(T))); + if (typeIt == resources.end()) + { + return 0; + } + + auto &typeResources = typeIt->second; + auto resourceIt = typeResources.find(id); + if (resourceIt == typeResources.end()) + { + return 0; + } + + return resourceIt->second.refCount; } /** @@ -255,10 +293,12 @@ class ResourceManager final } /** - * @brief Unload a resource. + * @brief Release a reference to a resource. Only actually unloads it once its + * reference count drops to zero, i.e. once every ResourceHandle that was + * loaded against this id has released its reference. * @tparam T The type of resource. * @param id The resource ID. - * @return True if the resource was unloaded, false otherwise. + * @return True if the resource existed and a reference was released, false otherwise. */ template bool UnloadResource(const std::string &id) @@ -278,8 +318,11 @@ class ResourceManager final return false; } - resourceIt->second->Unload(); - typeResources.erase(resourceIt); + if (--resourceIt->second.refCount <= 0) + { + resourceIt->second.resource->Unload(); + typeResources.erase(resourceIt); + } return true; } From 4e22d4b1396cf1318e9a395df4e5952931989f27 Mon Sep 17 00:00:00 2001 From: swinston Date: Thu, 17 Sep 2026 19:00:16 -0700 Subject: [PATCH 2/2] Replace hand-rolled refcounting in ResourceManager with std::shared_ptr --- .../simple_engine/resource_manager.cpp | 10 -- attachments/simple_engine/resource_manager.h | 99 +++++++------------ 2 files changed, 35 insertions(+), 74 deletions(-) diff --git a/attachments/simple_engine/resource_manager.cpp b/attachments/simple_engine/resource_manager.cpp index 3e4c3575f..02c094c33 100644 --- a/attachments/simple_engine/resource_manager.cpp +++ b/attachments/simple_engine/resource_manager.cpp @@ -37,15 +37,5 @@ void Resource::Unload() void ResourceManager::UnloadAllResources() { - for (auto &kv : resources) - { - auto &val = kv.second; - for (auto &innerKv : val) - { - auto &loadedResource = innerKv.second; - loadedResource.resource->Unload(); - } - val.clear(); - } resources.clear(); } diff --git a/attachments/simple_engine/resource_manager.h b/attachments/simple_engine/resource_manager.h index ec4cfecd8..33d5dec82 100644 --- a/attachments/simple_engine/resource_manager.h +++ b/attachments/simple_engine/resource_manager.h @@ -84,8 +84,7 @@ template class ResourceHandle { private: - std::string resourceId; - class ResourceManager *resourceManager = nullptr; + std::shared_ptr resource; public: /** @@ -94,25 +93,30 @@ class ResourceHandle ResourceHandle() = default; /** - * @brief Constructor with a resource ID and resource manager. - * @param id The resource ID. - * @param manager The resource manager. + * @brief Constructor wrapping an already-loaded resource. + * @param resource The resource to share ownership of. */ - ResourceHandle(const std::string &id, class ResourceManager *manager) : - resourceId(id), resourceManager(manager) + explicit ResourceHandle(std::shared_ptr resource) : + resource(std::move(resource)) {} /** * @brief Get the resource. - * @return A pointer to the resource, or nullptr if not found. + * @return A pointer to the resource, or nullptr if the handle is invalid. */ - T *Get() const; + T *Get() const + { + return resource.get(); + } /** * @brief Check if the handle is valid. * @return True if the handle is valid, false otherwise. */ - bool IsValid() const; + bool IsValid() const + { + return resource != nullptr; + } /** * @brief Get the resource ID. @@ -120,7 +124,7 @@ class ResourceHandle */ const std::string &GetId() const { - return resourceId; + return resource->GetId(); } /** @@ -160,16 +164,7 @@ class ResourceHandle class ResourceManager { private: - /** - * @brief A stored resource together with how many outstanding ResourceHandles reference it. - */ - struct ResourceData - { - std::unique_ptr resource; - int refCount = 0; - }; - - std::unordered_map> resources; + std::unordered_map>> resources; public: /** @@ -195,53 +190,52 @@ class ResourceManager { static_assert(std::is_base_of::value, "T must derive from Resource"); - // Check if the resource already exists auto &typeResources = resources[std::type_index(typeid(T))]; auto it = typeResources.find(id); if (it != typeResources.end()) { - // Resource already loaded: another handle now references it too. - ++it->second.refCount; - return ResourceHandle(id, this); + return ResourceHandle(std::static_pointer_cast(it->second)); } - // Create and load the resource - auto resource = std::make_unique(id, std::forward(args)...); + // The deleter calls Unload() before delete so it still goes through T's vtable. + std::shared_ptr resource(new T(id, std::forward(args)...), [](T *r) { + r->Unload(); + delete r; + }); if (!resource->Load()) { throw std::runtime_error("Failed to load resource: " + id); } - // Store the resource with an initial reference count of 1 - typeResources[id] = ResourceData{std::move(resource), 1}; - return ResourceHandle(id, this); + typeResources[id] = resource; + return ResourceHandle(resource); } /** - * @brief Get a resource. + * @brief Get a handle to an already-loaded resource. * @tparam T The type of resource. * @param id The resource ID. - * @return A pointer to the resource, or nullptr if not found. + * @return A handle sharing ownership of the resource, or an invalid handle if not found. */ template - T *GetResource(const std::string &id) + ResourceHandle GetResource(const std::string &id) { static_assert(std::is_base_of::value, "T must derive from Resource"); auto typeIt = resources.find(std::type_index(typeid(T))); if (typeIt == resources.end()) { - return nullptr; + return ResourceHandle(); } auto &typeResources = typeIt->second; auto resourceIt = typeResources.find(id); if (resourceIt == typeResources.end()) { - return nullptr; + return ResourceHandle(); } - return static_cast(resourceIt->second.resource.get()); + return ResourceHandle(std::static_pointer_cast(resourceIt->second)); } /** @@ -251,7 +245,7 @@ class ResourceManager * @return The number of outstanding references, or 0 if the resource doesn't exist. */ template - int GetRefCount(const std::string &id) const + long UseCount(const std::string &id) const { static_assert(std::is_base_of::value, "T must derive from Resource"); @@ -268,7 +262,7 @@ class ResourceManager return 0; } - return resourceIt->second.refCount; + return resourceIt->second.use_count(); } /** @@ -293,12 +287,10 @@ class ResourceManager } /** - * @brief Release a reference to a resource. Only actually unloads it once its - * reference count drops to zero, i.e. once every ResourceHandle that was - * loaded against this id has released its reference. + * @brief Release the manager's own reference to a resource. * @tparam T The type of resource. * @param id The resource ID. - * @return True if the resource existed and a reference was released, false otherwise. + * @return True if the resource existed, false otherwise. */ template bool UnloadResource(const std::string &id) @@ -318,11 +310,7 @@ class ResourceManager return false; } - if (--resourceIt->second.refCount <= 0) - { - resourceIt->second.resource->Unload(); - typeResources.erase(resourceIt); - } + typeResources.erase(resourceIt); return true; } @@ -331,20 +319,3 @@ class ResourceManager */ void UnloadAllResources(); }; - -// Implementation of ResourceHandle methods -template -T *ResourceHandle::Get() const -{ - if (!resourceManager) - return nullptr; - return resourceManager->GetResource(resourceId); -} - -template -bool ResourceHandle::IsValid() const -{ - if (!resourceManager) - return false; - return resourceManager->HasResource(resourceId); -}