Core/Vmaps: Replace manual reference counting with shared_ptr and slightly reduce memory use by deduplicating model name strings

9 files changed, 83 insertions, 101 deletions

diff --git a/src/common/Collision/Management/VMapManager2.cpp b/src/common/Collision/Management/VMapManager2.cpp
index 3885d3f40df..82fcd47f3d2 100644
--- a/src/common/Collision/Management/VMapManager2.cpp
+++ b/src/common/Collision/Management/VMapManager2.cpp
@@ -33,14 +33,18 @@ namespace VMAP
 {
     class ManagedModel
     {
-        public:
-            ManagedModel() : iRefCount(0) { }
-            WorldModel* getModel() { return &iModel; }
-            void incRefCount() { ++iRefCount; }
-            int decRefCount() { return --iRefCount; }
-        protected:
-            WorldModel iModel;
-            int iRefCount;
+    public:
+        explicit ManagedModel(VMapManager2& mgr) : _mgr(mgr) { }
+
+        ~ManagedModel()
+        {
+            _mgr.releaseModelInstance(Model.GetName());
+        }
+
+        WorldModel Model;
+
+    private:
+        VMapManager2& _mgr;
     };
 
     bool readChunk(FILE* rf, char* dest, const char* compare, uint32 len)
@@ -60,9 +64,6 @@ namespace VMAP
     {
         for (std::pair<uint32 const, StaticMapTree*>& iInstanceMapTree : iInstanceMapTrees)
             delete iInstanceMapTree.second;
-
-        for (std::pair<std::string const, ManagedModel*>& iLoadedModelFile : iLoadedModelFiles)
-            delete iLoadedModelFile.second;
     }
 
     InstanceTreeMap::const_iterator VMapManager2::GetMapTree(uint32 mapId) const
@@ -159,7 +160,7 @@ namespace VMAP
         auto instanceTree = iInstanceMapTrees.find(mapId);
         if (instanceTree != iInstanceMapTrees.end() && instanceTree->second)
         {
-            instanceTree->second->UnloadMap(this);
+            instanceTree->second->UnloadMap();
             if (instanceTree->second->numLoadedTiles() == 0)
             {
                 delete instanceTree->second;
@@ -267,29 +268,29 @@ namespace VMAP
         return false;
     }
 
-    WorldModel* VMapManager2::acquireModelInstance(std::string const& basepath, std::string const& filename)
+    std::shared_ptr<WorldModel> VMapManager2::acquireModelInstance(std::string const& basepath, std::string const& filename)
     {
         //! Critical section, thread safe access to iLoadedModelFiles
         std::lock_guard<std::mutex> lock(LoadedModelFilesLock);
 
-        auto model = iLoadedModelFiles.find(filename);
-        if (model == iLoadedModelFiles.end())
+        auto& [key, model] = *iLoadedModelFiles.try_emplace(filename).first;
+        std::shared_ptr<ManagedModel> worldmodel = model.lock();
+        if (worldmodel)
+            return std::shared_ptr<WorldModel>(worldmodel, &worldmodel->Model);
+
+        worldmodel = std::make_shared<ManagedModel>(*this);
+        if (!worldmodel->Model.readFile(basepath + filename + ".vmo"))
         {
-            ManagedModel* worldmodel = new ManagedModel();
-            if (!worldmodel->getModel()->readFile(basepath + filename + ".vmo"))
-            {
-                TC_LOG_ERROR("misc", "VMapManager2: could not load '{}{}.vmo'", basepath, filename);
-                delete worldmodel;
-                return nullptr;
-            }
-            TC_LOG_DEBUG("maps", "VMapManager2: loading file '{}{}'", basepath, filename);
+            TC_LOG_ERROR("misc", "VMapManager2: could not load '{}{}.vmo'", basepath, filename);
+            return nullptr;
+        }
+        TC_LOG_DEBUG("maps", "VMapManager2: loading file '{}{}'", basepath, filename);
 
-            worldmodel->getModel()->SetName(filename);
+        worldmodel->Model.SetName(&key);
 
-            model = iLoadedModelFiles.insert(std::pair<std::string, ManagedModel*>(filename, worldmodel)).first;
-        }
-        model->second->incRefCount();
-        return model->second->getModel();
+        model = worldmodel;
+
+        return std::shared_ptr<WorldModel>(worldmodel, &worldmodel->Model);
     }
 
     void VMapManager2::releaseModelInstance(std::string const& filename)
@@ -297,18 +298,13 @@ namespace VMAP
         //! Critical section, thread safe access to iLoadedModelFiles
         std::lock_guard<std::mutex> lock(LoadedModelFilesLock);
 
-        auto model = iLoadedModelFiles.find(filename);
-        if (model == iLoadedModelFiles.end())
+        std::size_t erased = iLoadedModelFiles.erase(filename);
+        if (!erased)
         {
             TC_LOG_ERROR("misc", "VMapManager2: trying to unload non-loaded file '{}'", filename);
             return;
         }
-        if (model->second->decRefCount() == 0)
-        {
-            TC_LOG_DEBUG("maps", "VMapManager2: unloading file '{}'", filename);
-            delete model->second;
-            iLoadedModelFiles.erase(model);
-        }
+        TC_LOG_DEBUG("maps", "VMapManager2: unloading file '{}'", filename);
     }
 
     LoadResult VMapManager2::existsMap(char const* basePath, unsigned int mapId, int x, int y)
