Don't rely on subrecord order when reading (Fixes #2361)

components/esm/aipackage.cpp

@@ -11,14 +11,10 @@ namespace ESM
         mServices = 0;
     }
 
-    void AIPackageList::load(ESMReader &esm)
+    void AIPackageList::add(ESMReader &esm)
     {
-        mList.clear();
-        while (esm.hasMoreSubs()) {
-            // initialize every iteration
         AIPackage pack;
-                esm.getSubName();
-            if (esm.retSubName() == 0x54444e43) { // CNDT
+        if (esm.retSubName() == AI_CNDT) {
             mList.back().mCellName = esm.getHString();
         } else if (esm.retSubName() == AI_Wander) {
             pack.mType = AI_Wander;
@@ -42,6 +38,29 @@ namespace ESM
         } else { // not AI package related data, so leave
              return;
         }
+
+    }
+
+    void AIPackageList::load(ESMReader &esm)
+    {
+        mList.clear();
+        while (esm.hasMoreSubs()) {
+            // initialize every iteration
+            esm.getSubName();
+            switch (esm.retSubName().val)
+            {
+                case AI_Wander:
+                case AI_Activate:
+                case AI_Escort:
+                case AI_Follow:
+                case AI_Travel:
+                case AI_CNDT:
+
+                    add(esm);
+                    break;
+            default:
+                return;
+            }
         }
     }
 

components/esm/aipackage.hpp

@@ -63,7 +63,8 @@ namespace ESM
         AI_Travel = 0x545f4941,
         AI_Follow = 0x465f4941,
         AI_Escort = 0x455f4941,
-        AI_Activate = 0x415f4941
+        AI_Activate = 0x415f4941,
+        AI_CNDT = 0x54444e43
     };
 
     /// \note Used for storaging packages in a single container
@@ -90,11 +91,12 @@ namespace ESM
     {
         std::vector<AIPackage> mList;
 
-        /// \note This breaks consistency of subrecords reading:
-        /// after calling it subrecord name is already read, so
-        /// it needs to use retSubName() if needed. But, hey, there
-        /// is only one field left (XSCL) and only two records uses AI
+        /// Add a single AIPackage, assumes subrecord name was already read
+        void add(ESMReader &esm);
+
+        /// TODO: remove this method. The ESM format does not guarantee that all AI packages follow one another
         void load(ESMReader &esm);
+
         void save(ESMWriter &esm) const;
     };
 }

components/esm/loadarmo.cpp

