Merge branch 'navigator_debug_mesh' into 'master'

Make navigator debug meshes generation safer

See merge request OpenMW/openmw!4602

apps/components_tests/detournavigator/asyncnavmeshupdater.cpp

@@ -5,7 +5,9 @@
 #include <components/detournavigator/makenavmesh.hpp>
 #include <components/detournavigator/navmeshdbutils.hpp>
 #include <components/detournavigator/serialization.hpp>
+#include <components/files/conversion.hpp>
 #include <components/loadinglistener/loadinglistener.hpp>
+#include <components/testing/util.hpp>
 
 #include <BulletCollision/CollisionShapes/btBoxShape.h>
 
@@ -372,6 +374,106 @@ namespace
         }
     }
 
+    TEST_F(DetourNavigatorAsyncNavMeshUpdaterTest, should_write_debug_recast_mesh)
+    {
+        mRecastMeshManager.setWorldspace(mWorldspace, nullptr);
+        addHeightFieldPlane(mRecastMeshManager);
+        mSettings.mEnableWriteRecastMeshToFile = true;
+        const std::filesystem::path dir = TestingOpenMW::outputDirPath("DetourNavigatorAsyncNavMeshUpdaterTest");
+        mSettings.mRecastMeshPathPrefix = Files::pathToUnicodeString(dir) + "/";
+        Log(Debug::Verbose) << mSettings.mRecastMeshPathPrefix;
+        AsyncNavMeshUpdater updater(mSettings, mRecastMeshManager, mOffMeshConnectionsManager, nullptr);
+        const auto navMeshCacheItem = std::make_shared<GuardedNavMeshCacheItem>(1, mSettings);
+        const std::map<TilePosition, ChangeType> changedTiles{ { TilePosition{ 0, 0 }, ChangeType::add } };
+        updater.post(mAgentBounds, navMeshCacheItem, mPlayerTile, mWorldspace, changedTiles);
+        updater.wait(WaitConditionType::allJobsDone, &mListener);
+        EXPECT_TRUE(std::filesystem::exists(dir / "0.0.recastmesh.obj"));
+    }
+
+    TEST_F(DetourNavigatorAsyncNavMeshUpdaterTest, should_write_debug_recast_mesh_with_revision)
+    {
+        mRecastMeshManager.setWorldspace(mWorldspace, nullptr);
+        addHeightFieldPlane(mRecastMeshManager);
+        mSettings.mEnableWriteRecastMeshToFile = true;
+        mSettings.mEnableRecastMeshFileNameRevision = true;
+        const std::filesystem::path dir = TestingOpenMW::outputDirPath("DetourNavigatorAsyncNavMeshUpdaterTest");
+        mSettings.mRecastMeshPathPrefix = Files::pathToUnicodeString(dir) + "/";
+        Log(Debug::Verbose) << mSettings.mRecastMeshPathPrefix;
+        AsyncNavMeshUpdater updater(mSettings, mRecastMeshManager, mOffMeshConnectionsManager, nullptr);
+        const auto navMeshCacheItem = std::make_shared<GuardedNavMeshCacheItem>(1, mSettings);
+        const std::map<TilePosition, ChangeType> changedTiles{ { TilePosition{ 0, 0 }, ChangeType::add } };
+        updater.post(mAgentBounds, navMeshCacheItem, mPlayerTile, mWorldspace, changedTiles);
+        updater.wait(WaitConditionType::allJobsDone, &mListener);
+        EXPECT_TRUE(std::filesystem::exists(dir / "0.0.recastmesh.1.2.obj"));
+    }
+
+    TEST_F(DetourNavigatorAsyncNavMeshUpdaterTest, writing_recast_mesh_to_absent_file_should_not_fail_tile_generation)
+    {
+        mRecastMeshManager.setWorldspace(mWorldspace, nullptr);
+        addHeightFieldPlane(mRecastMeshManager);
+        mSettings.mEnableWriteRecastMeshToFile = true;
+        const std::filesystem::path dir = TestingOpenMW::outputDir() / "absent";
+        mSettings.mRecastMeshPathPrefix = Files::pathToUnicodeString(dir) + "/";
+        Log(Debug::Verbose) << mSettings.mRecastMeshPathPrefix;
+        AsyncNavMeshUpdater updater(mSettings, mRecastMeshManager, mOffMeshConnectionsManager, nullptr);
+        const auto navMeshCacheItem = std::make_shared<GuardedNavMeshCacheItem>(1, mSettings);
+        const std::map<TilePosition, ChangeType> changedTiles{ { TilePosition{ 0, 0 }, ChangeType::add } };
+        updater.post(mAgentBounds, navMeshCacheItem, mPlayerTile, mWorldspace, changedTiles);
+        updater.wait(WaitConditionType::allJobsDone, &mListener);
+        EXPECT_NE(navMeshCacheItem->lockConst()->getImpl().getTileRefAt(0, 0, 0), 0u);
+        EXPECT_FALSE(std::filesystem::exists(dir));
+    }
+
+    TEST_F(DetourNavigatorAsyncNavMeshUpdaterTest, should_write_debug_navmesh)
+    {
+        mRecastMeshManager.setWorldspace(mWorldspace, nullptr);
+        addHeightFieldPlane(mRecastMeshManager);
+        mSettings.mEnableWriteNavMeshToFile = true;
+        const std::filesystem::path dir = TestingOpenMW::outputDirPath("DetourNavigatorAsyncNavMeshUpdaterTest");
+        mSettings.mNavMeshPathPrefix = Files::pathToUnicodeString(dir) + "/";
+        Log(Debug::Verbose) << mSettings.mRecastMeshPathPrefix;
+        AsyncNavMeshUpdater updater(mSettings, mRecastMeshManager, mOffMeshConnectionsManager, nullptr);
+        const auto navMeshCacheItem = std::make_shared<GuardedNavMeshCacheItem>(1, mSettings);
+        const std::map<TilePosition, ChangeType> changedTiles{ { TilePosition{ 0, 0 }, ChangeType::add } };
+        updater.post(mAgentBounds, navMeshCacheItem, mPlayerTile, mWorldspace, changedTiles);
+        updater.wait(WaitConditionType::allJobsDone, &mListener);
+        EXPECT_TRUE(std::filesystem::exists(dir / "all_tiles_navmesh.bin"));
+    }
+
+    TEST_F(DetourNavigatorAsyncNavMeshUpdaterTest, should_write_debug_navmesh_with_revision)
+    {
+        mRecastMeshManager.setWorldspace(mWorldspace, nullptr);
+        addHeightFieldPlane(mRecastMeshManager);
+        mSettings.mEnableWriteNavMeshToFile = true;
+        mSettings.mEnableNavMeshFileNameRevision = true;
+        const std::filesystem::path dir = TestingOpenMW::outputDirPath("DetourNavigatorAsyncNavMeshUpdaterTest");
+        mSettings.mNavMeshPathPrefix = Files::pathToUnicodeString(dir) + "/";
+        Log(Debug::Verbose) << mSettings.mRecastMeshPathPrefix;
+        AsyncNavMeshUpdater updater(mSettings, mRecastMeshManager, mOffMeshConnectionsManager, nullptr);
+        const auto navMeshCacheItem = std::make_shared<GuardedNavMeshCacheItem>(1, mSettings);
+        const std::map<TilePosition, ChangeType> changedTiles{ { TilePosition{ 0, 0 }, ChangeType::add } };
+        updater.post(mAgentBounds, navMeshCacheItem, mPlayerTile, mWorldspace, changedTiles);
+        updater.wait(WaitConditionType::allJobsDone, &mListener);
+        EXPECT_TRUE(std::filesystem::exists(dir / "all_tiles_navmesh.1.1.bin"));
+    }
+
+    TEST_F(DetourNavigatorAsyncNavMeshUpdaterTest, writing_navmesh_to_absent_file_should_not_fail_tile_generation)
+    {
+        mRecastMeshManager.setWorldspace(mWorldspace, nullptr);
+        addHeightFieldPlane(mRecastMeshManager);
+        mSettings.mEnableWriteNavMeshToFile = true;
+        const std::filesystem::path dir = TestingOpenMW::outputDir() / "absent";
+        mSettings.mNavMeshPathPrefix = Files::pathToUnicodeString(dir) + "/";
+        Log(Debug::Verbose) << mSettings.mRecastMeshPathPrefix;
+        AsyncNavMeshUpdater updater(mSettings, mRecastMeshManager, mOffMeshConnectionsManager, nullptr);
+        const auto navMeshCacheItem = std::make_shared<GuardedNavMeshCacheItem>(1, mSettings);
+        const std::map<TilePosition, ChangeType> changedTiles{ { TilePosition{ 0, 0 }, ChangeType::add } };
+        updater.post(mAgentBounds, navMeshCacheItem, mPlayerTile, mWorldspace, changedTiles);
+        updater.wait(WaitConditionType::allJobsDone, &mListener);
+        EXPECT_NE(navMeshCacheItem->lockConst()->getImpl().getTileRefAt(0, 0, 0), 0u);
+        EXPECT_FALSE(std::filesystem::exists(dir));
+    }
+
     struct DetourNavigatorSpatialJobQueueTest : Test
     {
         const AgentBounds mAgentBounds{ CollisionShapeType::Aabb, osg::Vec3f(1, 1, 1) };

apps/components_tests/files/hash.cpp

@@ -30,7 +30,7 @@ namespace
 
     TEST(FilesGetHash, shouldClearErrors)
     {
-        const auto fileName = temporaryFilePath("fileName");
+        const auto fileName = outputFilePath("fileName");
         std::string content;
         std::fill_n(std::back_inserter(content), 1, 'a');
         std::istringstream stream(content);
@@ -41,7 +41,7 @@ namespace
 
     TEST_P(FilesGetHash, shouldReturnHashForStringStream)
     {
-        const auto fileName = temporaryFilePath("fileName");
+        const auto fileName = outputFilePath("fileName");
         std::string content;
         std::fill_n(std::back_inserter(content), GetParam().mSize, 'a');
         std::istringstream stream(content);

apps/components_tests/main.cpp

@@ -2,6 +2,7 @@
 #include <components/misc/strings/conversion.hpp>
 #include <components/settings/parser.hpp>
 #include <components/settings/values.hpp>
+#include <components/testing/util.hpp>
 
 #include <gtest/gtest.h>
 
@@ -24,5 +25,9 @@ int main(int argc, char** argv)
     Settings::StaticValues::init();
 
     testing::InitGoogleTest(&argc, argv);
-    return RUN_ALL_TESTS();
+
+    const int result = RUN_ALL_TESTS();
+    if (result == 0)
+        std::filesystem::remove_all(TestingOpenMW::outputDir());
+    return result;
 }

apps/components_tests/shader/shadermanager.cpp

@@ -16,7 +16,7 @@ namespace
         ShaderManager mManager;
         ShaderManager::DefineMap mDefines;
 
-        ShaderManagerTest() { mManager.setShaderPath("tests_output"); }
+        ShaderManagerTest() { mManager.setShaderPath(TestingOpenMW::outputDir()); }
 
         template <class F>
         void withShaderFile(const std::string& content, F&& f)

apps/opencs_tests/main.cpp

@@ -1,4 +1,5 @@
 #include <components/debug/debugging.hpp>
+#include <components/testing/util.hpp>
 
 #include <gtest/gtest.h>
 
@@ -7,5 +8,9 @@ int main(int argc, char* argv[])
     Log::sMinDebugLevel = Debug::getDebugLevel();
 
     testing::InitGoogleTest(&argc, argv);
-    return RUN_ALL_TESTS();
+
+    const int result = RUN_ALL_TESTS();
+    if (result == 0)
+        std::filesystem::remove_all(TestingOpenMW::outputDir());
+    return result;
 }

apps/openmw_tests/main.cpp

@@ -2,6 +2,7 @@
 #include <components/misc/strings/conversion.hpp>
 #include <components/settings/parser.hpp>
 #include <components/settings/values.hpp>
+#include <components/testing/util.hpp>
 
 #include <gtest/gtest.h>
 
@@ -24,5 +25,9 @@ int main(int argc, char* argv[])
     Settings::StaticValues::init();
 
     testing::InitGoogleTest(&argc, argv);
-    return RUN_ALL_TESTS();
+
+    const int result = RUN_ALL_TESTS();
+    if (result == 0)
+        std::filesystem::remove_all(TestingOpenMW::outputDir());
+    return result;
 }

