From fdb5cc0061b11d8207bfae2cbc03bd96fa643749 Mon Sep 17 00:00:00 2001 From: SmallJoker Date: Fri, 15 Dec 2023 10:24:04 +0100 Subject: [PATCH] Inventory: prevent item loss when stacking oversized ItemStacks (#14072) --- src/inventory.cpp | 17 +++++------ src/unittest/helpers/helper_moveaction.lua | 4 +++ src/unittest/test_moveaction.cpp | 35 ++++++++++++++++++++-- 3 files changed, 45 insertions(+), 11 deletions(-) diff --git a/src/inventory.cpp b/src/inventory.cpp index 9d42cee97..c99147323 100644 --- a/src/inventory.cpp +++ b/src/inventory.cpp @@ -777,20 +777,18 @@ u32 InventoryList::moveItem(u32 i, InventoryList *dest, u32 dest_i, return 0; // Try to add the item to destination list - u32 oldcount = item1.count; + count = item1.count; item1 = dest->addItem(dest_i, item1); // If something is returned, the item was not fully added if (!item1.empty()) { - // If olditem is returned, nothing was added. - bool nothing_added = (item1.count == oldcount); + bool nothing_added = (item1.count == count); - // If something else is returned, part of the item was left unadded. - // Add the other part back to the source item - addItem(i, item1); + // Add any leftover stack back to the source stack. + item1.add(getItem(i).count); // leftover + source count + changeItem(i, item1); // do NOT use addItem to allow oversized stacks! - // If olditem is returned, nothing was added. - // Swap the items + // Swap if no items could be moved if (nothing_added && swap_if_needed) { // Tell that we swapped if (did_swap != NULL) { @@ -802,9 +800,10 @@ u32 InventoryList::moveItem(u32 i, InventoryList *dest, u32 dest_i, ItemStack item2 = dest->changeItem(dest_i, item1); // Put item from destination list to the source list changeItem(i, item2); + item1.clear(); // no leftover } } - return (oldcount - item1.count); + return (count - item1.count); } void InventoryList::checkResizeLock() diff --git a/src/unittest/helpers/helper_moveaction.lua b/src/unittest/helpers/helper_moveaction.lua index 8cffe01b8..edb950e81 100644 --- a/src/unittest/helpers/helper_moveaction.lua +++ b/src/unittest/helpers/helper_moveaction.lua @@ -1,4 +1,8 @@ minetest.register_allow_player_inventory_action(function(player, action, inventory, info) + if action == "move" then + return info.count + end + if info.stack:get_name() == "default:water" then return 0 end diff --git a/src/unittest/test_moveaction.cpp b/src/unittest/test_moveaction.cpp index 7c2ecbb83..fb11faff8 100644 --- a/src/unittest/test_moveaction.cpp +++ b/src/unittest/test_moveaction.cpp @@ -36,6 +36,7 @@ public: void runTests(IGameDef *gamedef); void testMove(ServerActiveObject *obj, IGameDef *gamedef); + void testMoveFillStack(ServerActiveObject *obj, IGameDef *gamedef); void testMoveSomewhere(ServerActiveObject *obj, IGameDef *gamedef); void testMoveUnallowed(ServerActiveObject *obj, IGameDef *gamedef); void testMovePartial(ServerActiveObject *obj, IGameDef *gamedef); @@ -51,14 +52,24 @@ void TestMoveAction::runTests(IGameDef *gamedef) MockServer server; ServerScripting server_scripting(&server); - server_scripting.loadMod(Server::getBuiltinLuaPath() + DIR_DELIM "init.lua", BUILTIN_MOD_NAME); - server_scripting.loadMod(std::string(HELPERS_PATH) + DIR_DELIM "helper_moveaction.lua", BUILTIN_MOD_NAME); + try { + server_scripting.loadMod(Server::getBuiltinLuaPath() + DIR_DELIM "init.lua", BUILTIN_MOD_NAME); + server_scripting.loadMod( + std::string(HELPERS_PATH) + DIR_DELIM "helper_moveaction.lua", BUILTIN_MOD_NAME + ); + } catch (ModError &e) { + // Print backtrace in case of syntax errors + rawstream << e.what() << std::endl; + num_tests_failed = 1; + return; + } MetricsBackend mb; ServerEnvironment server_env(nullptr, &server_scripting, &server, "", &mb); MockServerActiveObject obj(&server_env); TEST(testMove, &obj, gamedef); + TEST(testMoveFillStack, &obj, gamedef); TEST(testMoveSomewhere, &obj, gamedef); TEST(testMoveUnallowed, &obj, gamedef); TEST(testMovePartial, &obj, gamedef); @@ -95,6 +106,26 @@ void TestMoveAction::testMove(ServerActiveObject *obj, IGameDef *gamedef) UASSERT(inv.p2.getList("main")->getItem(0).getItemString() == "default:stone 20"); } +void TestMoveAction::testMoveFillStack(ServerActiveObject *obj, IGameDef *gamedef) +{ + MockInventoryManager inv(gamedef); + + auto list = inv.p1.addList("main", 10); + list->changeItem(0, parse_itemstack("default:stone 209")); + list->changeItem(1, parse_itemstack("default:stone 90")); // 9 free slots + + apply_action("Move 209 player:p1 main 0 player:p1 main 1", &inv, obj, gamedef); + + UASSERT(list->getItem(0).getItemString() == "default:stone 200"); + UASSERT(list->getItem(1).getItemString() == "default:stone 99"); + + // Trigger stack swap + apply_action("Move 200 player:p1 main 0 player:p1 main 1", &inv, obj, gamedef); + + UASSERT(list->getItem(0).getItemString() == "default:stone 99"); + UASSERT(list->getItem(1).getItemString() == "default:stone 200"); +} + void TestMoveAction::testMoveSomewhere(ServerActiveObject *obj, IGameDef *gamedef) { MockInventoryManager inv(gamedef);