@@ -7,16 +7,22 @@
 namespace ESM
 {
 
-void PartReferenceList::load(ESMReader &esm)
-{
-    mParts.clear();
-    while (esm.isNextSub("INDX"))
+    void PartReferenceList::add(ESMReader &esm)
     {
         PartReference pr;
         esm.getHT(pr.mPart); // The INDX byte
         pr.mMale = esm.getHNOString("BNAM");
         pr.mFemale = esm.getHNOString("CNAM");
         mParts.push_back(pr);
+
+    }
+
+    void PartReferenceList::load(ESMReader &esm)
+    {
+        mParts.clear();
+        while (esm.isNextSub("INDX"))
+        {
+            add(esm);
         }
     }
 
@@ -34,13 +40,42 @@ unsigned int Armor::sRecordId = REC_ARMO;
 
     void Armor::load(ESMReader &esm)
     {
-    mModel = esm.getHNString("MODL");
-    mName = esm.getHNOString("FNAM");
-    mScript = esm.getHNOString("SCRI");
-    esm.getHNT(mData, "AODT", 24);
-    mIcon = esm.getHNOString("ITEX");
-    mParts.load(esm);
-    mEnchant = esm.getHNOString("ENAM");
+        mParts.mParts.clear();
+        bool hasData = false;
+        while (esm.hasMoreSubs())
+        {
+            esm.getSubName();
+            uint32_t name = esm.retSubName().val;
+            switch (name)
+            {
+                case ESM::FourCC<'M','O','D','L'>::value:
+                    mModel = esm.getHString();
+                    break;
+                case ESM::FourCC<'F','N','A','M'>::value:
+                    mName = esm.getHString();
+                    break;
+                case ESM::FourCC<'A','O','D','T'>::value:
+                    esm.getHT(mData, 24);
+                    hasData = true;
+                    break;
+                case ESM::FourCC<'S','C','R','I'>::value:
+                    mScript = esm.getHString();
+                    break;
+                case ESM::FourCC<'I','T','E','X'>::value:
+                    mIcon = esm.getHString();
+                    break;
+                case ESM::FourCC<'E','N','A','M'>::value:
+                    mEnchant = esm.getHString();
+                    break;
+                case ESM::FourCC<'I','N','D','X'>::value:
+                    mParts.add(esm);
+                    break;
+                default:
+                    esm.fail("Unknown subrecord");
+            }
+        }
+        if (!hasData)
+            esm.fail("Missing CTDT subrecord");
     }
 
     void Armor::save(ESMWriter &esm) const

components/esm/loadarmo.hpp

@@ -55,6 +55,10 @@ struct PartReferenceList
 {
     std::vector<PartReference> mParts;
 
+    /// Load one part, assumes the subrecord name was already read
+    void add(ESMReader &esm);
+
+    /// TODO: remove this method. The ESM format does not guarantee that all Part subrecords follow one another.
     void load(ESMReader &esm);
     void save(ESMWriter &esm) const;
 };

components/esm/loadbook.cpp

@@ -10,13 +10,41 @@ namespace ESM
 
     void Book::load(ESMReader &esm)
     {
-    mModel = esm.getHNString("MODL");
-    mName = esm.getHNOString("FNAM");
-    esm.getHNT(mData, "BKDT", 20);
-    mScript = esm.getHNOString("SCRI");
-    mIcon = esm.getHNOString("ITEX");
-    mText = esm.getHNOString("TEXT");
-    mEnchant = esm.getHNOString("ENAM");
+        bool hasData = false;
+        while (esm.hasMoreSubs())
+        {
+            esm.getSubName();
+            uint32_t name = esm.retSubName().val;
+            switch (name)
+            {
+                case ESM::FourCC<'M','O','D','L'>::value:
+                    mModel = esm.getHString();
+                    break;
+                case ESM::FourCC<'F','N','A','M'>::value:
+                    mName = esm.getHString();
+                    break;
+                case ESM::FourCC<'B','K','D','T'>::value:
+                    esm.getHT(mData, 20);
+                    hasData = true;
+                    break;
+                case ESM::FourCC<'S','C','R','I'>::value:
+                    mScript = esm.getHString();
+                    break;
+                case ESM::FourCC<'I','T','E','X'>::value:
+                    mIcon = esm.getHString();
+                    break;
+                case ESM::FourCC<'E','N','A','M'>::value:
+                    mEnchant = esm.getHString();
+                    break;
+                case ESM::FourCC<'T','E','X','T'>::value:
+                    mText = esm.getHString();
+                    break;
+                default:
+                    esm.fail("Unknown subrecord");
+            }
+        }
+        if (!hasData)
+            esm.fail("Missing BKDT subrecord");
     }
     void Book::save(ESMWriter &esm) const
     {

components/esm/loadclot.cpp

@@ -10,17 +10,42 @@ namespace ESM
 
     void Clothing::load(ESMReader &esm)
     {
-        mModel = esm.getHNString("MODL");
-        mName = esm.getHNOString("FNAM");
-        esm.getHNT(mData, "CTDT", 12);
-
-        mScript = esm.getHNOString("SCRI");
-        mIcon = esm.getHNOString("ITEX");
-
-        mParts.load(esm);
-
-
-        mEnchant = esm.getHNOString("ENAM");
+        mParts.mParts.clear();
+        bool hasData = false;
+        while (esm.hasMoreSubs())
+        {
+            esm.getSubName();
+            uint32_t name = esm.retSubName().val;
+            switch (name)
+            {
+                case ESM::FourCC<'M','O','D','L'>::value:
+                    mModel = esm.getHString();
+                    break;
+                case ESM::FourCC<'F','N','A','M'>::value:
+                    mName = esm.getHString();
+                    break;
+                case ESM::FourCC<'C','T','D','T'>::value:
+                    esm.getHT(mData, 12);
+                    hasData = true;
+                    break;
+                case ESM::FourCC<'S','C','R','I'>::value:
+                    mScript = esm.getHString();
+                    break;
+                case ESM::FourCC<'I','T','E','X'>::value:
+                    mIcon = esm.getHString();
+                    break;
+                case ESM::FourCC<'E','N','A','M'>::value:
+                    mEnchant = esm.getHString();
+                    break;
+                case ESM::FourCC<'I','N','D','X'>::value:
+                    mParts.add(esm);
+                    break;
+                default:
+                    esm.fail("Unknown subrecord");
+            }
+        }
+        if (!hasData)
+            esm.fail("Missing CTDT subrecord");
     }
 
     void Clothing::save(ESMWriter &esm) const

components/esm/loadcont.cpp

@@ -7,14 +7,19 @@
 namespace ESM
 {
 
+void InventoryList::add(ESMReader &esm)
+{
+    ContItem ci;
+    esm.getHT(ci, 36);
+    mList.push_back(ci);
+}
+
 void InventoryList::load(ESMReader &esm)
 {
     mList.clear();
-    ContItem ci;
     while (esm.isNextSub("NPCO"))
     {
-        esm.getHT(ci, 36);
-        mList.push_back(ci);
+        add(esm);
     }
 }
 

components/esm/loadcont.hpp

@@ -26,6 +26,10 @@ struct InventoryList
 {
     std::vector<ContItem> mList;
 
+    /// Load one item, assumes subrecord name is already read
+    void add(ESMReader &esm);
+
+    /// TODO: remove this method, the ESM format doesn't guarantee that all ContItems follow one another
     void load(ESMReader &esm);
     void save(ESMWriter &esm) const;
 };

components/esm/loaddoor.cpp

@@ -10,11 +10,31 @@ namespace ESM
 
     void Door::load(ESMReader &esm)
     {
-    mModel = esm.getHNString("MODL");
-    mName = esm.getHNOString("FNAM");
-    mScript = esm.getHNOString("SCRI");
-    mOpenSound = esm.getHNOString("SNAM");
-    mCloseSound = esm.getHNOString("ANAM");
+        while (esm.hasMoreSubs())
+        {
+            esm.getSubName();
+            uint32_t name = esm.retSubName().val;
+            switch (name)
+            {
+                case ESM::FourCC<'M','O','D','L'>::value:
+                    mModel = esm.getHString();
+                    break;
+                case ESM::FourCC<'F','N','A','M'>::value:
+                    mName = esm.getHString();
+                    break;
+                case ESM::FourCC<'S','C','R','I'>::value:
+                    mScript = esm.getHString();
+                    break;
+                case ESM::FourCC<'S','N','A','M'>::value:
+                    mOpenSound = esm.getHString();
+                    break;
+                case ESM::FourCC<'A','N','A','M'>::value:
+                    mCloseSound = esm.getHString();
+                    break;
+                default:
+                    esm.fail("Unknown subrecord");
+            }
+        }
     }
 
     void Door::save(ESMWriter &esm) const

components/esm/loadmgef.cpp

@@ -204,20 +204,51 @@ void MagicEffect::load(ESMReader &esm)
         mData.mFlags |= HardcodedFlags[mIndex];
     }
 
-  mIcon = esm.getHNOString("ITEX");
-  mParticle = esm.getHNOString("PTEX");
-
-  mBoltSound = esm.getHNOString("BSND");
-  mCastSound = esm.getHNOString("CSND");
-  mHitSound = esm.getHNOString("HSND");
-  mAreaSound = esm.getHNOString("ASND");
-
-  mCasting = esm.getHNOString("CVFX");
-  mBolt = esm.getHNOString("BVFX");
-  mHit = esm.getHNOString("HVFX");
-  mArea = esm.getHNOString("AVFX");
-
-  mDescription = esm.getHNOString("DESC");
+    // vanilla MW accepts the _SND subrecords before or after DESC... I hope
+    // this isn't true for other records, or we have to do a mass-refactor
+    while (esm.hasMoreSubs())
+    {
+        esm.getSubName();
+        uint32_t name = esm.retSubName().val;
+        switch (name)
+        {
+            case ESM::FourCC<'I','T','E','X'>::value:
+                mIcon = esm.getHString();
+                break;
+            case ESM::FourCC<'P','T','E','X'>::value:
+                mParticle = esm.getHString();
+                break;
+            case ESM::FourCC<'B','S','N','D'>::value:
+                mBoltSound = esm.getHString();
+                break;
+            case ESM::FourCC<'C','S','N','D'>::value:
+                mCastSound = esm.getHString();
+                break;
+            case ESM::FourCC<'H','S','N','D'>::value:
+                mHitSound = esm.getHString();
+                break;
+            case ESM::FourCC<'A','S','N','D'>::value:
+                mAreaSound = esm.getHString();
+                break;
+            case ESM::FourCC<'C','V','F','X'>::value:
+                mCasting = esm.getHString();
+                break;
+            case ESM::FourCC<'B','V','F','X'>::value:
+                mBolt = esm.getHString();
+                break;
+            case ESM::FourCC<'H','V','F','X'>::value:
+                mHit = esm.getHString();
+                break;
+            case ESM::FourCC<'A','V','F','X'>::value:
+                mArea = esm.getHString();
+                break;
+            case ESM::FourCC<'D','E','S','C'>::value:
+                mDescription = esm.getHString();
+                break;
+            default:
+                esm.fail("Unknown subrecord " + esm.retSubName().toString());
+        }
+    }
 }
 void MagicEffect::save(ESMWriter &esm) const
 {

components/esm/loadmisc.cpp

@@ -10,12 +10,35 @@ namespace ESM
 
     void Miscellaneous::load(ESMReader &esm)
     {
-    mModel = esm.getHNString("MODL");
-    mName = esm.getHNOString("FNAM");
-    esm.getHNT(mData, "MCDT", 12);
-    mScript = esm.getHNOString("SCRI");
-    mIcon = esm.getHNOString("ITEX");
+        bool hasData = false;
+        while (esm.hasMoreSubs())
+        {
+            esm.getSubName();
+            uint32_t name = esm.retSubName().val;
+            switch (name)
+            {
+                case ESM::FourCC<'M','O','D','L'>::value:
+                    mModel = esm.getHString();
+                    break;
+                case ESM::FourCC<'F','N','A','M'>::value:
+                    mName = esm.getHString();
+                    break;
+                case ESM::FourCC<'M','C','D','T'>::value:
+                    esm.getHT(mData, 12);
+                    hasData = true;
+                    break;
+                case ESM::FourCC<'S','C','R','I'>::value:
+                    mScript = esm.getHString();
+                    break;
+                case ESM::FourCC<'I','T','E','X'>::value:
+                    mIcon = esm.getHString();
+                    break;
+            }
         }
+        if (!hasData)
+            esm.fail("Missing MCDT subrecord");
+    }
+
     void Miscellaneous::save(ESMWriter &esm) const
     {
         esm.writeHNCString("MODL", mModel);

components/esm/loadnpc.cpp

@@ -12,18 +12,46 @@ void NPC::load(ESMReader &esm)
     {
         mPersistent = esm.getRecordFlags() & 0x0400;
 
-    mModel = esm.getHNOString("MODL");
-    mName = esm.getHNOString("FNAM");
-
-    mRace = esm.getHNString("RNAM");
-    mClass = esm.getHNString("CNAM");
-    mFaction = esm.getHNString("ANAM");
-    mHead = esm.getHNString("BNAM");
-    mHair = esm.getHNString("KNAM");
-
-    mScript = esm.getHNOString("SCRI");
+        mSpells.mList.clear();
+        mInventory.mList.clear();
+        mTransport.clear();
+        mAiPackage.mList.clear();
 
-    esm.getSubNameIs("NPDT");
+        bool hasNpdt = false;
+        bool hasFlags = false;
+        mHasAI = false;
+        while (esm.hasMoreSubs())
+        {
+            esm.getSubName();
+            uint32_t name = esm.retSubName().val;
+            switch (name)
+            {
+                case ESM::FourCC<'M','O','D','L'>::value:
+                    mModel = esm.getHString();
+                    break;
+                case ESM::FourCC<'F','N','A','M'>::value:
+                    mName = esm.getHString();
+                    break;
+                case ESM::FourCC<'R','N','A','M'>::value:
+                    mRace = esm.getHString();
+                    break;
+                case ESM::FourCC<'C','N','A','M'>::value:
+                    mClass = esm.getHString();
+                    break;
+                case ESM::FourCC<'A','N','A','M'>::value:
+                    mFaction = esm.getHString();
+                    break;
+                case ESM::FourCC<'B','N','A','M'>::value:
+                    mHead = esm.getHString();
+                    break;
+                case ESM::FourCC<'K','N','A','M'>::value:
+                    mHair = esm.getHString();
+                    break;
+                case ESM::FourCC<'S','C','R','I'>::value:
+                    mScript = esm.getHString();
+                    break;
+                case ESM::FourCC<'N','P','D','T'>::value:
+                    hasNpdt = true;
                     esm.getSubHeader();
                     if (esm.getSubSize() == 52)
                     {
@@ -37,31 +65,47 @@ void NPC::load(ESMReader &esm)
                     }
                     else
                         esm.fail("NPC_NPDT must be 12 or 52 bytes long");
-
-    esm.getHNT(mFlags, "FLAG");
-
-    mInventory.load(esm);
-    mSpells.load(esm);
-
-    if (esm.isNextSub("AIDT"))
-    {
+                    break;
+                case ESM::FourCC<'F','L','A','G'>::value:
+                    hasFlags = true;
+                    esm.getHT(mFlags);
+                    break;
+                case ESM::FourCC<'N','P','C','S'>::value:
+                    mSpells.add(esm);
+                    break;
+                case ESM::FourCC<'N','P','C','O'>::value:
+                    mInventory.add(esm);
+                    break;
+                case ESM::FourCC<'A','I','D','T'>::value:
                     esm.getHExact(&mAiData, sizeof(mAiData));
                     mHasAI= true;
-    }
-    else
-        mHasAI = false;
-
-    mTransport.clear();
-    while (esm.isNextSub("DODT") || esm.isNextSub("DNAM")) {
-        if (esm.retSubName() == 0x54444f44) { // DODT struct
+                    break;
+                case ESM::FourCC<'D','O','D','T'>::value:
+                {
                     Dest dodt;
                     esm.getHExact(&dodt.mPos, 24);
                     mTransport.push_back(dodt);
-        } else if (esm.retSubName() == 0x4d414e44) { // DNAM struct
+                    break;
+                }
+                case ESM::FourCC<'D','N','A','M'>::value:
                     mTransport.back().mCellName = esm.getHString();
+                    break;
+                case AI_Wander:
+                case AI_Activate:
+                case AI_Escort:
+                case AI_Follow:
+                case AI_Travel:
+                case AI_CNDT:
+                    mAiPackage.add(esm);
+                    break;
+                default:
+                    esm.fail("Unknown subrecord " + esm.retSubName().toString());
             }
         }
-    mAiPackage.load(esm);
+        if (!hasNpdt)
+            esm.fail("Missing NPDT subrecord");
+        if (!hasFlags)
+            esm.fail("Missing FLAG subrecord");
     }
     void NPC::save(ESMWriter &esm) const
     {

components/esm/loadrace.cpp

@@ -20,10 +20,34 @@ namespace ESM
 
 void Race::load(ESMReader &esm)
 {
-    mName = esm.getHNOString("FNAM");
-    esm.getHNT(mData, "RADT", 140);
-    mPowers.load(esm);
-    mDescription = esm.getHNOString("DESC");
+    mPowers.mList.clear();
+
+    bool hasData = false;
+    while (esm.hasMoreSubs())
+    {
+        esm.getSubName();
+        uint32_t name = esm.retSubName().val;
+        switch (name)
+        {
+            case ESM::FourCC<'F','N','A','M'>::value:
+                mName = esm.getHString();
+                break;
+            case ESM::FourCC<'R','A','D','T'>::value:
+                esm.getHT(mData, 140);
+                hasData = true;
+                break;
+            case ESM::FourCC<'D','E','S','C'>::value:
+                mDescription = esm.getHString();
+                break;
+            case ESM::FourCC<'N','P','C','S'>::value:
+                mPowers.add(esm);
+                break;
+            default:
+                esm.fail("Unknown subrecord " + esm.retSubName().toString());
+        }
+    }
+    if (!hasData)
+        esm.fail("Missing RADT subrecord");
 }
 void Race::save(ESMWriter &esm) const
 {

components/esm/loadspel.cpp

@@ -10,9 +10,30 @@ namespace ESM
 
 void Spell::load(ESMReader &esm)
 {
-    mName = esm.getHNOString("FNAM");
-    esm.getHNT(mData, "SPDT", 12);
-    mEffects.load(esm);
+    bool hasData = false;
+    while (esm.hasMoreSubs())
+    {
+        esm.getSubName();
+        uint32_t val = esm.retSubName().val;
+
+        switch (val)
+        {
+            case ESM::FourCC<'F','N','A','M'>::value:
+                mName = esm.getHString();
+                break;
+            case ESM::FourCC<'S','P','D','T'>::value:
+                esm.getHT(mData, 12);
+                hasData = true;
+                break;
+            case ESM::FourCC<'E','N','A','M'>::value:
+                ENAMstruct s;
+                esm.getHT(s, 24);
+                mEffects.mList.push_back(s);
+                break;
+        }
+    }
+    if (!hasData)
+        esm.fail("Missing SPDT subrecord");
 }
 
 void Spell::save(ESMWriter &esm) const

components/esm/loadweap.cpp

@@ -10,12 +10,38 @@ namespace ESM
 
     void Weapon::load(ESMReader &esm)
     {
-    mModel = esm.getHNString("MODL");
-    mName = esm.getHNOString("FNAM");
-    esm.getHNT(mData, "WPDT", 32);
-    mScript = esm.getHNOString("SCRI");
-    mIcon = esm.getHNOString("ITEX");
-    mEnchant = esm.getHNOString("ENAM");
+        bool hasData = false;
+        while (esm.hasMoreSubs())
+        {
+            esm.getSubName();
+            uint32_t name = esm.retSubName().val;
+            switch (name)
+            {
+                case ESM::FourCC<'M','O','D','L'>::value:
+                    mModel = esm.getHString();
+                    break;
+                case ESM::FourCC<'F','N','A','M'>::value:
+                    mName = esm.getHString();
+                    break;
+                case ESM::FourCC<'W','P','D','T'>::value:
+                    esm.getHT(mData, 32);
+                    hasData = true;
+                    break;
+                case ESM::FourCC<'S','C','R','I'>::value:
+                    mScript = esm.getHString();
+                    break;
+                case ESM::FourCC<'I','T','E','X'>::value:
+                    mIcon = esm.getHString();
+                    break;
+                case ESM::FourCC<'E','N','A','M'>::value:
+                    mEnchant = esm.getHString();
+                    break;
+                default:
+                    esm.fail("Unknown subrecord");
+            }
+        }
+        if (!hasData)
+            esm.fail("Missing WPDT subrecord");
     }
     void Weapon::save(ESMWriter &esm) const
     {

components/esm/spelllist.cpp

@@ -9,8 +9,13 @@ void SpellList::load(ESMReader &esm)
 {
     mList.clear();
     while (esm.isNextSub("NPCS")) {
-        mList.push_back(esm.getHString());
+        add(esm);
+    }
 }
+
+void SpellList::add(ESMReader &esm)
+{
+    mList.push_back(esm.getHString());
 }
 
 void SpellList::save(ESMWriter &esm) const

components/esm/spelllist.hpp

@@ -19,6 +19,11 @@ namespace ESM
         /// Is this spell ID in mList?
         bool exists(const std::string& spell) const;
 
+        /// Load one spell, assumes the subrecord name was already read
+        void add(ESMReader &esm);
+
+        /// Load all spells
+        /// TODO: remove this method, the ESM format doesn't guarantee that all spell subrecords follow one another
         void load(ESMReader &esm);
         void save(ESMWriter &esm) const;
     };