Merge branch 'OpenCS-moved-reference' into 'master'

OpenCS - Fix moved reference - Issues #3514 and #4752

See merge request OpenMW/openmw!1051

(cherry picked from commit 2bee171c7990522da33c2667f7d079fa35f4ede0)

36c30f7f Fix for Issue #3514 where moving a reference to another cell is not handled properly.
40327681 Update the changelog.

CHANGELOG.md

@@ -1,10 +1,12 @@
 0.48.0
 ------
 
+    Bug #3514: Editing a reference's position after loading an esp file makes the reference disappear
     Bug #3737: Scripts from The Underground 2 .esp do not play (all patched versions)
     Bug #3846: Strings starting with "-" fail to compile if not enclosed in quotes
     Bug #3905: Great House Dagoth issues
     Bug #4203: Resurrecting an actor should close the loot GUI
+    Bug #4752: UpdateCellCommand doesn't undo properly
     Bug #5100: Persuasion doesn't always clamp the resulting disposition
     Bug #5120: Scripted object spawning updates physics system
     Bug #5379: Wandering NPCs falling through cantons

apps/opencs/model/doc/savingstages.cpp

@@ -285,7 +285,8 @@ void CSMDoc::WriteCellCollectionStage::writeReferences (const std::deque<int>& r
             {
                 refRecord.mRefNum.mIndex = newRefNum++;
             }
-            else if ((refRecord.mOriginalCell.empty() ? refRecord.mCell : refRecord.mOriginalCell)
+
+            if ((refRecord.mOriginalCell.empty() ? refRecord.mCell : refRecord.mOriginalCell)
                     != stream.str() && !interior)
             {
                 // An empty mOriginalCell is meant to indicate that it is the same as

apps/opencs/model/world/commanddispatcher.cpp

@@ -139,13 +139,28 @@ void CSMWorld::CommandDispatcher::executeModify (QAbstractItemModel *model, cons
     if (mLocked)
         return;
 
-    std::unique_ptr<CSMWorld::UpdateCellCommand> modifyCell;
+    std::unique_ptr<CSMWorld::CloneCommand> clonedData;
+    std::unique_ptr<CSMWorld::DeleteCommand> deleteData;
 
-    std::unique_ptr<CSMWorld::ModifyCommand> modifyDataRefNum;
+    std::string newId;
+
+    std::unique_ptr<CSMWorld::ModifyCommand> modifyData;
+    std::unique_ptr<CSMWorld::UpdateCellCommand> modifyCell;
 
     int columnId = model->data (index, ColumnBase::Role_ColumnId).toInt();
 
-    if (columnId==Columns::ColumnId_PositionXPos || columnId==Columns::ColumnId_PositionYPos)
+    int stateColumn = dynamic_cast<CSMWorld::IdTable&>(*model).findColumnIndex(Columns::ColumnId_Modification);
+
+    CSMWorld::IdTable& table = dynamic_cast<CSMWorld::IdTable&>(*model); // for getId()
+    QModelIndex stateIndex = table.getModelIndex(table.getId(index.row()), stateColumn);
+    RecordBase::State state = static_cast<RecordBase::State> (model->data(stateIndex).toInt());
+
+    // This is not guaranteed to be the same as \a model, since a proxy could be used.
+    IdTable& model2 = dynamic_cast<IdTable&> (*mDocument.getData().getTableModel(mId));
+
+    // DeleteCommand triggers a signal to the whole row from IdTable::setData(), so ignore the second call
+    if (state != RecordBase::State_Deleted &&
+        (columnId==Columns::ColumnId_PositionXPos || columnId==Columns::ColumnId_PositionYPos))
     {
         const float oldPosition = model->data (index).toFloat();
 
@@ -157,12 +172,9 @@ void CSMWorld::CommandDispatcher::executeModify (QAbstractItemModel *model, cons
 
             int row = proxy ? proxy->mapToSource (index).row() : index.row();
 
-            // This is not guaranteed to be the same as \a model, since a proxy could be used.
-            IdTable& model2 = dynamic_cast<IdTable&> (*mDocument.getData().getTableModel (mId));
-
             int cellColumn = model2.searchColumnIndex (Columns::ColumnId_Cell);
 
-            if (cellColumn!=-1)
+            if (cellColumn != -1)
             {
                 QModelIndex cellIndex = model2.index (row, cellColumn);
 
@@ -170,31 +182,46 @@ void CSMWorld::CommandDispatcher::executeModify (QAbstractItemModel *model, cons
 
                 if (cellId.find ('#')!=std::string::npos)
                 {
-                    // Need to recalculate the cell and (if necessary) clear the instance's refNum
+                    RefCollection& collection = mDocument.getData().getReferences();
-                    modifyCell.reset (new UpdateCellCommand (model2, row));
+                    newId = collection.getNewId();
 
-                    // Not sure which model this should be applied to
+                    clonedData.reset(new CloneCommand(table,
-                    int refNumColumn = model2.searchColumnIndex (Columns::ColumnId_RefNum);
+                                table.getId(row),
+                                newId,
+                                CSMWorld::UniversalId::Type_Reference));
 
-                    if (refNumColumn!=-1)
+                    deleteData.reset(new DeleteCommand(table,
-                        modifyDataRefNum.reset (new ModifyCommand(*model, model->index(row, refNumColumn), 0));
+                                table.getId(row),
+                                CSMWorld::UniversalId::Type_Reference));
                 }
             }
         }
     }
 
-    std::unique_ptr<CSMWorld::ModifyCommand> modifyData (
+    if (!clonedData.get())
-        new CSMWorld::ModifyCommand (*model, index, new_));
+    {
+        // DeleteCommand will trigger executeModify after setting the state to State_Deleted
+        // from CommandDelegate::setModelDataImp() - ignore
+        if (state != RecordBase::State_Deleted)
+            modifyData.reset(new CSMWorld::ModifyCommand(*model, index, new_));
+    }
 
-    if (modifyCell.get())
+    if (clonedData.get())
     {
         CommandMacro macro (mDocument.getUndoStack());
-        macro.push (modifyData.release());
+        macro.push(clonedData.release());
-        macro.push (modifyCell.release());
+        macro.push(deleteData.release());
-        if (modifyDataRefNum.get())
+
-            macro.push (modifyDataRefNum.release());
+        // cannot do these earlier because newIndex is not available until CloneCommand is executed
+        QModelIndex newIndex = model2.getModelIndex (newId, index.column());
+        modifyData.reset (new CSMWorld::ModifyCommand (*model, newIndex, new_));
+        macro.push(modifyData.release());
+
+        // once the data is updated update the cell location
+        modifyCell.reset(new UpdateCellCommand(model2, newIndex.row()));
+        macro.push(modifyCell.release());
     }
-    else
+    else if (!clonedData.get() && modifyData.get())
         mDocument.getUndoStack().push (modifyData.release());
 }
 

components/esm/loadcell.cpp

@@ -311,7 +311,7 @@ namespace ESM
         mWater = 0;
         mWaterInt = false;
         mMapColor = 0;
-        mRefNumCounter = -1;
+        mRefNumCounter = 0;
 
         mData.mFlags = 0;
         mData.mX = 0;

components/esm/loadcell.hpp

@@ -94,7 +94,7 @@ struct Cell
            mWater(0),
            mWaterInt(false),
            mMapColor(0),
-           mRefNumCounter(-1)
+           mRefNumCounter(0)
   {}
 
   // Interior cells are indexed by this (it's the 'id'), for exterior