Inventory: Fix order of callbacks when swapping items

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

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,
 })

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

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

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<s32>(src_can_take_count, move_count);
+	if (dst_can_put_count != -1)
+		move_count = (u32)std::min<s32>(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="<<old_count
+				<<" move_count="<<old_move_count
 				<<" from inv=\""<<from_inv.dump()<<"\""
 				<<" list=\""<<from_list<<"\""
 				<<" i="<<from_i
@@ -479,8 +476,7 @@ void IMoveAction::apply(InventoryManager *mgr, ServerActiveObject *player, IGame
 		return;
 	}
 
-	src_item = list_from->getItem(from_i);
-	src_item.count = count;
+	// Backups stacks for infinite sources
 	ItemStack from_stack_was = list_from->getItem(from_i);
 	ItemStack to_stack_was = list_to->getItem(to_i);
 
@@ -491,13 +487,13 @@ void IMoveAction::apply(InventoryManager *mgr, ServerActiveObject *player, IGame
 		same as source), nothing happens
 	*/
 	bool did_swap = false;
-	move_count = list_from->moveItem(from_i,
-		list_to.get(), to_i, count, allow_swap, &did_swap);
-	if (caused_by_move_somewhere)
-		count = old_count;
+	src_item = list_from->moveItem(from_i,
+		list_to.get(), to_i, move_count, allow_swap, &did_swap);
+	move_count = src_item.count;
+
 	assert(allow_swap == did_swap);
 
-	// If source is infinite, reset it's stack
+	// If source is infinite, reset its stack
 	if (src_can_take_count == -1) {
 		// For the caused_by_move_somewhere == true case we didn't force-put the item,
 		// which guarantees there is no leftover, and code below would duplicate the
@@ -517,23 +513,23 @@ void IMoveAction::apply(InventoryManager *mgr, ServerActiveObject *player, IGame
 			}
 		}
 		if (move_count > 0 || did_swap) {
-			list_from->deleteItem(from_i);
-			list_from->addItem(from_i, from_stack_was);
+			list_from->changeItem(from_i, from_stack_was);
 		}
 	}
-	// If destination is infinite, reset it's stack and take count from source
+	// If destination is infinite, reset its stack and take count from source
 	if (dst_can_put_count == -1) {
-		list_to->deleteItem(to_i);
-		list_to->addItem(to_i, to_stack_was);
-		list_from->deleteItem(from_i);
-		list_from->addItem(from_i, from_stack_was);
-		list_from->takeItem(from_i, count);
+		list_to->changeItem(to_i, to_stack_was);
+		if (did_swap) {
+			// Undo swap result: set the expected stack + size
+			list_from->changeItem(from_i, from_stack_was);
+			list_from->takeItem(from_i, move_count);
+		}
 	}
 
 	infostream << "IMoveAction::apply(): moved"
 			<< " msom=" << move_somewhere
 			<< " caused=" << caused_by_move_somewhere
-			<< " count=" << count
+			<< " move_count=" << move_count
 			<< " from inv=\"" << from_inv.dump() << "\""
 			<< " list=\"" << from_list << "\""
 			<< " i=" << from_i
@@ -589,9 +585,9 @@ void IMoveAction::apply(InventoryManager *mgr, ServerActiveObject *player, IGame
 
 	// Source = destination => move
 	if (from_inv == to_inv) {
-		onMove(count, player);
+		onMove(move_count, player);
 		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);
@@ -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);
 	}

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;