Use ESM::ReadersCache in the editor

CHANGELOG.md

@@ -167,6 +167,7 @@
     Bug #7872: Region sounds use wrong odds
     Bug #7886: Equip and unequip animations can't share the animation track section
     Bug #7887: Editor: Mismatched reported script data size and actual data size causes a crash during save
+    Bug #7896: Editor: Loading cellrefs incorrectly transforms Refnums, causing load failures
     Bug #7898: Editor: Invalid reference scales are allowed
     Bug #7899: Editor: Doors can't be unlocked
     Bug #7901: Editor: Teleport-related fields shouldn't be editable if a ref does not teleport

apps/opencs/model/doc/loader.cpp

@@ -17,13 +17,6 @@
 
 #include "document.hpp"
 
-CSMDoc::Loader::Stage::Stage()
-    : mFile(0)
-    , mRecordsLoaded(0)
-    , mRecordsLeft(false)
-{
-}
-
 CSMDoc::Loader::Loader()
     : mShouldStop(false)
 {
@@ -105,7 +98,7 @@ void CSMDoc::Loader::load()
 
         if (iter->second.mFile < size) // start loading the files
         {
-            std::filesystem::path path = document->getContentFiles()[iter->second.mFile];
+            const std::filesystem::path& path = document->getContentFiles()[iter->second.mFile];
 
             int steps = document->getData().startLoading(path, iter->second.mFile != editedIndex, /*project*/ false);
             iter->second.mRecordsLeft = true;

apps/opencs/model/doc/loader.hpp

@@ -23,11 +23,9 @@ namespace CSMDoc
 
         struct Stage
         {
-            int mFile;
+            int mFile = 0;
-            int mRecordsLoaded;
+            int mRecordsLoaded = 0;
-            bool mRecordsLeft;
+            bool mRecordsLeft = false;
-
-            Stage();
         };
 
         QMutex mMutex;

apps/opencs/model/doc/savingstages.cpp

@@ -305,9 +305,8 @@ void CSMDoc::WriteCellCollectionStage::writeReferences(
         {
             CSMWorld::CellRef refRecord = ref.get();
 
-            // Check for uninitialized content file
+            // -1 is the current file, saved indices are 1-based
-            if (!refRecord.mRefNum.hasContentFile())
+            refRecord.mRefNum.mContentFile++;
-                refRecord.mRefNum.mContentFile = 0;
 
             // recalculate the ref's cell location
             std::ostringstream stream;

apps/opencs/model/tools/mergestages.cpp

@@ -117,7 +117,7 @@ void CSMTools::MergeReferencesStage::perform(int stage, CSMDoc::Messages& messag
         ref.mOriginalCell = ref.mCell;
 
         ref.mRefNum.mIndex = mIndex[ref.mCell]++;
-        ref.mRefNum.mContentFile = 0;
+        ref.mRefNum.mContentFile = -1;
         ref.mNew = false;
 
         mState.mTarget->getData().getReferences().appendRecord(std::make_unique<CSMWorld::Record<CSMWorld::CellRef>>(

apps/opencs/model/world/data.cpp

@@ -137,9 +137,8 @@ CSMWorld::Data::Data(ToUTF8::FromType encoding, const Files::PathContainer& data
     : mEncoder(encoding)
     , mPathgrids(mCells)
     , mRefs(mCells)
-    , mReader(nullptr)
     , mDialogue(nullptr)
-    , mReaderIndex(1)
+    , mReaderIndex(0)
     , mDataPaths(dataPaths)
     , mArchives(archives)
     , mVFS(std::make_unique<VFS::Manager>())
@@ -688,8 +687,6 @@ CSMWorld::Data::~Data()
 {
     for (std::vector<QAbstractItemModel*>::iterator iter(mModels.begin()); iter != mModels.end(); ++iter)
         delete *iter;
-
-    delete mReader;
 }
 
 std::shared_ptr<Resource::ResourceSystem> CSMWorld::Data::getResourceSystem()
@@ -1068,17 +1065,13 @@ int CSMWorld::Data::startLoading(const std::filesystem::path& path, bool base, b
 {
     Log(Debug::Info) << "Loading content file " << path;
 
-    // Don't delete the Reader yet. Some record types store a reference to the Reader to handle on-demand loading
+    if (!mReaders)
-    std::shared_ptr<ESM::ESMReader> ptr(mReader);
+        mReaders.emplace();
-    mReaders.push_back(ptr);
-    mReader = nullptr;
-
     mDialogue = nullptr;
 
-    mReader = new ESM::ESMReader;
+    auto reader = mReaders->get(mReaderIndex++);
-    mReader->setEncoder(&mEncoder);
+    reader->setEncoder(&mEncoder);
-    mReader->setIndex((project || !base) ? 0 : mReaderIndex++);
+    reader->open(path);
-    mReader->open(path);
 
     mBase = base;
     mProject = project;
@@ -1087,13 +1080,13 @@ int CSMWorld::Data::startLoading(const std::filesystem::path& path, bool base, b
     {
         MetaData metaData;
         metaData.mId = ESM::RefId::stringRefId("sys::meta");
-        metaData.load(*mReader);
+        metaData.load(*reader);
 
         mMetaData.setRecord(0,
             std::make_unique<Record<MetaData>>(Record<MetaData>(RecordBase::State_ModifiedOnly, nullptr, &metaData)));
     }
 
-    return mReader->getRecordCount();
+    return reader->getRecordCount();
 }
 
 void CSMWorld::Data::loadFallbackEntries()
@@ -1140,24 +1133,17 @@ void CSMWorld::Data::loadFallbackEntries()
 
 bool CSMWorld::Data::continueLoading(CSMDoc::Messages& messages)
 {
-    if (!mReader)
+    if (mReaderIndex == 0 || !mReaders)
         throw std::logic_error("can't continue loading, because no load has been started");
+    ESM::ReadersCache::BusyItem reader = mReaders->get(mReaderIndex - 1);
+    if (!reader->isOpen())
+        throw std::logic_error("can't continue loading, because no load has been started");
+    reader->setEncoder(&mEncoder);
+    reader->setIndex(static_cast<int>(mReaderIndex - 1));
+    reader->resolveParentFileIndices(*mReaders);
 
-    if (!mReader->hasMoreRecs())
+    if (!reader->hasMoreRecs())
     {
-        if (mBase)
-        {
-            // Don't delete the Reader yet. Some record types store a reference to the Reader to handle on-demand
-            // loading. We don't store non-base reader, because everything going into modified will be fully loaded
-            // during the initial loading process.
-            std::shared_ptr<ESM::ESMReader> ptr(mReader);
-            mReaders.push_back(ptr);
-        }
-        else
-            delete mReader;
-
-        mReader = nullptr;
-
         mDialogue = nullptr;
 
         loadFallbackEntries();
@@ -1165,76 +1151,76 @@ bool CSMWorld::Data::continueLoading(CSMDoc::Messages& messages)
         return true;
     }
 
-    ESM::NAME n = mReader->getRecName();
+    ESM::NAME n = reader->getRecName();
-    mReader->getRecHeader();
+    reader->getRecHeader();
 
     bool unhandledRecord = false;
 
     switch (n.toInt())
     {
         case ESM::REC_GLOB:
-            mGlobals.load(*mReader, mBase);
+            mGlobals.load(*reader, mBase);
             break;
         case ESM::REC_GMST:
-            mGmsts.load(*mReader, mBase);
+            mGmsts.load(*reader, mBase);
             break;
         case ESM::REC_SKIL:
-            mSkills.load(*mReader, mBase);
+            mSkills.load(*reader, mBase);
             break;
         case ESM::REC_CLAS:
-            mClasses.load(*mReader, mBase);
+            mClasses.load(*reader, mBase);
             break;
         case ESM::REC_FACT:
-            mFactions.load(*mReader, mBase);
+            mFactions.load(*reader, mBase);
             break;
         case ESM::REC_RACE:
-            mRaces.load(*mReader, mBase);
+            mRaces.load(*reader, mBase);
             break;
         case ESM::REC_SOUN:
-            mSounds.load(*mReader, mBase);
+            mSounds.load(*reader, mBase);
             break;
         case ESM::REC_SCPT:
-            mScripts.load(*mReader, mBase);
+            mScripts.load(*reader, mBase);
             break;
         case ESM::REC_REGN:
-            mRegions.load(*mReader, mBase);
+            mRegions.load(*reader, mBase);
             break;
         case ESM::REC_BSGN:
-            mBirthsigns.load(*mReader, mBase);
+            mBirthsigns.load(*reader, mBase);
             break;
         case ESM::REC_SPEL:
-            mSpells.load(*mReader, mBase);
+            mSpells.load(*reader, mBase);
             break;
         case ESM::REC_ENCH:
-            mEnchantments.load(*mReader, mBase);
+            mEnchantments.load(*reader, mBase);
             break;
         case ESM::REC_BODY:
-            mBodyParts.load(*mReader, mBase);
+            mBodyParts.load(*reader, mBase);
             break;
         case ESM::REC_SNDG:
-            mSoundGens.load(*mReader, mBase);
+            mSoundGens.load(*reader, mBase);
             break;
         case ESM::REC_MGEF:
-            mMagicEffects.load(*mReader, mBase);
+            mMagicEffects.load(*reader, mBase);
             break;
         case ESM::REC_PGRD:
-            mPathgrids.load(*mReader, mBase);
+            mPathgrids.load(*reader, mBase);
             break;
         case ESM::REC_SSCR:
-            mStartScripts.load(*mReader, mBase);
+            mStartScripts.load(*reader, mBase);
             break;
 
         case ESM::REC_LTEX:
-            mLandTextures.load(*mReader, mBase);
+            mLandTextures.load(*reader, mBase);
             break;
 
         case ESM::REC_LAND:
-            mLand.load(*mReader, mBase);
+            mLand.load(*reader, mBase);
             break;
 
         case ESM::REC_CELL:
         {
-            int index = mCells.load(*mReader, mBase);
+            int index = mCells.load(*reader, mBase);
             if (index < 0 || index >= mCells.getSize())
             {
                 // log an error and continue loading the refs to the last loaded cell
@@ -1243,69 +1229,69 @@ bool CSMWorld::Data::continueLoading(CSMDoc::Messages& messages)
                 index = mCells.getSize() - 1;
             }
 
-            mRefs.load(*mReader, index, mBase, mRefLoadCache[mCells.getId(index)], messages);
+            mRefs.load(*reader, index, mBase, mRefLoadCache[mCells.getId(index)], messages);
             break;
         }
 
         case ESM::REC_ACTI:
-            mReferenceables.load(*mReader, mBase, UniversalId::Type_Activator);
+            mReferenceables.load(*reader, mBase, UniversalId::Type_Activator);
             break;
         case ESM::REC_ALCH:
-            mReferenceables.load(*mReader, mBase, UniversalId::Type_Potion);
+            mReferenceables.load(*reader, mBase, UniversalId::Type_Potion);
             break;
         case ESM::REC_APPA:
-            mReferenceables.load(*mReader, mBase, UniversalId::Type_Apparatus);
+            mReferenceables.load(*reader, mBase, UniversalId::Type_Apparatus);
             break;
         case ESM::REC_ARMO:
-            mReferenceables.load(*mReader, mBase, UniversalId::Type_Armor);
+            mReferenceables.load(*reader, mBase, UniversalId::Type_Armor);
             break;
         case ESM::REC_BOOK:
-            mReferenceables.load(*mReader, mBase, UniversalId::Type_Book);
+            mReferenceables.load(*reader, mBase, UniversalId::Type_Book);
             break;
         case ESM::REC_CLOT:
-            mReferenceables.load(*mReader, mBase, UniversalId::Type_Clothing);
+            mReferenceables.load(*reader, mBase, UniversalId::Type_Clothing);
             break;
         case ESM::REC_CONT:
-            mReferenceables.load(*mReader, mBase, UniversalId::Type_Container);
+            mReferenceables.load(*reader, mBase, UniversalId::Type_Container);
             break;
         case ESM::REC_CREA:
-            mReferenceables.load(*mReader, mBase, UniversalId::Type_Creature);
+            mReferenceables.load(*reader, mBase, UniversalId::Type_Creature);
             break;
         case ESM::REC_DOOR:
-            mReferenceables.load(*mReader, mBase, UniversalId::Type_Door);
+            mReferenceables.load(*reader, mBase, UniversalId::Type_Door);
             break;
         case ESM::REC_INGR:
-            mReferenceables.load(*mReader, mBase, UniversalId::Type_Ingredient);
+            mReferenceables.load(*reader, mBase, UniversalId::Type_Ingredient);
             break;
         case ESM::REC_LEVC:
-            mReferenceables.load(*mReader, mBase, UniversalId::Type_CreatureLevelledList);
+            mReferenceables.load(*reader, mBase, UniversalId::Type_CreatureLevelledList);
             break;
         case ESM::REC_LEVI:
-            mReferenceables.load(*mReader, mBase, UniversalId::Type_ItemLevelledList);
+            mReferenceables.load(*reader, mBase, UniversalId::Type_ItemLevelledList);
             break;
         case ESM::REC_LIGH:
-            mReferenceables.load(*mReader, mBase, UniversalId::Type_Light);
+            mReferenceables.load(*reader, mBase, UniversalId::Type_Light);
             break;
         case ESM::REC_LOCK:
-            mReferenceables.load(*mReader, mBase, UniversalId::Type_Lockpick);
+            mReferenceables.load(*reader, mBase, UniversalId::Type_Lockpick);
             break;
         case ESM::REC_MISC:
-            mReferenceables.load(*mReader, mBase, UniversalId::Type_Miscellaneous);
+            mReferenceables.load(*reader, mBase, UniversalId::Type_Miscellaneous);
             break;
         case ESM::REC_NPC_:
-            mReferenceables.load(*mReader, mBase, UniversalId::Type_Npc);
+            mReferenceables.load(*reader, mBase, UniversalId::Type_Npc);
             break;
         case ESM::REC_PROB:
-            mReferenceables.load(*mReader, mBase, UniversalId::Type_Probe);
+            mReferenceables.load(*reader, mBase, UniversalId::Type_Probe);
             break;
         case ESM::REC_REPA:
-            mReferenceables.load(*mReader, mBase, UniversalId::Type_Repair);
+            mReferenceables.load(*reader, mBase, UniversalId::Type_Repair);
             break;
         case ESM::REC_STAT:
-            mReferenceables.load(*mReader, mBase, UniversalId::Type_Static);
+            mReferenceables.load(*reader, mBase, UniversalId::Type_Static);
             break;
         case ESM::REC_WEAP:
-            mReferenceables.load(*mReader, mBase, UniversalId::Type_Weapon);
+            mReferenceables.load(*reader, mBase, UniversalId::Type_Weapon);
             break;
 
         case ESM::REC_DIAL:
@@ -1313,7 +1299,7 @@ bool CSMWorld::Data::continueLoading(CSMDoc::Messages& messages)
             ESM::Dialogue record;
             bool isDeleted = false;
 
-            record.load(*mReader, isDeleted);
+            record.load(*reader, isDeleted);
 
             if (isDeleted)
             {
@@ -1359,14 +1345,14 @@ bool CSMWorld::Data::continueLoading(CSMDoc::Messages& messages)
                 messages.add(UniversalId::Type_None, "Found info record not following a dialogue record", "",
                     CSMDoc::Message::Severity_Error);
 
-                mReader->skipRecord();
+                reader->skipRecord();
                 break;
             }
 
             if (mDialogue->mType == ESM::Dialogue::Journal)
-                mJournalInfos.load(*mReader, mBase, *mDialogue, mJournalInfoOrder);
+                mJournalInfos.load(*reader, mBase, *mDialogue, mJournalInfoOrder);
             else
-                mTopicInfos.load(*mReader, mBase, *mDialogue, mTopicInfoOrder);
+                mTopicInfos.load(*reader, mBase, *mDialogue, mTopicInfoOrder);
 
             break;
         }
@@ -1379,7 +1365,7 @@ bool CSMWorld::Data::continueLoading(CSMDoc::Messages& messages)
                 break;
             }
 
-            mFilters.load(*mReader, mBase);
+            mFilters.load(*reader, mBase);
             break;
 
         case ESM::REC_DBGP:
@@ -1390,7 +1376,7 @@ bool CSMWorld::Data::continueLoading(CSMDoc::Messages& messages)
                 break;
             }
 
-            mDebugProfiles.load(*mReader, mBase);
+            mDebugProfiles.load(*reader, mBase);
             break;
 
         case ESM::REC_SELG:
@@ -1401,7 +1387,7 @@ bool CSMWorld::Data::continueLoading(CSMDoc::Messages& messages)
                 break;
             }
 
-            mSelectionGroups.load(*mReader, mBase);
+            mSelectionGroups.load(*reader, mBase);
             break;
 
         default:
@@ -1414,7 +1400,7 @@ bool CSMWorld::Data::continueLoading(CSMDoc::Messages& messages)
         messages.add(
             UniversalId::Type_None, "Unsupported record type: " + n.toString(), "", CSMDoc::Message::Severity_Error);
 
-        mReader->skipRecord();
+        reader->skipRecord();
     }
 
     return false;
@@ -1424,6 +1410,8 @@ void CSMWorld::Data::finishLoading()
 {
     mTopicInfos.sort(mTopicInfoOrder);
     mJournalInfos.sort(mJournalInfoOrder);
+    // Release file locks so we can actually write to the file we're editing
+    mReaders.reset();
 }
 
 bool CSMWorld::Data::hasId(const std::string& id) const

apps/opencs/model/world/data.hpp

@@ -33,6 +33,7 @@
 #include <components/esm3/loadsoun.hpp>
 #include <components/esm3/loadspel.hpp>
 #include <components/esm3/loadsscr.hpp>
+#include <components/esm3/readerscache.hpp>
 #include <components/esm3/selectiongroup.hpp>
 #include <components/files/multidircollection.hpp>
 #include <components/misc/algorithm.hpp>
@@ -122,12 +123,12 @@ namespace CSMWorld
         std::unique_ptr<ActorAdapter> mActorAdapter;
         std::vector<QAbstractItemModel*> mModels;
         std::map<UniversalId::Type, QAbstractItemModel*> mModelIndex;
-        ESM::ESMReader* mReader;
+        std::optional<ESM::ReadersCache> mReaders;
         const ESM::Dialogue* mDialogue; // last loaded dialogue
         bool mBase;
         bool mProject;
-        std::map<ESM::RefId, std::map<unsigned int, unsigned int>> mRefLoadCache;
+        std::map<ESM::RefId, std::map<ESM::RefNum, unsigned int>> mRefLoadCache;
-        int mReaderIndex;
+        std::size_t mReaderIndex;
 
         Files::PathContainer mDataPaths;
         std::vector<std::string> mArchives;
@@ -135,14 +136,11 @@ namespace CSMWorld
         ResourcesManager mResourcesManager;
         std::shared_ptr<Resource::ResourceSystem> mResourceSystem;
 
-        std::vector<std::shared_ptr<ESM::ESMReader>> mReaders;
-
         InfoOrderByTopic mJournalInfoOrder;
         InfoOrderByTopic mTopicInfoOrder;
 
-        // not implemented
+        Data(const Data&) = delete;
-        Data(const Data&);
+        Data& operator=(const Data&) = delete;
-        Data& operator=(const Data&);
 
         void addModel(QAbstractItemModel* model, UniversalId::Type type, bool update = true);
 

apps/opencs/model/world/ref.cpp

@@ -8,8 +8,6 @@ CSMWorld::CellRef::CellRef()
     : mNew(true)
     , mIdNum(0)
 {
-    mRefNum.mIndex = 0;
-    mRefNum.mContentFile = 0;
 }
 
 std::pair<int, int> CSMWorld::CellRef::getCellIndex() const

apps/opencs/model/world/refcollection.cpp

@@ -8,6 +8,7 @@
 #include <apps/opencs/model/world/collection.hpp>
 
 #include <components/esm3/cellref.hpp>
+#include <components/esm3/esmreader.hpp>
 #include <components/esm3/loadcell.hpp>
 #include <components/misc/strings/conversion.hpp>
 
@@ -47,7 +48,7 @@ namespace CSMWorld
 }
 
 void CSMWorld::RefCollection::load(ESM::ESMReader& reader, int cellIndex, bool base,
-    std::map<unsigned int, unsigned int>& cache, CSMDoc::Messages& messages)
+    std::map<ESM::RefNum, unsigned int>& cache, CSMDoc::Messages& messages)
 {
     Record<Cell> cell = mCells.getRecord(cellIndex);
 
@@ -64,6 +65,8 @@ void CSMWorld::RefCollection::load(ESM::ESMReader& reader, int cellIndex, bool b
 
         if (!ESM::Cell::getNextRef(reader, ref, isDeleted, mref, isMoved))
             break;
+        if (!base && reader.getIndex() == ref.mRefNum.mContentFile)
+            ref.mRefNum.mContentFile = -1;
         // Keep mOriginalCell empty when in modified (as an indicator that the
         // original cell will always be equal the current cell).
         ref.mOriginalCell = base ? cell2.mId : ESM::RefId();
@@ -102,16 +105,7 @@ void CSMWorld::RefCollection::load(ESM::ESMReader& reader, int cellIndex, bool b
         else
             ref.mCell = cell2.mId;
 
-        if (ref.mRefNum.mContentFile != -1 && !base)
+        auto iter = cache.find(ref.mRefNum);
-        {
-            ref.mRefNum.mContentFile = ref.mRefNum.mIndex >> 24;
-            ref.mRefNum.mIndex &= 0x00ffffff;
-        }
-
-        unsigned int refNum = (ref.mRefNum.mIndex & 0x00ffffff)
-            | (ref.mRefNum.hasContentFile() ? ref.mRefNum.mContentFile : 0xff) << 24;
-
-        std::map<unsigned int, unsigned int>::iterator iter = cache.find(refNum);
 
         if (isMoved)
         {
@@ -181,7 +175,7 @@ void CSMWorld::RefCollection::load(ESM::ESMReader& reader, int cellIndex, bool b
             ref.mIdNum = mNextId; // FIXME: fragile
             ref.mId = ESM::RefId::stringRefId(getNewId());
 
-            cache.emplace(refNum, ref.mIdNum);
+            cache.emplace(ref.mRefNum, ref.mIdNum);
 
             auto record = std::make_unique<Record<CellRef>>();
             record->mState = base ? RecordBase::State_BaseOnly : RecordBase::State_ModifiedOnly;
@@ -305,10 +299,10 @@ void CSMWorld::RefCollection::cloneRecord(
     copy->get().mId = destination;
     copy->get().mIdNum = extractIdNum(destination.getRefIdString());
 
-    if (copy->get().mRefNum.mContentFile != 0)
+    if (copy->get().mRefNum.hasContentFile())
     {
         mRefIndex.insert(std::make_pair(static_cast<Record<CellRef>*>(copy.get())->get().mIdNum, index));
-        copy->get().mRefNum.mContentFile = 0;
+        copy->get().mRefNum.mContentFile = -1;
         copy->get().mRefNum.mIndex = index;
     }
     else

apps/opencs/model/world/refcollection.hpp

@@ -55,7 +55,7 @@ namespace CSMWorld
         {
         }
 
-        void load(ESM::ESMReader& reader, int cellIndex, bool base, std::map<unsigned int, unsigned int>& cache,
+        void load(ESM::ESMReader& reader, int cellIndex, bool base, std::map<ESM::RefNum, unsigned int>& cache,
             CSMDoc::Messages& messages);
         ///< Load a sequence of references.