Keep refnum when moving objects to/from inventory (#6148)

apps/openmw/mwclass/misc.cpp

@@ -173,35 +173,33 @@ namespace MWClass
         return info;
     }
 
+    static MWWorld::Ptr createGold(MWWorld::CellStore& cell, int goldAmount)
+    {
+        std::string_view base = "gold_001";
+        if (goldAmount >= 100)
+            base = "gold_100";
+        else if (goldAmount >= 25)
+            base = "gold_025";
+        else if (goldAmount >= 10)
+            base = "gold_010";
+        else if (goldAmount >= 5)
+            base = "gold_005";
+
+        const MWWorld::ESMStore& store = *MWBase::Environment::get().getESMStore();
+        MWWorld::ManualRef newRef(store, ESM::RefId::stringRefId(base));
+        const MWWorld::LiveCellRef<ESM::Miscellaneous>* ref = newRef.getPtr().get<ESM::Miscellaneous>();
+
+        MWWorld::Ptr ptr(cell.insert(ref), &cell);
+        ptr.getCellRef().setGoldValue(goldAmount);
+        ptr.getRefData().setCount(1);
+        return ptr;
+    }
+
     MWWorld::Ptr Miscellaneous::copyToCell(const MWWorld::ConstPtr& ptr, MWWorld::CellStore& cell, int count) const
     {
         MWWorld::Ptr newPtr;
-
-        const MWWorld::ESMStore& store = *MWBase::Environment::get().getESMStore();
-
         if (isGold(ptr))
-        {
+            newPtr = createGold(cell, getValue(ptr) * count);
-            int goldAmount = getValue(ptr) * count;
-
-            std::string_view base = "gold_001";
-            if (goldAmount >= 100)
-                base = "gold_100";
-            else if (goldAmount >= 25)
-                base = "gold_025";
-            else if (goldAmount >= 10)
-                base = "gold_010";
-            else if (goldAmount >= 5)
-                base = "gold_005";
-
-            // Really, I have no idea why moving ref out of conditional
-            // scope causes list::push_back throwing std::bad_alloc
-            MWWorld::ManualRef newRef(store, ESM::RefId::stringRefId(base));
-            const MWWorld::LiveCellRef<ESM::Miscellaneous>* ref = newRef.getPtr().get<ESM::Miscellaneous>();
-
-            newPtr = MWWorld::Ptr(cell.insert(ref), &cell);
-            newPtr.getCellRef().setGoldValue(goldAmount);
-            newPtr.getRefData().setCount(1);
-        }
         else
         {
             const MWWorld::LiveCellRef<ESM::Miscellaneous>* ref = ptr.get<ESM::Miscellaneous>();
@@ -209,8 +207,29 @@ namespace MWClass
             newPtr.getRefData().setCount(count);
         }
         newPtr.getCellRef().unsetRefNum();
+        newPtr.getRefData().setLuaScripts(nullptr);
         MWBase::Environment::get().getWorldModel()->registerPtr(newPtr);
+        return newPtr;
+    }
 
+    MWWorld::Ptr Miscellaneous::moveToCell(const MWWorld::Ptr& ptr, MWWorld::CellStore& cell) const
+    {
+        MWWorld::Ptr newPtr;
+        if (isGold(ptr))
+        {
+            newPtr = createGold(cell, getValue(ptr));
+            newPtr.getRefData() = ptr.getRefData();
+            newPtr.getCellRef().setRefNum(ptr.getCellRef().getRefNum());
+            newPtr.getCellRef().setGoldValue(ptr.getCellRef().getGoldValue());
+            newPtr.getRefData().setCount(1);
+        }
+        else
+        {
+            const MWWorld::LiveCellRef<ESM::Miscellaneous>* ref = ptr.get<ESM::Miscellaneous>();
+            newPtr = MWWorld::Ptr(cell.insert(ref), &cell);
+        }
+        ptr.getRefData().setLuaScripts(nullptr);
+        MWBase::Environment::get().getWorldModel()->registerPtr(newPtr);
         return newPtr;
     }
 

apps/openmw/mwclass/misc.hpp

@@ -13,6 +13,7 @@ namespace MWClass
 
     public:
         MWWorld::Ptr copyToCell(const MWWorld::ConstPtr& ptr, MWWorld::CellStore& cell, int count) const override;
+        MWWorld::Ptr moveToCell(const MWWorld::Ptr& ptr, MWWorld::CellStore& cell) const override;
 
         void insertObjectRendering(const MWWorld::Ptr& ptr, const std::string& model,
             MWRender::RenderingInterface& renderingInterface) const override;

