From 4657c655b10621f2be75a6fc60ea6dcb721d3727 Mon Sep 17 00:00:00 2001 From: Bo Svensson <90132211+bosvensson1@users.noreply.github.com> Date: Wed, 3 Nov 2021 08:02:06 +0000 Subject: [PATCH] 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`'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 | 2 +- apps/openmw/mwworld/esmstore.cpp | 40 ++----------------- apps/openmw/mwworld/store.cpp | 5 --- apps/openmw/mwworld/worldimp.cpp | 21 ++++++++++ apps/openmw/mwworld/worldimp.hpp | 1 + apps/openmw_test_suite/mwworld/test_store.cpp | 13 +----- components/esm/esmreader.cpp | 27 ++++++++++++- components/esm/esmreader.hpp | 13 +++--- components/esmloader/load.cpp | 1 - 9 files changed, 60 insertions(+), 63 deletions(-) diff --git a/apps/openmw/mwworld/esmloader.cpp b/apps/openmw/mwworld/esmloader.cpp index 06debf4a97..647f5d3609 100644 --- a/apps/openmw/mwworld/esmloader.cpp +++ b/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); diff --git a/apps/openmw/mwworld/esmstore.cpp b/apps/openmw/mwworld/esmstore.cpp index 5ec95ecf47..183b13ca09 100644 --- a/apps/openmw/mwworld/esmstore.cpp +++ b/apps/openmw/mwworld/esmstore.cpp @@ -4,8 +4,6 @@ #include #include -#include - #include #include #include @@ -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 &masters = esm.getGameFiles(); - std::vector *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()) diff --git a/apps/openmw/mwworld/store.cpp b/apps/openmw/mwworld/store.cpp index 32458a2e84..06ea97a859 100644 --- a/apps/openmw/mwworld/store.cpp +++ b/apps/openmw/mwworld/store.cpp @@ -293,11 +293,6 @@ namespace MWWorld //========================================================================= Store::Store() { - mStatic.emplace_back(); - LandTextureList <exl = 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::search(size_t index, size_t plugin) const { diff --git a/apps/openmw/mwworld/worldimp.cpp b/apps/openmw/mwworld/worldimp.cpp index 34907691e9..d35f72e80c 100644 --- a/apps/openmw/mwworld/worldimp.cpp +++ b/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& readers) + { + for (const auto& esm : readers) + { + assert(esm.getGameFiles().size() == esm.getParentFileIndices().size()); + for (unsigned int i=0; i gmst; diff --git a/apps/openmw/mwworld/worldimp.hpp b/apps/openmw/mwworld/worldimp.hpp index a795b4eafd..afad359cfd 100644 --- a/apps/openmw/mwworld/worldimp.hpp +++ b/apps/openmw/mwworld/worldimp.hpp @@ -157,6 +157,7 @@ namespace MWWorld void updateNavigatorObject(const MWPhysics::Object& object); void ensureNeededRecords(); + void validateMasterFiles(const std::vector& readers); void fillGlobalVariables(); diff --git a/apps/openmw_test_suite/mwworld/test_store.cpp b/apps/openmw_test_suite/mwworld/test_store.cpp index 3fe479587c..29240a1f7f 100644 --- a/apps/openmw_test_suite/mwworld/test_store.cpp +++ b/apps/openmw_test_suite/mwworld/test_store.cpp @@ -29,19 +29,14 @@ struct ContentFileTest : public ::testing::Test readContentFiles(); // load the content files - std::vector 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 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 readerList; - readerList.push_back(reader); - reader.setGlobalReaderList(&readerList); // master file inserts a record Files::IStreamPtr file = getEsmFile(record, false); diff --git a/components/esm/esmreader.cpp b/components/esm/esmreader.cpp index dbf713315b..316748b53a 100644 --- a/components/esm/esmreader.cpp +++ b/components/esm/esmreader.cpp @@ -1,5 +1,8 @@ #include "esmreader.hpp" +#include +#include + #include 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& allPlugins) +{ + mCtx.parentFileIndices.clear(); + const std::vector &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(); diff --git a/components/esm/esmreader.hpp b/components/esm/esmreader.hpp index a438dca0cd..92f2a6673b 100644 --- a/components/esm/esmreader.hpp +++ b/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 *list) {mGlobalReaderList = list;} - std::vector *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& files); const std::vector& getParentFileIndices() const { return mCtx.parentFileIndices; } + bool isValidParentFileIndex(int i) const { return i != getIndex(); } /************************************************************************* * @@ -279,7 +281,6 @@ private: Header mHeader; - std::vector *mGlobalReaderList; ToUTF8::Utf8Encoder* mEncoder; size_t mFileSize; diff --git a/components/esmloader/load.cpp b/components/esmloader/load.cpp index 789e7619b6..9879f33274 100644 --- a/components/esmloader/load.cpp +++ b/components/esmloader/load.cpp @@ -214,7 +214,6 @@ namespace EsmLoader ESM::ESMReader& reader = readers[i]; reader.setEncoder(encoder); reader.setIndex(static_cast(i)); - reader.setGlobalReaderList(&readers); reader.open(collection.getPath(file).string()); loadEsm(query, readers[i], result);