diff --git a/src/common/Collision/Management/VMapManager2.h b/src/common/Collision/Management/VMapManager2.h
index 57fca55d5f6..b621fc72d02 100644
--- a/src/common/Collision/Management/VMapManager2.h
+++ b/src/common/Collision/Management/VMapManager2.h
@@ -18,11 +18,12 @@
 #ifndef _VMAPMANAGER2_H
 #define _VMAPMANAGER2_H
 
+#include "Define.h"
+#include "IVMapManager.h"
+#include <memory>
 #include <mutex>
 #include <unordered_map>
 #include <vector>
-#include "Define.h"
-#include "IVMapManager.h"
 
 //===========================================================
 
@@ -52,7 +53,7 @@ namespace VMAP
     class WorldModel;
 
     typedef std::unordered_map<uint32, StaticMapTree*> InstanceTreeMap;
-    typedef std::unordered_map<std::string, ManagedModel*> ModelFileMap;
+    typedef std::unordered_map<std::string, std::weak_ptr<ManagedModel>> ModelFileMap;
 
     enum DisableTypes
     {
@@ -105,7 +106,7 @@ namespace VMAP
 
             bool getAreaAndLiquidData(uint32 mapId, float x, float y, float z, Optional<uint8> reqLiquidType, AreaAndLiquidData& data) const override;
 
-            WorldModel* acquireModelInstance(std::string const& basepath, std::string const& filename);
+            std::shared_ptr<WorldModel> acquireModelInstance(std::string const& basepath, std::string const& filename);
             void releaseModelInstance(std::string const& filename);
 
             // what's the use of this? o.O
diff --git a/src/common/Collision/Maps/MapTree.cpp b/src/common/Collision/Maps/MapTree.cpp
index 61eb7e3fab0..38b0a6e00cc 100644
--- a/src/common/Collision/Maps/MapTree.cpp
+++ b/src/common/Collision/Maps/MapTree.cpp
@@ -18,6 +18,7 @@
 #include "MapTree.h"
 #include "Errors.h"
 #include "Log.h"
+#include "Memory.h"
 #include "Metric.h"
 #include "ModelInstance.h"
 #include "VMapDefinitions.h"
@@ -276,56 +277,48 @@ namespace VMAP
     {
         TC_LOG_DEBUG("maps", "StaticMapTree::InitMap() : initializing StaticMapTree '{}'", fname);
         std::string fullname = iBasePath + fname;
-        FILE* rf = fopen(fullname.c_str(), "rb");
+        auto rf = Trinity::make_unique_ptr_with_deleter(fopen(fullname.c_str(), "rb"), &::fclose);
         if (!rf)
             return LoadResult::FileNotFound;
 
-        LoadResult result = LoadResult::Success;
         char chunk[8];
 
-        if (!readChunk(rf, chunk, VMAP_MAGIC, 8))
-            result = LoadResult::VersionMismatch;
+        if (!readChunk(rf.get(), chunk, VMAP_MAGIC, 8))
+            return LoadResult::VersionMismatch;
 
-        if (result == LoadResult::Success &&
-            readChunk(rf, chunk, "NODE", 4) &&
-            iTree.readFromFile(rf))
-        {
-            iNTreeValues = iTree.primCount();
-            iTreeValues = new ModelInstance[iNTreeValues];
-            result = LoadResult::Success;
-        }
+        if (!readChunk(rf.get(), chunk, "NODE", 4)
+            || !iTree.readFromFile(rf.get()))
+            return LoadResult::ReadFromFileFailed;
+
+        iNTreeValues = iTree.primCount();
+        iTreeValues = new ModelInstance[iNTreeValues];
+
+        if (!readChunk(rf.get(), chunk, "SIDX", 4))
+            return LoadResult::ReadFromFileFailed;
+
+        uint32 spawnIndicesSize = 0;
+        if (fread(&spawnIndicesSize, sizeof(uint32), 1, rf.get()) != 1)
+            return LoadResult::ReadFromFileFailed;
 
-        if (result == LoadResult::Success)
+        uint32 spawnId;
+        for (uint32 i = 0; i < spawnIndicesSize; ++i)
         {
-            result = readChunk(rf, chunk, "SIDX", 4) ? LoadResult::Success : LoadResult::ReadFromFileFailed;
-            uint32 spawnIndicesSize = 0;
-            uint32 spawnId;
-            if (result == LoadResult::Success && fread(&spawnIndicesSize, sizeof(uint32), 1, rf) != 1)
-                result = LoadResult::ReadFromFileFailed;
-            for (uint32 i = 0; i < spawnIndicesSize && result == LoadResult::Success; ++i)
-            {
-                if (fread(&spawnId, sizeof(uint32), 1, rf) == 1)
-                    iSpawnIndices[spawnId] = i;
-                else
-                    result = LoadResult::ReadFromFileFailed;
-            }
+            if (fread(&spawnId, sizeof(uint32), 1, rf.get()) == 1)
+                iSpawnIndices[spawnId] = i;
+            else
+                return LoadResult::ReadFromFileFailed;
         }
 
-        fclose(rf);
-        return result;
+        return LoadResult::Success;
     }
 
     //=========================================================
 
-    void StaticMapTree::UnloadMap(VMapManager2* vm)
+    void StaticMapTree::UnloadMap()
     {
         for (std::pair<uint32 const, uint32>& iLoadedSpawn : iLoadedSpawns)
-        {
-            for (uint32 refCount = 0; refCount < iLoadedSpawn.second; ++refCount)
-                vm->releaseModelInstance(iTreeValues[iLoadedSpawn.first].getWorldModel()->GetName());
-
             iTreeValues[iLoadedSpawn.first].setUnloaded();
-        }
+
         iLoadedSpawns.clear();
         iLoadedTiles.clear();
     }
@@ -359,7 +352,7 @@ namespace VMAP
                 if (ModelSpawn::readFromFile(fileResult.File, spawn))
                 {
                     // acquire model instance
-                    WorldModel* model = vm->acquireModelInstance(iBasePath, spawn.name);
+                    std::shared_ptr<WorldModel> model = vm->acquireModelInstance(iBasePath, spawn.name);
                     if (!model)
                         TC_LOG_ERROR("misc", "StaticMapTree::LoadMapTile() : could not acquire WorldModel pointer [{}, {}]", tileX, tileY);
 
@@ -376,7 +369,7 @@ namespace VMAP
                                 continue;
                             }
 
-                            iTreeValues[referencedVal] = ModelInstance(spawn, model);
+                            iTreeValues[referencedVal] = ModelInstance(spawn, std::move(model));
                             iLoadedSpawns[referencedVal] = 1;
                         }
                         else
