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>

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`
 -----------
 

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);
 

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;
+	ItemStack src_item = list_from->getItem(from_i);
-	if (try_take_count == 0)
+	if (count > 0)
-		try_take_count = list_from->getItem(from_i).count;
+		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
+	// Shift-click: Cannot fill this stack, proceed with next slot
-	if (from_inv.type == InventoryLocation::DETACHED &&
+	if (caused_by_move_somewhere && restitem.count == src_item.count)
-			from_inv == to_inv) {
+		return;
-		src_can_take_count = PLAYER_TO_SA(player)->detached_inventory_AllowMove(
+
-			*this, try_take_count, player);
+	if (allow_swap) {
-		dst_can_put_count = src_can_take_count;
+		// 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 {
-		// 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 */
-
-	// 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;
+		}
+		if (swap_expected != allow_swap)
+			src_can_take_count = dst_can_put_count = 0;
 	} else {
-		// Destination is nodemeta
+		// Take from one inventory, put into another
-		if (to_inv.type == InventoryLocation::NODEMETA) {
+		dst_can_put_count = allowPut(src_item, player);
-			ItemStack src_item = list_from->getItem(from_i);
+		src_can_take_count = allowTake(src_item, player);
-			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);
-		}
-	}
 		
-	// Query player inventories
+		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();
 
-	// Move occurs in the same player inventory
+			int src_can_take = allowPut(dst_item, player);
-	if (from_inv.type == InventoryLocation::PLAYER &&
+			int dst_can_put = allowTake(dst_item, player);
-			from_inv == to_inv) {
+			allow_swap = allow_swap
-		src_can_take_count = PLAYER_TO_SA(player)->player_inventory_AllowMove(
+				&& (src_can_take == -1 || src_can_take >= dst_item.count)
-			*this, try_take_count, player);
+				&& (dst_can_put == -1 || dst_can_put >= dst_item.count);
-		dst_can_put_count = src_can_take_count;
+			swapDirections();
-	} 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);
 		}
+		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 */
+	// Source = destination => move
-
+	if (from_inv == to_inv) {
-	// Both endpoints are same detached
+		onMove(count, player);
-	if (from_inv.type == InventoryLocation::DETACHED &&
+		if (did_swap) {
-			from_inv == to_inv) {
+			// Item is now placed in source list
-		PLAYER_TO_SA(player)->detached_inventory_OnMove(
+			src_item = list_from->getItem(from_i);
-				*this, count, player);
+			swapDirections();
-	} else {
+			onMove(src_item.count, player);
-		// Destination is detached
+			swapDirections();
-		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);
-		}
-	}
-
-	/* 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)
+	} else {
+		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();
+		}
 		mgr->setInventoryModified(to_inv);
+		mgr->setInventoryModified(from_inv);
+	}
 }
 
 void IMoveAction::clientApply(InventoryManager *mgr, IGameDef *gamedef)

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