Refactor NiGeometry/BSTriShape

Don't pass invalid geometry data links to the loaders

apps/openmw_test_suite/nif/node.hpp

@@ -33,8 +33,8 @@ namespace Nif::Testing
     inline void init(NiGeometry& value)
     {
         init(static_cast<NiAVObject&>(value));
-        value.data = NiGeometryDataPtr(nullptr);
+        value.mData = NiGeometryDataPtr(nullptr);
-        value.skin = NiSkinInstancePtr(nullptr);
+        value.mSkin = NiSkinInstancePtr(nullptr);
     }
 
     inline void init(NiTriShape& value)

apps/openmw_test_suite/nifloader/testbulletnifloader.cpp

@@ -326,20 +326,20 @@ namespace
             mNiTriShapeData.mVertices = { osg::Vec3f(0, 0, 0), osg::Vec3f(1, 0, 0), osg::Vec3f(1, 1, 0) };
             mNiTriShapeData.mNumTriangles = 1;
             mNiTriShapeData.mTriangles = { 0, 1, 2 };
-            mNiTriShape.data = Nif::NiGeometryDataPtr(&mNiTriShapeData);
+            mNiTriShape.mData = Nif::NiGeometryDataPtr(&mNiTriShapeData);
 
             mNiTriShapeData2.recType = Nif::RC_NiTriShapeData;
             mNiTriShapeData2.mVertices = { osg::Vec3f(0, 0, 1), osg::Vec3f(1, 0, 1), osg::Vec3f(1, 1, 1) };
             mNiTriShapeData2.mNumTriangles = 1;
             mNiTriShapeData2.mTriangles = { 0, 1, 2 };
-            mNiTriShape2.data = Nif::NiGeometryDataPtr(&mNiTriShapeData2);
+            mNiTriShape2.mData = Nif::NiGeometryDataPtr(&mNiTriShapeData2);
 
             mNiTriStripsData.recType = Nif::RC_NiTriStripsData;
             mNiTriStripsData.mVertices
                 = { osg::Vec3f(0, 0, 0), osg::Vec3f(1, 0, 0), osg::Vec3f(1, 1, 0), osg::Vec3f(0, 1, 0) };
             mNiTriStripsData.mNumTriangles = 2;
             mNiTriStripsData.mStrips = { { 0, 1, 2, 3 } };
-            mNiTriStrips.data = Nif::NiGeometryDataPtr(&mNiTriStripsData);
+            mNiTriStrips.mData = Nif::NiGeometryDataPtr(&mNiTriStripsData);
         }
     };
 
