Merge branch 'cell_ref_skip_load' into 'master'

Skip load cell ref when there is no need

See merge request OpenMW/openmw!1752

apps/openmw/mwrender/objectpaging.cpp

@@ -435,7 +435,7 @@ namespace MWRender
                         cMRef.mRefNum.mIndex = 0;
                         bool deleted = false;
                         bool moved = false;
-                        while(cell->getNextRef(esm[index], ref, deleted, cMRef, moved))
+                        while (ESM::Cell::getNextRef(esm[index], ref, deleted, cMRef, moved, ESM::Cell::GetNextRefMode::LoadOnlyNotMoved))
                         {
                             if (moved)
                                 continue;

apps/openmw/mwworld/cellstore.cpp

@@ -568,7 +568,7 @@ namespace MWWorld
                 cMRef.mRefNum.mIndex = 0;
                 bool deleted = false;
                 bool moved = false;
-                while(mCell->getNextRef(esm[index], ref, deleted, cMRef, moved))
+                while (ESM::Cell::getNextRef(esm[index], ref, deleted, cMRef, moved, ESM::Cell::GetNextRefMode::LoadOnlyNotMoved))
                 {
                     if (deleted || moved)
                         continue;
@@ -628,7 +628,7 @@ namespace MWWorld
                 cMRef.mRefNum.mIndex = 0;
                 bool deleted = false;
                 bool moved = false;
-                while(mCell->getNextRef(esm[index], ref, deleted, cMRef, moved))
+                while (ESM::Cell::getNextRef(esm[index], ref, deleted, cMRef, moved, ESM::Cell::GetNextRefMode::LoadOnlyNotMoved))
                 {
                     if (moved)
                         continue;

apps/openmw/mwworld/store.cpp

@@ -448,7 +448,7 @@ namespace MWWorld
         //
         // Get regular moved reference data. Adapted from CellStore::loadRefs. Maybe we can optimize the following
         //  implementation when the oher implementation works as well.
-        while (cell->getNextRef(esm, ref, deleted, cMRef, moved))
+        while (ESM::Cell::getNextRef(esm, ref, deleted, cMRef, moved, ESM::Cell::GetNextRefMode::LoadOnlyMoved))
         {
             if (!moved)
                 continue;

components/esm3/cellref.cpp

@@ -5,6 +5,148 @@
 #include "esmreader.hpp"
 #include "esmwriter.hpp"
 
+namespace ESM
+{
+    namespace
+    {
+        template <bool load>
+        void loadIdImpl(ESMReader& esm, bool wideRefNum, CellRef& cellRef)
+        {
+            // According to Hrnchamd, this does not belong to the actual ref. Instead, it is a marker indicating that
+            // the following refs are part of a "temp refs" section. A temp ref is not being tracked by the moved references system.
+            // Its only purpose is a performance optimization for "immovable" things. We don't need this, and it's problematic anyway,
+            // because any item can theoretically be moved by a script.
+            if (esm.isNextSub("NAM0"))
+                esm.skipHSub();
+
+            if constexpr (load)
+            {
+                cellRef.blank();
+                cellRef.mRefNum.load (esm, wideRefNum);
+                cellRef.mRefID = esm.getHNOString("NAME");
+
+                if (cellRef.mRefID.empty())
+                    Log(Debug::Warning) << "Warning: got CellRef with empty RefId in " << esm.getName() << " 0x" << std::hex << esm.getFileOffset();
+            }
+            else
+            {
+                RefNum {}.load(esm, wideRefNum);
+                esm.skipHNOString("NAME");
+            }
+        }
+
+        template <bool load>
+        void loadDataImpl(ESMReader &esm, bool &isDeleted, CellRef& cellRef)
+        {
+            const auto getHStringOrSkip = [&] (std::string& value)
+            {
+                if constexpr (load)
+                    value = esm.getHString();
+                else
+                    esm.skipHString();
+            };
+
+            const auto getHTOrSkip = [&] (auto& value)
+            {
+                if constexpr (load)
+                    esm.getHT(value);
+                else
+                    esm.skipHT<std::decay_t<decltype(value)>>();
+            };
+
+            if constexpr (load)
+                isDeleted = false;
+
+            bool isLoaded = false;
+            while (!isLoaded && esm.hasMoreSubs())
+            {
+                esm.getSubName();
+                switch (esm.retSubName().toInt())
+                {
+                    case ESM::fourCC("UNAM"):
+                        getHTOrSkip(cellRef.mReferenceBlocked);
+                        break;
+                    case ESM::fourCC("XSCL"):
+                        getHTOrSkip(cellRef.mScale);
+                        if constexpr (load)
+                            cellRef.mScale = std::clamp(cellRef.mScale, 0.5f, 2.0f);
+                        break;
+                    case ESM::fourCC("ANAM"):
+                        getHStringOrSkip(cellRef.mOwner);
+                        break;
+                    case ESM::fourCC("BNAM"):
+                        getHStringOrSkip(cellRef.mGlobalVariable);
+                        break;
+                    case ESM::fourCC("XSOL"):
+                        getHStringOrSkip(cellRef.mSoul);
+                        break;
+                    case ESM::fourCC("CNAM"):
+                        getHStringOrSkip(cellRef.mFaction);
+                        break;
+                    case ESM::fourCC("INDX"):
+                        getHTOrSkip(cellRef.mFactionRank);
+                        break;
+                    case ESM::fourCC("XCHG"):
+                        getHTOrSkip(cellRef.mEnchantmentCharge);
+                        break;
+                    case ESM::fourCC("INTV"):
+                        getHTOrSkip(cellRef.mChargeInt);
+                        break;
+                    case ESM::fourCC("NAM9"):
+                        getHTOrSkip(cellRef.mGoldValue);
+                        break;
+                    case ESM::fourCC("DODT"):
+                        getHTOrSkip(cellRef.mDoorDest);
+                        if constexpr (load)
+                            cellRef.mTeleport = true;
+                        break;
+                    case ESM::fourCC("DNAM"):
+                        getHStringOrSkip(cellRef.mDestCell);
+                        break;
+                    case ESM::fourCC("FLTV"):
+                        getHTOrSkip(cellRef.mLockLevel);
+                        break;
+                    case ESM::fourCC("KNAM"):
+                        getHStringOrSkip(cellRef.mKey);
+                        break;
+                    case ESM::fourCC("TNAM"):
+                        getHStringOrSkip(cellRef.mTrap);
+                        break;
+                    case ESM::fourCC("DATA"):
+                        if constexpr (load)
+                            esm.getHT(cellRef.mPos, 24);
+                        else
+                            esm.skip(24);
+                        break;
+                    case ESM::fourCC("NAM0"):
+                    {
+                        esm.skipHSub();
+                        break;
+                    }
+                    case ESM::SREC_DELE:
+                        esm.skipHSub();
+                        if constexpr (load)
+                            isDeleted = true;
+                        break;
+                    default:
+                        esm.cacheSubName();
+                        isLoaded = true;
+                        break;
+                }
+            }
+
+            if constexpr (load)
+            {
+                if (cellRef.mLockLevel == 0 && !cellRef.mKey.empty())
+                {
+                    cellRef.mLockLevel = UnbreakableLock;
+                    cellRef.mTrap.clear();
+                }
+            }
+        }
+    }
+}
+
 void ESM::RefNum::load(ESMReader& esm, bool wide, ESM::NAME tag)
 {
     if (wide)
@@ -26,7 +168,6 @@ void ESM::RefNum::save(ESMWriter &esm, bool wide, ESM::NAME tag) const
     }
 }
 
-
 void ESM::CellRef::load (ESMReader& esm, bool &isDeleted, bool wideRefNum)
 {
     loadId(esm, wideRefNum);
@@ -35,105 +176,12 @@ void ESM::CellRef::load (ESMReader& esm, bool &isDeleted, bool wideRefNum)
 
 void ESM::CellRef::loadId (ESMReader& esm, bool wideRefNum)
 {
-    // According to Hrnchamd, this does not belong to the actual ref. Instead, it is a marker indicating that
+    loadIdImpl<true>(esm, wideRefNum, *this);
-    // the following refs are part of a "temp refs" section. A temp ref is not being tracked by the moved references system.
-    // Its only purpose is a performance optimization for "immovable" things. We don't need this, and it's problematic anyway,
-    // because any item can theoretically be moved by a script.
-    if (esm.isNextSub ("NAM0"))
-        esm.skipHSub();
-
-    blank();
-
-    mRefNum.load (esm, wideRefNum);
-
-    mRefID = esm.getHNOString ("NAME");
-    if (mRefID.empty())
-    {
-        Log(Debug::Warning) << "Warning: got CellRef with empty RefId in " << esm.getName() << " 0x" << std::hex << esm.getFileOffset();
-    }
 }
 
 void ESM::CellRef::loadData(ESMReader &esm, bool &isDeleted)
 {
-    isDeleted = false;
+    loadDataImpl<true>(esm, isDeleted, *this);
-
-    bool isLoaded = false;
-    while (!isLoaded && esm.hasMoreSubs())
-    {
-        esm.getSubName();
-        switch (esm.retSubName().toInt())
-        {
-            case ESM::fourCC("UNAM"):
-                esm.getHT(mReferenceBlocked);
-                break;
-            case ESM::fourCC("XSCL"):
-                esm.getHT(mScale);
-                mScale = std::clamp(mScale, 0.5f, 2.0f);
-                break;
-            case ESM::fourCC("ANAM"):
-                mOwner = esm.getHString();
-                break;
-            case ESM::fourCC("BNAM"):
-                mGlobalVariable = esm.getHString();
-                break;
-            case ESM::fourCC("XSOL"):
-                mSoul = esm.getHString();
-                break;
-            case ESM::fourCC("CNAM"):
-                mFaction = esm.getHString();
-                break;
-            case ESM::fourCC("INDX"):
-                esm.getHT(mFactionRank);
-                break;
-            case ESM::fourCC("XCHG"):
-                esm.getHT(mEnchantmentCharge);
-                break;
-            case ESM::fourCC("INTV"):
-                esm.getHT(mChargeInt);
-                break;
-            case ESM::fourCC("NAM9"):
-                esm.getHT(mGoldValue);
-                break;
-            case ESM::fourCC("DODT"):
-                esm.getHT(mDoorDest);
-                mTeleport = true;
-                break;
-            case ESM::fourCC("DNAM"):
-                mDestCell = esm.getHString();
-                break;
-            case ESM::fourCC("FLTV"):
-                esm.getHT(mLockLevel);
-                break;
-            case ESM::fourCC("KNAM"):
-                mKey = esm.getHString();
-                break;
-            case ESM::fourCC("TNAM"):
-                mTrap = esm.getHString();
-                break;
-            case ESM::fourCC("DATA"):
-                esm.getHT(mPos, 24);
-                break;
-            case ESM::fourCC("NAM0"):
-            {
-                esm.skipHSub();
-                break;
-            }
-            case ESM::SREC_DELE:
-                esm.skipHSub();
-                isDeleted = true;
-                break;
-            default:
-                esm.cacheSubName();
-                isLoaded = true;
-                break;
-        }
-    }
-
-    if (mLockLevel == 0 && !mKey.empty())
-    {
-        mLockLevel = UnbreakableLock;
-        mTrap.clear();
-    }
 }
 
 void ESM::CellRef::save (ESMWriter &esm, bool wideRefNum, bool inInventory, bool isDeleted) const
@@ -227,3 +275,11 @@ void ESM::CellRef::blank()
         mPos.rot[i] = 0;
     }
 }
+
+void ESM::skipLoadCellRef(ESMReader& esm, bool wideRefNum)
+{
+    CellRef cellRef;
+    loadIdImpl<false>(esm, wideRefNum, cellRef);
+    bool isDeleted;
+    loadDataImpl<false>(esm, isDeleted, cellRef);
+}

components/esm3/cellref.hpp

@@ -116,6 +116,8 @@ namespace ESM
             void blank();
     };
 
+    void skipLoadCellRef(ESMReader& esm, bool wideRefNum = false);
+
     inline bool operator== (const RefNum& left, const RefNum& right)
     {
         return left.mIndex==right.mIndex && left.mContentFile==right.mContentFile;

components/esm3/esmreader.cpp

@@ -120,6 +120,12 @@ std::string ESMReader::getHNOString(NAME name)
     return "";
 }
 
+void ESMReader::skipHNOString(NAME name)
+{
+    if (isNextSub(name))
+        skipHString();
+}
+
 std::string ESMReader::getHNString(NAME name)
 {
     getSubNameIs(name);
@@ -147,6 +153,26 @@ std::string ESMReader::getHString()
     return getString(mCtx.leftSub);
 }
 
+void ESMReader::skipHString()
+{
+    getSubHeader();
+
+    // Hack to make MultiMark.esp load. Zero-length strings do not
+    // occur in any of the official mods, but MultiMark makes use of
+    // them. For some reason, they break the rules, and contain a byte
+    // (value 0) even if the header says there is no data. If
+    // Morrowind accepts it, so should we.
+    if (mCtx.leftSub == 0 && hasMoreSubs() && !mEsm->peek())
+    {
+        // Skip the following zero byte
+        mCtx.leftRec--;
+        skipT<char>();
+        return;
+    }
+
+    skip(mCtx.leftSub);
+}
+
 void ESMReader::getHExact(void*p, int size)
 {
     getSubHeader();

components/esm3/esmreader.hpp

@@ -140,6 +140,15 @@ public:
       getT(x);
   }
 
+  template <typename T>
+  void skipHT()
+  {
+      getSubHeader();
+      if (mCtx.leftSub != sizeof(T))
+          reportSubSizeMismatch(sizeof(T), mCtx.leftSub);
+      skipT<T>();
+  }
+
   // Version with extra size checking, to make sure the compiler
   // doesn't mess up our struct padding.
   template <typename X>
@@ -152,12 +161,16 @@ public:
   // Read a string by the given name if it is the next record.
   std::string getHNOString(NAME name);
 
+  void skipHNOString(NAME name);
+
   // Read a string with the given sub-record name
   std::string getHNString(NAME name);
 
   // Read a string, including the sub-record header (but not the name)
   std::string getHString();
 
+  void skipHString();
+
   // Read the given number of bytes from a subrecord
   void getHExact(void*p, int size);
 
@@ -237,6 +250,9 @@ public:
   template <typename X>
   void getT(X &x) { getExact(&x, sizeof(X)); }
 
+  template <typename T>
+  void skipT() { skip(sizeof(T)); }
+
   void getExact(void* x, int size) { mEsm->read((char*)x, size); }
   void getName(NAME &name) { getT(name); }
   void getUint(uint32_t &u) { getT(u); }

components/esm3/loadcell.cpp

@@ -17,7 +17,7 @@
 namespace
 {
     ///< Translate 8bit/24bit code (stored in refNum.mIndex) into a proper refNum
-    void adjustRefNum (ESM::RefNum& refNum, ESM::ESMReader& reader)
+    void adjustRefNum (ESM::RefNum& refNum, const ESM::ESMReader& reader)
     {
         unsigned int local = (refNum.mIndex & 0xff000000) >> 24;
 
@@ -271,7 +271,8 @@ namespace ESM
         return false;
     }
 
-    bool Cell::getNextRef(ESMReader& esm, CellRef& cellRef, bool& deleted, MovedCellRef& movedCellRef, bool& moved)
+    bool Cell::getNextRef(ESMReader& esm, CellRef& cellRef, bool& deleted, MovedCellRef& movedCellRef, bool& moved,
+        GetNextRefMode mode)
     {
         deleted = false;
         moved = false;
@@ -288,6 +289,13 @@ namespace ESM
         if (!esm.peekNextSub("FRMR"))
             return false;
 
+        if ((!moved && mode == GetNextRefMode::LoadOnlyMoved)
+                || (moved && mode == GetNextRefMode::LoadOnlyNotMoved))
+        {
+            skipLoadCellRef(esm);
+            return true;
+        }
+
         cellRef.load(esm, deleted);
         adjustRefNum(cellRef.mRefNum, esm);
 

components/esm3/loadcell.hpp

@@ -67,6 +67,13 @@ struct Cell
     /// Return a string descriptor for this record type. Currently used for debugging / error logs only.
     static std::string_view getRecordType() { return "Cell"; }
 
+    enum class GetNextRefMode
+    {
+        LoadAll,
+        LoadOnlyMoved,
+        LoadOnlyNotMoved,
+    };
+
   enum Flags
     {
       Interior  = 0x01, // Interior cell
@@ -183,7 +190,8 @@ struct Cell
   */
     static bool getNextRef(ESMReader& esm, CellRef& ref, bool& deleted);
 
-    static bool getNextRef(ESMReader& esm, CellRef& cellRef, bool& deleted, MovedCellRef& movedCellRef, bool& moved);
+    static bool getNextRef(ESMReader& esm, CellRef& cellRef, bool& deleted, MovedCellRef& movedCellRef, bool& moved,
+        GetNextRefMode mode = GetNextRefMode::LoadAll);
 
   /* This fetches an MVRF record, which is used to track moved references.
    * Since they are comparably rare, we use a separate method for this.