Merge branch 'recast_context' into 'master'

Check input and report errors via RecastContext (#7093)

Closes #7093

See merge request OpenMW/openmw!2544

apps/openmw_test_suite/detournavigator/navigator.cpp

@@ -1203,4 +1203,29 @@ namespace
 
         EXPECT_EQ(mNavigator->getNavMesh(mAgentBounds)->lockConst()->getVersion(), version);
     }
+
+    TEST_F(DetourNavigatorNavigatorTest, add_agent_with_zero_coordinate_should_not_have_nav_mesh)
+    {
+        constexpr std::array<float, 5 * 5> heightfieldData{ {
+            0, 0, 0, 0, 0, // row 0
+            0, -25, -25, -25, -25, // row 1
+            0, -25, -100, -100, -100, // row 2
+            0, -25, -100, -100, -100, // row 3
+            0, -25, -100, -100, -100, // row 4
+        } };
+        const HeightfieldSurface surface = makeSquareHeightfieldSurface(heightfieldData);
+        const int cellSize = mHeightfieldTileSize * (surface.mSize - 1);
+
+        const AgentBounds agentBounds{ CollisionShapeType::RotatingBox, { 0, 1, 1 } };
+        mNavigator->addAgent(agentBounds);
+        auto updateGuard = mNavigator->makeUpdateGuard();
+        mNavigator->addHeightfield(mCellPosition, cellSize, surface, updateGuard.get());
+        mNavigator->update(mPlayerPosition, updateGuard.get());
+        updateGuard.reset();
+        mNavigator->wait(WaitConditionType::requiredTilesPresent, &mListener);
+
+        EXPECT_EQ(
+            findPath(*mNavigator, agentBounds, mStepSize, mStart, mEnd, Flag_walk, mAreaCosts, mEndTolerance, mOut),
+            Status::StartPolygonNotFound);
+    }
 }

components/CMakeLists.txt

@@ -315,6 +315,7 @@ add_component_dir(detournavigator
     collisionshapetype
     stats
     commulativeaabb
+    recastcontext
     )
 
 add_component_dir(loadinglistener

components/detournavigator/debug.cpp

@@ -86,7 +86,8 @@ namespace DetourNavigator
 
     std::ostream& operator<<(std::ostream& s, const AgentBounds& v)
     {
-        return s << "AgentBounds {" << v.mShapeType << ", " << v.mHalfExtents << "}";
+        return s << "AgentBounds {" << v.mShapeType << ", {" << v.mHalfExtents.x() << ", " << v.mHalfExtents.y() << ", "
+                 << v.mHalfExtents.z() << "}}";
     }
 
     namespace

components/detournavigator/makenavmesh.cpp

@@ -7,6 +7,7 @@
 #include "navmeshtilescache.hpp"
 #include "offmeshconnection.hpp"
 #include "preparednavmeshdata.hpp"
+#include "recastcontext.hpp"
 #include "recastmesh.hpp"
 #include "recastmeshbuilder.hpp"
 #include "recastparams.hpp"
@@ -14,6 +15,8 @@
 #include "settingsutils.hpp"
 #include "sharednavmesh.hpp"
 
+#include "components/debug/debuglog.hpp"
+
 #include <DetourNavMesh.h>
 #include <DetourNavMeshBuilder.h>
 #include <Recast.h>
@@ -138,8 +141,8 @@ namespace DetourNavigator
             return result;
         }
 
