Skip to content
Permalink
Browse files
Fix item duplication if player dies during interact callback (alterna…
…tive) (#11662)
  • Loading branch information
sfan5 committed Oct 25, 2021
1 parent d4b89eb commit 660e63dbae5901f8178b39ab40e6f770b8d7a215
Showing with 118 additions and 56 deletions.
  1. +28 −22 builtin/game/item.lua
  2. +7 −4 doc/lua_api.txt
  3. +26 −18 src/network/serverpackethandler.cpp
  4. +15 −6 src/script/cpp_api/s_item.cpp
  5. +11 −3 src/script/cpp_api/s_item.h
  6. +1 −1 src/script/lua_api/l_env.cpp
  7. +30 −2 src/util/Optional.h
@@ -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)
@@ -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.
@@ -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);
}

@@ -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;
@@ -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);
@@ -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;
@@ -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; }

0 comments on commit 660e63d

Please sign in to comment.