components/detournavigator/asyncnavmeshupdater.cpp

@@ -453,9 +453,9 @@ namespace DetourNavigator
         Misc::setCurrentThreadIdlePriority();
         while (!mShouldStop)
         {
-            try
+            if (JobIt job = getNextJob(); job != mJobs.end())
             {
-                if (JobIt job = getNextJob(); job != mJobs.end())
+                try
                 {
                     const JobStatus status = processJob(*job);
                     Log(Debug::Debug) << "Processed job " << job->mId << " with status=" << status
@@ -480,12 +480,20 @@ namespace DetourNavigator
                         }
                     }
                 }
-                else
+                catch (const std::exception& e)
-                    cleanupLastUpdates();
+                {
+                    Log(Debug::Warning) << "Failed to process navmesh job " << job->mId
+                                        << " for worldspace=" << job->mWorldspace << " agent=" << job->mAgentBounds
+                                        << " changedTile=(" << job->mChangedTile << ")"
+                                        << " changeType=" << job->mChangeType
+                                        << " by thread=" << std::this_thread::get_id() << ": " << e.what();
+                    unlockTile(job->mId, job->mAgentBounds, job->mChangedTile);
+                    removeJob(job);
+                }
             }
-            catch (const std::exception& e)
+            else
             {
-                Log(Debug::Error) << "AsyncNavMeshUpdater::process exception: " << e.what();
+                cleanupLastUpdates();
             }
         }
         Log(Debug::Debug) << "Stop navigator jobs processing by thread=" << std::this_thread::get_id();
