Fix on_drop return item semantics

doc/lua_api.md

@@ -6367,8 +6367,8 @@ Call these functions only at load time!
     * Determines how much of a stack may be taken, put or moved to a
       player inventory.
     * Function arguments: see `core.register_on_player_inventory_action`
-    * Return a numeric value to limit the amount of items to be taken, put or
-      moved. A value of `-1` for `take` will make the source stack infinite.
+    * The return value behaves like the respective `allow_metadata_inventory_*`
+      item callback.
 * `core.register_on_player_inventory_action(function(player, action, inventory, inventory_info))`
     * Called after an item take, put or move event from/to/in a player inventory
     * These inventory actions are recognized:
@@ -10104,8 +10104,13 @@ Used by `core.register_node`, `core.register_craftitem`, and
     -- default: core.item_secondary_use
 
     on_drop = function(itemstack, dropper, pos),
-    -- Shall drop item and return the leftover itemstack.
-    -- The dropper may be any ObjectRef or nil.
+    -- Called when the player drops this item from an inventory.
+    -- Must return the left-over itemstack. Returning `nil` is equivalent to
+    -- the original itemstack (= inventory not modified).
+    -- Parameters:
+    -- * `itemstack`: the `ItemStack` to be dropped.
+    -- * `dropped`: any `ObjectRef` or `nil`.
+    -- * `pos`: position to drop the item at.
     -- default: core.item_drop
 
     on_pickup = function(itemstack, picker, pointed_thing, time_from_last_punch, ...),
@@ -10113,11 +10118,11 @@ Used by `core.register_node`, `core.register_craftitem`, and
     -- Shall pick-up the item and return the leftover itemstack or nil to not
     -- modify the dropped item.
     -- Parameters:
-    -- * `itemstack`: The `ItemStack` to be picked up.
-    -- * `picker`: Any `ObjectRef` or `nil`.
-    -- * `pointed_thing` (optional): The dropped item (a `"__builtin:item"`
-    --   luaentity) as `type="object"` `pointed_thing`.
-    -- * `time_from_last_punch, ...` (optional): Other parameters from
+    -- * `itemstack`: the `ItemStack` to be picked up.
+    -- * `picker`: any `ObjectRef` or `nil`.
+    -- * `pointed_thing` (optional): the dropped item (a `"__builtin:item"`
+    --   luaentity) as a `pointed_thing` with `type="object"`.
+    -- * `time_from_last_punch, ...` (optional): other parameters from
     --   `luaentity:on_punch`.
     -- default: core.item_pickup
 
@@ -10593,12 +10598,12 @@ Used by `core.register_node`.
     allow_metadata_inventory_put = function(pos, listname, index, stack, player),
     -- Called when a player wants to put something into the inventory.
     -- Return value: number of items allowed to put.
-    -- Return value -1: Allow and don't modify item count in inventory.
+    -- Return value -1: Allow and don't modify destination item.
 
     allow_metadata_inventory_take = function(pos, listname, index, stack, player),
     -- Called when a player wants to take something out of the inventory.
     -- Return value: number of items allowed to take.
-    -- Return value -1: Allow and don't modify item count in inventory.
+    -- Return value -1: Allow and don't modify source item.
 
     on_metadata_inventory_move = function(pos, from_list, from_index, to_list, to_index, count, player),
     on_metadata_inventory_put = function(pos, listname, index, stack, player),
@@ -11483,12 +11488,12 @@ Used by `core.create_detached_inventory`.
     allow_put = function(inv, listname, index, stack, player),
     -- Called when a player wants to put something into the inventory.
     -- Return value: number of items allowed to put.
-    -- Return value -1: Allow and don't modify item count in inventory.
+    -- Return value -1: Allow and don't modify destination item.
 
     allow_take = function(inv, listname, index, stack, player),
     -- Called when a player wants to take something out of the inventory.
     -- Return value: number of items allowed to take.
-    -- Return value -1: Allow and don't modify item count in inventory.
+    -- Return value -1: Allow and don't modify source item.
 
     on_move = function(inv, from_list, from_index, to_list, to_index, count, player),
     on_put = function(inv, listname, index, stack, player),

src/inventorymanager.cpp

@@ -751,10 +751,8 @@ void IDropAction::apply(InventoryManager *mgr, ServerActiveObject *player, IGame
 	item1.count = take_count;
 	if(PLAYER_TO_SA(player)->item_OnDrop(item1, player,
 				player->getBasePosition())) {
-		int actually_dropped_count = take_count - item1.count;
-
-		if (actually_dropped_count == 0) {
-			infostream<<"Actually dropped no items"<<std::endl;
+		if (item1 == src_item) {
+			infostream << "Actually dropped no items" << std::endl;
 
 			// Revert client prediction. See 'clientApply'
 			if (from_inv.type == InventoryLocation::PLAYER)
@@ -762,17 +760,20 @@ void IDropAction::apply(InventoryManager *mgr, ServerActiveObject *player, IGame
 			return;
 		}
 
-		// If source isn't infinite
-		if (src_can_take_count != -1) {
-			// Take item from source list
-			ItemStack item2 = list_from->takeItem(from_i, actually_dropped_count);
+		// We need this number for OnTake reporting, but the problem here is
+		// that OnDrop can return a completely different item stack.
+		// Cut a few corners and report the count difference as far as sensible.
+		// (maybe OnTake should run before OnDrop??)
+		int really_dropped_count = src_item.count;
+		if (item1.count < src_item.count)
+			really_dropped_count = src_item.count - item1.count;
 
-			if (item2.count != actually_dropped_count)
-				errorstream<<"Could not take dropped count of items"<<std::endl;
-		}
-
-		src_item.count = actually_dropped_count;
+		// Modify source
+		if (src_can_take_count != -1)
+			list_from->changeItem(from_i, item1);
 		mgr->setInventoryModified(from_inv);
+
+		src_item.count = really_dropped_count;
 	}
 
 	infostream<<"IDropAction::apply(): dropped "