Merge branch 'fix_navmeshtool_avoid_shape' into 'master'

Use different object id for avoid shape

See merge request OpenMW/openmw!3021

apps/bulletobjecttool/main.cpp

@@ -10,6 +10,7 @@
 #include <components/files/collections.hpp>
 #include <components/files/configurationmanager.hpp>
 #include <components/files/multidircollection.hpp>
+#include <components/misc/strings/conversion.hpp>
 #include <components/platform/platform.hpp>
 #include <components/resource/bulletshape.hpp>
 #include <components/resource/bulletshapemanager.hpp>
@@ -112,21 +113,6 @@ namespace
         }
     };
 
-    std::string toHex(std::string_view value)
-    {
-        std::string buffer(value.size() * 2, '0');
-        char* out = buffer.data();
-        for (const char v : value)
-        {
-            const std::ptrdiff_t space = static_cast<std::ptrdiff_t>(static_cast<std::uint8_t>(v) <= 0xf);
-            const auto [ptr, ec] = std::to_chars(out + space, out + space + 2, static_cast<std::uint8_t>(v), 16);
-            if (ec != std::errc())
-                throw std::system_error(std::make_error_code(ec));
-            out += 2;
-        }
-        return buffer;
-    }
-
     int runBulletObjectTool(int argc, char* argv[])
     {
         Platform::init();
@@ -204,7 +190,7 @@ namespace
                 Log(Debug::Verbose) << "Found bullet object in " << (cell.isExterior() ? "exterior" : "interior")
                                     << " cell \"" << cell.getDescription() << "\":"
                                     << " fileName=\"" << object.mShape->mFileName << '"'
-                                    << " fileHash=" << toHex(object.mShape->mFileHash)
+                                    << " fileHash=" << Misc::StringUtils::toHex(object.mShape->mFileHash)
                                     << " collisionShape=" << std::boolalpha
                                     << (object.mShape->mCollisionShape == nullptr)
                                     << " avoidCollisionShape=" << std::boolalpha

apps/navmeshtool/worldspacedata.cpp

@@ -3,11 +3,13 @@
 #include <components/bullethelpers/aabb.hpp>
 #include <components/debug/debugging.hpp>
 #include <components/debug/debuglog.hpp>
+#include <components/detournavigator/debug.hpp>
 #include <components/detournavigator/gettilespositions.hpp>
 #include <components/detournavigator/objectid.hpp>
 #include <components/detournavigator/recastmesh.hpp>
 #include <components/detournavigator/settings.hpp>
 #include <components/detournavigator/tilecachedrecastmeshmanager.hpp>
+#include <components/esm/refid.hpp>
 #include <components/esm3/cellref.hpp>
 #include <components/esm3/esmreader.hpp>
 #include <components/esm3/loadcell.hpp>
@@ -17,6 +19,7 @@
 #include <components/esmloader/lessbyid.hpp>
 #include <components/esmloader/record.hpp>
 #include <components/misc/resourcehelpers.hpp>
+#include <components/misc/strings/conversion.hpp>
 #include <components/misc/strings/lower.hpp>
 #include <components/navmeshtool/protocol.hpp>
 #include <components/resource/bulletshapemanager.hpp>
@@ -29,14 +32,15 @@
 #include <osg/ref_ptr>
 
 #include <algorithm>
-#include <components/esm/refid.hpp>
 #include <memory>
+#include <sstream>
 #include <stdexcept>
 #include <string>
 #include <string_view>
 #include <tuple>
 #include <utility>
 #include <vector>
+
 namespace NavMeshTool
 {
     namespace
@@ -218,6 +222,16 @@ namespace NavMeshTool
             const std::vector<std::byte> data = serialize(value);
             getRawStderr().write(reinterpret_cast<const char*>(data.data()), static_cast<std::streamsize>(data.size()));
         }
+
+        std::string makeAddObjectErrorMessage(
+            ObjectId objectId, DetourNavigator::AreaType areaType, const CollisionShape& shape)
+        {
+            std::ostringstream stream;
+            stream << "Failed to add object to recast mesh objectId=" << objectId.value() << " areaType=" << areaType
+                   << " fileName=" << shape.getInstance()->mFileName
+                   << " fileHash=" << Misc::StringUtils::toHex(shape.getInstance()->mFileHash);
+            return stream.str();
+        }
     }
 
     WorldspaceNavMeshInput::WorldspaceNavMeshInput(
@@ -317,14 +331,19 @@ namespace NavMeshTool
                 const CollisionShape shape(object.getShapeInstance(), *object.getCollisionObject().getCollisionShape(),
                     object.getObjectTransform());
 
-                navMeshInput.mTileCachedRecastMeshManager.addObject(
+                if (!navMeshInput.mTileCachedRecastMeshManager.addObject(
-                    objectId, shape, transform, DetourNavigator::AreaType_ground, guard.get());
+                        objectId, shape, transform, DetourNavigator::AreaType_ground, guard.get()))
+                    throw std::logic_error(
+                        makeAddObjectErrorMessage(objectId, DetourNavigator::AreaType_ground, shape));
 
                 if (const btCollisionShape* avoid = object.getShapeInstance()->mAvoidCollisionShape.get())
                 {
+                    const ObjectId avoidObjectId(++objectsCounter);
                     const CollisionShape avoidShape(object.getShapeInstance(), *avoid, object.getObjectTransform());
-                    navMeshInput.mTileCachedRecastMeshManager.addObject(
+                    if (!navMeshInput.mTileCachedRecastMeshManager.addObject(
-                        objectId, avoidShape, transform, DetourNavigator::AreaType_null, guard.get());
+                            avoidObjectId, avoidShape, transform, DetourNavigator::AreaType_null, guard.get()))
+                        throw std::logic_error(
+                            makeAddObjectErrorMessage(avoidObjectId, DetourNavigator::AreaType_null, avoidShape));
                 }
 
                 data.mObjects.emplace_back(std::move(object));

apps/openmw_test_suite/detournavigator/asyncnavmeshupdater.cpp

@@ -197,7 +197,7 @@ namespace
         ASSERT_NE(recastMesh, nullptr);
         const auto objects = makeDbRefGeometryObjects(
             recastMesh->getMeshSources(), [&](const MeshSource& v) { return resolveMeshSource(*dbPtr, v); });
-        EXPECT_FALSE(objects.has_value());
+        EXPECT_TRUE(std::holds_alternative<MeshSource>(objects));
     }
 
     TEST_F(DetourNavigatorAsyncNavMeshUpdaterTest, post_should_read_from_db_on_cache_miss)
@@ -285,13 +285,14 @@ namespace
                 const TilePosition tilePosition(x, y);
                 const auto recastMesh = mRecastMeshManager.getMesh(mWorldspace, tilePosition);
                 ASSERT_NE(recastMesh, nullptr);
-                const std::optional<std::vector<DbRefGeometryObject>> objects = makeDbRefGeometryObjects(
+                const auto objects = makeDbRefGeometryObjects(
                     recastMesh->getMeshSources(), [&](const MeshSource& v) { return resolveMeshSource(*dbPtr, v); });
-                if (!objects.has_value())
+                if (std::holds_alternative<MeshSource>(objects))
                     continue;
                 EXPECT_EQ(dbPtr
                               ->findTile(mWorldspace, tilePosition,
-                                  serialize(mSettings.mRecast, mAgentBounds, *recastMesh, *objects))
+                                  serialize(mSettings.mRecast, mAgentBounds, *recastMesh,
+                                      std::get<std::vector<DbRefGeometryObject>>(objects)))
                               .has_value(),
                     present.find(tilePosition) != present.end())
                     << tilePosition.x() << " " << tilePosition.y()

components/detournavigator/asyncnavmeshupdater.cpp

@@ -9,6 +9,7 @@
 
 #include <components/debug/debuglog.hpp>
 #include <components/loadinglistener/loadinglistener.hpp>
+#include <components/misc/strings/conversion.hpp>
 #include <components/misc/thread.hpp>
 
 #include <DetourNavMesh.h>
@@ -199,7 +200,8 @@ namespace DetourNavigator
                     changeType, getManhattanDistance(changedTile, playerTile), processTime);
 
                 Log(Debug::Debug) << "Post job " << it->mId << " for agent=(" << it->mAgentBounds << ")"
-                                  << " changedTile=(" << it->mChangedTile << ")";
+                                  << " changedTile=(" << it->mChangedTile << ") "
+                                  << " changeType=" << it->mChangeType;
 
                 if (playerTileChanged)
                     mWaiting.push_back(it);
@@ -834,11 +836,31 @@ namespace DetourNavigator
             }
             else
             {
-                const auto objects = makeDbRefGeometryObjects(job->mRecastMesh->getMeshSources(),
+                struct HandleResult
+                {
+                    const RecastSettings& mRecastSettings;
+                    Job& mJob;
+
+                    bool operator()(const std::vector<DbRefGeometryObject>& objects) const
+                    {
+                        mJob.mInput = serialize(mRecastSettings, mJob.mAgentBounds, *mJob.mRecastMesh, objects);
+                        return true;
+                    }
+
+                    bool operator()(const MeshSource& meshSource) const
+                    {
+                        Log(Debug::Debug) << "No object for mesh source (fileName=\"" << meshSource.mShape->mFileName
+                                          << "\", areaType=" << meshSource.mAreaType
+                                          << ", fileHash=" << Misc::StringUtils::toHex(meshSource.mShape->mFileHash)
+                                          << ") for job " << mJob.mId;
+                        return false;
+                    }
+                };
+
+                const auto result = makeDbRefGeometryObjects(job->mRecastMesh->getMeshSources(),
                     [&](const MeshSource& v) { return resolveMeshSource(*mDb, v); });
-                if (!objects.has_value())
+                if (!std::visit(HandleResult{ mRecastSettings, *job }, result))
                     return;
-                job->mInput = serialize(mRecastSettings, job->mAgentBounds, *job->mRecastMesh, *objects);
             }
         }
 

components/detournavigator/dbrefgeometryobject.hpp

@@ -11,6 +11,7 @@
 #include <optional>
 #include <tuple>
 #include <type_traits>
+#include <variant>
 #include <vector>
 
 namespace DetourNavigator
@@ -32,7 +33,7 @@ namespace DetourNavigator
     inline auto makeDbRefGeometryObjects(
         const std::vector<MeshSource>& meshSources, ResolveMeshSource&& resolveMeshSource)
         -> std::conditional_t<Misc::isOptional<std::decay_t<decltype(resolveMeshSource(meshSources.front()))>>,
-            std::optional<std::vector<DbRefGeometryObject>>, std::vector<DbRefGeometryObject>>
+            std::variant<std::vector<DbRefGeometryObject>, MeshSource>, std::vector<DbRefGeometryObject>>
     {
         std::vector<DbRefGeometryObject> result;
         result.reserve(meshSources.size());
@@ -42,7 +43,7 @@ namespace DetourNavigator
             if constexpr (Misc::isOptional<std::decay_t<decltype(shapeId)>>)
             {
                 if (!shapeId.has_value())
-                    return std::nullopt;
+                    return meshSource;
                 result.push_back(DbRefGeometryObject{ *shapeId, meshSource.mObjectTransform });
             }
             else

components/detournavigator/navmeshdbutils.cpp

@@ -2,7 +2,8 @@
 #include "navmeshdb.hpp"
 #include "recastmesh.hpp"
 
-#include <components/debug/debuglog.hpp>
+#include "components/debug/debuglog.hpp"
+#include "components/misc/strings/conversion.hpp"
 
 #include <cassert>
 #include <optional>
@@ -27,7 +28,8 @@ namespace DetourNavigator
                 return *existingShapeId;
             const ShapeId newShapeId = nextShapeId;
             db.insertShape(newShapeId, name, type, hashData);
-            Log(Debug::Verbose) << "Added " << name << " " << type << " shape to navmeshdb with id " << newShapeId;
+            Log(Debug::Verbose) << "Added " << name << " " << Misc::StringUtils::toHex(hash) << " " << type
+                                << " shape to navmeshdb with id " << newShapeId;
             ++nextShapeId;
             return newShapeId;
         }

components/misc/strings/conversion.hpp

@@ -129,6 +129,21 @@ namespace Misc::StringUtils
         return std::nullopt;
     }
 #endif
+
+    inline std::string toHex(std::string_view value)
+    {
+        std::string buffer(value.size() * 2, '0');
+        char* out = buffer.data();
+        for (const char v : value)
+        {
+            const std::ptrdiff_t space = static_cast<std::ptrdiff_t>(static_cast<std::uint8_t>(v) <= 0xf);
+            const auto [ptr, ec] = std::to_chars(out + space, out + space + 2, static_cast<std::uint8_t>(v), 16);
+            if (ec != std::errc())
+                throw std::system_error(std::make_error_code(ec));
+            out += 2;
+        }
+        return buffer;
+    }
 }
 
 #endif // COMPONENTS_MISC_STRINGS_CONVERSION_H

components/resource/bulletshape.cpp

@@ -71,13 +71,15 @@ namespace Resource
         deleteShape(shape);
     }
 
-    BulletShape::BulletShape(const BulletShape& copy, const osg::CopyOp& copyop)
+    BulletShape::BulletShape(const BulletShape& other, const osg::CopyOp& copyOp)
-        : mCollisionShape(duplicateCollisionShape(copy.mCollisionShape.get()))
+        : Object(other, copyOp)
-        , mAvoidCollisionShape(duplicateCollisionShape(copy.mAvoidCollisionShape.get()))
+        , mCollisionShape(duplicateCollisionShape(other.mCollisionShape.get()))
-        , mCollisionBox(copy.mCollisionBox)
+        , mAvoidCollisionShape(duplicateCollisionShape(other.mAvoidCollisionShape.get()))
-        , mAnimatedShapes(copy.mAnimatedShapes)
+        , mCollisionBox(other.mCollisionBox)
-        , mFileName(copy.mFileName)
+        , mAnimatedShapes(other.mAnimatedShapes)
-        , mFileHash(copy.mFileHash)
+        , mFileName(other.mFileName)
+        , mFileHash(other.mFileHash)
+        , mVisualCollisionType(other.mVisualCollisionType)
     {
     }
 
@@ -94,13 +96,8 @@ namespace Resource
     }
 
     BulletShapeInstance::BulletShapeInstance(osg::ref_ptr<const BulletShape> source)
-        : mSource(std::move(source))
+        : BulletShape(*source)
+        , mSource(std::move(source))
     {
-        mCollisionBox = mSource->mCollisionBox;
-        mAnimatedShapes = mSource->mAnimatedShapes;
-        mVisualCollisionType = mSource->mVisualCollisionType;
-        mCollisionShape = duplicateCollisionShape(mSource->mCollisionShape.get());
-        mAvoidCollisionShape = duplicateCollisionShape(mSource->mAvoidCollisionShape.get());
     }
-
 }

components/resource/bulletshape.hpp

@@ -61,7 +61,7 @@ namespace Resource
         VisualCollisionType mVisualCollisionType = VisualCollisionType::None;
 
         BulletShape() = default;
-        BulletShape(const BulletShape& copy, const osg::CopyOp& copyop);
+        BulletShape(const BulletShape& other, const osg::CopyOp& copyOp = osg::CopyOp());
 
         META_Object(Resource, BulletShape)
 
@@ -76,7 +76,7 @@ namespace Resource
     class BulletShapeInstance : public BulletShape
     {
     public:
-        BulletShapeInstance(osg::ref_ptr<const BulletShape> source);
+        explicit BulletShapeInstance(osg::ref_ptr<const BulletShape> source);
 
         const osg::ref_ptr<const BulletShape>& getSource() const { return mSource; }