@@ -493,7 +501,8 @@ namespace DetourNavigator
 
     JobStatus AsyncNavMeshUpdater::processJob(Job& job)
     {
-        Log(Debug::Debug) << "Processing job " << job.mId << " for agent=(" << job.mAgentBounds << ")"
+        Log(Debug::Debug) << "Processing job " << job.mId << "  for worldspace=" << job.mWorldspace
+                          << " agent=" << job.mAgentBounds << ""
                           << " changedTile=(" << job.mChangedTile << ")"
                           << " changeType=" << job.mChangeType << " by thread=" << std::this_thread::get_id();
 
@@ -543,7 +552,14 @@ namespace DetourNavigator
             return JobStatus::Done;
         }
 
-        writeDebugRecastMesh(mSettings, job.mChangedTile, *recastMesh);
+        try
+        {
+            writeDebugRecastMesh(mSettings, job.mChangedTile, *recastMesh);
+        }
+        catch (const std::exception& e)
+        {
+            Log(Debug::Warning) << "Failed to write debug recast mesh: " << e.what();
+        }
 
         NavMeshTilesCache::Value cachedNavMeshData
             = mNavMeshTilesCache.get(job.mAgentBounds, job.mChangedTile, *recastMesh);
@@ -666,12 +682,19 @@ namespace DetourNavigator
             mPresentTiles.insert(std::make_tuple(job.mAgentBounds, job.mChangedTile));
         }
 
