Fix item duplication if player dies during interact callback (alterna…

…tive) (#11662)
minetest · Oct 25, 2021 · 660e63d · 660e63d
1 parent d4b89eb
commit 660e63d
Show file tree

Hide file tree

Showing 7 changed files with 118 additions and 56 deletions.
diff --git a/builtin/game/item.lua b/builtin/game/item.lua
@@ -499,34 +499,40 @@ function core.do_item_eat(hp_change, replace_with_item, itemstack, user, pointed
 			return result
 		end
 	end
+	if itemstack:take_item():is_empty() then
+		return itemstack
+	end
+
 	local def = itemstack:get_definition()
-	if itemstack:take_item() ~= nil then
-		user:set_hp(user:get_hp() + hp_change)
-
-		if def and def.sound and def.sound.eat then
-			core.sound_play(def.sound.eat, {
-				pos = user:get_pos(),
-				max_hear_distance = 16
-			}, true)
-		end
+	if def and def.sound and def.sound.eat then
+		core.sound_play(def.sound.eat, {
+			pos = user:get_pos(),
+			max_hear_distance = 16
+		}, true)
+	end
 
-		if replace_with_item then
-			if itemstack:is_empty() then
-				itemstack:add_item(replace_with_item)
+	-- Changing hp might kill the player causing mods to do who-knows-what to the
+	-- inventory, so do this before set_hp().
+	if replace_with_item then
+		if itemstack:is_empty() then
+			itemstack:add_item(replace_with_item)
+		else
+			local inv = user:get_inventory()
+			-- Check if inv is null, since non-players don't have one
+			if inv and inv:room_for_item("main", {name=replace_with_item}) then
+				inv:add_item("main", replace_with_item)
 			else
-				local inv = user:get_inventory()
-				-- Check if inv is null, since non-players don't have one
-				if inv and inv:room_for_item("main", {name=replace_with_item}) then
-					inv:add_item("main", replace_with_item)
-				else
-					local pos = user:get_pos()
-					pos.y = math.floor(pos.y + 0.5)
-					core.add_item(pos, replace_with_item)
-				end
+				local pos = user:get_pos()
+				pos.y = math.floor(pos.y + 0.5)
+				core.add_item(pos, replace_with_item)
 			end
 		end
 	end
-	return itemstack
+	user:set_wielded_item(itemstack)
+
+	user:set_hp(user:get_hp() + hp_change)
+
+	return nil -- don't overwrite wield item a second time
 end
 
 function core.item_eat(hp_change, replace_with_item)

diff --git a/doc/lua_api.txt b/doc/lua_api.txt
@@ -5421,7 +5421,7 @@ Inventory
 * `minetest.remove_detached_inventory(name)`
     * Returns a `boolean` indicating whether the removal succeeded.
 * `minetest.do_item_eat(hp_change, replace_with_item, itemstack, user, pointed_thing)`:
-  returns left over ItemStack.
+  returns leftover ItemStack or nil to indicate no inventory change
     * See `minetest.item_eat` and `minetest.register_on_item_eat`
 
 Formspec
@@ -7542,12 +7542,15 @@ Used by `minetest.register_node`, `minetest.register_craftitem`, and
         on_place = function(itemstack, placer, pointed_thing),
         -- When the 'place' key was pressed with the item in hand
         -- and a node was pointed at.
-        -- Shall place item and return the leftover itemstack.
+        -- Shall place item and return the leftover itemstack
+        -- or nil to not modify the inventory.
         -- The placer may be any ObjectRef or nil.
         -- default: minetest.item_place
 
         on_secondary_use = function(itemstack, user, pointed_thing),
         -- Same as on_place but called when not pointing at a node.
+        -- Function must return either nil if inventory shall not be modified,
+        -- or an itemstack to replace the original itemstack.
         -- The user may be any ObjectRef or nil.
         -- default: nil
 
@@ -7559,8 +7562,8 @@ Used by `minetest.register_node`, `minetest.register_craftitem`, and
         on_use = function(itemstack, user, pointed_thing),
         -- default: nil
         -- When user pressed the 'punch/mine' key with the item in hand.
-        -- Function must return either nil if no item shall be removed from
-        -- inventory, or an itemstack to replace the original itemstack.
+        -- Function must return either nil if inventory shall not be modified,
+        -- or an itemstack to replace the original itemstack.
         -- e.g. itemstack:take_item(); return itemstack
         -- Otherwise, the function is free to do what it wants.
         -- The user may be any ObjectRef or nil.

diff --git a/src/network/serverpackethandler.cpp b/src/network/serverpackethandler.cpp
@@ -921,6 +921,13 @@ bool Server::checkInteractDistance(RemotePlayer *player, const f32 d, const std:
 	return true;
 }
 
+// Tiny helper to retrieve the selected item into an Optional
+static inline void getWieldedItem(const PlayerSAO *playersao, Optional<ItemStack> &ret)
+{
+	ret = ItemStack();
+	playersao->getWieldedItem(&(*ret));
+}
+
 void Server::handleCommand_Interact(NetworkPacket *pkt)
 {
 	/*
@@ -1228,14 +1235,17 @@ void Server::handleCommand_Interact(NetworkPacket *pkt)
 
 	// Place block or right-click object
 	case INTERACT_PLACE: {
-		ItemStack selected_item;
-		playersao->getWieldedItem(&selected_item, nullptr);
+		Optional<ItemStack> selected_item;
+		getWieldedItem(playersao, selected_item);
 
 		// Reset build time counter
 		if (pointed.type == POINTEDTHING_NODE &&
-				selected_item.getDefinition(m_itemdef).type == ITEM_NODE)
+				selected_item->getDefinition(m_itemdef).type == ITEM_NODE)
 			getClient(peer_id)->m_time_from_building = 0.0;
 
+		const bool had_prediction = !selected_item->getDefinition(m_itemdef).
+			node_placement_prediction.empty();
+
 		if (pointed.type == POINTEDTHING_OBJECT) {
 			// Right click object
 
@@ -1248,19 +1258,17 @@ void Server::handleCommand_Interact(NetworkPacket *pkt)
 					<< pointed_object->getDescription() << std::endl;
 
 			// Do stuff
-			if (m_script->item_OnSecondaryUse(
-					selected_item, playersao, pointed)) {
-				if (playersao->setWieldedItem(selected_item)) {
+			if (m_script->item_OnSecondaryUse(selected_item, playersao, pointed)) {
+				if (selected_item.has_value() && playersao->setWieldedItem(*selected_item))
 					SendInventory(playersao, true);
-				}
 			}
 
 			pointed_object->rightClick(playersao);
 		} else if (m_script->item_OnPlace(selected_item, playersao, pointed)) {
 			// Placement was handled in lua
 
 			// Apply returned ItemStack
-			if (playersao->setWieldedItem(selected_item))
+			if (selected_item.has_value() && playersao->setWieldedItem(*selected_item))
 				SendInventory(playersao, true);
 		}
 
@@ -1272,8 +1280,7 @@ void Server::handleCommand_Interact(NetworkPacket *pkt)
 		RemoteClient *client = getClient(peer_id);
 		v3s16 blockpos = getNodeBlockPos(pointed.node_abovesurface);
 		v3s16 blockpos2 = getNodeBlockPos(pointed.node_undersurface);
-		if (!selected_item.getDefinition(m_itemdef
-				).node_placement_prediction.empty()) {
+		if (had_prediction) {
 			client->SetBlockNotSent(blockpos);
 			if (blockpos2 != blockpos)
 				client->SetBlockNotSent(blockpos2);
@@ -1287,15 +1294,15 @@ void Server::handleCommand_Interact(NetworkPacket *pkt)
 	} // action == INTERACT_PLACE
 
 	case INTERACT_USE: {
-		ItemStack selected_item;
-		playersao->getWieldedItem(&selected_item, nullptr);
+		Optional<ItemStack> selected_item;
+		getWieldedItem(playersao, selected_item);
 
-		actionstream << player->getName() << " uses " << selected_item.name
+		actionstream << player->getName() << " uses " << selected_item->name
 				<< ", pointing at " << pointed.dump() << std::endl;
 
 		if (m_script->item_OnUse(selected_item, playersao, pointed)) {
 			// Apply returned ItemStack
-			if (playersao->setWieldedItem(selected_item))
+			if (selected_item.has_value() && playersao->setWieldedItem(*selected_item))
 				SendInventory(playersao, true);
 		}
 
@@ -1304,16 +1311,17 @@ void Server::handleCommand_Interact(NetworkPacket *pkt)
 
 	// Rightclick air
 	case INTERACT_ACTIVATE: {
-		ItemStack selected_item;
-		playersao->getWieldedItem(&selected_item, nullptr);
+		Optional<ItemStack> selected_item;
+		getWieldedItem(playersao, selected_item);
 
 		actionstream << player->getName() << " activates "
-				<< selected_item.name << std::endl;
+				<< selected_item->name << std::endl;
 
 		pointed.type = POINTEDTHING_NOTHING; // can only ever be NOTHING
 
 		if (m_script->item_OnSecondaryUse(selected_item, playersao, pointed)) {
-			if (playersao->setWieldedItem(selected_item))
+			// Apply returned ItemStack
+			if (selected_item.has_value() && playersao->setWieldedItem(*selected_item))
 				SendInventory(playersao, true);
 		}
 

diff --git a/src/script/cpp_api/s_item.cpp b/src/script/cpp_api/s_item.cpp
@@ -59,13 +59,14 @@ bool ScriptApiItem::item_OnDrop(ItemStack &item,
 	return true;
 }
 
-bool ScriptApiItem::item_OnPlace(ItemStack &item,
+bool ScriptApiItem::item_OnPlace(Optional<ItemStack> &ret_item,
 		ServerActiveObject *placer, const PointedThing &pointed)
 {
 	SCRIPTAPI_PRECHECKHEADER
 
 	int error_handler = PUSH_ERROR_HANDLER(L);
 
+	const ItemStack &item = *ret_item;
 	// Push callback function on stack
 	if (!getItemCallback(item.name.c_str(), "on_place"))
 		return false;
@@ -82,22 +83,25 @@ bool ScriptApiItem::item_OnPlace(ItemStack &item,
 	PCALL_RES(lua_pcall(L, 3, 1, error_handler));
 	if (!lua_isnil(L, -1)) {
 		try {
-			item = read_item(L, -1, getServer()->idef());
+			ret_item = read_item(L, -1, getServer()->idef());
 		} catch (LuaError &e) {
 			throw WRAP_LUAERROR(e, "item=" + item.name);
 		}
+	} else {
+		ret_item = nullopt;
 	}
 	lua_pop(L, 2);  // Pop item and error handler
 	return true;
 }
 
-bool ScriptApiItem::item_OnUse(ItemStack &item,
+bool ScriptApiItem::item_OnUse(Optional<ItemStack> &ret_item,
 		ServerActiveObject *user, const PointedThing &pointed)
 {
 	SCRIPTAPI_PRECHECKHEADER
 
 	int error_handler = PUSH_ERROR_HANDLER(L);
 
+	const ItemStack &item = *ret_item;
 	// Push callback function on stack
 	if (!getItemCallback(item.name.c_str(), "on_use"))
 		return false;
@@ -109,22 +113,25 @@ bool ScriptApiItem::item_OnUse(ItemStack &item,
 	PCALL_RES(lua_pcall(L, 3, 1, error_handler));
 	if(!lua_isnil(L, -1)) {
 		try {
-			item = read_item(L, -1, getServer()->idef());
+			ret_item = read_item(L, -1, getServer()->idef());
 		} catch (LuaError &e) {
 			throw WRAP_LUAERROR(e, "item=" + item.name);
 		}
+	} else {
+		ret_item = nullopt;
 	}
 	lua_pop(L, 2);  // Pop item and error handler
 	return true;
 }
 
-bool ScriptApiItem::item_OnSecondaryUse(ItemStack &item,
+bool ScriptApiItem::item_OnSecondaryUse(Optional<ItemStack> &ret_item,
 		ServerActiveObject *user, const PointedThing &pointed)
 {
 	SCRIPTAPI_PRECHECKHEADER
 
 	int error_handler = PUSH_ERROR_HANDLER(L);
 
+	const ItemStack &item = *ret_item;
 	if (!getItemCallback(item.name.c_str(), "on_secondary_use"))
 		return false;
 
@@ -134,10 +141,12 @@ bool ScriptApiItem::item_OnSecondaryUse(ItemStack &item,
 	PCALL_RES(lua_pcall(L, 3, 1, error_handler));
 	if (!lua_isnil(L, -1)) {
 		try {
-			item = read_item(L, -1, getServer()->idef());
+			ret_item = read_item(L, -1, getServer()->idef());
 		} catch (LuaError &e) {
 			throw WRAP_LUAERROR(e, "item=" + item.name);
 		}
+	} else {
+		ret_item = nullopt;
 	}
 	lua_pop(L, 2);  // Pop item and error handler
 	return true;

diff --git a/src/script/cpp_api/s_item.h b/src/script/cpp_api/s_item.h
@@ -21,6 +21,7 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 
 #include "cpp_api/s_base.h"
 #include "irr_v3d.h"
+#include "util/Optional.h"
 
 struct PointedThing;
 struct ItemStack;
@@ -35,13 +36,20 @@ class ScriptApiItem
 : virtual public ScriptApiBase
 {
 public:
+	/*
+	 * Functions with Optional<ItemStack> are for callbacks where Lua may
+	 * want to prevent the engine from modifying the inventory after it's done.
+	 * This has a longer backstory where on_use may need to empty the player's
+	 * inventory without the engine interfering (see issue #6546).
+	 */
+
 	bool item_OnDrop(ItemStack &item,
 			ServerActiveObject *dropper, v3f pos);
-	bool item_OnPlace(ItemStack &item,
+	bool item_OnPlace(Optional<ItemStack> &item,
 			ServerActiveObject *placer, const PointedThing &pointed);
-	bool item_OnUse(ItemStack &item,
+	bool item_OnUse(Optional<ItemStack> &item,
 			ServerActiveObject *user, const PointedThing &pointed);
-	bool item_OnSecondaryUse(ItemStack &item,
+	bool item_OnSecondaryUse(Optional<ItemStack> &item,
 			ServerActiveObject *user, const PointedThing &pointed);
 	bool item_OnCraft(ItemStack &item, ServerActiveObject *user,
 			const InventoryList *old_craft_grid, const InventoryLocation &craft_inv);

diff --git a/src/script/lua_api/l_env.cpp b/src/script/lua_api/l_env.cpp
@@ -477,7 +477,7 @@ int ModApiEnvMod::l_place_node(lua_State *L)
 		return 1;
 	}
 	// Create item to place
-	ItemStack item(ndef->get(n).name, 1, 0, idef);
+	Optional<ItemStack> item = ItemStack(ndef->get(n).name, 1, 0, idef);
 	// Make pointed position
 	PointedThing pointed;
 	pointed.type = POINTEDTHING_NODE;

diff --git a/src/util/Optional.h b/src/util/Optional.h
@@ -19,6 +19,7 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 
 #pragma once
 
+#include <utility>
 #include "debug.h"
 
 struct nullopt_t
@@ -43,18 +44,38 @@ class Optional
 public:
 	Optional() noexcept {}
 	Optional(nullopt_t) noexcept {}
+
 	Optional(const T &value) noexcept : m_has_value(true), m_value(value) {}
+	Optional(T &&value) noexcept : m_has_value(true), m_value(std::move(value)) {}
+
 	Optional(const Optional<T> &other) noexcept :
 			m_has_value(other.m_has_value), m_value(other.m_value)
+	{}
+	Optional(Optional<T> &&other) noexcept :
+			m_has_value(other.m_has_value), m_value(std::move(other.m_value))
 	{
+		other.m_has_value = false;
 	}
 
-	void operator=(nullopt_t) noexcept { m_has_value = false; }
+	Optional<T> &operator=(nullopt_t) noexcept { m_has_value = false; return *this; }
 
-	void operator=(const Optional<T> &other) noexcept
+	Optional<T> &operator=(const Optional<T> &other) noexcept
 	{
+		if (&other == this)
+			return *this;
 		m_has_value = other.m_has_value;
 		m_value = other.m_value;
+		return *this;
+	}
+
+	Optional<T> &operator=(Optional<T> &&other) noexcept
+	{
+		if (&other == this)
+			return *this;
+		m_has_value = other.m_has_value;
+		m_value = std::move(other.m_value);
+		other.m_has_value = false;
+		return *this;
 	}
 
 	T &value()
@@ -71,6 +92,13 @@ class Optional
 
 	const T &value_or(const T &def) const { return m_has_value ? m_value : def; }
 
+	// Unchecked access consistent with std::optional
+	T* operator->() { return &m_value; }
+	const T* operator->() const { return &m_value; }
+
+	T& operator*() { return m_value; }
+	const T& operator*() const { return m_value; }
+
 	bool has_value() const noexcept { return m_has_value; }
 
 	explicit operator bool() const { return m_has_value; }