refactors parentFileIndices (#3211)

This PR aims to start addressing `ESM` design issues that have silenced errors we incorporated into groundcover `ESM` loading approaches.

- We move the resolution of `parentFileIndices` from `ESMStore` to `ESMReader` as suggested in a `TODO` comment.
- We improve a highly misleading comment which downplayed the significance of `parentFileIndices`.
- We document important preconditions.
- We move a user facing error message to the highest level and improve its context.
- We remove an inappropriate `setGlobalReaderList` method. We now pass this reader list into the method that requires it.
- We remove a thoroughly pointless optimisation of `Store<ESM::LandTexture>`'s construction that has unnecessarily depended on `getGlobalReaderList`.

There should be no functional changes for `master`, but this PR should remove an issue blocking PR #3208.

apps/openmw/mwworld/esmloader.cpp

@@ -42,7 +42,7 @@ void EsmLoader::load(const boost::filesystem::path& filepath, int& index)
   ESM::ESMReader lEsm;
   lEsm.setEncoder(mEncoder);
   lEsm.setIndex(index);
-  lEsm.setGlobalReaderList(&mEsm);
+  lEsm.resolveParentFileIndices(mEsm);
   lEsm.open(filepath.string());
   mEsm[index] = lEsm;
   mStore.load(mEsm[index], &mListener);

apps/openmw/mwworld/esmstore.cpp

@@ -4,8 +4,6 @@
 #include <fstream>
 #include <set>
 
-#include <boost/filesystem/operations.hpp>
-
 #include <components/debug/debuglog.hpp>
 #include <components/esm/esmreader.hpp>
 #include <components/esm/esmwriter.hpp>
@@ -153,41 +151,9 @@ void ESMStore::load(ESM::ESMReader &esm, Loading::Listener* listener)
     ESM::Dialogue *dialogue = nullptr;
 
     // Land texture loading needs to use a separate internal store for each plugin.
-    // We set the number of plugins here to avoid continual resizes during loading,
-    // and so we can properly verify if valid plugin indices are being passed to the
-    // LandTexture Store retrieval methods.
-    mLandTextures.resize(esm.getGlobalReaderList()->size());
-
-    /// \todo Move this to somewhere else. ESMReader?
-    // Cache parent esX files by tracking their indices in the global list of
-    //  all files/readers used by the engine. This will greaty accelerate
-    //  refnumber mangling, as required for handling moved references.
-    const std::vector<ESM::Header::MasterData> &masters = esm.getGameFiles();
-    std::vector<ESM::ESMReader> *allPlugins = esm.getGlobalReaderList();
-    for (size_t j = 0; j < masters.size(); j++) {
-        const ESM::Header::MasterData &mast = masters[j];
-        std::string fname = mast.name;
-        int index = ~0;
-        for (int i = 0; i < esm.getIndex(); i++) {
-            ESM::ESMReader& reader = allPlugins->at(i);
-            if (reader.getFileSize() == 0)
-                continue;  // Content file in non-ESM format
-            const std::string candidate = reader.getContext().filename;
-            std::string fnamecandidate = boost::filesystem::path(candidate).filename().string();
-            if (Misc::StringUtils::ciEqual(fname, fnamecandidate)) {
-                index = i;
-                break;
-            }
-        }
-        if (index == (int)~0) {
-            // Tried to load a parent file that has not been loaded yet. This is bad,
-            //  the launcher should have taken care of this.
-            std::string fstring = "File " + esm.getName() + " asks for parent file " + masters[j].name
-                + ", but it has not been loaded yet. Please check your load order.";
-            esm.fail(fstring);
-        }
-        esm.addParentFileIndex(index);
-    }
+    // We set the number of plugins here so we can properly verify if valid plugin
+    // indices are being passed to the LandTexture Store retrieval methods.
+    mLandTextures.resize(mLandTextures.getSize()+1);
 
     // Loop through all records
     while(esm.hasMoreRecs())

apps/openmw/mwworld/store.cpp

@@ -293,11 +293,6 @@ namespace MWWorld
     //=========================================================================
     Store<ESM::LandTexture>::Store()
     {
-        mStatic.emplace_back();
-        LandTextureList &ltexl = mStatic[0];
-        // More than enough to hold Morrowind.esm. Extra lists for plugins will we
-        //  added on-the-fly in a different method.
-        ltexl.reserve(128);
     }
     const ESM::LandTexture *Store<ESM::LandTexture>::search(size_t index, size_t plugin) const
     {

apps/openmw/mwworld/worldimp.cpp

@@ -178,6 +178,10 @@ namespace MWWorld
         if (mEsm[0].getFormat() == 0)
             ensureNeededRecords();
 
+        // TODO: We can and should validate before we call loadContentFiles().
+        // Currently we validate here to prevent merge conflicts with groundcover ESMStore fixes.
+        validateMasterFiles(mEsm);
+
         mCurrentDate.reset(new DateTimeManager());
 
         fillGlobalVariables();
@@ -407,6 +411,23 @@ namespace MWWorld
         }
     }
 