-        writeDebugNavMesh(mSettings, navMeshCacheItem, navMeshVersion);
+        try
+        {
+            writeDebugNavMesh(mSettings, navMeshCacheItem, navMeshVersion);
+        }
+        catch (const std::exception& e)
+        {
+            Log(Debug::Warning) << "Failed to write debug navmesh: " << e.what();
+        }
 
         return isSuccess(status) ? JobStatus::Done : JobStatus::Fail;
     }
 
-    JobIt AsyncNavMeshUpdater::getNextJob()
+    JobIt AsyncNavMeshUpdater::getNextJob() noexcept
     {
         std::unique_lock<std::mutex> lock(mMutex);
 
@@ -746,7 +769,7 @@ namespace DetourNavigator
         return mJobs.size();
     }
 
-    void AsyncNavMeshUpdater::cleanupLastUpdates()
+    void AsyncNavMeshUpdater::cleanupLastUpdates() noexcept
     {
         const auto now = std::chrono::steady_clock::now();
 

components/detournavigator/asyncnavmeshupdater.hpp

@@ -244,7 +244,7 @@ namespace DetourNavigator
         inline JobStatus handleUpdateNavMeshStatus(UpdateNavMeshStatus status, const Job& job,
             const GuardedNavMeshCacheItem& navMeshCacheItem, const RecastMesh& recastMesh);
 
-        JobIt getNextJob();
+        inline JobIt getNextJob() noexcept;
 
         void postThreadJob(JobIt job, std::deque<JobIt>& queue);
 
@@ -254,7 +254,7 @@ namespace DetourNavigator
 
         inline std::size_t getTotalJobs() const;
 
-        void cleanupLastUpdates();
+        inline void cleanupLastUpdates() noexcept;
 
         inline void waitUntilJobsDoneForNotPresentTiles(Loading::Listener* listener);
 

components/detournavigator/makenavmesh.cpp

@@ -523,7 +523,7 @@ namespace DetourNavigator
     std::unique_ptr<PreparedNavMeshData> prepareNavMeshTileData(const RecastMesh& recastMesh, ESM::RefId worldspace,
         const TilePosition& tilePosition, const AgentBounds& agentBounds, const RecastSettings& settings)
     {
-        RecastContext context(worldspace, tilePosition, agentBounds, settings.mMaxLogLevel);
+        RecastContext context(worldspace, tilePosition, agentBounds, recastMesh.getVersion(), settings.mMaxLogLevel);
 
         const auto [minZ, maxZ] = getBoundsByZ(recastMesh, agentBounds.mHalfExtents.z(), settings);
 

components/detournavigator/recastcontext.cpp

@@ -23,20 +23,20 @@ namespace DetourNavigator
             return Debug::Debug;
         }
 
-        std::string formatPrefix(
+        std::string formatPrefix(ESM::RefId worldspace, const TilePosition& tilePosition,
-            ESM::RefId worldspace, const TilePosition& tilePosition, const AgentBounds& agentBounds)
+            const AgentBounds& agentBounds, const Version& version)
         {
             std::ostringstream stream;
             stream << "Worldspace: " << worldspace << "; tile position: " << tilePosition.x() << ", "
-                   << tilePosition.y() << "; agent bounds: " << agentBounds << "; ";
+                   << tilePosition.y() << "; agent bounds: " << agentBounds << "; version: " << version << "; ";
             return stream.str();
         }
     }
 
     RecastContext::RecastContext(ESM::RefId worldspace, const TilePosition& tilePosition,
-        const AgentBounds& agentBounds, Debug::Level maxLogLevel)
+        const AgentBounds& agentBounds, const Version& version, Debug::Level maxLogLevel)
         : mMaxLogLevel(maxLogLevel)
