From 0479ebf5ae9a0e4ddee58c3a3b4277ead38477ca Mon Sep 17 00:00:00 2001 From: elsid Date: Sun, 10 Mar 2019 14:36:16 +0300 Subject: [PATCH 1/3] Remove unused actors and navmeshes on update When there is only one actor (player) on a scene and it moving to other cell first it will be removed from navigator then added. Remove cause navmesh removing for its half extents. After it is added navmesh for same half extents is created and added. While this all happens there are still jobs for old navmesh are processing. Old navmesh still exists because it is stored by shared pointer. So jobs take tiles from cache and place them into old navmesh. After that other jobs take same tiles from cache (half extents and coordinates are equal) and place them into other navmesh. dtNavMesh changes tile data on add and remove. Adding tile to two dtNavMesh corrupts tile in both nameshes. --- .../detournavigator/navigator.cpp | 8 ------- components/detournavigator/navigatorimpl.cpp | 18 ++++++++++++--- components/detournavigator/navigatorimpl.hpp | 1 + components/detournavigator/navmeshmanager.cpp | 22 ++++++++++++++++++- components/detournavigator/navmeshmanager.hpp | 2 +- 5 files changed, 38 insertions(+), 13 deletions(-) diff --git a/apps/openmw_test_suite/detournavigator/navigator.cpp b/apps/openmw_test_suite/detournavigator/navigator.cpp index 2bf7bf643..5f5433b05 100644 --- a/apps/openmw_test_suite/detournavigator/navigator.cpp +++ b/apps/openmw_test_suite/detournavigator/navigator.cpp @@ -80,14 +80,6 @@ namespace EXPECT_THROW(mNavigator->findPath(mAgentHalfExtents, mStepSize, mStart, mEnd, Flag_walk, mOut), NavigatorException); } - TEST_F(DetourNavigatorNavigatorTest, find_path_for_removed_agent_should_return_empty) - { - mNavigator->addAgent(mAgentHalfExtents); - mNavigator->removeAgent(mAgentHalfExtents); - mNavigator->findPath(mAgentHalfExtents, mStepSize, mStart, mEnd, Flag_walk, mOut); - EXPECT_EQ(mPath, std::deque()); - } - TEST_F(DetourNavigatorNavigatorTest, add_agent_should_count_each_agent) { mNavigator->addAgent(mAgentHalfExtents); diff --git a/components/detournavigator/navigatorimpl.cpp b/components/detournavigator/navigatorimpl.cpp index 1b83769f4..2e16b6d04 100644 --- a/components/detournavigator/navigatorimpl.cpp +++ b/components/detournavigator/navigatorimpl.cpp @@ -21,10 +21,10 @@ namespace DetourNavigator void NavigatorImpl::removeAgent(const osg::Vec3f& agentHalfExtents) { const auto it = mAgents.find(agentHalfExtents); - if (it == mAgents.end() || --it->second) + if (it == mAgents.end()) return; - mAgents.erase(it); - mNavMeshManager.reset(agentHalfExtents); + if (it->second > 0) + --it->second; } bool NavigatorImpl::addObject(const ObjectId id, const btCollisionShape& shape, const btTransform& transform) @@ -113,6 +113,7 @@ namespace DetourNavigator void NavigatorImpl::update(const osg::Vec3f& playerPosition) { + removeUnusedNavMeshes(); for (const auto& v : mAgents) mNavMeshManager.update(playerPosition, v.first); } @@ -156,4 +157,15 @@ namespace DetourNavigator inserted.first->second = updateId; } } + + void NavigatorImpl::removeUnusedNavMeshes() + { + for (auto it = mAgents.begin(); it != mAgents.end();) + { + if (it->second == 0 && mNavMeshManager.reset(it->first)) + it = mAgents.erase(it); + else + ++it; + } + } } diff --git a/components/detournavigator/navigatorimpl.hpp b/components/detournavigator/navigatorimpl.hpp index 975055984..8156f6655 100644 --- a/components/detournavigator/navigatorimpl.hpp +++ b/components/detournavigator/navigatorimpl.hpp @@ -58,6 +58,7 @@ namespace DetourNavigator void updateAvoidShapeId(const ObjectId id, const ObjectId avoidId); void updateWaterShapeId(const ObjectId id, const ObjectId waterId); void updateId(const ObjectId id, const ObjectId waterId, std::unordered_map& ids); + void removeUnusedNavMeshes(); }; } diff --git a/components/detournavigator/navmeshmanager.cpp b/components/detournavigator/navmeshmanager.cpp index 217c17e41..3e2691565 100644 --- a/components/detournavigator/navmeshmanager.cpp +++ b/components/detournavigator/navmeshmanager.cpp @@ -16,6 +16,20 @@ namespace { return current == add ? current : ChangeType::mixed; } + + /// Safely reset shared_ptr with definite underlying object destrutor call. + /// Assuming there is another thread holding copy of this shared_ptr or weak_ptr to this shared_ptr. + template + bool resetIfUnique(std::shared_ptr& ptr) + { + const std::weak_ptr weak = std::move(ptr); + if (auto shared = weak.lock()) + { + ptr = std::move(shared); + return false; + } + return true; + } } namespace DetourNavigator @@ -83,9 +97,15 @@ namespace DetourNavigator log("cache add for agent=", agentHalfExtents); } - void NavMeshManager::reset(const osg::Vec3f& agentHalfExtents) + bool NavMeshManager::reset(const osg::Vec3f& agentHalfExtents) { + const auto it = mCache.find(agentHalfExtents); + if (it == mCache.end()) + return true; + if (!resetIfUnique(it->second)) + return false; mCache.erase(agentHalfExtents); + return true; } void NavMeshManager::addOffMeshConnection(const ObjectId id, const osg::Vec3f& start, const osg::Vec3f& end) diff --git a/components/detournavigator/navmeshmanager.hpp b/components/detournavigator/navmeshmanager.hpp index ae006da73..cee33c63d 100644 --- a/components/detournavigator/navmeshmanager.hpp +++ b/components/detournavigator/navmeshmanager.hpp @@ -36,7 +36,7 @@ namespace DetourNavigator bool removeWater(const osg::Vec2i& cellPosition); - void reset(const osg::Vec3f& agentHalfExtents); + bool reset(const osg::Vec3f& agentHalfExtents); void addOffMeshConnection(const ObjectId id, const osg::Vec3f& start, const osg::Vec3f& end); From ccc709a31627e0d702c378e22ab5537fe04581ff Mon Sep 17 00:00:00 2001 From: elsid Date: Sun, 10 Mar 2019 16:49:56 +0300 Subject: [PATCH 2/3] Store guarded navmesh cache item in shared_ptr Remove useless SharedGuarded type. --- apps/openmw/mwrender/renderingmanager.cpp | 2 +- .../detournavigator/asyncnavmeshupdater.cpp | 4 +-- components/detournavigator/makenavmesh.cpp | 6 ++-- components/detournavigator/navigator.hpp | 2 +- .../detournavigator/navmeshcacheitem.hpp | 3 +- components/detournavigator/navmeshmanager.cpp | 4 +-- components/misc/guarded.hpp | 32 ------------------- 7 files changed, 11 insertions(+), 42 deletions(-) diff --git a/apps/openmw/mwrender/renderingmanager.cpp b/apps/openmw/mwrender/renderingmanager.cpp index f54e423f1..c93707374 100644 --- a/apps/openmw/mwrender/renderingmanager.cpp +++ b/apps/openmw/mwrender/renderingmanager.cpp @@ -1446,7 +1446,7 @@ namespace MWRender { try { - const auto locked = it->second.lockConst(); + const auto locked = it->second->lockConst(); mNavMesh->update(locked->getValue(), mNavMeshNumber, locked->getGeneration(), locked->getNavMeshRevision(), mNavigator.getSettings()); } diff --git a/components/detournavigator/asyncnavmeshupdater.cpp b/components/detournavigator/asyncnavmeshupdater.cpp index ee3b6d77a..667fff88e 100644 --- a/components/detournavigator/asyncnavmeshupdater.cpp +++ b/components/detournavigator/asyncnavmeshupdater.cpp @@ -143,7 +143,7 @@ namespace DetourNavigator using FloatMs = std::chrono::duration; { - const auto locked = job.mNavMeshCacheItem.lockConst(); + const auto locked = job.mNavMeshCacheItem->lockConst(); log("cache updated for agent=", job.mAgentHalfExtents, " status=", status, " generation=", locked->getGeneration(), " revision=", locked->getNavMeshRevision(), @@ -194,7 +194,7 @@ namespace DetourNavigator writeToFile(*recastMesh, mSettings.get().mRecastMeshPathPrefix + std::to_string(job.mChangedTile.x()) + "_" + std::to_string(job.mChangedTile.y()) + "_", recastMeshRevision); if (mSettings.get().mEnableWriteNavMeshToFile) - writeToFile(job.mNavMeshCacheItem.lockConst()->getValue(), mSettings.get().mNavMeshPathPrefix, navMeshRevision); + writeToFile(job.mNavMeshCacheItem->lockConst()->getValue(), mSettings.get().mNavMeshPathPrefix, navMeshRevision); } std::chrono::steady_clock::time_point AsyncNavMeshUpdater::setFirstStart(const std::chrono::steady_clock::time_point& value) diff --git a/components/detournavigator/makenavmesh.cpp b/components/detournavigator/makenavmesh.cpp index 5ac28127e..5d2de481e 100644 --- a/components/detournavigator/makenavmesh.cpp +++ b/components/detournavigator/makenavmesh.cpp @@ -521,7 +521,7 @@ namespace UpdateNavMeshStatus replaceTile(const SharedNavMeshCacheItem& navMeshCacheItem, const TilePosition& changedTile, T&& navMeshData) { - const auto locked = navMeshCacheItem.lock(); + const auto locked = navMeshCacheItem->lock(); auto& navMesh = locked->getValue(); const int layer = 0; const auto tileRef = navMesh.getTileRefAt(changedTile.x(), changedTile.y(), layer); @@ -591,14 +591,14 @@ namespace DetourNavigator " playerTile=", playerTile, " changedTileDistance=", getDistance(changedTile, playerTile)); - const auto params = *navMeshCacheItem.lockConst()->getValue().getParams(); + const auto params = *navMeshCacheItem->lockConst()->getValue().getParams(); const osg::Vec3f origin(params.orig[0], params.orig[1], params.orig[2]); const auto x = changedTile.x(); const auto y = changedTile.y(); const auto removeTile = [&] { - const auto locked = navMeshCacheItem.lock(); + const auto locked = navMeshCacheItem->lock(); auto& navMesh = locked->getValue(); const auto tileRef = navMesh.getTileRefAt(x, y, 0); const auto removed = dtStatusSucceed(navMesh.removeTile(tileRef, nullptr, nullptr)); diff --git a/components/detournavigator/navigator.hpp b/components/detournavigator/navigator.hpp index a146fe0fb..cc62d6164 100644 --- a/components/detournavigator/navigator.hpp +++ b/components/detournavigator/navigator.hpp @@ -174,7 +174,7 @@ namespace DetourNavigator if (!navMesh) return out; const auto settings = getSettings(); - return findSmoothPath(navMesh.lock()->getValue(), toNavMeshCoordinates(settings, agentHalfExtents), + return findSmoothPath(navMesh->lockConst()->getValue(), toNavMeshCoordinates(settings, agentHalfExtents), toNavMeshCoordinates(settings, stepSize), toNavMeshCoordinates(settings, start), toNavMeshCoordinates(settings, end), includeFlags, settings, out); } diff --git a/components/detournavigator/navmeshcacheitem.hpp b/components/detournavigator/navmeshcacheitem.hpp index e64b9d138..e6ca60ef7 100644 --- a/components/detournavigator/navmeshcacheitem.hpp +++ b/components/detournavigator/navmeshcacheitem.hpp @@ -64,7 +64,8 @@ namespace DetourNavigator std::map> mUsedTiles; }; - using SharedNavMeshCacheItem = Misc::SharedGuarded; + using GuardedNavMeshCacheItem = Misc::ScopeGuarded; + using SharedNavMeshCacheItem = std::shared_ptr; } #endif diff --git a/components/detournavigator/navmeshmanager.cpp b/components/detournavigator/navmeshmanager.cpp index 3e2691565..03a8b055f 100644 --- a/components/detournavigator/navmeshmanager.cpp +++ b/components/detournavigator/navmeshmanager.cpp @@ -93,7 +93,7 @@ namespace DetourNavigator if (cached != mCache.end()) return; mCache.insert(std::make_pair(agentHalfExtents, - std::make_shared(makeEmptyNavMesh(mSettings), ++mGenerationCounter))); + std::make_shared(makeEmptyNavMesh(mSettings), ++mGenerationCounter))); log("cache add for agent=", agentHalfExtents); } @@ -159,7 +159,7 @@ namespace DetourNavigator } const auto changedTiles = mChangedTiles.find(agentHalfExtents); { - const auto locked = cached.lock(); + const auto locked = cached->lockConst(); const auto& navMesh = locked->getValue(); if (changedTiles != mChangedTiles.end()) { diff --git a/components/misc/guarded.hpp b/components/misc/guarded.hpp index 114a77b10..559476867 100644 --- a/components/misc/guarded.hpp +++ b/components/misc/guarded.hpp @@ -83,38 +83,6 @@ namespace Misc std::mutex mMutex; T mValue; }; - - template - class SharedGuarded - { - public: - SharedGuarded() - : mMutex(std::make_shared()), mValue() - {} - - SharedGuarded(std::shared_ptr value) - : mMutex(std::make_shared()), mValue(std::move(value)) - {} - - Locked lock() const - { - return Locked(*mMutex, *mValue); - } - - Locked lockConst() const - { - return Locked(*mMutex, *mValue); - } - - operator bool() const - { - return static_cast(mValue); - } - - private: - std::shared_ptr mMutex; - std::shared_ptr mValue; - }; } #endif From f6a1d3cecfac2291c3ff6f9c9a5213c57c218550 Mon Sep 17 00:00:00 2001 From: elsid Date: Sun, 10 Mar 2019 16:04:44 +0300 Subject: [PATCH 3/3] Store weak pointers to navmesh in jobs queue To avoid useless processing for removed navmeshes. --- components/detournavigator/asyncnavmeshupdater.cpp | 12 +++++++++--- components/detournavigator/asyncnavmeshupdater.hpp | 2 +- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/components/detournavigator/asyncnavmeshupdater.cpp b/components/detournavigator/asyncnavmeshupdater.cpp index 667fff88e..e9172f041 100644 --- a/components/detournavigator/asyncnavmeshupdater.cpp +++ b/components/detournavigator/asyncnavmeshupdater.cpp @@ -129,12 +129,17 @@ namespace DetourNavigator const auto firstStart = setFirstStart(start); + const auto navMeshCacheItem = job.mNavMeshCacheItem.lock(); + + if (!navMeshCacheItem) + return true; + const auto recastMesh = mRecastMeshManager.get().getMesh(job.mChangedTile); const auto playerTile = *mPlayerTile.lockConst(); const auto offMeshConnections = mOffMeshConnectionsManager.get().get(job.mChangedTile); const auto status = updateNavMesh(job.mAgentHalfExtents, recastMesh.get(), job.mChangedTile, playerTile, - offMeshConnections, mSettings, job.mNavMeshCacheItem, mNavMeshTilesCache); + offMeshConnections, mSettings, navMeshCacheItem, mNavMeshTilesCache); const auto finish = std::chrono::steady_clock::now(); @@ -143,7 +148,7 @@ namespace DetourNavigator using FloatMs = std::chrono::duration; { - const auto locked = job.mNavMeshCacheItem->lockConst(); + const auto locked = navMeshCacheItem->lockConst(); log("cache updated for agent=", job.mAgentHalfExtents, " status=", status, " generation=", locked->getGeneration(), " revision=", locked->getNavMeshRevision(), @@ -194,7 +199,8 @@ namespace DetourNavigator writeToFile(*recastMesh, mSettings.get().mRecastMeshPathPrefix + std::to_string(job.mChangedTile.x()) + "_" + std::to_string(job.mChangedTile.y()) + "_", recastMeshRevision); if (mSettings.get().mEnableWriteNavMeshToFile) - writeToFile(job.mNavMeshCacheItem->lockConst()->getValue(), mSettings.get().mNavMeshPathPrefix, navMeshRevision); + if (const auto shared = job.mNavMeshCacheItem.lock()) + writeToFile(shared->lockConst()->getValue(), mSettings.get().mNavMeshPathPrefix, navMeshRevision); } std::chrono::steady_clock::time_point AsyncNavMeshUpdater::setFirstStart(const std::chrono::steady_clock::time_point& value) diff --git a/components/detournavigator/asyncnavmeshupdater.hpp b/components/detournavigator/asyncnavmeshupdater.hpp index 98359964d..55e502b45 100644 --- a/components/detournavigator/asyncnavmeshupdater.hpp +++ b/components/detournavigator/asyncnavmeshupdater.hpp @@ -48,7 +48,7 @@ namespace DetourNavigator struct Job { osg::Vec3f mAgentHalfExtents; - SharedNavMeshCacheItem mNavMeshCacheItem; + std::weak_ptr mNavMeshCacheItem; TilePosition mChangedTile; unsigned mTryNumber; ChangeType mChangeType;