From 99a4f8b5ec837190791549fee70484204167e6e9 Mon Sep 17 00:00:00 2001 From: morfidon <57798071+morfidon@users.noreply.github.com> Date: Wed, 11 Mar 2026 17:17:05 +0100 Subject: [PATCH] Fix torn note inventory combine semantics Keep Na-Krul note auto-combine limited to player-facing inventory inserts so internal inventory reflow does not trigger partial side effects.\n\nTrack the inserted note through inventory compaction when combining, and add inventory regression tests for duplicate notes and sort behavior. --- Source/inv.cpp | 143 ++++++++++++++++++++++++++-------------------- Source/inv.h | 39 ++++++------- test/inv_test.cpp | 106 ++++++++++++++++++++++++++++++++++ 3 files changed, 205 insertions(+), 83 deletions(-) diff --git a/Source/inv.cpp b/Source/inv.cpp index a6c4147bb..fbd578ac0 100644 --- a/Source/inv.cpp +++ b/Source/inv.cpp @@ -140,8 +140,73 @@ namespace { OptionalOwnedClxSpriteList pInvCels; -/** - * @brief Adds an item to a player's InvGrid array +bool IsTornNaKrulNote(_item_indexes id) +{ + return IsAnyOf(id, IDI_NOTE1, IDI_NOTE2, IDI_NOTE3); +} + +bool PlayerHasAllTornNaKrulNotes(const Player &player) +{ + return HasInventoryItemWithId(player, IDI_NOTE1) + && HasInventoryItemWithId(player, IDI_NOTE2) + && HasInventoryItemWithId(player, IDI_NOTE3); +} + +void ConvertItemToFullNote(Item &item) +{ + item = {}; + GetItemAttrs(item, IDI_FULLNOTE, 16); + SetupItem(item); +} + +int TryCombineInsertedNaKrulNote(Player &player, int insertedInvIndex) +{ + if (insertedInvIndex < 0 || insertedInvIndex >= player._pNumInv) { + return insertedInvIndex; + } + + const _item_indexes insertedId = player.InvList[insertedInvIndex].IDidx; + if (!IsTornNaKrulNote(insertedId) || !PlayerHasAllTornNaKrulNotes(player)) { + return insertedInvIndex; + } + + player.Say(HeroSpeech::JustWhatIWasLookingFor, 10); + + std::array removedNoteIndices {}; + size_t removeCount = 0; + for (const _item_indexes note : { IDI_NOTE1, IDI_NOTE2, IDI_NOTE3 }) { + if (note == insertedId) { + continue; + } + + for (int i = 0; i < player._pNumInv; i++) { + if (player.InvList[i].IDidx == note) { + removedNoteIndices[removeCount++] = i; + break; + } + } + } + + if (removeCount != removedNoteIndices.size()) { + return insertedInvIndex; + } + + std::sort(removedNoteIndices.begin(), removedNoteIndices.end(), std::greater()); + for (const int removedNoteIndex : removedNoteIndices) { + player.RemoveInvItem(removedNoteIndex, false); + if (removedNoteIndex < insertedInvIndex) { + insertedInvIndex--; + } + } + + Item &combinedNote = player.InvList[insertedInvIndex]; + ConvertItemToFullNote(combinedNote); + combinedNote.updateRequiredStatsCacheForPlayer(player); + return insertedInvIndex; +} + +/** + * @brief Adds an item to a player's InvGrid array * @param player The player reference * @param invGridIndex Item's position in InvGrid (this should be the item's topleft grid tile) * @param invListIndex The item's InvList index (it's expected this already has +1 added to it since InvGrid can't store a 0 index) @@ -498,7 +563,7 @@ bool ChangeInvItem(Player &player, int slot, Size itemSize) if (prevItemId == 0) { player.InvList[player._pNumInv] = player.HoldItem.pop(); player._pNumInv++; - prevItemId = CheckSpecialInventoryItem(player, player._pNumInv - 1) + 1; + prevItemId = TryCombineInsertedNaKrulNote(player, player._pNumInv - 1) + 1; } else { const int invIndex = prevItemId - 1; if (player.HoldItem._itype == ItemType::Gold) @@ -512,7 +577,7 @@ bool ChangeInvItem(Player &player, int slot, Size itemSize) if (itemIndex == -prevItemId) itemIndex = 0; } - prevItemId = CheckSpecialInventoryItem(player, invIndex) + 1; + prevItemId = TryCombineInsertedNaKrulNote(player, invIndex) + 1; } itemSize = GetInventorySize(player.InvList[prevItemId - 1]); @@ -960,31 +1025,25 @@ void CheckInvCut(Player &player, Point cursorPosition, bool automaticMove, bool void TryCombineNaKrulNotes(Player &player, Item ¬eItem) { - const int idx = noteItem.IDidx; - const _item_indexes notes[] = { IDI_NOTE1, IDI_NOTE2, IDI_NOTE3 }; - - if (IsNoneOf(idx, IDI_NOTE1, IDI_NOTE2, IDI_NOTE3)) { + const _item_indexes idx = noteItem.IDidx; + if (!IsTornNaKrulNote(idx)) { return; } - for (const _item_indexes note : notes) { - if (idx != note && !HasInventoryItemWithId(player, note)) { - return; // the player doesn't have all notes - } + if (!PlayerHasAllTornNaKrulNotes(player)) { + return; // the player doesn't have all notes } MyPlayer->Say(HeroSpeech::JustWhatIWasLookingFor, 10); - for (const _item_indexes note : notes) { + for (const _item_indexes note : { IDI_NOTE1, IDI_NOTE2, IDI_NOTE3 }) { if (idx != note) { RemoveInventoryItemById(player, note); } } const Point position = noteItem.position; // copy the position to restore it after re-initialising the item - noteItem = {}; - GetItemAttrs(noteItem, IDI_FULLNOTE, 16); - SetupItem(noteItem); + ConvertItemToFullNote(noteItem); noteItem.position = position; // this ensures CleanupItem removes the entry in the dropped items lookup table } @@ -1124,49 +1183,6 @@ int CreateGoldItemInInventorySlot(Player &player, int slotIndex, int value) } // namespace -int CheckSpecialInventoryItem(Player &player, int invIndex) -{ - if (invIndex < 0 || invIndex >= player._pNumInv) { - return invIndex; - } - - const _item_indexes currentId = player.InvList[invIndex].IDidx; - const _item_indexes notes[] = { IDI_NOTE1, IDI_NOTE2, IDI_NOTE3 }; - - if (IsNoneOf(currentId, IDI_NOTE1, IDI_NOTE2, IDI_NOTE3)) { - return invIndex; - } - - for (const _item_indexes note : notes) { - if (!HasInventoryItemWithId(player, note)) { - return invIndex; - } - } - - player.Say(HeroSpeech::JustWhatIWasLookingFor, 10); - - for (const _item_indexes note : notes) { - if (note != currentId) { - RemoveInventoryItemById(player, note); - } - } - - for (int i = 0; i < player._pNumInv; i++) { - Item ¬eItem = player.InvList[i]; - if (noteItem.IDidx != currentId) { - continue; - } - - noteItem = {}; - GetItemAttrs(noteItem, IDI_FULLNOTE, 16); - SetupItem(noteItem); - noteItem.updateRequiredStatsCacheForPlayer(player); - return i; - } - - return invIndex; -} - void InvDrawSlotBack(const Surface &out, Point targetPosition, Size size, item_quality itemQuality) { SDL_Rect srcRect = MakeSdlRect(0, 0, size.width, size.height); @@ -1438,7 +1454,7 @@ bool CanFitItemInInventory(const Player &player, const Item &item) return static_cast(FindSlotForItem(player, GetInventorySize(item))); } -bool AutoPlaceItemInInventory(Player &player, const Item &item, bool sendNetworkMessage) +bool AutoPlaceItemInInventory(Player &player, const Item &item, bool sendNetworkMessage, InventoryInsertSemantics semantics) { const Size itemSize = GetInventorySize(item); std::optional targetSlot = FindSlotForItem(player, itemSize); @@ -1446,7 +1462,10 @@ bool AutoPlaceItemInInventory(Player &player, const Item &item, bool sendNetwork if (targetSlot) { player.InvList[player._pNumInv] = item; player._pNumInv++; - const int invIndex = CheckSpecialInventoryItem(player, player._pNumInv - 1); + int invIndex = player._pNumInv - 1; + if (semantics == InventoryInsertSemantics::PlayerAction) { + invIndex = TryCombineInsertedNaKrulNote(player, invIndex); + } AddItemToInvGrid(player, *targetSlot, invIndex + 1, GetInventorySize(player.InvList[invIndex]), sendNetworkMessage); player.CalcScrolls(); @@ -1506,7 +1525,7 @@ void ReorganizeInventory(Player &player) bool reorganizationFailed = false; for (const int index : sortedIndices) { const Item &item = tempStorage[index]; - if (!AutoPlaceItemInInventory(player, item, false)) { + if (!AutoPlaceItemInInventory(player, item, false, InventoryInsertSemantics::InternalRebuild)) { reorganizationFailed = true; break; } diff --git a/Source/inv.h b/Source/inv.h index 737af5858..a4bad9fc6 100644 --- a/Source/inv.h +++ b/Source/inv.h @@ -74,14 +74,19 @@ enum inv_xy_slot : uint8_t { }; enum item_color : uint8_t { - // clang-format off - ICOL_YELLOW = PAL16_YELLOW + 5, - ICOL_WHITE = PAL16_GRAY + 5, - ICOL_BLUE = PAL16_BLUE + 5, + // clang-format off + ICOL_YELLOW = PAL16_YELLOW + 5, + ICOL_WHITE = PAL16_GRAY + 5, + ICOL_BLUE = PAL16_BLUE + 5, ICOL_RED = PAL16_RED + 5, // clang-format on }; +enum class InventoryInsertSemantics { + PlayerAction, + InternalRebuild, +}; + extern bool invflag; extern const Rectangle InvRect[NUM_XY_SLOTS]; @@ -148,24 +153,16 @@ bool AutoEquip(Player &player, const Item &item, bool persistItem = true, bool s */ bool CanFitItemInInventory(const Player &player, const Item &item); -/** - * @brief Handles special item behavior after an item has been inserted into InvList. - * @param player The player whose inventory was modified. - * @param invIndex Index of the inserted item in InvList. - * @return The item's current InvList index. This can change if handling the item - * removes other inventory entries and the inventory gets compacted. - */ -int CheckSpecialInventoryItem(Player &player, int invIndex); - -/** - * @brief Attempts to place the given item in the specified player's inventory. - * @param player The player whose inventory will be used. - * @param item The item to be placed. - * @param sendNetworkMessage Set to true if you want a network message to be generated if the item is persisted. - * Should only be set if a local player is placing an item in a play session (not when creating a new game) - * @return 'True' if the item was placed on the player's inventory and 'False' otherwise. +/** + * @brief Attempts to place the given item in the specified player's inventory. + * @param player The player whose inventory will be used. + * @param item The item to be placed. + * @param sendNetworkMessage Set to true if you want a network message to be generated if the item is persisted. + * Should only be set if a local player is placing an item in a play session (not when creating a new game) + * @param semantics Distinguishes player-facing item insertion from internal inventory rebuilds. + * @return 'True' if the item was placed on the player's inventory and 'False' otherwise. */ -bool AutoPlaceItemInInventory(Player &player, const Item &item, bool sendNetworkMessage = false); +bool AutoPlaceItemInInventory(Player &player, const Item &item, bool sendNetworkMessage = false, InventoryInsertSemantics semantics = InventoryInsertSemantics::PlayerAction); /** * @brief Checks whether the given item can be placed on the specified player's belt. Returns 'True' when the item can be placed diff --git a/test/inv_test.cpp b/test/inv_test.cpp index c32119c4e..4cd5dc735 100644 --- a/test/inv_test.cpp +++ b/test/inv_test.cpp @@ -1,3 +1,5 @@ +#include + #include #include "cursor.h" @@ -64,6 +66,39 @@ void clear_inventory() MyPlayer->_pNumInv = 0; } +void place_inventory_item(int invIndex, int gridIndex, _item_indexes itemId) +{ + Item &item = MyPlayer->InvList[invIndex]; + InitializeItem(item, itemId); + item.updateRequiredStatsCacheForPlayer(*MyPlayer); + MyPlayer->InvGrid[gridIndex] = invIndex + 1; + MyPlayer->_pNumInv = std::max(MyPlayer->_pNumInv, invIndex + 1); +} + +int count_inventory_items_with_id(_item_indexes itemId) +{ + int count = 0; + for (int i = 0; i < MyPlayer->_pNumInv; i++) { + if (MyPlayer->InvList[i].IDidx == itemId) { + count++; + } + } + + return count; +} + +int count_positive_inv_grid_slots() +{ + int count = 0; + for (const int8_t cell : MyPlayer->InvGrid) { + if (cell > 0) { + count++; + } + } + + return count; +} + // Test that the scroll is used in the inventory in correct conditions TEST_F(InvTest, UseScroll_from_inventory) { @@ -385,5 +420,76 @@ TEST_F(InvTest, ItemSizeLastDiabloItem) EXPECT_EQ(GetInventorySize(testItem), Size(2, 3)); } +TEST_F(InvTest, AutoPlaceItemInInventoryCombinesInsertedNaKrulNote) +{ + if (!gbIsHellfire) return; + SNetInitializeProvider(SELCONN_LOOPBACK, nullptr); + + clear_inventory(); + place_inventory_item(0, 0, IDI_NOTE2); + place_inventory_item(1, 1, IDI_NOTE3); + + Item insertedNote {}; + InitializeItem(insertedNote, IDI_NOTE1); + insertedNote.updateRequiredStatsCacheForPlayer(*MyPlayer); + + ASSERT_TRUE(AutoPlaceItemInInventory(*MyPlayer, insertedNote)); + EXPECT_EQ(MyPlayer->_pNumInv, 1); + EXPECT_EQ(count_inventory_items_with_id(IDI_FULLNOTE), 1); + EXPECT_EQ(count_inventory_items_with_id(IDI_NOTE1), 0); + EXPECT_EQ(count_inventory_items_with_id(IDI_NOTE2), 0); + EXPECT_EQ(count_inventory_items_with_id(IDI_NOTE3), 0); + EXPECT_EQ(MyPlayer->InvGrid[0], 0); + EXPECT_EQ(MyPlayer->InvGrid[1], 0); + EXPECT_EQ(MyPlayer->InvGrid[2], 1); +} + +TEST_F(InvTest, AutoPlaceItemInInventoryCombinesInsertedDuplicateNaKrulNote) +{ + if (!gbIsHellfire) return; + SNetInitializeProvider(SELCONN_LOOPBACK, nullptr); + + clear_inventory(); + place_inventory_item(0, 0, IDI_NOTE1); + place_inventory_item(1, 1, IDI_NOTE2); + place_inventory_item(2, 2, IDI_NOTE3); + + Item insertedNote {}; + InitializeItem(insertedNote, IDI_NOTE1); + insertedNote.updateRequiredStatsCacheForPlayer(*MyPlayer); + + ASSERT_TRUE(AutoPlaceItemInInventory(*MyPlayer, insertedNote)); + EXPECT_EQ(MyPlayer->_pNumInv, 2); + EXPECT_EQ(count_inventory_items_with_id(IDI_FULLNOTE), 1); + EXPECT_EQ(count_inventory_items_with_id(IDI_NOTE1), 1); + EXPECT_EQ(count_inventory_items_with_id(IDI_NOTE2), 0); + EXPECT_EQ(count_inventory_items_with_id(IDI_NOTE3), 0); + EXPECT_EQ(count_positive_inv_grid_slots(), 2); + EXPECT_EQ(MyPlayer->InvGrid[0], 1); + EXPECT_EQ(MyPlayer->InvGrid[3], 2); + EXPECT_EQ(MyPlayer->InvList[0].IDidx, IDI_NOTE1); + EXPECT_EQ(MyPlayer->InvList[1].IDidx, IDI_FULLNOTE); +} + +TEST_F(InvTest, ReorganizeInventoryDoesNotCombineNaKrulNotes) +{ + if (!gbIsHellfire) return; + SNetInitializeProvider(SELCONN_LOOPBACK, nullptr); + + clear_inventory(); + place_inventory_item(0, 0, IDI_NOTE1); + place_inventory_item(1, 1, IDI_NOTE2); + place_inventory_item(2, 2, IDI_NOTE3); + + ReorganizeInventory(*MyPlayer); + + EXPECT_EQ(MyPlayer->_pNumInv, 3); + EXPECT_EQ(count_inventory_items_with_id(IDI_FULLNOTE), 0); + EXPECT_EQ(count_inventory_items_with_id(IDI_NOTE1), 1); + EXPECT_EQ(count_inventory_items_with_id(IDI_NOTE2), 1); + EXPECT_EQ(count_inventory_items_with_id(IDI_NOTE3), 1); + EXPECT_EQ(count_positive_inv_grid_slots(), 3); +} + } // namespace } // namespace devilution