Use ScopeGuarded instead of raw mutex

components/detournavigator/asyncnavmeshupdater.cpp

@@ -72,9 +72,7 @@ namespace DetourNavigator
         const std::shared_ptr<NavMeshCacheItem>& navMeshCacheItem, const TilePosition& playerTile,
         const std::map<TilePosition, ChangeType>& changedTiles)
     {
-        log("post jobs playerTile=", playerTile);
-
-        setPlayerTile(playerTile);
+        *mPlayerTile.lock() = playerTile;
 
         if (changedTiles.empty())
             return;
@@ -124,10 +122,10 @@ namespace DetourNavigator
 
         const auto start = std::chrono::steady_clock::now();
 
-        setFirstStart(start);
+        const auto firstStart = setFirstStart(start);
 
         const auto recastMesh = mRecastMeshManager.get().getMesh(job.mChangedTile);
-        const auto playerTile = getPlayerTile();
+        const auto playerTile = *mPlayerTile.lockConst();
         const auto offMeshConnections = mOffMeshConnectionsManager.get().get(job.mChangedTile);
 
         const auto status = updateNavMesh(job.mAgentHalfExtents, recastMesh.get(), job.mChangedTile, playerTile,
@@ -143,7 +141,7 @@ namespace DetourNavigator
             " generation=", job.mNavMeshCacheItem->mGeneration,
             " revision=", job.mNavMeshCacheItem->mNavMeshRevision,
             " time=", std::chrono::duration_cast<FloatMs>(finish - start).count(), "ms",
-            " total_time=", std::chrono::duration_cast<FloatMs>(finish - getFirstStart()).count(), "ms");
+            " total_time=", std::chrono::duration_cast<FloatMs>(finish - firstStart).count(), "ms");
     }
 
     boost::optional<AsyncNavMeshUpdater::Job> AsyncNavMeshUpdater::getNextJob()
@@ -188,28 +186,11 @@ namespace DetourNavigator
             writeToFile(*job.mNavMeshCacheItem->mValue.lock(), mSettings.get().mNavMeshPathPrefix, navMeshRevision);
     }
 
-    std::chrono::steady_clock::time_point AsyncNavMeshUpdater::getFirstStart()
+    std::chrono::steady_clock::time_point AsyncNavMeshUpdater::setFirstStart(const std::chrono::steady_clock::time_point& value)
     {
-        const std::lock_guard<std::mutex> lock(mFirstStartMutex);
-        return *mFirstStart;
-    }
-
-    void AsyncNavMeshUpdater::setFirstStart(const std::chrono::steady_clock::time_point& value)
-    {
-        const std::lock_guard<std::mutex> lock(mFirstStartMutex);
-        if (!mFirstStart)
-            mFirstStart = value;
-    }
-
-    TilePosition AsyncNavMeshUpdater::getPlayerTile()
-    {
-        const std::lock_guard<std::mutex> lock(mPlayerTileMutex);
-        return mPlayerTile;
-    }
-
-    void AsyncNavMeshUpdater::setPlayerTile(const TilePosition& value)
-    {
-        const std::lock_guard<std::mutex> lock(mPlayerTileMutex);
-        mPlayerTile = value;
+        const auto locked = mFirstStart.lock();
+        if (!*locked)
+            *locked = value;
+        return *locked.get();
     }
 }

components/detournavigator/asyncnavmeshupdater.hpp

@@ -67,10 +67,8 @@ namespace DetourNavigator
         std::condition_variable mDone;
         Jobs mJobs;
         std::map<osg::Vec3f, std::set<TilePosition>> mPushed;
-        std::mutex mPlayerTileMutex;
-        TilePosition mPlayerTile;
-        std::mutex mFirstStartMutex;
-        boost::optional<std::chrono::steady_clock::time_point> mFirstStart;
+        Misc::ScopeGuarded<TilePosition> mPlayerTile;
+        Misc::ScopeGuarded<boost::optional<std::chrono::steady_clock::time_point>> mFirstStart;
         std::vector<std::thread> mThreads;
 
         void process() throw();
@@ -79,17 +77,9 @@ namespace DetourNavigator
 
         boost::optional<Job> getNextJob();
 
-        void notifyHasJob();
-
         void writeDebugFiles(const Job& job, const RecastMesh* recastMesh) const;
 
-        std::chrono::steady_clock::time_point getFirstStart();
-
-        void setFirstStart(const std::chrono::steady_clock::time_point& value);
-
-        TilePosition getPlayerTile();
-
-        void setPlayerTile(const TilePosition& value);
+        std::chrono::steady_clock::time_point setFirstStart(const std::chrono::steady_clock::time_point& value);
     };
 }
 

