Changes required during review

apps/openmw/mwclass/esm4npc.cpp

@@ -32,6 +32,9 @@ namespace MWClass
 
     static const ESM4::Npc* chooseTemplate(const std::vector<const ESM4::Npc*>& recs, uint16_t flag)
     {
+        // If the record is neither TES4 nor TES5 (though maybe FO4 is compatible with tes5.templateFlags), then
+        // the function can return nullptr that will lead to "ESM4 NPC traits not found" exception and the NPC
+        // will not be added to the scene. But in any way it shouldn't cause a crash.
         for (const auto* rec : recs)
             if (rec->mIsTES4 || !(rec->mBaseConfig.tes5.templateFlags & flag))
                 return rec;
@@ -68,9 +71,9 @@ namespace MWClass
         data->mBaseData = chooseTemplate(npcRecs, ESM4::Npc::TES5_UseBaseData);
 
         if (!data->mTraits)
-            throw std::runtime_error("ESM4 Npc traits not found");
+            throw std::runtime_error("ESM4 NPC traits not found");
         if (!data->mBaseData)
-            throw std::runtime_error("ESM4 Npc base data not found");
+            throw std::runtime_error("ESM4 NPC base data not found");
 
         data->mRace = store->get<ESM4::Race>().find(data->mTraits->mRace);
         if (data->mTraits->mIsTES4)

apps/openmw/mwrender/esm4npcanimation.cpp

@@ -8,13 +8,13 @@
 #include <components/esm4/loadnpc.hpp>
 #include <components/esm4/loadrace.hpp>
 
-#include "../mwworld/customdata.hpp"
+#include <components/misc/resourcehelpers.hpp>
-#include "../mwworld/esmstore.hpp"
-
 #include <components/resource/resourcesystem.hpp>
 #include <components/resource/scenemanager.hpp>
 
 #include "../mwclass/esm4npc.hpp"
+#include "../mwworld/customdata.hpp"
+#include "../mwworld/esmstore.hpp"
 
 namespace MWRender
 {
@@ -23,18 +23,28 @@ namespace MWRender
         : Animation(ptr, std::move(parentNode), resourceSystem)
     {
         getOrCreateObjectRoot();
-        const ESM4::Npc* traits = MWClass::ESM4Npc::getTraitsRecord(mPtr);
+        updateParts();
-        if (traits->mIsTES4)
-            insertTes4NpcBodyPartsAndEquipment();
-        else
-            insertTes5NpcBodyPartsAndEquipment();
     }
 
-    void ESM4NpcAnimation::insertMesh(std::string_view model)
+    void ESM4NpcAnimation::updateParts()
     {
-        std::string path = "meshes\\";
+        mObjectRoot->removeChildren(0, mObjectRoot->getNumChildren());
-        path.append(model);
+        const ESM4::Npc* traits = MWClass::ESM4Npc::getTraitsRecord(mPtr);
-        mResourceSystem->getSceneManager()->getInstance(path, mObjectRoot.get());
+        // There is no flag "mIsTES5", so we can not distinguish from other cases.
+        // But calling wrong `updateParts*` function shouldn't crash the game and will
+        // only lead to the NPC not being rendered.
+        if (traits->mIsTES4)
+            updatePartsTES4();
+        else
+            updatePartsTES5();
+    }
+
+    void ESM4NpcAnimation::insertPart(std::string_view model)
+    {
+        if (model.empty())
+            return;
+        mResourceSystem->getSceneManager()->getInstance(
+            Misc::ResourceHelpers::correctMeshPath(model, mResourceSystem->getVFS()), mObjectRoot.get());
     }
 
     template <class Record>
@@ -48,7 +58,7 @@ namespace MWRender
             return rec->mModel;
     }
 
-    void ESM4NpcAnimation::insertTes4NpcBodyPartsAndEquipment()
+    void ESM4NpcAnimation::updatePartsTES4()
     {
         const ESM4::Npc* traits = MWClass::ESM4Npc::getTraitsRecord(mPtr);
         const ESM4::Race* race = MWClass::ESM4Npc::getRace(mPtr);
@@ -57,25 +67,23 @@ namespace MWRender
         // TODO: Body and head parts are placed incorrectly, need to attach to bones
 
         for (const ESM4::Race::BodyPart& bodyPart : (isFemale ? race->mBodyPartsFemale : race->mBodyPartsMale))
-            if (!bodyPart.mesh.empty())
+            insertPart(bodyPart.mesh);
-                insertMesh(bodyPart.mesh);
         for (const ESM4::Race::BodyPart& bodyPart : (isFemale ? race->mHeadPartsFemale : race->mHeadParts))
-            if (!bodyPart.mesh.empty())
+            insertPart(bodyPart.mesh);
-                insertMesh(bodyPart.mesh);
         if (!traits->mHair.isZeroOrUnset())
         {
             const MWWorld::ESMStore* store = MWBase::Environment::get().getESMStore();
             if (const ESM4::Hair* hair = store->get<ESM4::Hair>().search(traits->mHair))
-                insertMesh(hair->mModel);
+                insertPart(hair->mModel);
         }
 
         for (const ESM4::Armor* armor : MWClass::ESM4Npc::getEquippedArmor(mPtr))
-            insertMesh(chooseTes4EquipmentModel(armor, isFemale));
+            insertPart(chooseTes4EquipmentModel(armor, isFemale));
         for (const ESM4::Clothing* clothing : MWClass::ESM4Npc::getEquippedClothing(mPtr))
-            insertMesh(chooseTes4EquipmentModel(clothing, isFemale));
+            insertPart(chooseTes4EquipmentModel(clothing, isFemale));
     }
 
-    void ESM4NpcAnimation::insertTes5NpcBodyPartsAndEquipment()
+    void ESM4NpcAnimation::updatePartsTES5()
     {
         const MWWorld::ESMStore* store = MWBase::Environment::get().getESMStore();
 
@@ -95,10 +103,8 @@ namespace MWRender
                     Log(Debug::Error) << "Head part not found: " << ESM::RefId(partId);
                     continue;
                 }
-                if (usedHeadPartTypes.contains(part->mType))
+                if (usedHeadPartTypes.emplace(part->mType).second)
-                    continue;
+                    insertPart(part->mModel);
-                usedHeadPartTypes.insert(part->mType);
-                insertMesh(part->mModel);
             }
         };
 