@@ -445,9 +438,6 @@ namespace VMAP
                     result = ModelSpawn::readFromFile(fileResult.File, spawn);
                     if (result)
                     {
-                        // release model instance
-                        vm->releaseModelInstance(spawn.name);
-
                         // update tree
                         auto spawnIndex = iSpawnIndices.find(spawn.ID);
                         if (spawnIndex != iSpawnIndices.end())
diff --git a/src/common/Collision/Maps/MapTree.h b/src/common/Collision/Maps/MapTree.h
index 4a5e60d6fe2..5260d7d602e 100644
--- a/src/common/Collision/Maps/MapTree.h
+++ b/src/common/Collision/Maps/MapTree.h
@@ -91,7 +91,7 @@ namespace VMAP
             bool GetLocationInfo(const G3D::Vector3 &pos, LocationInfo &info) const;
 
             LoadResult InitMap(std::string const& fname);
-            void UnloadMap(VMapManager2* vm);
+            void UnloadMap();
             LoadResult LoadMapTile(uint32 tileX, uint32 tileY, VMapManager2* vm);
             void UnloadMapTile(uint32 tileX, uint32 tileY, VMapManager2* vm);
             uint32 numLoadedTiles() const { return uint32(iLoadedTiles.size()); }
diff --git a/src/common/Collision/Models/GameObjectModel.cpp b/src/common/Collision/Models/GameObjectModel.cpp
index f2b19e038fa..24e9489326e 100644
--- a/src/common/Collision/Models/GameObjectModel.cpp
+++ b/src/common/Collision/Models/GameObjectModel.cpp
@@ -93,11 +93,7 @@ bool LoadGameObjectModelList(std::string const& dataPath)
     return true;
 }
 
-GameObjectModel::~GameObjectModel()
-{
-    if (iModel)
-        VMAP::VMapFactory::createOrGetVMapManager()->releaseModelInstance(iModel->GetName());
-}
+GameObjectModel::~GameObjectModel() = default;
 
 bool GameObjectModel::initialize(std::unique_ptr<GameObjectModelOwnerBase> modelOwner, std::string const& dataPath)
 {
diff --git a/src/common/Collision/Models/GameObjectModel.h b/src/common/Collision/Models/GameObjectModel.h
index f875d9e3197..1cfe27a050e 100644
--- a/src/common/Collision/Models/GameObjectModel.h
+++ b/src/common/Collision/Models/GameObjectModel.h
@@ -18,12 +18,11 @@
 #ifndef _GAMEOBJECT_MODEL_H
 #define _GAMEOBJECT_MODEL_H
 
-#include <G3D/Matrix3.h>
-#include <G3D/Vector3.h>
+#include "Define.h"
 #include <G3D/AABox.h>
+#include <G3D/Matrix3.h>
 #include <G3D/Ray.h>
-
-#include "Define.h"
+#include <G3D/Vector3.h>
 #include <memory>
 
 namespace G3D
@@ -91,7 +90,7 @@ private:
     G3D::Vector3 iPos;
     float iInvScale;
     float iScale;
-    VMAP::WorldModel* iModel;
+    std::shared_ptr<VMAP::WorldModel> iModel;
     std::unique_ptr<GameObjectModelOwnerBase> owner;
 };
 
diff --git a/src/common/Collision/Models/ModelInstance.cpp b/src/common/Collision/Models/ModelInstance.cpp
index 6838babcbe2..2205d10833a 100644
--- a/src/common/Collision/Models/ModelInstance.cpp
+++ b/src/common/Collision/Models/ModelInstance.cpp
@@ -24,7 +24,7 @@ using G3D::Ray;
 
 namespace VMAP
 {
-    ModelInstance::ModelInstance(ModelSpawn const& spawn, WorldModel* model) : ModelMinimalData(spawn), iModel(model)
+    ModelInstance::ModelInstance(ModelSpawn const& spawn, std::shared_ptr<WorldModel> model) : ModelMinimalData(spawn), iModel(std::move(model))
     {
         iInvRot = G3D::Matrix3::fromEulerAnglesZYX(G3D::pif() * spawn.iRot.y / 180.f, G3D::pif() * spawn.iRot.x / 180.f, G3D::pif() * spawn.iRot.z / 180.f).inverse();
         iInvScale = 1.f / iScale;
diff --git a/src/common/Collision/Models/ModelInstance.h b/src/common/Collision/Models/ModelInstance.h
index 7361fbb9335..78b07f11f4b 100644
--- a/src/common/Collision/Models/ModelInstance.h
+++ b/src/common/Collision/Models/ModelInstance.h
@@ -18,12 +18,12 @@
 #ifndef _MODELINSTANCE_H_
 #define _MODELINSTANCE_H_
 
-#include <G3D/Matrix3.h>
-#include <G3D/Vector3.h>
+#include "Define.h"
+#include <memory>
 #include <G3D/AABox.h>
+#include <G3D/Matrix3.h>
 #include <G3D/Ray.h>
-
-#include "Define.h"
+#include <G3D/Vector3.h>
 
 namespace VMAP
 {
@@ -70,17 +70,17 @@ namespace VMAP
     {
         public:
             ModelInstance() : iInvScale(0.0f), iModel(nullptr) { }
-            ModelInstance(ModelSpawn const& spawn, WorldModel* model);
+            ModelInstance(ModelSpawn const& spawn, std::shared_ptr<WorldModel> model);
             void setUnloaded() { iModel = nullptr; }
             bool intersectRay(G3D::Ray const& pRay, float& pMaxDist, bool pStopAtFirstHit, ModelIgnoreFlags ignoreFlags) const;
             bool GetLocationInfo(G3D::Vector3 const& p, LocationInfo& info) const;
             bool GetLiquidLevel(G3D::Vector3 const& p, LocationInfo& info, float& liqHeight) const;
             G3D::Matrix3 const& GetInvRot() const { return iInvRot; }
-            WorldModel const* getWorldModel() const { return iModel; }
+            WorldModel const* getWorldModel() const { return iModel.get(); }
         protected:
             G3D::Matrix3 iInvRot;
             float iInvScale;
-            WorldModel* iModel;
+            std::shared_ptr<WorldModel> iModel;
     };
 } // namespace VMAP
 
diff --git a/src/common/Collision/Models/WorldModel.h b/src/common/Collision/Models/WorldModel.h
index 48e75f3b797..9d2ba99bb80 100644
--- a/src/common/Collision/Models/WorldModel.h
+++ b/src/common/Collision/Models/WorldModel.h
@@ -117,7 +117,7 @@ namespace VMAP
     class TC_COMMON_API WorldModel
     {
         public:
-            WorldModel(): Flags(ModelFlags::None), RootWMOID(0) { }
+            WorldModel(): Flags(ModelFlags::None), RootWMOID(0), name(nullptr) { }
 
             //! pass group models to WorldModel and create BIH. Passed vector is swapped with old geometry!
             void setGroupModels(std::vector<GroupModel> &models);
@@ -129,14 +129,14 @@ namespace VMAP
             bool readFile(const std::string &filename);
             bool IsM2() const { return Flags.HasFlag(ModelFlags::IsM2); }
             std::vector<GroupModel> const& getGroupModels() const { return groupModels; }
-            std::string const& GetName() const { return name; }
-            void SetName(std::string newName) { name = std::move(newName); }
+            std::string const& GetName() const { return *name; }
+            void SetName(std::string const* newName) { name = newName; }
         protected:
             EnumFlag<ModelFlags> Flags;
             uint32 RootWMOID;
             std::vector<GroupModel> groupModels;
             BIH groupTree;
-            std::string name;
+            std::string const* name;    // valid only while model is held in VMapManager2::iLoadedModelFiles
     };
 } // namespace VMAP