From 4245a7604b96b516b747e22f1410dee825922af1 Mon Sep 17 00:00:00 2001 From: SmallJoker Date: Tue, 26 Jul 2022 20:40:27 +0200 Subject: [PATCH] Inventory: Fix order of callbacks when swapping items --- doc/lua_api.md | 13 +++ games/devtest/mods/chest/chest.lua | 15 +++- src/inventory.cpp | 39 ++++---- src/inventory.h | 4 +- src/inventorymanager.cpp | 140 +++++++++++++++-------------- src/inventorymanager.h | 3 +- 6 files changed, 119 insertions(+), 95 deletions(-) diff --git a/doc/lua_api.md b/doc/lua_api.md index 13f4571ff..3a5f0460e 100644 --- a/doc/lua_api.md +++ b/doc/lua_api.md @@ -3640,6 +3640,19 @@ Player Inventory lists * Is not created automatically, use `InvRef:set_size` * Is only used to enhance the empty hand's tool capabilities +ItemStack transaction order +--------------------------- + +This list describes the situation for non-empty ItemStacks in both slots +that cannot be stacked at all, hence triggering an ItemStack swap operation. +Put/take callbacks on empty ItemStack are not executed. + +1. The "allow take" and "allow put" callbacks are each run once for the source + and destination inventory. +2. The allowed ItemStacks are exchanged. +3. The "on take" callbacks are run for the source and destination inventories +4. The "on put" callbacks are run for the source and destination inventories + Colors ====== diff --git a/games/devtest/mods/chest/chest.lua b/games/devtest/mods/chest/chest.lua index e347459ed..1f165fed4 100644 --- a/games/devtest/mods/chest/chest.lua +++ b/games/devtest/mods/chest/chest.lua @@ -29,12 +29,16 @@ minetest.register_node("chest:chest", { return inv:is_empty("main") end, allow_metadata_inventory_put = function(pos, listname, index, stack, player) - print_to_everything("Chest: ".. player:get_player_name() .. " triggered 'allow put' event for " .. stack:to_string()) - return stack:get_count() + print_to_everything("Chest: ".. player:get_player_name() .. " triggered 'allow put' (10) event for " .. stack:to_string()) + return 10 end, allow_metadata_inventory_take = function(pos, listname, index, stack, player) - print_to_everything("Chest: ".. player:get_player_name() .. " triggered 'allow take' event for " .. stack:to_string()) - return stack:get_count() + print_to_everything("Chest: ".. player:get_player_name() .. " triggered 'allow take' (20) event for " .. stack:to_string()) + return 20 + end, + allow_metadata_inventory_move = function(pos, from_list, from_index, to_list, to_index, count, player) + print_to_everything("Chest: ".. player:get_player_name() .. " triggered 'allow move' (30) event") + return 30 end, on_metadata_inventory_put = function(pos, listname, index, stack, player) print_to_everything("Chest: ".. player:get_player_name() .. " put " .. stack:to_string()) @@ -42,4 +46,7 @@ minetest.register_node("chest:chest", { on_metadata_inventory_take = function(pos, listname, index, stack, player) print_to_everything("Chest: ".. player:get_player_name() .. " took " .. stack:to_string()) end, + on_metadata_inventory_move = function(pos, from_list, from_index, to_list, to_index, count, player) + print_to_everything("Chest: ".. player:get_player_name() .. " moved " .. count) + end, }) diff --git a/src/inventory.cpp b/src/inventory.cpp index 2440c7f0c..65bbe0390 100644 --- a/src/inventory.cpp +++ b/src/inventory.cpp @@ -758,54 +758,53 @@ void InventoryList::moveItemSomewhere(u32 i, InventoryList *dest, u32 count) if (!leftover.empty()) { // Add the remaining part back to the source item - addItem(i, leftover); + ItemStack &source = getItem(i); + source.add(leftover.count); // do NOT use addItem to allow oversized stacks! } } -u32 InventoryList::moveItem(u32 i, InventoryList *dest, u32 dest_i, +ItemStack InventoryList::moveItem(u32 i, InventoryList *dest, u32 dest_i, u32 count, bool swap_if_needed, bool *did_swap) { + ItemStack moved; if (this == dest && i == dest_i) - return count; + return moved; // Take item from source list - ItemStack item1; if (count == 0) - item1 = changeItem(i, ItemStack()); + moved = changeItem(i, ItemStack()); else - item1 = takeItem(i, count); - - if (item1.empty()) - return 0; + moved = takeItem(i, count); // Try to add the item to destination list - count = item1.count; - item1 = dest->addItem(dest_i, item1); + ItemStack leftover = dest->addItem(dest_i, moved); // If something is returned, the item was not fully added - if (!item1.empty()) { - bool nothing_added = (item1.count == count); + if (!leftover.empty()) { + // Keep track of how many we actually moved + moved.remove(leftover.count); // Add any leftover stack back to the source stack. - item1.add(getItem(i).count); // leftover + source count - changeItem(i, item1); // do NOT use addItem to allow oversized stacks! + leftover.add(getItem(i).count); // leftover + source count + changeItem(i, leftover); // do NOT use addItem to allow oversized stacks! + leftover.clear(); // Swap if no items could be moved - if (nothing_added && swap_if_needed) { + if (moved.empty() && swap_if_needed) { // Tell that we swapped if (did_swap != NULL) { *did_swap = true; } // Take item from source list - item1 = changeItem(i, ItemStack()); + moved = changeItem(i, ItemStack()); // Adding was not possible, swap the items. - ItemStack item2 = dest->changeItem(dest_i, item1); + ItemStack item2 = dest->changeItem(dest_i, moved); // Put item from destination list to the source list changeItem(i, item2); - item1.clear(); // no leftover } } - return (count - item1.count); + + return moved; } void InventoryList::checkResizeLock() diff --git a/src/inventory.h b/src/inventory.h index e92893a8e..9f7265b76 100644 --- a/src/inventory.h +++ b/src/inventory.h @@ -286,8 +286,8 @@ public: // Move an item to a different list (or a different stack in the same list) // count is the maximum number of items to move (0 for everything) - // returns number of moved items - u32 moveItem(u32 i, InventoryList *dest, u32 dest_i, + // returns the moved stack + ItemStack moveItem(u32 i, InventoryList *dest, u32 dest_i, u32 count = 0, bool swap_if_needed = true, bool *did_swap = NULL); // like moveItem, but without a fixed destination index diff --git a/src/inventorymanager.cpp b/src/inventorymanager.cpp index 75d3936a9..6c66fd351 100644 --- a/src/inventorymanager.cpp +++ b/src/inventorymanager.cpp @@ -162,7 +162,20 @@ void IMoveAction::swapDirections() std::swap(from_i, to_i); } -void IMoveAction::onPutAndOnTake(const ItemStack &src_item, ServerActiveObject *player) const +void IMoveAction::onTake(const ItemStack &src_item, ServerActiveObject *player) const +{ + ServerScripting *sa = PLAYER_TO_SA(player); + 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::onPut(const ItemStack &src_item, ServerActiveObject *player) const { ServerScripting *sa = PLAYER_TO_SA(player); if (to_inv.type == InventoryLocation::DETACHED) @@ -173,15 +186,6 @@ void IMoveAction::onPutAndOnTake(const ItemStack &src_item, ServerActiveObject * 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 @@ -244,6 +248,8 @@ int IMoveAction::allowMove(int try_take_count, ServerActiveObject *player) const void IMoveAction::apply(InventoryManager *mgr, ServerActiveObject *player, IGameDef *gamedef) { + /// Necessary for executing Lua callbacks which may manipulate the inventory, + /// hence invalidate pointers needed by IMoveAction::apply auto get_borrow_checked_invlist = [mgr](const InventoryLocation &invloc, const std::string &listname) -> InventoryList::ResizeLocked { @@ -375,6 +381,8 @@ void IMoveAction::apply(InventoryManager *mgr, ServerActiveObject *player, IGame bool allow_swap = !list_to->itemFits(to_i, src_item, &restitem) && restitem.count == src_item.count && !caused_by_move_somewhere; + // move_count : How many items that were moved at the end + // count : Total items "in the queue" of being moved. Do not touch. move_count = src_item.count - restitem.count; // Shift-click: Cannot fill this stack, proceed with next slot @@ -383,10 +391,11 @@ void IMoveAction::apply(InventoryManager *mgr, ServerActiveObject *player, IGame } if (allow_swap) { - // Swap will affect the entire stack if it can performed. + // Swap will affect the entire stack (= count) if it can performed. src_item = list_from->getItem(from_i); - count = src_item.count; + move_count = src_item.count; } + src_item.count = move_count; // Temporary movement stack if (from_inv == to_inv) { // Move action within the same inventory @@ -409,16 +418,9 @@ void IMoveAction::apply(InventoryManager *mgr, ServerActiveObject *player, IGame src_can_take_count = dst_can_put_count = 0; } else { // Take from one inventory, put into another - int src_item_count = src_item.count; - if (caused_by_move_somewhere) - // When moving somewhere: temporarily use the actual movable stack - // size to ensure correct callback execution. - src_item.count = move_count; dst_can_put_count = allowPut(src_item, player); src_can_take_count = allowTake(src_item, player); - if (caused_by_move_somewhere) - // Reset source item count - src_item.count = src_item_count; + bool swap_expected = allow_swap; allow_swap = allow_swap && (src_can_take_count == -1 || src_can_take_count >= src_item.count) @@ -440,25 +442,20 @@ void IMoveAction::apply(InventoryManager *mgr, ServerActiveObject *player, IGame src_can_take_count = dst_can_put_count = 0; } - int old_count = count; + int old_move_count = move_count; - /* Modify count according to collected data */ - 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) - count = dst_can_put_count; + // Apply limits given by allow_* callbacks + if (src_can_take_count != -1) + move_count = (u32)std::min(src_can_take_count, move_count); + if (dst_can_put_count != -1) + move_count = (u32)std::min(dst_can_put_count, move_count); - /* Limit according to source item count */ - if (count > list_from->getItem(from_i).count) - count = list_from->getItem(from_i).count; + // allow_* callbacks should not modify the stack - but if they do - handle that. + if (move_count > list_from->getItem(from_i).count) + move_count = list_from->getItem(from_i).count; /* If no items will be moved, don't go further */ - if (count == 0) { - if (caused_by_move_somewhere) - // Set move count to zero, as no items have been moved - move_count = 0; - + if (move_count == 0) { // Undo client prediction. See 'clientApply' if (from_inv.type == InventoryLocation::PLAYER) list_from->setModified(); @@ -467,7 +464,7 @@ void IMoveAction::apply(InventoryManager *mgr, ServerActiveObject *player, IGame list_to->setModified(); infostream<<"IMoveAction::apply(): move was completely disallowed:" - <<" count="<getItem(from_i); @@ -603,26 +599,34 @@ void IMoveAction::apply(InventoryManager *mgr, ServerActiveObject *player, IGame } mgr->setInventoryModified(from_inv); } else { - int src_item_count = src_item.count; - if (caused_by_move_somewhere) - // When moving somewhere: temporarily use the actual movable stack - // size to ensure correct callback execution. - src_item.count = move_count; - onPutAndOnTake(src_item, player); - if (caused_by_move_somewhere) - // Reset source item count - src_item.count = src_item_count; + ItemStack swap_item; if (did_swap) { - // Item is now placed in source list + // Already swapped. The other stack is now placed in "from" list list_from = get_borrow_checked_invlist(from_inv, from_list); if (list_from) { - src_item = list_from->getItem(from_i); + swap_item = list_from->getItem(from_i); list_from.reset(); - swapDirections(); - onPutAndOnTake(src_item, player); - swapDirections(); } } + + // 1. Take the ItemStack (visually: freely detached) + onTake(src_item, player); + if (!swap_item.empty() && get_borrow_checked_invlist(to_inv, to_list)) { + swapDirections(); + onTake(swap_item, player); + swapDirections(); + } + + // 2. Put the ItemStack + if (get_borrow_checked_invlist(to_inv, to_list)) + onPut(src_item, player); + + if (!swap_item.empty() && get_borrow_checked_invlist(to_inv, to_list)) { + swapDirections(); + onPut(swap_item, player); + swapDirections(); + } + mgr->setInventoryModified(to_inv); mgr->setInventoryModified(from_inv); } diff --git a/src/inventorymanager.h b/src/inventorymanager.h index 704a493fd..8bc44f483 100644 --- a/src/inventorymanager.h +++ b/src/inventorymanager.h @@ -191,7 +191,8 @@ struct IMoveAction : public InventoryAction, public MoveAction void swapDirections(); - void onPutAndOnTake(const ItemStack &src_item, ServerActiveObject *player) const; + void onTake(const ItemStack &src_item, ServerActiveObject *player) const; + void onPut(const ItemStack &src_item, ServerActiveObject *player) const; void onMove(int count, ServerActiveObject *player) const;