Merge ESM::Cell fields by subrecord

Fixes an issue with the Morrowind Patched mod where cell ambient values would become black due to the new cell records not including an AMBI subrecord.

Also fixes a bug where mLeasedRefs was incorrectly cleared when overwriting a cell (*oldcell = *cell;)

apps/openmw/mwworld/store.cpp

@@ -3,22 +3,8 @@
 
 namespace MWWorld {
 
-
+void Store<ESM::Cell>::handleMovedCellRefs(ESM::ESMReader& esm, ESM::Cell* cell)
-void Store<ESM::Cell>::load(ESM::ESMReader &esm, const std::string &id)
 {
-    // Don't automatically assume that a new cell must be spawned. Multiple plugins write to the same cell,
-    //  and we merge all this data into one Cell object. However, we can't simply search for the cell id,
-    //  as many exterior cells do not have a name. Instead, we need to search by (x,y) coordinates - and they
-    //  are not available until both cells have been loaded! So first, proceed as usual.
-
-    // All cells have a name record, even nameless exterior cells.
-    std::string idLower = Misc::StringUtils::lowerCase(id);
-    ESM::Cell *cell = new ESM::Cell;
-    cell->mName = id;
-
-    //First part of cell loading
-    cell->preLoad(esm);
-
     //Handling MovedCellRefs, there is no way to do it inside loadcell
     while (esm.isNextSub("MVRF")) {
         ESM::CellRef ref;
@@ -43,35 +29,56 @@ void Store<ESM::Cell>::load(ESM::ESMReader &esm, const std::string &id)
         else
           *iter = ref;
     }
+}
 
-    //Second part of cell loading
+void Store<ESM::Cell>::load(ESM::ESMReader &esm, const std::string &id)
-    cell->postLoad(esm);
+{
+    // Don't automatically assume that a new cell must be spawned. Multiple plugins write to the same cell,
+    //  and we merge all this data into one Cell object. However, we can't simply search for the cell id,
+    //  as many exterior cells do not have a name. Instead, we need to search by (x,y) coordinates - and they
+    //  are not available until both cells have been loaded at least partially!
 
-    if(cell->mData.mFlags & ESM::Cell::Interior)
+    // All cells have a name record, even nameless exterior cells.
+    std::string idLower = Misc::StringUtils::lowerCase(id);
+    ESM::Cell cell;
+    cell.mName = id;
+
+    // Load the (x,y) coordinates of the cell, if it is an exterior cell,
+    // so we can find the cell we need to merge with
+    cell.loadData(esm);
+
+    if(cell.mData.mFlags & ESM::Cell::Interior)
     {
         // Store interior cell by name, try to merge with existing parent data.
         ESM::Cell *oldcell = const_cast<ESM::Cell*>(search(idLower));
         if (oldcell) {
-            // push the new references on the list of references to manage
+            // merge new cell into old cell
-            oldcell->mContextList.push_back(cell->mContextList.at(0));
+            // push the new references on the list of references to manage (saveContext = true)
-            // copy list into new cell
+            oldcell->mData = cell.mData;
-            cell->mContextList = oldcell->mContextList;
+            oldcell->loadCell(esm, true);
-            // have new cell replace old cell
-            ESM::Cell::merge(oldcell, cell);
         } else
-            mInt[idLower] = *cell;
+        {
+            // spawn a new cell
+            cell.loadCell(esm, true);
+
+            mInt[idLower] = cell;
+        }
     }
     else
     {
         // Store exterior cells by grid position, try to merge with existing parent data.
-        ESM::Cell *oldcell = const_cast<ESM::Cell*>(search(cell->getGridX(), cell->getGridY()));
+        ESM::Cell *oldcell = const_cast<ESM::Cell*>(search(cell.getGridX(), cell.getGridY()));
         if (oldcell) {
-            // push the new references on the list of references to manage
+            // merge new cell into old cell
-            oldcell->mContextList.push_back(cell->mContextList.at(0));
+            // push the new references on the list of references to manage (saveContext = true)
-            // copy list into new cell
+            oldcell->mData = cell.mData;
-            cell->mContextList = oldcell->mContextList;
+            oldcell->loadCell(esm, true);
+
+            // handle moved ref (MVRF) subrecords
+            handleMovedCellRefs (esm, &cell);
+
             // merge lists of leased references, use newer data in case of conflict
-            for (ESM::MovedCellRefTracker::const_iterator it = cell->mMovedRefs.begin(); it != cell->mMovedRefs.end(); ++it) {
+            for (ESM::MovedCellRefTracker::const_iterator it = cell.mMovedRefs.begin(); it != cell.mMovedRefs.end(); ++it) {
                 // remove reference from current leased ref tracker and add it to new cell
                 ESM::MovedCellRefTracker::iterator itold = std::find(oldcell->mMovedRefs.begin(), oldcell->mMovedRefs.end(), it->mRefNum);
                 if (itold != oldcell->mMovedRefs.end()) {
@@ -82,13 +89,21 @@ void Store<ESM::Cell>::load(ESM::ESMReader &esm, const std::string &id)
                     *itold = *it;
                 }
             }
-            cell->mMovedRefs = oldcell->mMovedRefs;
+
-            // have new cell replace old cell
+            // We don't need to merge mLeasedRefs of cell / oldcell. This list is filled when another cell moves a
-            ESM::Cell::merge(oldcell, cell);
+            // reference to this cell, so the list for the new cell should be empty. The list for oldcell,
+            // however, could have leased refs in it and so should be kept.
         } else
-            mExt[std::make_pair(cell->mData.mX, cell->mData.mY)] = *cell;
+        {
+            // spawn a new cell
+            cell.loadCell(esm, true);
+
+            // handle moved ref (MVRF) subrecords
+            handleMovedCellRefs (esm, &cell);
+
+            mExt[std::make_pair(cell.mData.mX, cell.mData.mY)] = cell;
+        }
     }
-    delete cell;
 }
 
 }

apps/openmw/mwworld/store.hpp

@@ -591,6 +591,8 @@ namespace MWWorld
             return search(cell.mName);
         }
 
