Merge branch 'fix_esmstore_double_initialization' into 'master'

Fix the bug "ESMStore::setUp() is called twice" that causes duplicated objects in ESM4 cells

See merge request OpenMW/openmw!3084

apps/openmw/mwworld/esmstore.cpp

@@ -248,6 +248,7 @@ namespace MWWorld
     {
         for (const auto& store : mDynamicStores)
             store->clearDynamic();
+        mStoreImp->mIds = mStoreImp->mStaticIds;
 
         movePlayerRecord();
     }
@@ -426,32 +427,36 @@ namespace MWWorld
 
     void ESMStore::setUp()
     {
-        mStoreImp->mIds.clear();
+        if (mIsSetUpDone)
+            throw std::logic_error("ESMStore::setUp() is called twice");
+        mIsSetUpDone = true;
 
-        std::map<ESM::RecNameInts, DynamicStore*>::iterator storeIt = mStoreImp->mRecNameToStore.begin();
+        for (const auto& [_, store] : mStoreImp->mRecNameToStore)
-        for (; storeIt != mStoreImp->mRecNameToStore.end(); ++storeIt)
+            store->setUp();
-        {
-            storeIt->second->setUp();
-
-            if (isCacheableRecord(storeIt->first))
-            {
-                std::vector<ESM::RefId> identifiers;
-                storeIt->second->listIdentifier(identifiers);
-
-                for (auto& record : identifiers)
-                    mStoreImp->mIds[record] = storeIt->first;
-            }
-        }
-
-        if (mStoreImp->mStaticIds.empty())
-            for (const auto& [k, v] : mStoreImp->mIds)
-                mStoreImp->mStaticIds.emplace(k, v);
 
         getWritable<ESM::Skill>().setUp(get<ESM::GameSetting>());
         getWritable<ESM::MagicEffect>().setUp();
         getWritable<ESM::Attribute>().setUp(get<ESM::GameSetting>());
         getWritable<ESM4::Land>().updateLandPositions(get<ESM4::Cell>());
         getWritable<ESM4::Reference>().preprocessReferences(get<ESM4::Cell>());
+
+        rebuildIdsIndex();
+        mStoreImp->mStaticIds = mStoreImp->mIds;
+    }
+
+    void ESMStore::rebuildIdsIndex()
+    {
+        mStoreImp->mIds.clear();
+        for (const auto& [recordType, store] : mStoreImp->mRecNameToStore)
+        {
+            if (isCacheableRecord(recordType))
+            {
+                std::vector<ESM::RefId> identifiers;
+                store->listIdentifier(identifiers);
+                for (auto& record : identifiers)
+                    mStoreImp->mIds[record] = recordType;
+            }
+        }
     }
 
     void ESMStore::validateRecords(ESM::ReadersCache& readers)
@@ -704,8 +709,6 @@ namespace MWWorld
 
     void ESMStore::checkPlayer()
     {
-        setUp();
-
         const ESM::NPC* player = get<ESM::NPC>().find(ESM::RefId::stringRefId("Player"));
 
         if (!get<ESM::Race>().find(player->mRace) || !get<ESM::Class>().find(player->mClass))

apps/openmw/mwworld/esmstore.hpp

@@ -165,6 +165,8 @@ namespace MWWorld
             std::filesystem::path>; // path to an omwscripts file
         std::vector<LuaContent> mLuaContent;
 
+        bool mIsSetUpDone = false;
+
     public:
         void addOMWScripts(std::filesystem::path filePath) { mLuaContent.push_back(std::move(filePath)); }
         ESM::LuaScriptsCfg getLuaScriptsCfg() const;
@@ -185,6 +187,7 @@ namespace MWWorld
         ~ESMStore();
 
         void clearDynamic();
+        void rebuildIdsIndex();
 
         void movePlayerRecord();
 

apps/openmw/mwworld/store.cpp

@@ -488,7 +488,7 @@ namespace MWWorld
     {
         // The land is static for given game session, there is no need to refresh it every load.
         if (mBuilt)
-            return;
+            throw std::logic_error("Store<ESM::Land>::setUp() is called twice");
 
         mBuilt = true;
     }

apps/openmw/mwworld/worldimp.cpp

@@ -2284,6 +2284,7 @@ namespace MWWorld
 
     void World::saveLoaded()
     {
+        mStore.rebuildIdsIndex();
         mStore.validateDynamic();
         mCurrentDate->setup(mGlobalVariables);
     }

apps/openmw_test_suite/mwworld/test_store.cpp

@@ -325,22 +325,31 @@ TYPED_TEST_P(StoreTest, delete_test)
 
         ESM::ESMReader reader;
         ESM::Dialogue* dialogue = nullptr;
-        MWWorld::ESMStore esmStore;
 
-        // master file inserts a record
+        {
+            MWWorld::ESMStore esmStore;
             reader.open(getEsmFile(record, false, formatVersion), "filename");
-        esmStore.load(reader, &dummyListener, dialogue);
+            esmStore.load(reader, &dummyListener, dialogue); // master file inserts a record
             esmStore.setUp();
 
             EXPECT_EQ(esmStore.get<RecordType>().getSize(), 1);
-
+        }
-        // now a plugin deletes it
+        {
+            MWWorld::ESMStore esmStore;
+            reader.open(getEsmFile(record, false, formatVersion), "filename");
+            esmStore.load(reader, &dummyListener, dialogue); // master file inserts a record
             reader.open(getEsmFile(record, true, formatVersion), "filename");
-        esmStore.load(reader, &dummyListener, dialogue);
+            esmStore.load(reader, &dummyListener, dialogue); // now a plugin deletes it
             esmStore.setUp();
 
             EXPECT_EQ(esmStore.get<RecordType>().getSize(), 0);
-
+        }
+        {
+            MWWorld::ESMStore esmStore;
+            reader.open(getEsmFile(record, false, formatVersion), "filename");
+            esmStore.load(reader, &dummyListener, dialogue); // master file inserts a record
+            reader.open(getEsmFile(record, true, formatVersion), "filename");
+            esmStore.load(reader, &dummyListener, dialogue); // now a plugin deletes it
             // now another plugin inserts it again
             // expected behaviour is the record to reappear rather than staying deleted
             reader.open(getEsmFile(record, false, formatVersion), "filename");
@@ -349,6 +358,7 @@ TYPED_TEST_P(StoreTest, delete_test)
 
             EXPECT_EQ(esmStore.get<RecordType>().getSize(), 1);
         }
+    }
 }
 
 template <typename T>
@@ -410,13 +420,13 @@ TYPED_TEST_P(StoreTest, overwrite_test)
         // master file inserts a record
         reader.open(getEsmFile(record, false, formatVersion), "filename");
         esmStore.load(reader, &dummyListener, dialogue);
-        esmStore.setUp();
 
         // now a plugin overwrites it with changed data
         record.mId = recordIdUpper; // change id to uppercase, to test case smashing while we're at it
         record.mModel = "the_new_model";
         reader.open(getEsmFile(record, false, formatVersion), "filename");
         esmStore.load(reader, &dummyListener, dialogue);
+
         esmStore.setUp();
 
         // verify that changes were actually applied