components/detournavigator/debug.hpp

@@ -4,6 +4,7 @@
 #include "tilebounds.hpp"
 
 #include <components/bullethelpers/operators.hpp>
+#include <components/misc/guarded.hpp>
 #include <components/osghelpers/operators.hpp>
 
 #include <chrono>
@@ -11,7 +12,6 @@
 #include <iomanip>
 #include <iostream>
 #include <memory>
-#include <mutex>
 #include <sstream>
 #include <string>
 #include <thread>
@@ -77,21 +77,19 @@ namespace DetourNavigator
     public:
         void setSink(std::unique_ptr<Sink> sink)
         {
-            const std::lock_guard<std::mutex> guard(mMutex);
-            mSink = std::move(sink);
+            *mSink.lock() = std::move(sink);
         }
 
-        bool isEnabled() const
+        bool isEnabled()
         {
-            const std::lock_guard<std::mutex> guard(mMutex);
-            return bool(mSink);
+            return bool(*mSink.lockConst());
         }
 
         void write(const std::string& text)
         {
-            const std::lock_guard<std::mutex> guard(mMutex);
-            if (mSink)
-                mSink->write(text);
+            const auto sink = mSink.lock();
+            if (*sink)
+                sink.get()->write(text);
         }
 
         static Log& instance()
@@ -101,8 +99,7 @@ namespace DetourNavigator
         }
 
     private:
-        mutable std::mutex mMutex;
-        std::unique_ptr<Sink> mSink;
+        Misc::ScopeGuarded<std::unique_ptr<Sink>> mSink;
     };
 
     inline void write(std::ostream& stream)

components/detournavigator/offmeshconnectionsmanager.hpp

@@ -6,6 +6,8 @@
 #include "tileposition.hpp"
 #include "objectid.hpp"
 
+#include <components/misc/guarded.hpp>
+
 #include <osg/Vec3f>
 
 #include <boost/optional.hpp>
@@ -33,42 +35,42 @@ namespace DetourNavigator
 
         bool add(const ObjectId id, const OffMeshConnection& value)
         {
-            const std::lock_guard<std::mutex> lock(mMutex);
+            const auto values = mValues.lock();
 
-            if (!mValuesById.insert(std::make_pair(id, value)).second)
+            if (!values->mById.insert(std::make_pair(id, value)).second)
                 return false;
 
             const auto startTilePosition = getTilePosition(mSettings, value.mStart);
             const auto endTilePosition = getTilePosition(mSettings, value.mEnd);
 
-            mValuesByTilePosition[startTilePosition].insert(id);
+            values->mByTilePosition[startTilePosition].insert(id);
 
             if (startTilePosition != endTilePosition)
-                mValuesByTilePosition[endTilePosition].insert(id);
+                values->mByTilePosition[endTilePosition].insert(id);
 
             return true;
         }
 
         boost::optional<OffMeshConnection> remove(const ObjectId id)
         {
-            const std::lock_guard<std::mutex> lock(mMutex);
+            const auto values = mValues.lock();
 
-            const auto itById = mValuesById.find(id);
+            const auto itById = values->mById.find(id);
 
-            if (itById == mValuesById.end())
+            if (itById == values->mById.end())
                 return boost::none;
 
             const auto result = itById->second;
 
-            mValuesById.erase(itById);
+            values->mById.erase(itById);
 
             const auto startTilePosition = getTilePosition(mSettings, result.mStart);
             const auto endTilePosition = getTilePosition(mSettings, result.mEnd);
 
-            removeByTilePosition(startTilePosition, id);
+            removeByTilePosition(values->mByTilePosition, startTilePosition, id);
 
             if (startTilePosition != endTilePosition)
-                removeByTilePosition(endTilePosition, id);
+                removeByTilePosition(values->mByTilePosition, endTilePosition, id);
 
             return result;
         }
@@ -77,18 +79,18 @@ namespace DetourNavigator
         {
             std::vector<OffMeshConnection> result;
 
-            const std::lock_guard<std::mutex> lock(mMutex);
+            const auto values = mValues.lock();
 
-            const auto itByTilePosition = mValuesByTilePosition.find(tilePosition);
+            const auto itByTilePosition = values->mByTilePosition.find(tilePosition);
 
-            if (itByTilePosition == mValuesByTilePosition.end())
+            if (itByTilePosition == values->mByTilePosition.end())
                 return result;
 
             std::for_each(itByTilePosition->second.begin(), itByTilePosition->second.end(),
                 [&] (const ObjectId v)
                 {
-                    const auto itById = mValuesById.find(v);
-                    if (itById != mValuesById.end())
+                    const auto itById = values->mById.find(v);
+                    if (itById != values->mById.end())
                         result.push_back(itById->second);
                 });
 
@@ -96,15 +98,20 @@ namespace DetourNavigator
         }
 
     private:
-        const Settings& mSettings;
-        std::mutex mMutex;
-        std::unordered_map<ObjectId, OffMeshConnection> mValuesById;
-        std::map<TilePosition, std::unordered_set<ObjectId>> mValuesByTilePosition;
-
-        void removeByTilePosition(const TilePosition& tilePosition, const ObjectId id)
+        struct Values
         {
-            const auto it = mValuesByTilePosition.find(tilePosition);
-            if (it != mValuesByTilePosition.end())
+            std::unordered_map<ObjectId, OffMeshConnection> mById;
+            std::map<TilePosition, std::unordered_set<ObjectId>> mByTilePosition;
+        };
+
+        const Settings& mSettings;
+        Misc::ScopeGuarded<Values> mValues;
+
+        void removeByTilePosition(std::map<TilePosition, std::unordered_set<ObjectId>>& valuesByTilePosition,
+            const TilePosition& tilePosition, const ObjectId id)
+        {
+            const auto it = valuesByTilePosition.find(tilePosition);
+            if (it != valuesByTilePosition.end())
                 it->second.erase(id);
         }
     };

components/detournavigator/tilecachedrecastmeshmanager.cpp

@@ -17,19 +17,18 @@ namespace DetourNavigator
         const auto border = getBorderSize(mSettings);
         getTilesPositions(shape, transform, mSettings, [&] (const TilePosition& tilePosition)
             {
-                std::unique_lock<std::mutex> lock(mTilesMutex);
-                auto tile = mTiles.find(tilePosition);
-                if (tile == mTiles.end())
+                const auto tiles = mTiles.lock();
+                auto tile = tiles->find(tilePosition);
+                if (tile == tiles->end())
                 {
                     auto tileBounds = makeTileBounds(mSettings, tilePosition);
                     tileBounds.mMin -= osg::Vec2f(border, border);
                     tileBounds.mMax += osg::Vec2f(border, border);
-                    tile = mTiles.insert(std::make_pair(tilePosition,
+                    tile = tiles->insert(std::make_pair(tilePosition,
                             CachedRecastMeshManager(mSettings, tileBounds))).first;
                 }
                 if (tile->second.addObject(id, shape, transform, areaType))
                 {
-                    lock.unlock();
                     tilesPositions.push_back(tilePosition);
                     result = true;
                 }
@@ -46,14 +45,15 @@ namespace DetourNavigator
         if (object == mObjectsTilesPositions.end())
             return false;
         bool result = false;
-        std::unique_lock<std::mutex> lock(mTilesMutex);
-        for (const auto& tilePosition : object->second)
         {
-            const auto tile = mTiles.find(tilePosition);
-            if (tile != mTiles.end())
-                result = tile->second.updateObject(id, transform, areaType) || result;
+            const auto tiles = mTiles.lock();
+            for (const auto& tilePosition : object->second)
+            {
+                const auto tile = tiles->find(tilePosition);
+                if (tile != tiles->end())
+                    result = tile->second.updateObject(id, transform, areaType) || result;
+            }
         }
-        lock.unlock();
         if (result)
             ++mRevision;
         return result;
@@ -67,14 +67,13 @@ namespace DetourNavigator
         boost::optional<RemovedRecastMeshObject> result;
         for (const auto& tilePosition : object->second)
         {
-            std::unique_lock<std::mutex> lock(mTilesMutex);
-            const auto tile = mTiles.find(tilePosition);
-            if (tile == mTiles.end())
+            const auto tiles = mTiles.lock();
+            const auto tile = tiles->find(tilePosition);
+            if (tile == tiles->end())
                 continue;
             const auto tileResult = tile->second.removeObject(id);
             if (tile->second.isEmpty())
-                mTiles.erase(tile);
-            lock.unlock();
+                tiles->erase(tile);
             if (tileResult && !result)
                 result = tileResult;
         }
@@ -94,8 +93,8 @@ namespace DetourNavigator
 
         if (cellSize == std::numeric_limits<int>::max())
         {
-            const std::lock_guard<std::mutex> lock(mTilesMutex);
-            for (auto& tile : mTiles)
+            const auto tiles = mTiles.lock();
+            for (auto& tile : *tiles)
             {
                 if (tile.second.addWater(cellPosition, cellSize, transform))
                 {
@@ -108,19 +107,18 @@ namespace DetourNavigator
         {
             getTilesPositions(cellSize, transform, mSettings, [&] (const TilePosition& tilePosition)
                 {
-                    std::unique_lock<std::mutex> lock(mTilesMutex);
-                    auto tile = mTiles.find(tilePosition);
-                    if (tile == mTiles.end())
+                    const auto tiles = mTiles.lock();
+                    auto tile = tiles->find(tilePosition);
+                    if (tile == tiles->end())
                     {
                         auto tileBounds = makeTileBounds(mSettings, tilePosition);
                         tileBounds.mMin -= osg::Vec2f(border, border);
                         tileBounds.mMax += osg::Vec2f(border, border);
-                        tile = mTiles.insert(std::make_pair(tilePosition,
+                        tile = tiles->insert(std::make_pair(tilePosition,
                                 CachedRecastMeshManager(mSettings, tileBounds))).first;
                     }
                     if (tile->second.addWater(cellPosition, cellSize, transform))
                     {
-                        lock.unlock();
                         tilesPositions.push_back(tilePosition);
                         result = true;
                     }
@@ -141,14 +139,13 @@ namespace DetourNavigator
         boost::optional<RecastMeshManager::Water> result;
         for (const auto& tilePosition : object->second)
         {
-            std::unique_lock<std::mutex> lock(mTilesMutex);
-            const auto tile = mTiles.find(tilePosition);
-            if (tile == mTiles.end())
+            const auto tiles = mTiles.lock();
+            const auto tile = tiles->find(tilePosition);
+            if (tile == tiles->end())
                 continue;
             const auto tileResult = tile->second.removeWater(cellPosition);
             if (tile->second.isEmpty())
-                mTiles.erase(tile);
-            lock.unlock();
+                tiles->erase(tile);
             if (tileResult && !result)
                 result = tileResult;
         }
@@ -159,17 +156,16 @@ namespace DetourNavigator
 
     std::shared_ptr<RecastMesh> TileCachedRecastMeshManager::getMesh(const TilePosition& tilePosition)
     {
-        const std::lock_guard<std::mutex> lock(mTilesMutex);
-        const auto it = mTiles.find(tilePosition);
-        if (it == mTiles.end())
+        const auto tiles = mTiles.lock();
+        const auto it = tiles->find(tilePosition);
+        if (it == tiles->end())
             return nullptr;
         return it->second.getMesh();
     }
 
     bool TileCachedRecastMeshManager::hasTile(const TilePosition& tilePosition)
     {
-        const std::lock_guard<std::mutex> lock(mTilesMutex);
-        return mTiles.count(tilePosition);
+        return mTiles.lockConst()->count(tilePosition);
     }
 
     std::size_t TileCachedRecastMeshManager::getRevision() const

components/detournavigator/tilecachedrecastmeshmanager.hpp

@@ -4,6 +4,8 @@
 #include "cachedrecastmeshmanager.hpp"
 #include "tileposition.hpp"
 
+#include <components/misc/guarded.hpp>
+
 #include <map>
 #include <mutex>
 
@@ -32,8 +34,7 @@ namespace DetourNavigator
         template <class Function>
         void forEachTilePosition(Function&& function)
         {
-            const std::lock_guard<std::mutex> lock(mTilesMutex);
-            for (const auto& tile : mTiles)
+            for (const auto& tile : *mTiles.lock())
                 function(tile.first);
         }
 
@@ -41,8 +42,7 @@ namespace DetourNavigator
 
     private:
         const Settings& mSettings;
-        std::mutex mTilesMutex;
-        std::map<TilePosition, CachedRecastMeshManager> mTiles;
+        Misc::ScopeGuarded<std::map<TilePosition, CachedRecastMeshManager>> mTiles;
         std::unordered_map<ObjectId, std::vector<TilePosition>> mObjectsTilesPositions;
         std::map<osg::Vec2i, std::vector<TilePosition>> mWaterTilesPositions;
         std::size_t mRevision = 0;

components/misc/guarded.hpp

@@ -34,6 +34,35 @@ namespace Misc
             std::reference_wrapper<T> mValue;
     };
 
+    template <class T>
+    class ScopeGuarded
+    {
+        public:
+            ScopeGuarded() = default;
+
+            ScopeGuarded(const T& value)
+                : mValue(value)
+            {}
+
+            ScopeGuarded(T&& value)
+                : mValue(std::move(value))
+            {}
+
+            Locked<T> lock()
+            {
+                return Locked<T>(mMutex, mValue);
+            }
+
+            Locked<const T> lockConst()
+            {
+                return Locked<const T>(mMutex, mValue);
+            }
+
+        private:
+            std::mutex mMutex;
+            T mValue;
+    };
+
     template <class T>
     class SharedGuarded
     {