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, transform))
             return false;
-        mCached.reset();
+        mCached.lock()->reset();
         return true;
     }
 
@@ -46,15 +46,18 @@ namespace DetourNavigator
     {
         const auto water = mImpl.removeWater(cellPosition);
         if (water)
-            mCached.reset();
+            mCached.lock()->reset();
         return water;
     }
 
     std::shared_ptr<RecastMesh> CachedRecastMeshManager::getMesh()
     {
-        if (!mCached)
+        std::shared_ptr<RecastMesh> cached = *mCached.lock();
-            mCached = mImpl.getMesh();
+        if (cached != nullptr)
-        return mCached;
+            return cached;
+        cached = mImpl.getMesh();
+        *mCached.lock() = cached;
+        return cached;
     }
 
     bool CachedRecastMeshManager::isEmpty() const

components/detournavigator/cachedrecastmeshmanager.hpp

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

components/detournavigator/recastmeshmanager.cpp

@@ -13,6 +13,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;
@@ -25,6 +26,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;
@@ -38,6 +40,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;
@@ -51,6 +54,7 @@ namespace DetourNavigator
     bool RecastMeshManager::addWater(const osg::Vec2i& cellPosition, const int cellSize,
         const btTransform& transform)
     {
+        const std::lock_guard lock(mMutex);
         const auto iterator = mWaterOrder.emplace(mWaterOrder.end(), Water {cellSize, transform});
         if (!mWater.emplace(cellPosition, iterator).second)
         {
@@ -63,6 +67,7 @@ namespace DetourNavigator
 
     std::optional<RecastMeshManager::Water> 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;
@@ -76,18 +81,34 @@ namespace DetourNavigator
     std::shared_ptr<RecastMesh> RecastMeshManager::getMesh()
     {
         RecastMeshBuilder builder(mSettings, mTileBounds);
-        for (const auto& v : mWaterOrder)
+        using Object = std::tuple<
-            builder.addWater(v.mCellSize, v.mTransform);
+            osg::ref_ptr<const osg::Object>,
-        for (const auto& object : mObjectsOrder)
+            std::reference_wrapper<const btCollisionShape>,
+            btTransform,
+            AreaType
+        >;
+        std::vector<Object> objects;
+        std::size_t revision;
         {
-            const RecastMeshObject& v = object.getImpl();
+            const std::lock_guard lock(mMutex);
-            builder.addObject(v.getShape(), v.getTransform(), v.getAreaType());
+            for (const auto& v : mWaterOrder)
+                builder.addWater(v.mCellSize, v.mTransform);
+            objects.reserve(mObjectsOrder.size());
+            for (const auto& object : mObjectsOrder)
+            {
+                const RecastMeshObject& impl = object.getImpl();
+                objects.emplace_back(impl.getHolder(), impl.getShape(), impl.getTransform(), impl.getAreaType());
+            }
+            revision = mRevision;
         }
-        return std::move(builder).create(mGeneration, mRevision);
+        for (const auto& [holder, shape, transform, areaType] : objects)
+            builder.addObject(shape, transform, areaType);
+        return std::move(builder).create(mGeneration, revision);
     }
 
     bool RecastMeshManager::isEmpty() const
     {
+        const std::lock_guard lock(mMutex);
         return mObjects.empty();
     }
 
@@ -95,6 +116,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};
@@ -105,6 +127,7 @@ namespace DetourNavigator
 
     Version RecastMeshManager::getVersion() const
     {
+        const std::lock_guard lock(mMutex);
         return Version {mGeneration, mRevision};
     }
 }

components/detournavigator/recastmeshmanager.hpp

@@ -13,6 +13,7 @@
 #include <map>
 #include <optional>
 #include <memory>
+#include <mutex>
 
 class btCollisionShape;
 
@@ -65,9 +66,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::list<OscillatingRecastMeshObject> mObjectsOrder;
         std::map<ObjectId, std::list<OscillatingRecastMeshObject>::iterator> mObjects;
         std::list<Water> mWaterOrder;

components/detournavigator/recastmeshobject.hpp

@@ -39,6 +39,11 @@ namespace DetourNavigator
 
             bool update(const btTransform& transform, const AreaType areaType);
 
+            const osg::ref_ptr<const osg::Object>& getHolder() const
+            {
+                return mHolder;
+            }
+
             const btCollisionShape& getShape() const
             {
                 return mShape;

components/detournavigator/tilecachedrecastmeshmanager.cpp

@@ -67,7 +67,7 @@ namespace DetourNavigator
             const auto tiles = mTiles.lock();
             for (auto& tile : *tiles)
             {
-                if (tile.second.addWater(cellPosition, cellSize, transform))
+                if (tile.second->addWater(cellPosition, cellSize, transform))
                 {
                     tilesPositions.push_back(tile.first);
                     result = true;
@@ -86,9 +86,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, transform))
+                    if (tile->second->addWater(cellPosition, cellSize, transform))
                     {
                         tilesPositions.push_back(tilePosition);
                         result = true;
@@ -114,8 +114,8 @@ namespace DetourNavigator
             const auto tile = tiles->find(tilePosition);
             if (tile == tiles->end())
                 continue;
-            const auto tileResult = tile->second.removeWater(cellPosition);
+            const auto tileResult = tile->second->removeWater(cellPosition);
-            if (tile->second.isEmpty())
+            if (tile->second->isEmpty())
             {
                 tiles->erase(tile);
                 ++mTilesGeneration;
@@ -130,11 +130,17 @@ namespace DetourNavigator
 
     std::shared_ptr<RecastMesh> TileCachedRecastMeshManager::getMesh(const TilePosition& tilePosition)
     {
-        const auto tiles = mTiles.lock();
+        const auto manager = [&] () -> std::shared_ptr<CachedRecastMeshManager>
-        const auto it = tiles->find(tilePosition);
+        {
-        if (it == tiles->end())
+            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)
@@ -153,12 +159,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())
@@ -167,26 +173,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);
+        const auto tileResult = tile->second->removeObject(id);
-        if (tile->second.isEmpty())
+        if (tile->second->isEmpty())
         {
             tiles.erase(tile);
             ++mTilesGeneration;

components/detournavigator/tilecachedrecastmeshmanager.hpp

@@ -88,7 +88,7 @@ namespace DetourNavigator
         void forEachTile(Function&& function)
         {
             for (auto& [tilePosition, recastMeshManager] : *mTiles.lock())
-                function(tilePosition, recastMeshManager);
+                function(tilePosition, *recastMeshManager);
         }
 
         std::size_t getRevision() const;
@@ -96,22 +96,23 @@ 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::size_t mRevision = 0;
         std::size_t mTilesGeneration = 0;
 
         bool addTile(const ObjectId id, const CollisionShape& shape, const btTransform& transform,
-                     const AreaType areaType, const TilePosition& tilePosition, float border,
+                const AreaType areaType, const TilePosition& tilePosition, float border, TilesMap& tiles);
-                     std::map<TilePosition, CachedRecastMeshManager>& 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);
     };
 }