-        , mPrefix(formatPrefix(worldspace, tilePosition, agentBounds))
+        , mPrefix(formatPrefix(worldspace, tilePosition, agentBounds, version))
     {
     }
 

components/detournavigator/recastcontext.hpp

@@ -13,12 +13,13 @@
 namespace DetourNavigator
 {
     struct AgentBounds;
+    struct Version;
 
     class RecastContext final : public rcContext
     {
     public:
         explicit RecastContext(ESM::RefId worldspace, const TilePosition& tilePosition, const AgentBounds& agentBounds,
-            Debug::Level maxLogLevel);
+            const Version& version, Debug::Level maxLogLevel);
 
         const std::string& getPrefix() const { return mPrefix; }
 

components/testing/util.hpp

@@ -1,6 +1,7 @@
 #ifndef OPENMW_COMPONENTS_TESTING_UTIL_H
 #define OPENMW_COMPONENTS_TESTING_UTIL_H
 
+#include <chrono>
 #include <filesystem>
 #include <initializer_list>
 #include <memory>
@@ -14,26 +15,37 @@
 
 namespace TestingOpenMW
 {
-    inline std::filesystem::path outputFilePath(const std::string name)
+    inline std::filesystem::path outputDir()
     {
-        std::filesystem::path dir("tests_output");
+        static const std::string run
-        std::filesystem::create_directory(dir);
+            = std::to_string(std::chrono::system_clock::to_time_t(std::chrono::system_clock::now()));
+        std::filesystem::path dir = std::filesystem::temp_directory_path() / "openmw" / "tests" / run;
+        std::filesystem::create_directories(dir);
+        return dir;
+    }
+
+    inline std::filesystem::path outputFilePath(std::string_view name)
+    {
+        std::filesystem::path dir = outputDir();
         return dir / Misc::StringUtils::stringToU8String(name);
     }
 
+    inline std::filesystem::path outputDirPath(const std::filesystem::path& subpath)
+    {
+        std::filesystem::path path = outputDir();
+        path /= subpath;
+        std::filesystem::create_directories(path);
+        return path;
+    }
+
     inline std::filesystem::path outputFilePathWithSubDir(const std::filesystem::path& subpath)
     {
-        std::filesystem::path path("tests_output");
+        std::filesystem::path path = outputDir();
         path /= subpath;
         std::filesystem::create_directories(path.parent_path());
         return path;
     }
 
-    inline std::filesystem::path temporaryFilePath(const std::string name)
-    {
-        return std::filesystem::temp_directory_path() / name;
-    }
-
     class VFSTestFile : public VFS::File
     {
     public: