Fix inventory swapping not calling all callbacks (#9923)

"Predicts" whether something will be swapped for allow callbacks, then calls callbacks a second time with swapped properties.

Co-authored-by: SmallJoker <SmallJoker@users.noreply.github.com>

diff --git a/doc/lua_api.txt b/doc/lua_api.txt
index cc4af970ca9e2f4c462dbd61f9b1a8ad5e272002..86627c19ec276281cf1ca1f5944606af11a53d15 100644 (file)
--- a/doc/lua_api.txt
+++ b/doc/lua_api.txt
@@ -5888,6 +5888,31 @@ An `InvRef` is a reference to an inventory.
   `minetest.get_inventory(location)`.
     * returns `{type="undefined"}` in case location is not known
 
+### Callbacks
+
+Detached & nodemeta inventories provide the following callbacks for move actions:
+
+#### Before
+
+The `allow_*` callbacks return how many items can be moved.
+
+* `allow_move`/`allow_metadata_inventory_move`: Moving items in the inventory
+* `allow_take`/`allow_metadata_inventory_take`: Taking items from the inventory
+* `allow_put`/`allow_metadata_inventory_put`: Putting items to the inventory
+
+#### After
+
+The `on_*` callbacks are called after the items have been placed in the inventories.
+
+* `on_move`/`on_metadata_inventory_move`: Moving items in the inventory
+* `on_take`/`on_metadata_inventory_take`: Taking items from the inventory
+* `on_put`/`on_metadata_inventory_put`: Putting items to the inventory
+
+#### Swapping
+
+When a player tries to put an item to a place where another item is, the items are *swapped*.
+This means that all callbacks will be called twice (once for each action).
+
 `ItemStack`
 -----------
 

diff --git a/src/inventory.cpp b/src/inventory.cpp
index 349ee503d1ceab3a61a16b6593f10df90303e698..cf72cb0055961518ddc8d0056f267502f2a2d97b 100644 (file)
--- a/src/inventory.cpp
+++ b/src/inventory.cpp
@@ -732,17 +732,17 @@ void InventoryList::moveItemSomewhere(u32 i, InventoryList *dest, u32 count)
 u32 InventoryList::moveItem(u32 i, InventoryList *dest, u32 dest_i,
                u32 count, bool swap_if_needed, bool *did_swap)
 {
-       if(this == dest && i == dest_i)
+       if (this == dest && i == dest_i)
                return count;
 
        // Take item from source list
        ItemStack item1;
-       if(count == 0)
+       if (count == 0)
                item1 = changeItem(i, ItemStack());
        else
                item1 = takeItem(i, count);
 
-       if(item1.empty())
+       if (item1.empty())
                return 0;
 
        // Try to add the item to destination list
@@ -750,8 +750,7 @@ u32 InventoryList::moveItem(u32 i, InventoryList *dest, u32 dest_i,
        item1 = dest->addItem(dest_i, item1);
 
        // If something is returned, the item was not fully added
-       if(!item1.empty())
-       {
+       if (!item1.empty()) {
                // If olditem is returned, nothing was added.
                bool nothing_added = (item1.count == oldcount);
 

diff --git a/src/inventorymanager.cpp b/src/inventorymanager.cpp
index b6f4649014ae6c4635e4d3878cb0f7771df3d72e..635bd2e4b0bdc760a570ac2fd65cb95fb54ccfb0 100644 (file)
--- a/src/inventorymanager.cpp
+++ b/src/inventorymanager.cpp
@@ -154,6 +154,93 @@ IMoveAction::IMoveAction(std::istream &is, bool somewhere) :
        }
 }
 
+void IMoveAction::swapDirections()
+{
+       std::swap(from_inv, to_inv);
+       std::swap(from_list, to_list);
+       std::swap(from_i, to_i);
+}
+
+void IMoveAction::onPutAndOnTake(const ItemStack &src_item, ServerActiveObject *player) const
+{
+       ServerScripting *sa = PLAYER_TO_SA(player);
+       if (to_inv.type == InventoryLocation::DETACHED)
+               sa->detached_inventory_OnPut(*this, src_item, player);
+       else if (to_inv.type == InventoryLocation::NODEMETA)
+               sa->nodemeta_inventory_OnPut(*this, src_item, player);
+       else if (to_inv.type == InventoryLocation::PLAYER)
+               sa->player_inventory_OnPut(*this, src_item, player);
+       else
+               assert(false);
+       
+       if (from_inv.type == InventoryLocation::DETACHED)
+               sa->detached_inventory_OnTake(*this, src_item, player);
+       else if (from_inv.type == InventoryLocation::NODEMETA)
+               sa->nodemeta_inventory_OnTake(*this, src_item, player);
+       else if (from_inv.type == InventoryLocation::PLAYER)
+               sa->player_inventory_OnTake(*this, src_item, player);
+       else
+               assert(false);
+}
+
+void IMoveAction::onMove(int count, ServerActiveObject *player) const
+{
+       ServerScripting *sa = PLAYER_TO_SA(player);
+       if (from_inv.type == InventoryLocation::DETACHED)
+               sa->detached_inventory_OnMove(*this, count, player);
+       else if (from_inv.type == InventoryLocation::NODEMETA)
+               sa->nodemeta_inventory_OnMove(*this, count, player);
+       else if (from_inv.type == InventoryLocation::PLAYER)
+               sa->player_inventory_OnMove(*this, count, player);
+       else
+               assert(false);
+}
+
+int IMoveAction::allowPut(const ItemStack &dst_item, ServerActiveObject *player) const
+{
+       ServerScripting *sa = PLAYER_TO_SA(player);
+       int dst_can_put_count = 0xffff;
+       if (to_inv.type == InventoryLocation::DETACHED)
+               dst_can_put_count = sa->detached_inventory_AllowPut(*this, dst_item, player);
+       else if (to_inv.type == InventoryLocation::NODEMETA)
+               dst_can_put_count = sa->nodemeta_inventory_AllowPut(*this, dst_item, player);
+       else if (to_inv.type == InventoryLocation::PLAYER)
+               dst_can_put_count = sa->player_inventory_AllowPut(*this, dst_item, player);
+       else
+               assert(false);
+       return dst_can_put_count;
+}
+
+int IMoveAction::allowTake(const ItemStack &src_item, ServerActiveObject *player) const
+{
+       ServerScripting *sa = PLAYER_TO_SA(player);
+       int src_can_take_count = 0xffff;
+       if (from_inv.type == InventoryLocation::DETACHED)
+               src_can_take_count = sa->detached_inventory_AllowTake(*this, src_item, player);
+       else if (from_inv.type == InventoryLocation::NODEMETA)
+               src_can_take_count = sa->nodemeta_inventory_AllowTake(*this, src_item, player);
+       else if (from_inv.type == InventoryLocation::PLAYER)
+               src_can_take_count = sa->player_inventory_AllowTake(*this, src_item, player);
+       else
+               assert(false);
+       return src_can_take_count;
+}
+
+int IMoveAction::allowMove(int try_take_count, ServerActiveObject *player) const
+{
+       ServerScripting *sa = PLAYER_TO_SA(player);
+       int src_can_take_count = 0xffff;
+       if (from_inv.type == InventoryLocation::DETACHED)
+               src_can_take_count = sa->detached_inventory_AllowMove(*this, try_take_count, player);
+       else if (from_inv.type == InventoryLocation::NODEMETA)
+               src_can_take_count = sa->nodemeta_inventory_AllowMove(*this, try_take_count, player);
+       else if (from_inv.type == InventoryLocation::PLAYER)
+               src_can_take_count = sa->player_inventory_AllowMove(*this, try_take_count, player);
+       else
+               assert(false);
+       return src_can_take_count;
+}
+
 void IMoveAction::apply(InventoryManager *mgr, ServerActiveObject *player, IGameDef *gamedef)
 {
        Inventory *inv_from = mgr->getInventory(from_inv);
@@ -251,93 +338,80 @@ void IMoveAction::apply(InventoryManager *mgr, ServerActiveObject *player, IGame
                Collect information of endpoints
        */
 
-       int try_take_count = count;
-       if (try_take_count == 0)
-               try_take_count = list_from->getItem(from_i).count;
+       ItemStack src_item = list_from->getItem(from_i);
+       if (count > 0)
+               src_item.count = count;
+       if (src_item.empty())
+               return;
 
        int src_can_take_count = 0xffff;
        int dst_can_put_count = 0xffff;
 
-       /* Query detached inventories */
+       // this is needed for swapping items inside one inventory to work
+       ItemStack restitem;
+       bool allow_swap = !list_to->itemFits(to_i, src_item, &restitem)
+               && restitem.count == src_item.count
+               && !caused_by_move_somewhere;
 
-       // Move occurs in the same detached inventory
-       if (from_inv.type == InventoryLocation::DETACHED &&
-                       from_inv == to_inv) {
-               src_can_take_count = PLAYER_TO_SA(player)->detached_inventory_AllowMove(
-                       *this, try_take_count, player);
-               dst_can_put_count = src_can_take_count;
-       } else {
-               // Destination is detached
-               if (to_inv.type == InventoryLocation::DETACHED) {
-                       ItemStack src_item = list_from->getItem(from_i);
-                       src_item.count = try_take_count;
-                       dst_can_put_count = PLAYER_TO_SA(player)->detached_inventory_AllowPut(
-                               *this, src_item, player);
-               }
-               // Source is detached
-               if (from_inv.type == InventoryLocation::DETACHED) {
-                       ItemStack src_item = list_from->getItem(from_i);
-                       src_item.count = try_take_count;
-                       src_can_take_count = PLAYER_TO_SA(player)->detached_inventory_AllowTake(
-                               *this, src_item, player);
-               }
-       }
-
-       /* Query node metadata inventories */
+       // Shift-click: Cannot fill this stack, proceed with next slot
+       if (caused_by_move_somewhere && restitem.count == src_item.count)
+               return;
 
-       // Both endpoints are nodemeta
-       // Move occurs in the same nodemeta inventory
-       if (from_inv.type == InventoryLocation::NODEMETA &&
-                       from_inv == to_inv) {
-               src_can_take_count = PLAYER_TO_SA(player)->nodemeta_inventory_AllowMove(
-                       *this, try_take_count, player);
-               dst_can_put_count = src_can_take_count;
-       } else {
-               // Destination is nodemeta
-               if (to_inv.type == InventoryLocation::NODEMETA) {
-                       ItemStack src_item = list_from->getItem(from_i);
-                       src_item.count = try_take_count;
-                       dst_can_put_count = PLAYER_TO_SA(player)->nodemeta_inventory_AllowPut(
-                               *this, src_item, player);
+       if (allow_swap) {
+               // Swap will affect the entire stack if it can performed.
+               src_item = list_from->getItem(from_i);
+               count = src_item.count;
+       }
+
+       if (from_inv == to_inv) {
+               // Move action within the same inventory
+               src_can_take_count = allowMove(src_item.count, player);
+
+               bool swap_expected = allow_swap;
+               allow_swap = allow_swap
+                       && (src_can_take_count == -1 || src_can_take_count >= src_item.count);
+               if (allow_swap) {
+                       int try_put_count = list_to->getItem(to_i).count;
+                       swapDirections();
+                       dst_can_put_count = allowMove(try_put_count, player);
+                       allow_swap = allow_swap
+                               && (dst_can_put_count == -1 || dst_can_put_count >= try_put_count);
+                       swapDirections();
+               } else {
+                       dst_can_put_count = src_can_take_count;
                }
-               // Source is nodemeta
-               if (from_inv.type == InventoryLocation::NODEMETA) {
-                       ItemStack src_item = list_from->getItem(from_i);
-                       src_item.count = try_take_count;
-                       src_can_take_count = PLAYER_TO_SA(player)->nodemeta_inventory_AllowTake(
-                               *this, src_item, player);
-               }
-       }
-
-       // Query player inventories
-
-       // Move occurs in the same player inventory
-       if (from_inv.type == InventoryLocation::PLAYER &&
-                       from_inv == to_inv) {
-               src_can_take_count = PLAYER_TO_SA(player)->player_inventory_AllowMove(
-                       *this, try_take_count, player);
-               dst_can_put_count = src_can_take_count;
+               if (swap_expected != allow_swap)
+                       src_can_take_count = dst_can_put_count = 0;
        } else {
-               // Destination is a player
-               if (to_inv.type == InventoryLocation::PLAYER) {
-                       ItemStack src_item = list_from->getItem(from_i);
-                       src_item.count = try_take_count;
-                       dst_can_put_count = PLAYER_TO_SA(player)->player_inventory_AllowPut(
-                               *this, src_item, player);
-               }
-               // Source is a player
-               if (from_inv.type == InventoryLocation::PLAYER) {
-                       ItemStack src_item = list_from->getItem(from_i);
-                       src_item.count = try_take_count;
-                       src_can_take_count = PLAYER_TO_SA(player)->player_inventory_AllowTake(
-                               *this, src_item, player);
+               // Take from one inventory, put into another
+               dst_can_put_count = allowPut(src_item, player);
+               src_can_take_count = allowTake(src_item, player);
+               
+               bool swap_expected = allow_swap;
+               allow_swap = allow_swap
+                       && (src_can_take_count == -1 || src_can_take_count >= src_item.count)
+                       && (dst_can_put_count == -1 || dst_can_put_count >= src_item.count);
+               // A swap is expected, which means that we have to
+               // run the "allow" callbacks a second time with swapped inventories
+               if (allow_swap) {
+                       ItemStack dst_item = list_to->getItem(to_i);
+                       swapDirections();
+
+                       int src_can_take = allowPut(dst_item, player);
+                       int dst_can_put = allowTake(dst_item, player);
+                       allow_swap = allow_swap
+                               && (src_can_take == -1 || src_can_take >= dst_item.count)
+                               && (dst_can_put == -1 || dst_can_put >= dst_item.count);
+                       swapDirections();
                }
+               if (swap_expected != allow_swap)
+                       src_can_take_count = dst_can_put_count = 0;
        }
 
        int old_count = count;
 
        /* Modify count according to collected data */
-       count = try_take_count;
+       count = src_item.count;
        if (src_can_take_count != -1 && count > src_can_take_count)
                count = src_can_take_count;
        if (dst_can_put_count != -1 && count > dst_can_put_count)
@@ -367,7 +441,7 @@ void IMoveAction::apply(InventoryManager *mgr, ServerActiveObject *player, IGame
                return;
        }
 
-       ItemStack src_item = list_from->getItem(from_i);
+       src_item = list_from->getItem(from_i);
        src_item.count = count;
        ItemStack from_stack_was = list_from->getItem(from_i);
        ItemStack to_stack_was = list_to->getItem(to_i);
@@ -380,7 +454,8 @@ void IMoveAction::apply(InventoryManager *mgr, ServerActiveObject *player, IGame
        */
        bool did_swap = false;
        move_count = list_from->moveItem(from_i,
-               list_to, to_i, count, !caused_by_move_somewhere, &did_swap);
+               list_to, to_i, count, allow_swap, &did_swap);
+       assert(allow_swap == did_swap);
 
        // If source is infinite, reset it's stack
        if (src_can_take_count == -1) {
@@ -471,69 +546,29 @@ void IMoveAction::apply(InventoryManager *mgr, ServerActiveObject *player, IGame
                Report move to endpoints
        */
 
-       /* Detached inventories */
-
-       // Both endpoints are same detached
-       if (from_inv.type == InventoryLocation::DETACHED &&
-                       from_inv == to_inv) {
-               PLAYER_TO_SA(player)->detached_inventory_OnMove(
-                               *this, count, player);
-       } else {
-               // Destination is detached
-               if (to_inv.type == InventoryLocation::DETACHED) {
-                       PLAYER_TO_SA(player)->detached_inventory_OnPut(
-                               *this, src_item, player);
-               }
-               // Source is detached
-               if (from_inv.type == InventoryLocation::DETACHED) {
-                       PLAYER_TO_SA(player)->detached_inventory_OnTake(
-                               *this, src_item, player);
+       // Source = destination => move
+       if (from_inv == to_inv) {
+               onMove(count, player);
+               if (did_swap) {
+                       // Item is now placed in source list
+                       src_item = list_from->getItem(from_i);
+                       swapDirections();
+                       onMove(src_item.count, player);
+                       swapDirections();
                }
-       }
-
-       /* Node metadata inventories */
-
-       // Both endpoints are same nodemeta
-       if (from_inv.type == InventoryLocation::NODEMETA &&
-                       from_inv == to_inv) {
-               PLAYER_TO_SA(player)->nodemeta_inventory_OnMove(
-                       *this, count, player);
-       } else {
-               // Destination is nodemeta
-               if (to_inv.type == InventoryLocation::NODEMETA) {
-                       PLAYER_TO_SA(player)->nodemeta_inventory_OnPut(
-                               *this, src_item, player);
-               }
-               // Source is nodemeta
-               if (from_inv.type == InventoryLocation::NODEMETA) {
-                       PLAYER_TO_SA(player)->nodemeta_inventory_OnTake(
-                               *this, src_item, player);
-               }
-       }
-
-       // Player inventories
-
-       // Both endpoints are same player inventory
-       if (from_inv.type == InventoryLocation::PLAYER &&
-                       from_inv == to_inv) {
-               PLAYER_TO_SA(player)->player_inventory_OnMove(
-                       *this, count, player);
+               mgr->setInventoryModified(from_inv);
        } else {
-               // Destination is player inventory
-               if (to_inv.type == InventoryLocation::PLAYER) {
-                       PLAYER_TO_SA(player)->player_inventory_OnPut(
-                               *this, src_item, player);
+               onPutAndOnTake(src_item, player);
+               if (did_swap) {
+                       // Item is now placed in source list
+                       src_item = list_from->getItem(from_i);
+                       swapDirections();
+                       onPutAndOnTake(src_item, player);
+                       swapDirections();
                }
-               // Source is player inventory
-               if (from_inv.type == InventoryLocation::PLAYER) {
-                       PLAYER_TO_SA(player)->player_inventory_OnTake(
-                               *this, src_item, player);
-               }
-       }
-
-       mgr->setInventoryModified(from_inv);
-       if (inv_from != inv_to)
                mgr->setInventoryModified(to_inv);
+               mgr->setInventoryModified(from_inv);
+       }
 }
 
 void IMoveAction::clientApply(InventoryManager *mgr, IGameDef *gamedef)

diff --git a/src/inventorymanager.h b/src/inventorymanager.h
index 69bf3016905862ac8bf29399ef45ab1b20bdff9d..4ad5d3f49ae0b2d8eccae0f0ed4e440269873377 100644 (file)
--- a/src/inventorymanager.h
+++ b/src/inventorymanager.h
@@ -183,6 +183,18 @@ struct IMoveAction : public InventoryAction, public MoveAction
        void apply(InventoryManager *mgr, ServerActiveObject *player, IGameDef *gamedef);
 
        void clientApply(InventoryManager *mgr, IGameDef *gamedef);
+
+       void swapDirections();
+
+       void onPutAndOnTake(const ItemStack &src_item, ServerActiveObject *player) const;
+
+       void onMove(int count, ServerActiveObject *player) const;
+
+       int allowPut(const ItemStack &dst_item, ServerActiveObject *player) const;
+
+       int allowTake(const ItemStack &src_item, ServerActiveObject *player) const;
+
+       int allowMove(int try_take_count, ServerActiveObject *player) const;
 };
 
 struct IDropAction : public InventoryAction, public MoveAction