]> git.lizzy.rs Git - dragonfireclient.git/commitdiff
Fix item duplication if player dies during interact callback (alternative) (#11662)
authorsfan5 <sfan5@live.de>
Mon, 25 Oct 2021 18:30:27 +0000 (20:30 +0200)
committerGitHub <noreply@github.com>
Mon, 25 Oct 2021 18:30:27 +0000 (20:30 +0200)
builtin/game/item.lua
doc/lua_api.txt
src/network/serverpackethandler.cpp
src/script/cpp_api/s_item.cpp
src/script/cpp_api/s_item.h
src/script/lua_api/l_env.cpp
src/util/Optional.h

index c495a67bded05ecb67d1a0b5198ee61c3a29d1e0..03994758425b7ce912398e0c3d15d17c516d190a 100644 (file)
@@ -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)
index 69ac554936cef8ebd8f2e48d7e33111be8a74699..e47df468698728aa99f827cfeca43b2eed6c8834 100644 (file)
@@ -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.
index dc7be0e2380e2d2d9b927acaedd346b5c7863ecf..d4bef3ca2b3d7ed4dd9f8d42aece20577871f86c 100644 (file)
@@ -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);
                }
 
index 48dce14f39b37900eaab171fd2dd2dc846b6a860..b1916070e5baec90b5a08181e83c48afda97f4a3 100644 (file)
@@ -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;
index 25a3501f984afb96b2144ece575d59654e55695e..5015d8bd452188bc97de104685473180b8618e9f 100644 (file)
@@ -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);
index 98f8861faef9560e138184c5a0f3f02efefb1881..2c709d31b265215f5b474ef0cf31c387806859d6 100644 (file)
@@ -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;
index 9c2842b43201d025869a123614e25bfe38acae32..eda7fff8980839b8d040cf54a4a7b03173db3973 100644 (file)
@@ -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; }