+        void handleMovedCellRefs(ESM::ESMReader& esm, ESM::Cell* cell);
+
     public:
         ESMStore *mEsmStore;
 

components/esm/loadcell.cpp

@@ -50,18 +50,14 @@ namespace ESM
         return ref.mRefNum == refNum;
     }
 
-
 void Cell::load(ESMReader &esm, bool saveContext)
 {
-    // Ignore this for now, it might mean we should delete the entire
+    loadData(esm);
-    // cell?
+    loadCell(esm, saveContext);
-    // TODO: treat the special case "another plugin moved this ref, but we want to delete it"!
+}
-    if (esm.isNextSub("DELE")) {
-        esm.skipHSub();
-    }
-
-    esm.getHNT(mData, "DATA", 12);
 
+void Cell::loadCell(ESMReader &esm, bool saveContext)
+{
     mNAM0 = 0;
 
     if (mData.mFlags & Interior)
@@ -73,12 +69,10 @@ void Cell::load(ESMReader &esm, bool saveContext)
             esm.getHT(waterl);
             mWater = (float) waterl;
             mWaterInt = true;
-            mHasWaterLevelRecord = true;
         }
         else if (esm.isNextSub("WHGT"))
         {
             esm.getHT(mWater);
-            mHasWaterLevelRecord = true;
         }
 
         // Quasi-exterior cells have a region (which determines the
@@ -107,6 +101,18 @@ void Cell::load(ESMReader &esm, bool saveContext)
     }
 }
 
+void Cell::loadData(ESMReader &esm)
+{
+    // Ignore this for now, it might mean we should delete the entire
+    // cell?
+    // TODO: treat the special case "another plugin moved this ref, but we want to delete it"!
+    if (esm.isNextSub("DELE")) {
+        esm.skipHSub();
+    }
+
+    esm.getHNT(mData, "DATA", 12);
+}
+
 void Cell::preLoad(ESMReader &esm) //Can't be "load" because it conflicts with function in esmtool
 {
     this->load(esm, false);
@@ -124,14 +130,12 @@ void Cell::save(ESMWriter &esm) const
     esm.writeHNT("DATA", mData, 12);
     if (mData.mFlags & Interior)
     {
-        if (mHasWaterLevelRecord) {
+        if (mWaterInt) {
-            if (mWaterInt) {
+            int water =
-                int water =
+                (mWater >= 0) ? (int) (mWater + 0.5) : (int) (mWater - 0.5);
-                    (mWater >= 0) ? (int) (mWater + 0.5) : (int) (mWater - 0.5);
+            esm.writeHNT("INTV", water);
-                esm.writeHNT("INTV", water);
+        } else {
-            } else {
+            esm.writeHNT("WHGT", mWater);
-                esm.writeHNT("WHGT", mWater);
-            }
         }
 
         if (mData.mFlags & QuasiEx)
@@ -228,19 +232,6 @@ bool Cell::getNextMVRF(ESMReader &esm, MovedCellRef &mref)
         mAmbi.mFogDensity = 0;
     }
 
-    void Cell::merge(Cell *original, Cell *modified)
-    {
-        float waterLevel = original->mWater;
-        if (modified->mHasWaterLevelRecord)
-        {
-            waterLevel = modified->mWater;
-        }
-        // else: keep original water level, instead of resetting to 0
-
-        *original = *modified;
-        original->mWater = waterLevel;
-    }
-
     CellId Cell::getCellId() const
     {
         CellId id;

components/esm/loadcell.hpp

@@ -78,10 +78,7 @@ struct Cell
     float mFogDensity;
   };
 
-  Cell() : mWater(0), mHasWaterLevelRecord(false) {}
+  Cell() : mWater(0) {}
-
-  /// Merge \a modified into \a original
-  static void merge (Cell* original, Cell* modified);
 
   // Interior cells are indexed by this (it's the 'id'), for exterior
   // cells it is optional.
@@ -93,8 +90,8 @@ struct Cell
   std::vector<ESM_Context> mContextList; // File position; multiple positions for multiple plugin support
   DATAstruct mData;
   AMBIstruct mAmbi;
+
   float mWater; // Water level
-  bool mHasWaterLevelRecord;
   bool mWaterInt;
   int mMapColor;
   int mNAM0;
@@ -109,7 +106,10 @@ struct Cell
 
   // This method is left in for compatibility with esmtool. Parsing moved references currently requires
   //  passing ESMStore, bit it does not know about this parameter, so we do it this way.
-  void load(ESMReader &esm, bool saveContext = true);
+  void load(ESMReader &esm, bool saveContext = true); // Load everything (except references)
+  void loadData(ESMReader &esm); // Load DATAstruct only
+  void loadCell(ESMReader &esm, bool saveContext = true); // Load everything, except DATAstruct and references
+
   void save(ESMWriter &esm) const;
 
   bool isExterior() const