apps/openmw/mwgui/itemmodel.cpp

@@ -57,6 +57,8 @@ namespace MWGui
 
     MWWorld::Ptr ItemModel::moveItem(const ItemStack& item, size_t count, ItemModel* otherModel)
     {
+        // TODO(#6148): moving an item should preserve RefNum and Lua scripts (unless the item stack is merged with
+        // already existing stack).
         MWWorld::Ptr ret = otherModel->copyItem(item, count);
         removeItem(item, count);
         return ret;

apps/openmw/mwlua/objectbindings.cpp

@@ -284,81 +284,100 @@ namespace MWLua
                     localScripts->removeScript(*scriptId);
                 };
 
-                auto removeFn = [context](const GObject& object, int countToRemove) {
+                using DelayedRemovalFn = std::function<void(MWWorld::Ptr)>;
-                    MWWorld::Ptr ptr = object.ptr();
+                auto removeFn = [](const MWWorld::Ptr ptr, int countToRemove) -> std::optional<DelayedRemovalFn> {
                     int currentCount = ptr.getRefData().getCount();
                     if (countToRemove <= 0 || countToRemove > currentCount)
                         throw std::runtime_error("Can't remove " + std::to_string(countToRemove) + " of "
                             + std::to_string(currentCount) + " items");
                     ptr.getRefData().setCount(currentCount - countToRemove); // Immediately change count
-                    if (ptr.getContainerStore() || currentCount == countToRemove)
+                    if (!ptr.getContainerStore() && currentCount > countToRemove)
-                    {
+                        return std::nullopt;
-                        // Delayed action to trigger side effects
+                    // Delayed action to trigger side effects
-                        context.mLuaManager->addAction([object, countToRemove] {
+                    return [countToRemove](MWWorld::Ptr ptr) {
-                            MWWorld::Ptr ptr = object.ptr();
+                        // Restore the original count
-                            // Restore original count
+                        ptr.getRefData().setCount(ptr.getRefData().getCount() + countToRemove);
-                            ptr.getRefData().setCount(ptr.getRefData().getCount() + countToRemove);
+                        // And now remove properly
-                            // And now remove properly
+                        if (ptr.getContainerStore())
-                            if (ptr.getContainerStore())
+                            ptr.getContainerStore()->remove(ptr, countToRemove);
-                                ptr.getContainerStore()->remove(ptr, countToRemove);
+                        else
-                            else
+                        {
-                            {
+                            MWBase::Environment::get().getWorld()->disable(ptr);
-                                MWBase::Environment::get().getWorld()->disable(object.ptr());
+                            MWBase::Environment::get().getWorld()->deleteObject(ptr);
-                                MWBase::Environment::get().getWorld()->deleteObject(ptr);
+                        }
-                            }
+                    };
-                        });
-                    }
                 };
-                objectT["remove"] = [removeFn](const GObject& object, sol::optional<int> count) {
+                objectT["remove"] = [removeFn, context](const GObject& object, sol::optional<int> count) {
-                    removeFn(object, count.value_or(object.ptr().getRefData().getCount()));
+                    std::optional<DelayedRemovalFn> delayed
+                        = removeFn(object.ptr(), count.value_or(object.ptr().getRefData().getCount()));
+                    if (delayed.has_value())
+                        context.mLuaManager->addAction([fn = *delayed, object] { fn(object.ptr()); });
                 };
-                objectT["split"] = [removeFn](const GObject& object, int count) -> GObject {
+                objectT["split"] = [removeFn, context](const GObject& object, int count) -> GObject {
-                    removeFn(object, count);
-
                     // Doesn't matter which cell to use because the new instance will be in disabled state.
                     MWWorld::CellStore* cell = MWBase::Environment::get().getWorldScene()->getCurrentCell();
 
                     const MWWorld::Ptr& ptr = object.ptr();
                     MWWorld::Ptr splitted = ptr.getClass().copyToCell(ptr, *cell, count);
                     splitted.getRefData().disable();
-                    return GObject(getId(splitted));
+
+                    std::optional<DelayedRemovalFn> delayedRemovalFn = removeFn(ptr, count);
+                    if (delayedRemovalFn.has_value())
+                        context.mLuaManager->addAction([fn = *delayedRemovalFn, object] { fn(object.ptr()); });
+
+                    return GObject(splitted);
                 };
                 objectT["moveInto"] = [removeFn, context](const GObject& object, const Inventory<GObject>& inventory) {
-                    // Currently moving to or from containers makes a copy and removes the original.
+                    const MWWorld::Ptr& ptr = object.ptr();
-                    // TODO(#6148): actually move rather than copy and preserve RefNum
+                    int count = ptr.getRefData().getCount();
-                    int count = object.ptr().getRefData().getCount();
+                    std::optional<DelayedRemovalFn> delayedRemovalFn = removeFn(ptr, count);
-                    removeFn(object, count);
+                    context.mLuaManager->addAction([item = object, count, cont = inventory.mObj, delayedRemovalFn] {
-                    context.mLuaManager->addAction([item = object, count, cont = inventory.mObj] {
+                        const MWWorld::Ptr& oldPtr = item.ptr();
-                        auto& refData = item.ptr().getRefData();
+                        auto& refData = oldPtr.getRefData();
                         refData.setCount(count); // temporarily undo removal to run ContainerStore::add
                         refData.enable();
-                        cont.ptr().getClass().getContainerStore(cont.ptr()).add(item.ptr(), count, false);
+                        cont.ptr().getClass().getContainerStore(cont.ptr()).add(oldPtr, count, false);
                         refData.setCount(0);
+                        if (delayedRemovalFn.has_value())
+                            (*delayedRemovalFn)(oldPtr);
                     });
                 };
                 objectT["teleport"] = [removeFn, context](const GObject& object, const sol::object& cellOrName,
                                           const osg::Vec3f& pos, const sol::optional<osg::Vec3f>& optRot) {
                     MWWorld::CellStore* cell = findCell(cellOrName, pos);
                     MWWorld::Ptr ptr = object.ptr();
-                    if (ptr.getRefData().isDeleted())
+                    int count = ptr.getRefData().getCount();
-                        throw std::runtime_error("Object is removed");
+                    if (count == 0)
+                        throw std::runtime_error("Object is either removed or already in the process of teleporting");
+                    osg::Vec3f rot = optRot ? *optRot : ptr.getRefData().getPosition().asRotationVec3();
                     if (ptr.getContainerStore())
                     {
-                        // Currently moving to or from containers makes a copy and removes the original.
+                        DelayedRemovalFn delayedRemovalFn = *removeFn(ptr, count);
-                        // TODO(#6148): actually move rather than copy and preserve RefNum
+                        context.mLuaManager->addAction(
-                        MWWorld::Ptr newPtr = ptr.getClass().copyToCell(ptr, *cell, ptr.getRefData().getCount());
+                            [object, cell, pos, rot, count, delayedRemovalFn] {
-                        newPtr.getRefData().disable();
+                                MWWorld::Ptr oldPtr = object.ptr();
-                        removeFn(object, ptr.getRefData().getCount());
+                                oldPtr.getRefData().setCount(count);
-                        ptr = newPtr;
+                                MWWorld::Ptr newPtr = oldPtr.getClass().moveToCell(oldPtr, *cell);
+                                oldPtr.getRefData().setCount(0);
+                                newPtr.getRefData().disable();
+                                teleportNotPlayer(newPtr, cell, pos, rot);
+                                delayedRemovalFn(oldPtr);
+                            },
+                            "TeleportFromContainerAction");
                     }
-                    osg::Vec3f rot = optRot ? *optRot : ptr.getRefData().getPosition().asRotationVec3();
+                    else if (ptr == MWBase::Environment::get().getWorld()->getPlayerPtr())
-                    if (ptr == MWBase::Environment::get().getWorld()->getPlayerPtr())
                         context.mLuaManager->addTeleportPlayerAction(
                             [cell, pos, rot] { teleportPlayer(cell, pos, rot); });
                     else
+                    {
+                        ptr.getRefData().setCount(0);
                         context.mLuaManager->addAction(
-                            [obj = Object(ptr), cell, pos, rot] { teleportNotPlayer(obj.ptr(), cell, pos, rot); },
+                            [object, cell, pos, rot, count] {
+                                object.ptr().getRefData().setCount(count);
+                                teleportNotPlayer(object.ptr(), cell, pos, rot);
+                            },
                             "TeleportAction");
+                    }
                 };
             }
         }

apps/openmw/mwworld/cellref.cpp

@@ -25,8 +25,6 @@ namespace MWWorld
     {
     }
 
-    static const ESM::RefNum emptyRefNum = {};
-
     const ESM::RefNum& CellRef::getRefNum() const
     {
         return std::visit(ESM::VisitOverload{
@@ -61,11 +59,11 @@ namespace MWWorld
         return refNum;
     }
 
-    void CellRef::unsetRefNum()
+    void CellRef::setRefNum(ESM::RefNum refNum)
     {
         std::visit(ESM::VisitOverload{
-                       [&](ESM4::Reference& ref) { ref.mId = emptyRefNum; },
+                       [&](ESM4::Reference& ref) { ref.mId = refNum; },
-                       [&](ESM::CellRef& ref) { ref.mRefNum = emptyRefNum; },
+                       [&](ESM::CellRef& ref) { ref.mRefNum = refNum; },
                    },
             mCellRef.mVariant);
     }

apps/openmw/mwworld/cellref.hpp

@@ -31,8 +31,10 @@ namespace MWWorld
         // If RefNum is not set, assigns a generated one and changes the "lastAssignedRefNum" counter.
         const ESM::RefNum& getOrAssignRefNum(ESM::RefNum& lastAssignedRefNum);
 
+        void setRefNum(ESM::RefNum refNum);
+
         // Set RefNum to its default state.
-        void unsetRefNum();
+        void unsetRefNum() { setRefNum({}); }
 
         /// Does the RefNum have a content file?
         bool hasContentFile() const { return getRefNum().hasContentFile(); }

apps/openmw/mwworld/class.cpp

@@ -374,6 +374,17 @@ namespace MWWorld
         Ptr newPtr = copyToCellImpl(ptr, cell);
         newPtr.getCellRef().unsetRefNum(); // This RefNum is only valid within the original cell of the reference
         newPtr.getRefData().setCount(count);
+        newPtr.getRefData().setLuaScripts(nullptr);
+        MWBase::Environment::get().getWorldModel()->registerPtr(newPtr);
+        if (hasInventoryStore(newPtr))
+            getInventoryStore(newPtr).setActor(newPtr);
+        return newPtr;
+    }
+
+    MWWorld::Ptr Class::moveToCell(const Ptr& ptr, CellStore& cell) const
+    {
+        Ptr newPtr = copyToCellImpl(ptr, cell);
+        ptr.getRefData().setLuaScripts(nullptr);
         MWBase::Environment::get().getWorldModel()->registerPtr(newPtr);
         if (hasInventoryStore(newPtr))
             getInventoryStore(newPtr).setActor(newPtr);
@@ -384,7 +395,6 @@ namespace MWWorld
     {
         Ptr newPtr = copyToCell(ptr, cell, count);
         newPtr.getRefData().setPosition(pos);
-
         return newPtr;
     }
 

apps/openmw/mwworld/class.hpp

@@ -313,7 +313,12 @@ namespace MWWorld
 
         virtual Ptr copyToCell(const ConstPtr& ptr, CellStore& cell, int count) const;
 
-        virtual Ptr copyToCell(const ConstPtr& ptr, CellStore& cell, const ESM::Position& pos, int count) const;
+        // Similar to `copyToCell`, but preserves RefNum and moves LuaScripts.
+        // The original is expected to be removed after calling this function,
+        // but this function itself doesn't remove the original.
+        virtual Ptr moveToCell(const Ptr& ptr, CellStore& cell) const;
+
+        Ptr copyToCell(const ConstPtr& ptr, CellStore& cell, const ESM::Position& pos, int count) const;
 
         virtual bool isActivator() const { return false; }
 

apps/openmw/mwworld/containerstore.cpp

@@ -23,6 +23,7 @@
 #include "manualref.hpp"
 #include "player.hpp"
 #include "refdata.hpp"
+#include "worldmodel.hpp"
 
 namespace
 {
@@ -303,9 +304,11 @@ MWWorld::ContainerStoreIterator MWWorld::ContainerStore::add(
     Ptr player = MWBase::Environment::get().getWorld()->getPlayerPtr();
 
     MWWorld::ContainerStoreIterator it = addImp(itemPtr, count, resolve);
+    itemPtr.getRefData().setLuaScripts(nullptr); // clear Lua scripts on the original (removed) item.
 
     // The copy of the original item we just made
     MWWorld::Ptr item = *it;
+    MWBase::Environment::get().getWorldModel()->registerPtr(item);
 
     // we may have copied an item from the world, so reset a few things first
     item.getRefData().setBaseNode(
@@ -325,10 +328,6 @@ MWWorld::ContainerStoreIterator MWWorld::ContainerStore::add(
     item.getCellRef().setFaction(ESM::RefId());
     item.getCellRef().setFactionRank(-2);
 
-    // must reset the RefNum on the copied item, so that the RefNum on the original item stays unique
-    // maybe we should do this in the copy constructor instead?
-    item.getCellRef().unsetRefNum(); // destroy link to content file
-
     const ESM::RefId& script = item.getClass().getScript(item);
     if (!script.empty())
     {