-        void initHeightfield(rcContext& context, const TilePosition& tilePosition, float minZ, float maxZ,
+        [[nodiscard]] bool initHeightfield(RecastContext& context, const TilePosition& tilePosition, float minZ,
-            const RecastSettings& settings, rcHeightfield& solid)
+            float maxZ, const RecastSettings& settings, rcHeightfield& solid)
         {
             const int size = settings.mTileSize + settings.mBorderSize * 2;
             const int width = size;
@@ -150,14 +153,37 @@ namespace DetourNavigator
             const osg::Vec3f bmin(shift.x() - halfBoundsSize, minZ, shift.y() - halfBoundsSize);
             const osg::Vec3f bmax(shift.x() + halfBoundsSize, maxZ, shift.y() + halfBoundsSize);
 
-            const auto result = rcCreateHeightfield(
+            if (width < 0)
-                &context, solid, width, height, bmin.ptr(), bmax.ptr(), settings.mCellSize, settings.mCellHeight);
+            {
+                Log(Debug::Warning) << context.getPrefix() << "Invalid width to init heightfield: " << width;
+                return false;
+            }
 
-            if (!result)
+            if (height < 0)
-                throw NavigatorException("Failed to create heightfield for navmesh");
+            {
+                Log(Debug::Warning) << context.getPrefix() << "Invalid height to init heightfield: " << height;
+                return false;
+            }
+
+            if (settings.mCellHeight <= 0)
+            {
+                Log(Debug::Warning) << context.getPrefix()
+                                    << "Invalid cell height to init heightfield: " << settings.mCellHeight;
+                return false;
+            }
+
+            if (settings.mCellSize <= 0)
+            {
+                Log(Debug::Warning) << context.getPrefix()
+                                    << "Invalid cell size to init heightfield: " << settings.mCellSize;
+                return false;
+            }
+
+            return rcCreateHeightfield(
+                &context, solid, width, height, bmin.ptr(), bmax.ptr(), settings.mCellSize, settings.mCellHeight);
         }
 
-        bool rasterizeTriangles(rcContext& context, const Mesh& mesh, const RecastSettings& settings,
+        [[nodiscard]] bool rasterizeTriangles(RecastContext& context, const Mesh& mesh, const RecastSettings& settings,
             const RecastParams& params, rcHeightfield& solid)
         {
             std::vector<unsigned char> areas(mesh.getAreaTypes().begin(), mesh.getAreaTypes().end());
@@ -178,7 +204,7 @@ namespace DetourNavigator
                 mesh.getIndices().data(), areas.data(), static_cast<int>(areas.size()), solid, params.mWalkableClimb);
         }
 
-        bool rasterizeTriangles(rcContext& context, const Rectangle& rectangle, AreaType areaType,
+        [[nodiscard]] bool rasterizeTriangles(RecastContext& context, const Rectangle& rectangle, AreaType areaType,
             const RecastParams& params, rcHeightfield& solid)
         {
             const std::array vertices{
@@ -199,9 +225,9 @@ namespace DetourNavigator
                 indices.data(), areas.data(), static_cast<int>(areas.size()), solid, params.mWalkableClimb);
         }
 
-        bool rasterizeTriangles(rcContext& context, float agentHalfExtentsZ, const std::vector<CellWater>& water,
+        [[nodiscard]] bool rasterizeTriangles(RecastContext& context, float agentHalfExtentsZ,
-            const RecastSettings& settings, const RecastParams& params, const TileBounds& realTileBounds,
+            const std::vector<CellWater>& water, const RecastSettings& settings, const RecastParams& params,
-            rcHeightfield& solid)
+            const TileBounds& realTileBounds, rcHeightfield& solid)
         {
             for (const CellWater& cellWater : water)
             {
@@ -219,7 +245,7 @@ namespace DetourNavigator
             return true;
         }
 
-        bool rasterizeTriangles(rcContext& context, const TileBounds& realTileBounds,
+        [[nodiscard]] bool rasterizeTriangles(RecastContext& context, const TileBounds& realTileBounds,
             const std::vector<FlatHeightfield>& heightfields, const RecastSettings& settings,
             const RecastParams& params, rcHeightfield& solid)
         {
@@ -237,7 +263,7 @@ namespace DetourNavigator
             return true;
         }
 
-        bool rasterizeTriangles(rcContext& context, const std::vector<Heightfield>& heightfields,
+        [[nodiscard]] bool rasterizeTriangles(RecastContext& context, const std::vector<Heightfield>& heightfields,
             const RecastSettings& settings, const RecastParams& params, rcHeightfield& solid)
         {
             for (const Heightfield& heightfield : heightfields)
@@ -249,9 +275,9 @@ namespace DetourNavigator
             return true;
         }
 
-        bool rasterizeTriangles(rcContext& context, const TilePosition& tilePosition, float agentHalfExtentsZ,
+        [[nodiscard]] bool rasterizeTriangles(RecastContext& context, const TilePosition& tilePosition,
-            const RecastMesh& recastMesh, const RecastSettings& settings, const RecastParams& params,
+            float agentHalfExtentsZ, const RecastMesh& recastMesh, const RecastSettings& settings,
-            rcHeightfield& solid)
+            const RecastParams& params, rcHeightfield& solid)
         {
             const TileBounds realTileBounds = makeRealTileBoundsWithBorder(settings, tilePosition);
             return rasterizeTriangles(context, recastMesh.getMesh(), settings, params, solid)
@@ -262,66 +288,119 @@ namespace DetourNavigator
                     context, realTileBounds, recastMesh.getFlatHeightfields(), settings, params, solid);
         }
 
-        void buildCompactHeightfield(rcContext& context, const int walkableHeight, const int walkableClimb,
+        [[nodiscard]] bool buildCompactHeightfield(RecastContext& context, const int walkableHeight,
-            rcHeightfield& solid, rcCompactHeightfield& compact)
+            const int walkableClimb, rcHeightfield& solid, rcCompactHeightfield& compact)
         {
-            const auto result = rcBuildCompactHeightfield(&context, walkableHeight, walkableClimb, solid, compact);
+            if (walkableHeight < 3)
+            {
+                Log(Debug::Warning) << context.getPrefix()
+                                    << "Invalid walkableHeight to build compact heightfield: " << walkableHeight;
+                return false;
+            }
 
-            if (!result)
+            if (walkableClimb < 0)
-                throw NavigatorException("Failed to build compact heightfield for navmesh");
+            {
+                Log(Debug::Warning) << context.getPrefix()
+                                    << "Invalid walkableClimb to build compact heightfield: " << walkableClimb;
+                return false;
+            }
+
+            return rcBuildCompactHeightfield(&context, walkableHeight, walkableClimb, solid, compact);
         }
 
-        void erodeWalkableArea(rcContext& context, int walkableRadius, rcCompactHeightfield& compact)
+        [[nodiscard]] bool erodeWalkableArea(RecastContext& context, int walkableRadius, rcCompactHeightfield& compact)
         {
-            const auto result = rcErodeWalkableArea(&context, walkableRadius, compact);
+            if (walkableRadius <= 0 || 255 <= walkableRadius)
+            {
+                Log(Debug::Warning) << context.getPrefix()
+                                    << "Invalid walkableRadius to erode walkable area: " << walkableRadius;
+                return false;
+            }
 
-            if (!result)
+            return rcErodeWalkableArea(&context, walkableRadius, compact);
-                throw NavigatorException("Failed to erode walkable area for navmesh");
         }
 
-        void buildDistanceField(rcContext& context, rcCompactHeightfield& compact)
+        [[nodiscard]] bool buildDistanceField(RecastContext& context, rcCompactHeightfield& compact)
         {
-            const auto result = rcBuildDistanceField(&context, compact);
+            return rcBuildDistanceField(&context, compact);
-
-            if (!result)
-                throw NavigatorException("Failed to build distance field for navmesh");
         }
 
-        void buildRegions(rcContext& context, rcCompactHeightfield& compact, const int borderSize,
+        [[nodiscard]] bool buildRegions(RecastContext& context, rcCompactHeightfield& compact, const int borderSize,
             const int minRegionArea, const int mergeRegionArea)
         {
-            const auto result = rcBuildRegions(&context, compact, borderSize, minRegionArea, mergeRegionArea);
+            if (borderSize < 0)
+            {
+                Log(Debug::Warning) << context.getPrefix() << "Invalid borderSize to build regions: " << borderSize;
+                return false;
+            }
 
-            if (!result)
+            if (minRegionArea < 0)
-                throw NavigatorException("Failed to build distance field for navmesh");
+            {
+                Log(Debug::Warning) << context.getPrefix()
+                                    << "Invalid minRegionArea to build regions: " << minRegionArea;
+                return false;
+            }
+
+            if (mergeRegionArea < 0)
+            {
+                Log(Debug::Warning) << context.getPrefix()
+                                    << "Invalid mergeRegionArea to build regions: " << mergeRegionArea;
+                return false;
+            }
+
+            return rcBuildRegions(&context, compact, borderSize, minRegionArea, mergeRegionArea);
         }
 
-        void buildContours(rcContext& context, rcCompactHeightfield& compact, const float maxError,
+        [[nodiscard]] bool buildContours(RecastContext& context, rcCompactHeightfield& compact, const float maxError,
             const int maxEdgeLen, rcContourSet& contourSet, const int buildFlags = RC_CONTOUR_TESS_WALL_EDGES)
         {
-            const auto result = rcBuildContours(&context, compact, maxError, maxEdgeLen, contourSet, buildFlags);
+            if (maxError < 0)
+            {
+                Log(Debug::Warning) << context.getPrefix() << "Invalid maxError to build contours: " << maxError;
+                return false;
+            }
 
-            if (!result)
+            if (maxEdgeLen < 0)
-                throw NavigatorException("Failed to build contours for navmesh");
+            {
+                Log(Debug::Warning) << context.getPrefix() << "Invalid maxEdgeLen to build contours: " << maxEdgeLen;
+                return false;
+            }
+
+            return rcBuildContours(&context, compact, maxError, maxEdgeLen, contourSet, buildFlags);
         }
 
-        void buildPolyMesh(
+        [[nodiscard]] bool buildPolyMesh(
-            rcContext& context, rcContourSet& contourSet, const int maxVertsPerPoly, rcPolyMesh& polyMesh)
+            RecastContext& context, rcContourSet& contourSet, const int maxVertsPerPoly, rcPolyMesh& polyMesh)
         {
-            const auto result = rcBuildPolyMesh(&context, contourSet, maxVertsPerPoly, polyMesh);
+            if (maxVertsPerPoly < 3)
+            {
+                Log(Debug::Warning) << context.getPrefix()
+                                    << "Invalid maxVertsPerPoly to build poly mesh: " << maxVertsPerPoly;
+                return false;
+            }
 
-            if (!result)
+            return rcBuildPolyMesh(&context, contourSet, maxVertsPerPoly, polyMesh);
-                throw NavigatorException("Failed to build poly mesh for navmesh");
         }
 
-        void buildPolyMeshDetail(rcContext& context, const rcPolyMesh& polyMesh, const rcCompactHeightfield& compact,
+        [[nodiscard]] bool buildPolyMeshDetail(RecastContext& context, const rcPolyMesh& polyMesh,
-            const float sampleDist, const float sampleMaxError, rcPolyMeshDetail& polyMeshDetail)
+            const rcCompactHeightfield& compact, const float sampleDist, const float sampleMaxError,
+            rcPolyMeshDetail& polyMeshDetail)
         {
-            const auto result
+            if (sampleDist < 0)
-                = rcBuildPolyMeshDetail(&context, polyMesh, compact, sampleDist, sampleMaxError, polyMeshDetail);
+            {
+                Log(Debug::Warning) << context.getPrefix()
+                                    << "Invalid sampleDist to build poly mesh detail: " << sampleDist;
+                return false;
+            }
 
-            if (!result)
+            if (sampleMaxError < 0)
-                throw NavigatorException("Failed to build detail poly mesh for navmesh");
+            {
+                Log(Debug::Warning) << context.getPrefix()
+                                    << "Invalid sampleMaxError to build poly mesh detail: " << sampleMaxError;
+                return false;
+            }
+
+            return rcBuildPolyMeshDetail(&context, polyMesh, compact, sampleDist, sampleMaxError, polyMeshDetail);
         }
 
         void setPolyMeshFlags(rcPolyMesh& polyMesh)
@@ -330,25 +409,36 @@ namespace DetourNavigator
                 polyMesh.flags[i] = getFlag(static_cast<AreaType>(polyMesh.areas[i]));
         }
 
-        bool fillPolyMesh(rcContext& context, const RecastSettings& settings, const RecastParams& params,
+        [[nodiscard]] bool fillPolyMesh(RecastContext& context, const RecastSettings& settings,
-            rcHeightfield& solid, rcPolyMesh& polyMesh, rcPolyMeshDetail& polyMeshDetail)
+            const RecastParams& params, rcHeightfield& solid, rcPolyMesh& polyMesh, rcPolyMeshDetail& polyMeshDetail)
         {
             rcCompactHeightfield compact;
-            buildCompactHeightfield(context, params.mWalkableHeight, params.mWalkableClimb, solid, compact);
+            if (!buildCompactHeightfield(context, params.mWalkableHeight, params.mWalkableClimb, solid, compact))
+                return false;
 
-            erodeWalkableArea(context, params.mWalkableRadius, compact);
+            if (!erodeWalkableArea(context, params.mWalkableRadius, compact))
-            buildDistanceField(context, compact);
+                return false;
-            buildRegions(context, compact, settings.mBorderSize, settings.mRegionMinArea, settings.mRegionMergeArea);
+
+            if (!buildDistanceField(context, compact))
+                return false;
+
+            if (!buildRegions(
+                    context, compact, settings.mBorderSize, settings.mRegionMinArea, settings.mRegionMergeArea))
+                return false;
 
             rcContourSet contourSet;
-            buildContours(context, compact, settings.mMaxSimplificationError, params.mMaxEdgeLen, contourSet);
+            if (!buildContours(context, compact, settings.mMaxSimplificationError, params.mMaxEdgeLen, contourSet))
+                return false;
 
             if (contourSet.nconts == 0)
                 return false;
 
-            buildPolyMesh(context, contourSet, settings.mMaxVertsPerPoly, polyMesh);
+            if (!buildPolyMesh(context, contourSet, settings.mMaxVertsPerPoly, polyMesh))
+                return false;
 
-            buildPolyMeshDetail(context, polyMesh, compact, params.mSampleDist, params.mSampleMaxError, polyMeshDetail);
+            if (!buildPolyMeshDetail(
+                    context, polyMesh, compact, params.mSampleDist, params.mSampleMaxError, polyMeshDetail))
+                return false;
 
             setPolyMeshFlags(polyMesh);
 
@@ -410,13 +500,14 @@ namespace DetourNavigator
     std::unique_ptr<PreparedNavMeshData> prepareNavMeshTileData(const RecastMesh& recastMesh,
         const TilePosition& tilePosition, const AgentBounds& agentBounds, const RecastSettings& settings)
     {
-        rcContext context;
+        RecastContext context(tilePosition, agentBounds);
 
         const auto [minZ, maxZ] = getBoundsByZ(recastMesh, agentBounds.mHalfExtents.z(), settings);
 
         rcHeightfield solid;
-        initHeightfield(context, tilePosition, toNavMeshCoordinates(settings, minZ),
+        if (!initHeightfield(context, tilePosition, toNavMeshCoordinates(settings, minZ),
-            toNavMeshCoordinates(settings, maxZ), settings, solid);
+                toNavMeshCoordinates(settings, maxZ), settings, solid))
+            return nullptr;
 
         const RecastParams params = makeRecastParams(settings, agentBounds);
 

components/detournavigator/recastcontext.cpp

@@ -0,0 +1,45 @@
+#include "recastcontext.hpp"
+#include "debug.hpp"
+
+#include "components/debug/debuglog.hpp"
+
+#include <sstream>
+
+namespace DetourNavigator
+{
+    namespace
+    {
+        Debug::Level getLogLevel(rcLogCategory category)
+        {
+            switch (category)
+            {
+                case RC_LOG_PROGRESS:
+                    return Debug::Verbose;
+                case RC_LOG_WARNING:
+                    return Debug::Warning;
+                case RC_LOG_ERROR:
+                    return Debug::Error;
+            }
+            return Debug::Debug;
+        }
+
+        std::string formatPrefix(const TilePosition& tilePosition, const AgentBounds& agentBounds)
+        {
+            std::ostringstream stream;
+            stream << "Tile position: " << tilePosition.x() << ", " << tilePosition.y()
+                   << "; agent bounds: " << agentBounds << "; ";
+            return stream.str();
+        }
+    }
+
+    RecastContext::RecastContext(const TilePosition& tilePosition, const AgentBounds& agentBounds)
+        : mPrefix(formatPrefix(tilePosition, agentBounds))
+    {
+    }
+
+    void RecastContext::doLog(const rcLogCategory category, const char* msg, const int len)
+    {
+        if (len > 0)
+            Log(getLogLevel(category)) << mPrefix << std::string_view(msg, static_cast<std::size_t>(len));
+    }
+}

components/detournavigator/recastcontext.hpp

@@ -0,0 +1,28 @@
+#ifndef OPENMW_COMPONENTS_DETOURNAVIGATOR_RECASTCONTEXT_H
+#define OPENMW_COMPONENTS_DETOURNAVIGATOR_RECASTCONTEXT_H
+
+#include "tileposition.hpp"
+
+#include <string>
+
+#include <Recast.h>
+
+namespace DetourNavigator
+{
+    struct AgentBounds;
+
+    class RecastContext final : public rcContext
+    {
+    public:
+        explicit RecastContext(const TilePosition& tilePosition, const AgentBounds& agentBounds);
+
+        const std::string& getPrefix() const { return mPrefix; }
+
+    private:
+        std::string mPrefix;
+
+        void doLog(rcLogCategory category, const char* msg, int len) override;
+    };
+}
+
+#endif