Create RecastMesh outside critical section

To not lock main thread when it tries to update objects.

components/detournavigator/cachedrecastmeshmanager.cpp

@@ -13,7 +13,7 @@ namespace DetourNavigator
     {
         if (!mImpl.addObject(id, shape, transform, areaType))
             return false;
-        mCached.reset();
+        mCached.lock()->reset();
         return true;
     }
 
@@ -21,7 +21,7 @@ namespace DetourNavigator
     {
         if (!mImpl.updateObject(id, transform, areaType))
             return false;
-        mCached.reset();
+        mCached.lock()->reset();
         return true;
     }
 
@@ -29,7 +29,7 @@ namespace DetourNavigator
     {
         const auto object = mImpl.removeObject(id);
         if (object)
-            mCached.reset();
+            mCached.lock()->reset();
         return object;
     }
 
@@ -38,7 +38,7 @@ namespace DetourNavigator
     {
         if (!mImpl.addWater(cellPosition, cellSize, shift))
             return false;
-        mCached.reset();
+        mCached.lock()->reset();
         return true;
     }
 
@@ -46,7 +46,7 @@ namespace DetourNavigator
     {
         const auto water = mImpl.removeWater(cellPosition);
         if (water)
-            mCached.reset();
+            mCached.lock()->reset();
         return water;
     }
 
@@ -55,7 +55,7 @@ namespace DetourNavigator
     {
         if (!mImpl.addHeightfield(cellPosition, cellSize, shift, shape))
             return false;
-        mCached.reset();
+        mCached.lock()->reset();
         return true;
     }
 
@@ -63,15 +63,18 @@ namespace DetourNavigator
     {
         const auto cell = mImpl.removeHeightfield(cellPosition);
         if (cell)
-            mCached.reset();
+            mCached.lock()->reset();
         return cell;
     }
 
     std::shared_ptr<RecastMesh> CachedRecastMeshManager::getMesh()
     {
-        if (!mCached)
-            mCached = mImpl.getMesh();
-        return mCached;
+        std::shared_ptr<RecastMesh> cached = *mCached.lock();
+        if (cached != nullptr)
+            return cached;
+        cached = mImpl.getMesh();
+        *mCached.lock() = cached;
+        return cached;
     }
 
     bool CachedRecastMeshManager::isEmpty() const

components/detournavigator/cachedrecastmeshmanager.hpp

@@ -5,6 +5,8 @@
 #include "version.hpp"
 #include "heightfieldshape.hpp"
 
+#include <components/misc/guarded.hpp>
+
 namespace DetourNavigator
 {
     class CachedRecastMeshManager
@@ -38,7 +40,7 @@ namespace DetourNavigator
 
     private:
         RecastMeshManager mImpl;
-        std::shared_ptr<RecastMesh> mCached;
+        Misc::ScopeGuarded<std::shared_ptr<RecastMesh>> mCached;
     };
 }
 

components/detournavigator/recastmeshmanager.cpp

@@ -38,6 +38,7 @@ namespace DetourNavigator
     bool RecastMeshManager::addObject(const ObjectId id, const CollisionShape& shape, const btTransform& transform,
                                       const AreaType areaType)
     {
+        const std::lock_guard lock(mMutex);
         const auto object = mObjects.lower_bound(id);
         if (object != mObjects.end() && object->first == id)
             return false;
@@ -49,6 +50,7 @@ namespace DetourNavigator
 
     bool RecastMeshManager::updateObject(const ObjectId id, const btTransform& transform, const AreaType areaType)
     {
+        const std::lock_guard lock(mMutex);
         const auto object = mObjects.find(id);
         if (object == mObjects.end())
             return false;
@@ -62,6 +64,7 @@ namespace DetourNavigator
 
     std::optional<RemovedRecastMeshObject> RecastMeshManager::removeObject(const ObjectId id)
     {
+        const std::lock_guard lock(mMutex);
         const auto object = mObjects.find(id);
         if (object == mObjects.end())
             return std::nullopt;
@@ -73,6 +76,7 @@ namespace DetourNavigator
 
     bool RecastMeshManager::addWater(const osg::Vec2i& cellPosition, const int cellSize, const osg::Vec3f& shift)
     {
+        const std::lock_guard lock(mMutex);
         if (!mWater.emplace(cellPosition, Cell {cellSize, shift}).second)
             return false;
         ++mRevision;
@@ -81,6 +85,7 @@ namespace DetourNavigator
 
     std::optional<Cell> RecastMeshManager::removeWater(const osg::Vec2i& cellPosition)
     {
+        const std::lock_guard lock(mMutex);
         const auto water = mWater.find(cellPosition);
         if (water == mWater.end())
             return std::nullopt;
@@ -93,6 +98,7 @@ namespace DetourNavigator
     bool RecastMeshManager::addHeightfield(const osg::Vec2i& cellPosition, int cellSize, const osg::Vec3f& shift,
         const HeightfieldShape& shape)
     {
+        const std::lock_guard lock(mMutex);
         if (!mHeightfields.emplace(cellPosition, Heightfield {Cell {cellSize, shift}, shape}).second)
             return false;
         ++mRevision;
@@ -101,6 +107,7 @@ namespace DetourNavigator
 
     std::optional<Cell> RecastMeshManager::removeHeightfield(const osg::Vec2i& cellPosition)
     {
+        const std::lock_guard lock(mMutex);
         const auto it = mHeightfields.find(cellPosition);
         if (it == mHeightfields.end())
             return std::nullopt;
@@ -116,20 +123,27 @@ namespace DetourNavigator
         tileBounds.mMin /= mSettings.mRecastScaleFactor;
         tileBounds.mMax /= mSettings.mRecastScaleFactor;
         RecastMeshBuilder builder(tileBounds);
-        for (const auto& [k, v] : mWater)
-            builder.addWater(v.mSize, v.mShift);
-        for (const auto& [k, object] : mObjects)
+        std::vector<RecastMeshObject> objects;
+        std::size_t revision;
         {
-            const RecastMeshObject& v = object.getImpl();
-            builder.addObject(v.getShape(), v.getTransform(), v.getAreaType());
+            const std::lock_guard lock(mMutex);
+            for (const auto& [k, v] : mWater)
+                builder.addWater(v.mSize, v.mShift);
+            for (const auto& [cellPosition, v] : mHeightfields)
+                std::visit(AddHeightfield {v.mCell, builder}, v.mShape);
+            objects.reserve(mObjects.size());
+            for (const auto& [k, object] : mObjects)
+                objects.push_back(object.getImpl());
+            revision = mRevision;
         }
-        for (const auto& [cellPosition, v] : mHeightfields)
-            std::visit(AddHeightfield {v.mCell, builder}, v.mShape);
-        return std::move(builder).create(mGeneration, mRevision);
+        for (const auto& v : objects)
+            builder.addObject(v.getShape(), v.getTransform(), v.getAreaType());
+        return std::move(builder).create(mGeneration, revision);
     }
 
     bool RecastMeshManager::isEmpty() const
     {
+        const std::lock_guard lock(mMutex);
         return mObjects.empty() && mWater.empty() && mHeightfields.empty();
     }
 
@@ -137,6 +151,7 @@ namespace DetourNavigator
     {
         if (recastMeshVersion.mGeneration != mGeneration)
             return;
+        const std::lock_guard lock(mMutex);
         if (mLastNavMeshReport.has_value() && navMeshVersion < mLastNavMeshReport->mNavMeshVersion)
             return;
         mLastNavMeshReport = {recastMeshVersion.mRevision, navMeshVersion};
@@ -147,6 +162,7 @@ namespace DetourNavigator
 
     Version RecastMeshManager::getVersion() const
     {
+        const std::lock_guard lock(mMutex);
         return Version {mGeneration, mRevision};
     }
 }

components/detournavigator/recastmeshmanager.hpp

@@ -16,6 +16,7 @@
 #include <memory>
 #include <variant>
 #include <tuple>
+#include <mutex>
 
 class btCollisionShape;
 
@@ -73,9 +74,10 @@ namespace DetourNavigator
         };
 
         const Settings& mSettings;
+        const std::size_t mGeneration;
+        const TileBounds mTileBounds;
+        mutable std::mutex mMutex;
         std::size_t mRevision = 0;
-        std::size_t mGeneration;
-        TileBounds mTileBounds;
         std::map<ObjectId, OscillatingRecastMeshObject> mObjects;
         std::map<osg::Vec2i, Cell> mWater;
         std::map<osg::Vec2i, Heightfield> mHeightfields;

components/detournavigator/tilecachedrecastmeshmanager.cpp

@@ -69,7 +69,7 @@ namespace DetourNavigator
             const auto tiles = mTiles.lock();
             for (auto& tile : *tiles)
             {
-                if (tile.second.addWater(cellPosition, cellSize, shift))
+                if (tile.second->addWater(cellPosition, cellSize, shift))
                 {
                     tilesPositions.push_back(tile.first);
                     result = true;
@@ -88,9 +88,9 @@ namespace DetourNavigator
                         tileBounds.mMin -= osg::Vec2f(border, border);
                         tileBounds.mMax += osg::Vec2f(border, border);
                         tile = tiles->insert(std::make_pair(tilePosition,
-                                CachedRecastMeshManager(mSettings, tileBounds, mTilesGeneration))).first;
+                                std::make_shared<CachedRecastMeshManager>(mSettings, tileBounds, mTilesGeneration))).first;
                     }
-                    if (tile->second.addWater(cellPosition, cellSize, shift))
+                    if (tile->second->addWater(cellPosition, cellSize, shift))
                     {
                         tilesPositions.push_back(tilePosition);
                         result = true;
@@ -116,8 +116,8 @@ namespace DetourNavigator
             const auto tile = tiles->find(tilePosition);
             if (tile == tiles->end())
                 continue;
-            const auto tileResult = tile->second.removeWater(cellPosition);
-            if (tile->second.isEmpty())
+            const auto tileResult = tile->second->removeWater(cellPosition);
+            if (tile->second->isEmpty())
             {
                 tiles->erase(tile);
                 ++mTilesGeneration;
@@ -149,9 +149,9 @@ namespace DetourNavigator
                     tileBounds.mMin -= osg::Vec2f(border, border);
                     tileBounds.mMax += osg::Vec2f(border, border);
                     tile = tiles->insert(std::make_pair(tilePosition,
-                            CachedRecastMeshManager(mSettings, tileBounds, mTilesGeneration))).first;
+                            std::make_shared<CachedRecastMeshManager>(mSettings, tileBounds, mTilesGeneration))).first;
                 }
-                if (tile->second.addHeightfield(cellPosition, cellSize, shift, shape))
+                if (tile->second->addHeightfield(cellPosition, cellSize, shift, shape))
                 {
                     tilesPositions.push_back(tilePosition);
                     result = true;
@@ -176,8 +176,8 @@ namespace DetourNavigator
             const auto tile = tiles->find(tilePosition);
             if (tile == tiles->end())
                 continue;
-            const auto tileResult = tile->second.removeHeightfield(cellPosition);
-            if (tile->second.isEmpty())
+            const auto tileResult = tile->second->removeHeightfield(cellPosition);
+            if (tile->second->isEmpty())
             {
                 tiles->erase(tile);
                 ++mTilesGeneration;
@@ -192,11 +192,17 @@ namespace DetourNavigator
 
     std::shared_ptr<RecastMesh> TileCachedRecastMeshManager::getMesh(const TilePosition& tilePosition)
     {
-        const auto tiles = mTiles.lock();
-        const auto it = tiles->find(tilePosition);
-        if (it == tiles->end())
+        const auto manager = [&] () -> std::shared_ptr<CachedRecastMeshManager>
+        {
+            const auto tiles = mTiles.lock();
+            const auto it = tiles->find(tilePosition);
+            if (it == tiles->end())
+                return nullptr;
+            return it->second;
+        } ();
+        if (manager == nullptr)
             return nullptr;
-        return it->second.getMesh();
+        return manager->getMesh();
     }
 
     bool TileCachedRecastMeshManager::hasTile(const TilePosition& tilePosition)
@@ -215,12 +221,12 @@ namespace DetourNavigator
         const auto it = tiles->find(tilePosition);
         if (it == tiles->end())
             return;
-        it->second.reportNavMeshChange(recastMeshVersion, navMeshVersion);
+        it->second->reportNavMeshChange(recastMeshVersion, navMeshVersion);
     }
 
     bool TileCachedRecastMeshManager::addTile(const ObjectId id, const CollisionShape& shape,
         const btTransform& transform, const AreaType areaType, const TilePosition& tilePosition, float border,
-        std::map<TilePosition, CachedRecastMeshManager>& tiles)
+        TilesMap& tiles)
     {
         auto tile = tiles.find(tilePosition);
         if (tile == tiles.end())
@@ -229,26 +235,26 @@ namespace DetourNavigator
             tileBounds.mMin -= osg::Vec2f(border, border);
             tileBounds.mMax += osg::Vec2f(border, border);
             tile = tiles.insert(std::make_pair(
-                tilePosition, CachedRecastMeshManager(mSettings, tileBounds, mTilesGeneration))).first;
+                tilePosition, std::make_shared<CachedRecastMeshManager>(mSettings, tileBounds, mTilesGeneration))).first;
         }
-        return tile->second.addObject(id, shape, transform, areaType);
+        return tile->second->addObject(id, shape, transform, areaType);
     }
 
     bool TileCachedRecastMeshManager::updateTile(const ObjectId id, const btTransform& transform,
-        const AreaType areaType, const TilePosition& tilePosition, std::map<TilePosition, CachedRecastMeshManager>& tiles)
+        const AreaType areaType, const TilePosition& tilePosition, TilesMap& tiles)
     {
         const auto tile = tiles.find(tilePosition);
-        return tile != tiles.end() && tile->second.updateObject(id, transform, areaType);
+        return tile != tiles.end() && tile->second->updateObject(id, transform, areaType);
     }
 
     std::optional<RemovedRecastMeshObject> TileCachedRecastMeshManager::removeTile(const ObjectId id,
-        const TilePosition& tilePosition, std::map<TilePosition, CachedRecastMeshManager>& tiles)
+        const TilePosition& tilePosition, TilesMap& tiles)
     {
         const auto tile = tiles.find(tilePosition);
         if (tile == tiles.end())
             return std::optional<RemovedRecastMeshObject>();
-        const auto tileResult = tile->second.removeObject(id);
-        if (tile->second.isEmpty())
+        const auto tileResult = tile->second->removeObject(id);
+        if (tile->second->isEmpty())
         {
             tiles.erase(tile);
             ++mTilesGeneration;

components/detournavigator/tilecachedrecastmeshmanager.hpp

@@ -94,7 +94,7 @@ namespace DetourNavigator
         void forEachTile(Function&& function)
         {
             for (auto& [tilePosition, recastMeshManager] : *mTiles.lock())
-                function(tilePosition, recastMeshManager);
+                function(tilePosition, *recastMeshManager);
         }
 
         std::size_t getRevision() const;
@@ -102,8 +102,10 @@ namespace DetourNavigator
         void reportNavMeshChange(const TilePosition& tilePosition, Version recastMeshVersion, Version navMeshVersion);
 
     private:
+        using TilesMap = std::map<TilePosition, std::shared_ptr<CachedRecastMeshManager>>;
+
         const Settings& mSettings;
-        Misc::ScopeGuarded<std::map<TilePosition, CachedRecastMeshManager>> mTiles;
+        Misc::ScopeGuarded<TilesMap> mTiles;
         std::unordered_map<ObjectId, std::vector<TilePosition>> mObjectsTilesPositions;
         std::map<osg::Vec2i, std::vector<TilePosition>> mWaterTilesPositions;
         std::map<osg::Vec2i, std::vector<TilePosition>> mHeightfieldTilesPositions;
@@ -111,14 +113,13 @@ namespace DetourNavigator
         std::size_t mTilesGeneration = 0;
 
         bool addTile(const ObjectId id, const CollisionShape& shape, const btTransform& transform,
-                     const AreaType areaType, const TilePosition& tilePosition, float border,
-                     std::map<TilePosition, CachedRecastMeshManager>& tiles);
+                const AreaType areaType, const TilePosition& tilePosition, float border, TilesMap& tiles);
 
         bool updateTile(const ObjectId id, const btTransform& transform, const AreaType areaType,
-                        const TilePosition& tilePosition, std::map<TilePosition, CachedRecastMeshManager>& tiles);
+                const TilePosition& tilePosition, TilesMap& tiles);
 
         std::optional<RemovedRecastMeshObject> removeTile(const ObjectId id, const TilePosition& tilePosition,
-                                                            std::map<TilePosition, CachedRecastMeshManager>& tiles);
+                TilesMap& tiles);
     };
 }