@@ -126,7 +132,8 @@ namespace MWRender
             findArmorAddons(armor);
         if (!traits->mWornArmor.isZeroOrUnset())
             findArmorAddons(store->get<ESM4::Armor>().find(traits->mWornArmor));
-        findArmorAddons(store->get<ESM4::Armor>().find(race->mSkin));
+        if (!race->mSkin.isZeroOrUnset())
+            findArmorAddons(store->get<ESM4::Armor>().find(race->mSkin));
 
         if (isFemale)
             std::sort(armorAddons.begin(), armorAddons.end(),
@@ -139,12 +146,14 @@ namespace MWRender
         for (const ESM4::ArmorAddon* arma : armorAddons)
         {
             const uint32_t covers = arma->mBodyTemplate.bodyPart;
+            // if body is already covered, skip to avoid clipping
             if (covers & usedParts & ESM4::Armor::TES5_Body)
-                continue; // if body is already covered, skip to avoid clipping
+                continue;
+            // if covers at least something that wasn't covered before - add model
             if (covers & ~usedParts)
-            { // if covers at least something that wasn't covered before - add model
+            {
                 usedParts |= covers;
-                insertMesh(isFemale ? arma->mModelFemale : arma->mModelMale);
+                insertPart(isFemale ? arma->mModelFemale : arma->mModelMale);
             }
         }
 

apps/openmw/mwrender/esm4npcanimation.hpp

@@ -12,10 +12,11 @@ namespace MWRender
             const MWWorld::Ptr& ptr, osg::ref_ptr<osg::Group> parentNode, Resource::ResourceSystem* resourceSystem);
 
     private:
-        void insertMesh(std::string_view model);
+        void insertPart(std::string_view model);
 
-        void insertTes4NpcBodyPartsAndEquipment();
+        void updateParts();
-        void insertTes5NpcBodyPartsAndEquipment();
+        void updatePartsTES4();
+        void updatePartsTES5();
     };
 }
 

