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>

1 files changed, 169 insertions, 134 deletions

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);
-		}
-	}
-
-	/* 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)