+    void World::validateMasterFiles(const std::vector<ESM::ESMReader>& readers)
+    {
+        for (const auto& esm : readers)
+        {
+            assert(esm.getGameFiles().size() == esm.getParentFileIndices().size());
+            for (unsigned int i=0; i<esm.getParentFileIndices().size(); ++i)
+            {
+                if (!esm.isValidParentFileIndex(i))
+                {
+                    std::string fstring = "File " + esm.getName() + " asks for parent file " + esm.getGameFiles()[i].name
+                        + ", but it is not available or has been loaded in the wrong order. Please run the launcher to fix this issue.";
+                    throw std::runtime_error(fstring);
+                }
+            }
+        }
+    }
+
     void World::ensureNeededRecords()
     {
         std::map<std::string, ESM::Variant> gmst;

apps/openmw/mwworld/worldimp.hpp

@@ -157,6 +157,7 @@ namespace MWWorld
             void updateNavigatorObject(const MWPhysics::Object& object);
 
             void ensureNeededRecords();
+            void validateMasterFiles(const std::vector<ESM::ESMReader>& readers);
 
             void fillGlobalVariables();
 

apps/openmw_test_suite/mwworld/test_store.cpp

@@ -29,19 +29,14 @@ struct ContentFileTest : public ::testing::Test
         readContentFiles();
 
         // load the content files
-        std::vector<ESM::ESMReader> readerList;
-        readerList.resize(mContentFiles.size());
-
         int index=0;
         for (const auto & mContentFile : mContentFiles)
         {
             ESM::ESMReader lEsm;
             lEsm.setEncoder(nullptr);
             lEsm.setIndex(index);
-            lEsm.setGlobalReaderList(&readerList);
             lEsm.open(mContentFile.string());
-            readerList[index] = lEsm;
-            mEsmStore.load(readerList[index], &dummyListener);
+            mEsmStore.load(lEsm, &dummyListener);
 
             ++index;
         }
@@ -254,9 +249,6 @@ TEST_F(StoreTest, delete_test)
     record.mId = recordId;
 
     ESM::ESMReader reader;
-    std::vector<ESM::ESMReader> readerList;
-    readerList.push_back(reader);
-    reader.setGlobalReaderList(&readerList);
 
     // master file inserts a record
     Files::IStreamPtr file = getEsmFile(record, false);
@@ -297,9 +289,6 @@ TEST_F(StoreTest, overwrite_test)
     record.mId = recordId;
 
     ESM::ESMReader reader;
-    std::vector<ESM::ESMReader> readerList;
-    readerList.push_back(reader);
-    reader.setGlobalReaderList(&readerList);
 
     // master file inserts a record
     Files::IStreamPtr file = getEsmFile(record, false);

components/esm/esmreader.cpp

@@ -1,5 +1,8 @@
 #include "esmreader.hpp"
 
+#include <boost/filesystem/path.hpp>
+#include <components/misc/stringops.hpp>
+
 #include <stdexcept>
 
 namespace ESM
@@ -17,7 +20,6 @@ ESM_Context ESMReader::getContext()
 ESMReader::ESMReader()
     : mRecordFlags(0)
     , mBuffer(50*1024)
-    , mGlobalReaderList(nullptr)
     , mEncoder(nullptr)
     , mFileSize(0)
 {
@@ -55,6 +57,29 @@ void ESMReader::clearCtx()
    mCtx.subName.clear();
 }
 
+void ESMReader::resolveParentFileIndices(const std::vector<ESMReader>& allPlugins)
+{
+    mCtx.parentFileIndices.clear();
+    const std::vector<Header::MasterData> &masters = getGameFiles();
+    for (size_t j = 0; j < masters.size(); j++) {
+        const Header::MasterData &mast = masters[j];
+        std::string fname = mast.name;
+        int index = getIndex(); 
+        for (int i = 0; i < getIndex(); i++) {
+            const ESMReader& reader = allPlugins.at(i);
+            if (reader.getFileSize() == 0)
+                continue;  // Content file in non-ESM format
+            const std::string candidate = reader.getName();
+            std::string fnamecandidate = boost::filesystem::path(candidate).filename().string();
+            if (Misc::StringUtils::ciEqual(fname, fnamecandidate)) {
+                index = i;
+                break;
+            }
+        }
+        mCtx.parentFileIndices.push_back(index);
+    }
+}
+
 void ESMReader::openRaw(Files::IStreamPtr _esm, const std::string& name)
 {
     close();

components/esm/esmreader.hpp

@@ -80,13 +80,15 @@ public:
   //  to the individual load() methods. This hack allows to pass this reference
   //  indirectly to the load() method.
   void setIndex(const int index) { mCtx.index = index;}
-  int getIndex() {return mCtx.index;}
+  int getIndex() const {return mCtx.index;}
 
-  void setGlobalReaderList(std::vector<ESMReader> *list) {mGlobalReaderList = list;}
-  std::vector<ESMReader> *getGlobalReaderList() {return mGlobalReaderList;}
-
-  void addParentFileIndex(int index) { mCtx.parentFileIndices.push_back(index); }
+  // Assign parent esX files by tracking their indices in the global list of
+  // all files/readers used by the engine. This is required for correct adjustRefNum() results
+  // as required for handling moved, deleted and edited CellRefs.
+  /// @note Does not validate.
+  void resolveParentFileIndices(const std::vector<ESMReader>& files);
   const std::vector<int>& getParentFileIndices() const { return mCtx.parentFileIndices; }
+  bool isValidParentFileIndex(int i) const { return i != getIndex(); }
 
   /*************************************************************************
    *
@@ -279,7 +281,6 @@ private:
 
   Header mHeader;
 
-  std::vector<ESMReader> *mGlobalReaderList;
   ToUTF8::Utf8Encoder* mEncoder;
 
   size_t mFileSize;

components/esmloader/load.cpp

@@ -214,7 +214,6 @@ namespace EsmLoader
                 ESM::ESMReader& reader = readers[i];
                 reader.setEncoder(encoder);
                 reader.setIndex(static_cast<int>(i));
-                reader.setGlobalReaderList(&readers);
                 reader.open(collection.getPath(file).string());
 
                 loadEsm(query, readers[i], result);