Fix item duplication if player dies during interact callback (alternative) (#11662)

5 files changed, 83 insertions, 30 deletions

diff --git a/src/network/serverpackethandler.cpp b/src/network/serverpackethandler.cpp
index dc7be0e23..d4bef3ca2 100644
--- 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,11 +1258,9 @@ 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);
@@ -1260,7 +1268,7 @@ void Server::handleCommand_Interact(NetworkPacket *pkt)
 			// 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
index 48dce14f3..b1916070e 100644
--- 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
index 25a3501f9..5015d8bd4 100644
--- 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
index 98f8861fa..2c709d31b 100644
--- 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
index 9c2842b43..eda7fff89 100644
--- 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 @@ public:
 
 	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; }