From 050964bed6005f8d816ddf362b9fc8675581d190 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lars=20M=C3=BCller?= <34514239+appgurueu@users.noreply.github.com> Date: Fri, 4 Sep 2020 20:49:07 +0200 Subject: [PATCH] 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 --- doc/lua_api.txt | 25 ++++ src/inventory.cpp | 9 +- src/inventorymanager.cpp | 301 ++++++++++++++++++++++----------------- src/inventorymanager.h | 12 ++ 4 files changed, 209 insertions(+), 138 deletions(-) diff --git a/doc/lua_api.txt b/doc/lua_api.txt index cc4af970c..86627c19e 100644 --- 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 349ee503d..cf72cb005 100644 --- 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 b6f464901..635bd2e4b 100644 --- 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); - } + // Shift-click: Cannot fill this stack, proceed with next slot + if (caused_by_move_somewhere && restitem.count == src_item.count) + return; + + if (allow_swap) { + // Swap will affect the entire stack if it can performed. + src_item = list_from->getItem(from_i); + count = src_item.count; } - /* Query node metadata inventories */ + if (from_inv == to_inv) { + // Move action within the same inventory + src_can_take_count = allowMove(src_item.count, player); - // 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; + 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; + } + if (swap_expected != allow_swap) + src_can_take_count = dst_can_put_count = 0; } 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); - } - // 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); - } - } + // 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(); - // 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; - } 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); + 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); + // 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(); + } + mgr->setInventoryModified(from_inv); } else { - // Destination is detached - if (to_inv.type == InventoryLocation::DETACHED) { - PLAYER_TO_SA(player)->detached_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 detached - if (from_inv.type == InventoryLocation::DETACHED) { - PLAYER_TO_SA(player)->detached_inventory_OnTake( - *this, src_item, player); - } - } - - /* 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); - } else { - // Destination is player inventory - if (to_inv.type == InventoryLocation::PLAYER) { - PLAYER_TO_SA(player)->player_inventory_OnPut( - *this, src_item, player); - } - // 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 69bf30169..4ad5d3f49 100644 --- 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