@@ -703,7 +703,7 @@ namespace
     TEST_F(TestBulletNifLoader,
         for_tri_shape_child_node_and_filename_starting_with_x_and_not_empty_skin_should_return_static_shape)
     {
-        mNiTriShape.skin = Nif::NiSkinInstancePtr(&mNiSkinInstance);
+        mNiTriShape.mSkin = Nif::NiSkinInstancePtr(&mNiSkinInstance);
         mNiTriShape.mParents.push_back(&mNiNode);
         mNiNode.mChildren = Nif::NiAVObjectList{ Nif::NiAVObjectPtr(&mNiTriShape) };
 
@@ -943,7 +943,7 @@ namespace
 
     TEST_F(TestBulletNifLoader, for_tri_shape_child_node_with_empty_data_should_return_shape_with_null_collision_shape)
     {
-        mNiTriShape.data = Nif::NiGeometryDataPtr(nullptr);
+        mNiTriShape.mData = Nif::NiGeometryDataPtr(nullptr);
         mNiTriShape.mParents.push_back(&mNiNode);
         mNiNode.mChildren = Nif::NiAVObjectList{ Nif::NiAVObjectPtr(&mNiTriShape) };
 
@@ -961,7 +961,7 @@ namespace
     TEST_F(TestBulletNifLoader,
         for_tri_shape_child_node_with_empty_data_triangles_should_return_shape_with_null_collision_shape)
     {
-        auto data = static_cast<Nif::NiTriShapeData*>(mNiTriShape.data.getPtr());
+        auto data = static_cast<Nif::NiTriShapeData*>(mNiTriShape.mData.getPtr());
         data->mTriangles.clear();
         mNiTriShape.mParents.push_back(&mNiNode);
         mNiNode.mChildren = Nif::NiAVObjectList{ Nif::NiAVObjectPtr(&mNiTriShape) };
@@ -1095,7 +1095,7 @@ namespace
         init(niTriShape);
         init(emptyCollisionNode);
 
-        niTriShape.data = Nif::NiGeometryDataPtr(&mNiTriShapeData);
+        niTriShape.mData = Nif::NiGeometryDataPtr(&mNiTriShapeData);
         niTriShape.mParents.push_back(&mNiNode);
 
         emptyCollisionNode.recType = Nif::RC_RootCollisionNode;
@@ -1192,21 +1192,6 @@ namespace
         EXPECT_EQ(*result, expected);
     }
 
-    TEST_F(TestBulletNifLoader, should_ignore_tri_shape_data_with_mismatching_data_rec_type)
-    {
-        mNiTriShape.data = Nif::NiGeometryDataPtr(&mNiTriStripsData);
-
-        Nif::NIFFile file("test.nif");
-        file.mRoots.push_back(&mNiTriShape);
-        file.mHash = mHash;
-
-        const auto result = mLoader.load(file);
-
-        const Resource::BulletShape expected;
-
-        EXPECT_EQ(*result, expected);
-    }
-
     TEST_F(TestBulletNifLoader, for_tri_strips_root_node_should_return_static_shape)
     {
         Nif::NIFFile file("test.nif");
@@ -1227,21 +1212,6 @@ namespace
         EXPECT_EQ(*result, expected);
     }
 
-    TEST_F(TestBulletNifLoader, should_ignore_tri_strips_data_with_mismatching_data_rec_type)
-    {
-        mNiTriStrips.data = Nif::NiGeometryDataPtr(&mNiTriShapeData);
-
-        Nif::NIFFile file("test.nif");
-        file.mRoots.push_back(&mNiTriStrips);
-        file.mHash = mHash;
-
-        const auto result = mLoader.load(file);
-
-        const Resource::BulletShape expected;
-
-        EXPECT_EQ(*result, expected);
-    }
-
     TEST_F(TestBulletNifLoader, should_ignore_tri_strips_data_with_empty_strips)
     {
         mNiTriStripsData.mStrips.clear();

components/nif/node.cpp

@@ -167,14 +167,14 @@ namespace Nif
     {
         NiAVObject::read(nif);
 
-        data.read(nif);
+        mData.read(nif);
-        skin.read(nif);
+        mSkin.read(nif);
         mMaterial.read(nif);
         if (nif->getVersion() == NIFFile::NIFVersion::VER_BGS
             && nif->getBethVersion() > NIFFile::BethVersion::BETHVER_FO3)
         {
-            shaderprop.read(nif);
+            mShaderProperty.read(nif);
-            alphaprop.read(nif);
+            mAlphaProperty.read(nif);
         }
     }
 
@@ -182,12 +182,38 @@ namespace Nif
     {
         NiAVObject::post(nif);
 
-        data.post(nif);
+        mData.post(nif);
-        skin.post(nif);
+        mSkin.post(nif);
-        shaderprop.post(nif);
+        mShaderProperty.post(nif);
-        alphaprop.post(nif);
+        mAlphaProperty.post(nif);
-        if (recType != RC_NiParticles && !skin.empty())
+        if (recType != RC_NiParticles && !mSkin.empty())
             nif.setUseSkinning(true);
+
+        if (!mData.empty())
+        {
+            switch (recType)
+            {
+                case RC_NiTriShape:
+                case RC_BSLODTriShape:
+                    if (mData->recType != RC_NiTriShapeData)
+                        mData = NiGeometryDataPtr(nullptr);
+                    break;
+                case RC_NiTriStrips:
+                    if (mData->recType != RC_NiTriStripsData)
+                        mData = NiGeometryDataPtr(nullptr);
+                    break;
+                case RC_NiParticles:
+                    if (mData->recType != RC_NiParticlesData)
+                        mData = NiGeometryDataPtr(nullptr);
+                    break;
+                case RC_NiLines:
+                    if (mData->recType != RC_NiLinesData)
+                        mData = NiGeometryDataPtr(nullptr);
+                    break;
+                default:
+                    break;
+            }
+        }
     }
 
     void BSLODTriShape::read(NIFStream* nif)

components/nif/node.hpp

@@ -119,7 +119,14 @@ namespace Nif
         void post(Reader& nif) override;
     };
 
-    struct NiGeometry : NiAVObject
+    struct GeometryInterface
+    {
+        NiSkinInstancePtr mSkin;
+        BSShaderPropertyPtr mShaderProperty;
+        NiAlphaPropertyPtr mAlphaProperty;
+    };
+
+    struct NiGeometry : NiAVObject, GeometryInterface
     {
         /* Possible flags:
             0x40 - mesh has no vertex normals ?
@@ -138,17 +145,13 @@ namespace Nif
             void read(NIFStream* nif);
         };
 
-        NiGeometryDataPtr data;
+        NiGeometryDataPtr mData;
-        NiSkinInstancePtr skin;
         MaterialData mMaterial;
-        BSShaderPropertyPtr shaderprop;
-        NiAlphaPropertyPtr alphaprop;
 
         void read(NIFStream* nif) override;
         void post(Reader& nif) override;
     };
 
-    // TODO: consider checking the data record type here
     struct NiTriShape : NiGeometry
     {
     };
@@ -337,13 +340,10 @@ namespace Nif
         void read(NIFStream* nif, uint16_t flags);
     };
 
-    struct BSTriShape : NiAVObject
+    struct BSTriShape : NiAVObject, GeometryInterface
     {
         osg::BoundingSpheref mBoundingSphere;
         std::array<float, 6> mBoundMinMax;
-        NiSkinInstancePtr mSkin;
-        BSShaderPropertyPtr mShaderProperty;
-        NiAlphaPropertyPtr mAlphaProperty;
         BSVertexDesc mVertDesc;
         uint32_t mDataSize;
         std::vector<BSVertexData> mVertData;

components/nifbullet/bulletnifloader.cpp

@@ -79,14 +79,11 @@ namespace
 
     template <class Function>
     auto handleNiGeometry(const Nif::NiGeometry& geometry, Function&& function)
-        -> decltype(function(static_cast<const Nif::NiTriShapeData&>(geometry.data.get())))
+        -> decltype(function(static_cast<const Nif::NiTriShapeData&>(geometry.mData.get())))
     {
         if (geometry.recType == Nif::RC_NiTriShape || geometry.recType == Nif::RC_BSLODTriShape)
         {
-            if (geometry.data->recType != Nif::RC_NiTriShapeData)
+            auto data = static_cast<const Nif::NiTriShapeData*>(geometry.mData.getPtr());
-                return {};
-
-            auto data = static_cast<const Nif::NiTriShapeData*>(geometry.data.getPtr());
             if (data->mTriangles.empty())
                 return {};
 
@@ -95,10 +92,7 @@ namespace
 
         if (geometry.recType == Nif::RC_NiTriStrips)
         {
-            if (geometry.data->recType != Nif::RC_NiTriStripsData)
+            auto data = static_cast<const Nif::NiTriStripsData*>(geometry.mData.getPtr());
-                return {};
-
-            auto data = static_cast<const Nif::NiTriStripsData*>(geometry.data.getPtr());
             if (data->mStrips.empty())
                 return {};
 
@@ -366,10 +360,10 @@ namespace NifBullet
         if (args.mHasMarkers && Misc::StringUtils::ciStartsWith(niGeometry.mName, "EditorMarker"))
             return;
 
-        if (niGeometry.data.empty() || niGeometry.data->mVertices.empty())
+        if (niGeometry.mData.empty() || niGeometry.mData->mVertices.empty())
             return;
 
-        if (!niGeometry.skin.empty())
+        if (!niGeometry.mSkin.empty())
             args.mAnimated = false;
         // TODO: handle NiSkinPartition
 

components/nifosg/nifloader.cpp

@@ -128,10 +128,10 @@ namespace
         auto geometry = dynamic_cast<const Nif::NiGeometry*>(nifNode);
         if (geometry)
         {
-            if (!geometry->shaderprop.empty())
+            if (!geometry->mShaderProperty.empty())
-                out.emplace_back(geometry->shaderprop.getPtr());
+                out.emplace_back(geometry->mShaderProperty.getPtr());
-            if (!geometry->alphaprop.empty())
+            if (!geometry->mAlphaProperty.empty())
-                out.emplace_back(geometry->alphaprop.getPtr());
+                out.emplace_back(geometry->mAlphaProperty.getPtr());
         }
     }
 
@@ -444,8 +444,8 @@ namespace NifOsg
 
             auto geometry = dynamic_cast<const Nif::NiGeometry*>(nifNode);
             // NiGeometry's NiAlphaProperty doesn't get handled here because it's a drawable property
-            if (geometry && !geometry->shaderprop.empty())
+            if (geometry && !geometry->mShaderProperty.empty())
-                handleProperty(geometry->shaderprop.getPtr(), applyTo, composite, imageManager, boundTextures,
+                handleProperty(geometry->mShaderProperty.getPtr(), applyTo, composite, imageManager, boundTextures,
                     animflags, hasStencilProperty);
         }
 
@@ -761,7 +761,7 @@ namespace NifOsg
                     skip = args.mHasMarkers && Misc::StringUtils::ciStartsWith(nifNode->mName, "EditorMarker");
                 if (!skip)
                 {
-                    Nif::NiSkinInstancePtr skin = static_cast<const Nif::NiGeometry*>(nifNode)->skin;
+                    Nif::NiSkinInstancePtr skin = static_cast<const Nif::NiGeometry*>(nifNode)->mSkin;
 
                     if (skin.empty())
                         handleGeometry(nifNode, parent, node, composite, args.mBoundTextures, args.mAnimFlags);
@@ -1121,13 +1121,13 @@ namespace NifOsg
             const Nif::NiAVObject* nifNode, ParticleSystem* partsys, const Nif::NiParticleSystemController* partctrl)
         {
             auto particleNode = static_cast<const Nif::NiParticles*>(nifNode);
-            if (particleNode->data.empty() || particleNode->data->recType != Nif::RC_NiParticlesData)
+            if (particleNode->mData.empty())
             {
                 partsys->setQuota(partctrl->mParticles.size());
                 return;
             }
 
-            auto particledata = static_cast<const Nif::NiParticlesData*>(particleNode->data.getPtr());
+            auto particledata = static_cast<const Nif::NiParticlesData*>(particleNode->mData.getPtr());
             partsys->setQuota(particledata->mNumParticles);
 
             osg::BoundingBox box;
@@ -1379,13 +1379,13 @@ namespace NifOsg
             const std::vector<unsigned int>& boundTextures, int animflags)
         {
             const Nif::NiGeometry* niGeometry = static_cast<const Nif::NiGeometry*>(nifNode);
-            if (niGeometry->data.empty())
+            if (niGeometry->mData.empty())
                 return;
 
             bool hasPartitions = false;
-            if (!niGeometry->skin.empty())
+            if (!niGeometry->mSkin.empty())
             {
-                const Nif::NiSkinInstance* skin = niGeometry->skin.getPtr();
+                const Nif::NiSkinInstance* skin = niGeometry->mSkin.getPtr();
                 const Nif::NiSkinData* data = nullptr;
                 const Nif::NiSkinPartition* partitions = nullptr;
                 if (!skin->mData.empty())
@@ -1419,13 +1419,11 @@ namespace NifOsg
                 }
             }
 
-            const Nif::NiGeometryData* niGeometryData = niGeometry->data.getPtr();
+            const Nif::NiGeometryData* niGeometryData = niGeometry->mData.getPtr();
             if (!hasPartitions)
             {
                 if (niGeometry->recType == Nif::RC_NiTriShape || nifNode->recType == Nif::RC_BSLODTriShape)
                 {
-                    if (niGeometryData->recType != Nif::RC_NiTriShapeData)
-                        return;
                     auto data = static_cast<const Nif::NiTriShapeData*>(niGeometryData);
                     const std::vector<unsigned short>& triangles = data->mTriangles;
                     if (triangles.empty())
@@ -1435,8 +1433,6 @@ namespace NifOsg
                 }
                 else if (niGeometry->recType == Nif::RC_NiTriStrips)
                 {
-                    if (niGeometryData->recType != Nif::RC_NiTriStripsData)
-                        return;
                     auto data = static_cast<const Nif::NiTriStripsData*>(niGeometryData);
                     bool hasGeometry = false;
                     for (const std::vector<unsigned short>& strip : data->mStrips)
@@ -1452,8 +1448,6 @@ namespace NifOsg
                 }
                 else if (niGeometry->recType == Nif::RC_NiLines)
                 {
-                    if (niGeometryData->recType != Nif::RC_NiLinesData)
-                        return;
                     auto data = static_cast<const Nif::NiLinesData*>(niGeometryData);
                     const auto& line = data->mLines;
                     if (line.empty())
@@ -1545,7 +1539,7 @@ namespace NifOsg
             // Assign bone weights
             osg::ref_ptr<SceneUtil::RigGeometry::InfluenceMap> map(new SceneUtil::RigGeometry::InfluenceMap);
 
-            const Nif::NiSkinInstance* skin = static_cast<const Nif::NiGeometry*>(nifNode)->skin.getPtr();
+            const Nif::NiSkinInstance* skin = static_cast<const Nif::NiGeometry*>(nifNode)->mSkin.getPtr();
             const Nif::NiSkinData* data = skin->mData.getPtr();
             const Nif::NiAVObjectList& bones = skin->mBones;
             for (std::size_t i = 0; i < bones.size(); ++i)