components/esm4/loadarma.cpp

@@ -95,14 +95,21 @@ void ESM4::ArmorAddon::load(ESM4::Reader& reader)
 
                 break;
             case ESM4::SUB_DNAM:
-                reader.get(mMalePriority);
+                if (subHdr.dataSize == 12)
-                reader.get(mFemalePriority);
+                {
-                reader.get(mWeightSliderMale);
+                    std::uint16_t unknownInt16;
-                reader.get(mWeightSliderFemale);
+                    std::uint8_t unknownInt8;
-                reader.get(mUnknown1);
+                    reader.get(mMalePriority);
-                reader.get(mDetectionSoundValue);
+                    reader.get(mFemalePriority);
-                reader.get(mUnknown2);
+                    reader.get(mWeightSliderMale);
-                reader.get(mWeaponAdjust);
+                    reader.get(mWeightSliderFemale);
+                    reader.get(unknownInt16);
+                    reader.get(mDetectionSoundValue);
+                    reader.get(unknownInt8);
+                    reader.get(mWeaponAdjust);
+                }
+                else
+                    reader.skipSubRecordData();
                 break;
             case ESM4::SUB_MO2T: // FIXME: should group with MOD2
             case ESM4::SUB_MO2S: // FIXME: should group with MOD2

components/esm4/loadarma.hpp

@@ -59,17 +59,15 @@ namespace ESM4
 
         BodyTemplate mBodyTemplate; // TES5
 
-        std::uint8_t mMalePriority;
+        std::uint8_t mMalePriority = 0;
-        std::uint8_t mFemalePriority;
+        std::uint8_t mFemalePriority = 0;
 
         // Flag 0x2 in mWeightSlider means that there are 2 world models for different weights: _0.nif and _1.nif
-        std::uint8_t mWeightSliderMale;
+        std::uint8_t mWeightSliderMale = 0;
-        std::uint8_t mWeightSliderFemale;
+        std::uint8_t mWeightSliderFemale = 0;
 
-        std::uint16_t mUnknown1;
+        std::uint8_t mDetectionSoundValue = 0;
-        std::uint8_t mDetectionSoundValue;
+        float mWeaponAdjust = 0;
-        std::uint8_t mUnknown2;
-        float mWeaponAdjust;
 
         void load(ESM4::Reader& reader);
         // void save(ESM4::Writer& writer) const;

components/misc/resourcehelpers.cpp

@@ -159,9 +159,11 @@ std::string Misc::ResourceHelpers::correctActorModelPath(const std::string& resP
     return mdlname;
 }
 
-std::string Misc::ResourceHelpers::correctMeshPath(const std::string& resPath, const VFS::Manager* vfs)
+std::string Misc::ResourceHelpers::correctMeshPath(std::string_view resPath, const VFS::Manager* vfs)
 {
-    return "meshes\\" + resPath;
+    std::string res = "meshes\\";
+    res.append(resPath);
+    return res;
 }
 
 std::string Misc::ResourceHelpers::correctSoundPath(const std::string& resPath)

components/misc/resourcehelpers.hpp

@@ -33,7 +33,7 @@ namespace Misc
         std::string correctActorModelPath(const std::string& resPath, const VFS::Manager* vfs);
 
         // Adds "meshes\\".
-        std::string correctMeshPath(const std::string& resPath, const VFS::Manager* vfs);
+        std::string correctMeshPath(std::string_view resPath, const VFS::Manager* vfs);
 
         // Adds "sound\\".
         std::string correctSoundPath(const std::string& resPath);