From 628406a44ca71f398fc8cda49abfd1d7a684dfcc Mon Sep 17 00:00:00 2001 From: Gleb Mazovetskiy Date: Sun, 5 Jun 2022 10:57:25 +0100 Subject: [PATCH] Refactor inventory/belt item presence and removal Introduces new functions to check for presence of and remove player items. These functions do not rely on or expose item indices. They're implemented as free functions instead of Player methods due to complicated include dependencies between `player.h`, `inv.h`, and `inv_iterators.h`. We should probably look into cleaning this up at some point. --- Source/inv.cpp | 32 ++------- Source/inv.h | 149 ++++++++++++++++++++++++++++++++++++++- Source/inv_iterators.hpp | 1 - Source/monster.cpp | 4 +- Source/objects.cpp | 2 +- Source/player.cpp | 23 ------ Source/player.h | 8 --- Source/spells.cpp | 2 +- Source/towners.cpp | 32 ++++----- test/inv_test.cpp | 8 +-- 10 files changed, 176 insertions(+), 85 deletions(-) diff --git a/Source/inv.cpp b/Source/inv.cpp index e5f9e41a5..dc1eb8f91 100644 --- a/Source/inv.cpp +++ b/Source/inv.cpp @@ -862,17 +862,17 @@ void TryCombineNaKrulNotes(Player &player, Item ¬eItem) return; } - for (auto note : notes) { - if (idx != note && !player.HasItem(note)) { + for (_item_indexes note : notes) { + if (idx != note && !HasInventoryItemWithId(player, note)) { return; // the player doesn't have all notes } } MyPlayer->Say(HeroSpeech::JustWhatIWasLookingFor, 10); - for (auto note : notes) { + for (_item_indexes note : notes) { if (idx != note) { - player.TryRemoveInvItemById(note); + RemoveInventoryItemById(player, note); } } @@ -1943,30 +1943,6 @@ int8_t CheckInvHLight() return rv; } -void RemoveScroll(Player &player) -{ - const spell_id spellId = player._pSpell; - const auto isCurrentSpell = [spellId](const Item &item) { - return item.isScrollOf(spellId); - }; - { - const InventoryPlayerItemsRange items { player }; - const auto scrollIt = std::find_if(items.begin(), items.end(), isCurrentSpell); - if (scrollIt != items.end()) { - player.RemoveInvItem(static_cast(scrollIt.Index())); - return; - } - } - { - const BeltPlayerItemsRange items { player }; - const auto scrollIt = std::find_if(items.begin(), items.end(), isCurrentSpell); - if (scrollIt != items.end()) { - player.RemoveSpdBarItem(static_cast(scrollIt.Index())); - return; - } - } -} - bool UseScroll(const spell_id spell) { if (pcurs != CURSOR_HAND) diff --git a/Source/inv.h b/Source/inv.h index b5da7d856..eed9b47a1 100644 --- a/Source/inv.h +++ b/Source/inv.h @@ -8,6 +8,7 @@ #include #include "engine/point.hpp" +#include "inv_iterators.hpp" #include "items.h" #include "palette.h" #include "player.h" @@ -219,7 +220,6 @@ int InvPutItem(Player &player, Point position, Item &item); int SyncPutItem(Player &player, Point position, int idx, uint16_t icreateinfo, int iseed, int Id, int dur, int mdur, int ch, int mch, int ivalue, uint32_t ibuff, int toHit, int maxDam, int minStr, int minMag, int minDex, int ac); int SyncDropItem(Point position, int idx, uint16_t icreateinfo, int iseed, int id, int dur, int mdur, int ch, int mch, int ivalue, uint32_t ibuff, int toHit, int maxDam, int minStr, int minMag, int minDex, int ac); int8_t CheckInvHLight(); -void RemoveScroll(Player &player); bool UseScroll(spell_id spell); void UseStaffCharge(Player &player); bool UseStaff(spell_id spell); @@ -235,6 +235,153 @@ int CalculateGold(Player &player); */ Size GetInventorySize(const Item &item); +/** + * @brief Checks whether the player has an inventory item matching the predicate. + */ +template +bool HasInventoryItem(Player &player, Predicate &&predicate) +{ + const InventoryPlayerItemsRange items { player }; + return std::find_if(items.begin(), items.end(), std::forward(predicate)) != items.end(); +} + +/** + * @brief Checks whether the player has a belt item matching the predicate. + */ +template +bool HasBeltItem(Player &player, Predicate &&predicate) +{ + const BeltPlayerItemsRange items { player }; + return std::find_if(items.begin(), items.end(), std::forward(predicate)) != items.end(); +} + +/** + * @brief Checks whether the player has an inventory or a belt item matching the predicate. + */ +template +bool HasInventoryOrBeltItem(Player &player, Predicate &&predicate) +{ + return HasInventoryItem(player, predicate) || HasBeltItem(player, predicate); +} + +/** + * @brief Checks whether the player has an inventory item with the given ID (IDidx). + */ +inline bool HasInventoryItemWithId(Player &player, _item_indexes id) +{ + return HasInventoryItem(player, [id](const Item &item) { + return item.IDidx == id; + }); +} + +/** + * @brief Checks whether the player has a belt item with the given ID (IDidx). + */ +inline bool HasBeltItemWithId(Player &player, _item_indexes id) +{ + return HasBeltItem(player, [id](const Item &item) { + return item.IDidx == id; + }); +} + +/** + * @brief Checks whether the player has an inventory or a belt item with the given ID (IDidx). + */ +inline bool HasInventoryOrBeltItemWithId(Player &player, _item_indexes id) +{ + return HasInventoryItemWithId(player, id) || HasBeltItemWithId(player, id); +} + +/** + * @brief Removes the first inventory item matching the predicate. + * + * @return Whether an item was found and removed. + */ +template +bool RemoveInventoryItem(Player &player, Predicate &&predicate) +{ + const InventoryPlayerItemsRange items { player }; + const auto it = std::find_if(items.begin(), items.end(), std::forward(predicate)); + if (it == items.end()) + return false; + player.RemoveInvItem(static_cast(it.Index())); + return true; +} + +/** + * @brief Removes the first belt item matching the predicate. + * + * @return Whether an item was found and removed. + */ +template +bool RemoveBeltItem(Player &player, Predicate &&predicate) +{ + const BeltPlayerItemsRange items { player }; + const auto it = std::find_if(items.begin(), items.end(), std::forward(predicate)); + if (it == items.end()) + return false; + player.RemoveSpdBarItem(static_cast(it.Index())); + return true; +} + +/** + * @brief Removes the first inventory or belt item matching the predicate. + * + * @return Whether an item was found and removed. + */ +template +bool RemoveInventoryOrBeltItem(Player &player, Predicate &&predicate) +{ + return RemoveInventoryItem(player, predicate) || RemoveBeltItem(player, predicate); +} + +/** + * @brief Removes the first inventory item with the given id (IDidx). + * + * @return Whether an item was found and removed. + */ +inline bool RemoveInventoryItemById(Player &player, _item_indexes id) +{ + return RemoveInventoryItem(player, [id](const Item &item) { + return item.IDidx == id; + }); +} + +/** + * @brief Removes the first belt item with the given id (IDidx). + * + * @return Whether an item was found and removed. + */ +inline bool RemoveBeltItemById(Player &player, _item_indexes id) +{ + return RemoveInventoryItem(player, [id](const Item &item) { + return item.IDidx == id; + }); +} + +/** + * @brief Removes the first inventory or belt item with the given id (IDidx). + * + * @return Whether an item was found and removed. + */ +inline bool RemoveInventoryOrBeltItemById(Player &player, _item_indexes id) +{ + return RemoveInventoryItemById(player, id) || RemoveBeltItemById(player, id); +} + +/** + * @brief Removes the first inventory or belt scroll with the player's current spell. + * + * @return Whether a scroll was found and removed. + */ +inline bool RemoveCurrentSpellScroll(Player &player) +{ + const spell_id spellId = player._pSpell; + return RemoveInventoryOrBeltItem(player, [spellId](const Item &item) { + return item.isScrollOf(spellId); + }); +} + /* data */ } // namespace devilution diff --git a/Source/inv_iterators.hpp b/Source/inv_iterators.hpp index 3277ec187..b644f0186 100644 --- a/Source/inv_iterators.hpp +++ b/Source/inv_iterators.hpp @@ -5,7 +5,6 @@ #include #include -#include "inv.h" #include "items.h" #include "player.h" diff --git a/Source/monster.cpp b/Source/monster.cpp index 5cb6be63d..bc3ddfb81 100644 --- a/Source/monster.cpp +++ b/Source/monster.cpp @@ -4877,14 +4877,14 @@ void TalktoMonster(Monster &monster) } if (Quests[Q_LTBANNER].IsAvailable() && Quests[Q_LTBANNER]._qvar1 == 2) { - if (player.TryRemoveInvItemById(IDI_BANNER)) { + if (RemoveInventoryItemById(player, IDI_BANNER)) { Quests[Q_LTBANNER]._qactive = QUEST_DONE; monster.mtalkmsg = TEXT_BANNER12; monster._mgoal = MGOAL_INQUIRING; } } if (Quests[Q_VEIL].IsAvailable() && monster.mtalkmsg >= TEXT_VEIL9) { - if (player.TryRemoveInvItemById(IDI_GLDNELIX)) { + if (RemoveInventoryItemById(player, IDI_GLDNELIX)) { monster.mtalkmsg = TEXT_VEIL11; monster._mgoal = MGOAL_INQUIRING; } diff --git a/Source/objects.cpp b/Source/objects.cpp index 7c021ad4a..e6e80a5da 100644 --- a/Source/objects.cpp +++ b/Source/objects.cpp @@ -2633,7 +2633,7 @@ void OperatePedistal(int pnum, int i) return; } - if (Objects[i]._oVar6 == 3 || !Players[pnum].TryRemoveInvItemById(IDI_BLDSTONE)) { + if (Objects[i]._oVar6 == 3 || !RemoveInventoryItemById(Players[pnum], IDI_BLDSTONE)) { return; } diff --git a/Source/player.cpp b/Source/player.cpp index fd3037c81..89c97f4fb 100644 --- a/Source/player.cpp +++ b/Source/player.cpp @@ -1886,19 +1886,6 @@ void Player::CalcScrolls() EnsureValidReadiedSpell(*this); } -bool Player::HasItem(int item, int *idx) const -{ - for (int i = 0; i < _pNumInv; i++) { - if (InvList[i].IDidx == item) { - if (idx != nullptr) - *idx = i; - return true; - } - } - - return false; -} - void Player::RemoveInvItem(int iv, bool calcScrolls) { // Iterate through invGrid and remove every reference to item @@ -1931,16 +1918,6 @@ void Player::RemoveInvItem(int iv, bool calcScrolls) } } -bool Player::TryRemoveInvItemById(int item) -{ - int idx; - if (HasItem(item, &idx)) { - RemoveInvItem(idx); - return true; - } - return false; -} - void Player::RemoveSpdBarItem(int iv) { SpdList[iv].clear(); diff --git a/Source/player.h b/Source/player.h index 47a34b977..b0aa6becf 100644 --- a/Source/player.h +++ b/Source/player.h @@ -359,8 +359,6 @@ struct Player { && _pDexterity >= item._iMinDex; } - bool HasItem(int item, int *idx = nullptr) const; - /** * @brief Remove an item from player inventory * @param iv invList index of item to be removed @@ -368,12 +366,6 @@ struct Player { */ void RemoveInvItem(int iv, bool calcScrolls = true); - /** - * @brief Remove an item from player inventory and return true if the player has the item, return false otherwise - * @param item IDidx of item to be removed - */ - bool TryRemoveInvItemById(int item); - void RemoveSpdBarItem(int iv); /** diff --git a/Source/spells.cpp b/Source/spells.cpp index 5aabf931e..2b650934e 100644 --- a/Source/spells.cpp +++ b/Source/spells.cpp @@ -181,7 +181,7 @@ void UseMana(int id, spell_id sn) case RSPLTYPE_INVALID: break; case RSPLTYPE_SCROLL: - RemoveScroll(myPlayer); + RemoveCurrentSpellScroll(myPlayer); break; case RSPLTYPE_CHARGES: UseStaffCharge(myPlayer); diff --git a/Source/towners.cpp b/Source/towners.cpp index 38ee110e5..af8c0daa2 100644 --- a/Source/towners.cpp +++ b/Source/towners.cpp @@ -328,7 +328,7 @@ void TalkToBarOwner(Player &player, Towner &barOwner) return; } - if (bannerQuest._qvar2 == 1 && player.TryRemoveInvItemById(IDI_BANNER)) { + if (bannerQuest._qvar2 == 1 && RemoveInventoryItemById(player, IDI_BANNER)) { bannerQuest._qactive = QUEST_DONE; bannerQuest._qvar1 = 3; SpawnUnique(UITEM_HARCREST, barOwner.position + Direction::SouthWest); @@ -375,7 +375,7 @@ void TalkToBlackSmith(Player &player, Towner &blackSmith) return; } - if (Quests[Q_ROCK]._qvar2 == 1 && player.TryRemoveInvItemById(IDI_ROCK)) { + if (Quests[Q_ROCK]._qvar2 == 1 && RemoveInventoryItemById(player, IDI_ROCK)) { Quests[Q_ROCK]._qactive = QUEST_DONE; SpawnUnique(UITEM_INFRARING, blackSmith.position + Direction::SouthWest); InitQTextMsg(TEXT_INFRA7); @@ -395,7 +395,7 @@ void TalkToBlackSmith(Player &player, Towner &blackSmith) return; } - if (Quests[Q_ANVIL]._qvar2 == 1 && player.TryRemoveInvItemById(IDI_ANVIL)) { + if (Quests[Q_ANVIL]._qvar2 == 1 && RemoveInventoryItemById(player, IDI_ANVIL)) { Quests[Q_ANVIL]._qactive = QUEST_DONE; SpawnUnique(UITEM_GRISWOLD, blackSmith.position + Direction::SouthWest); InitQTextMsg(TEXT_ANVIL7); @@ -411,7 +411,7 @@ void TalkToBlackSmith(Player &player, Towner &blackSmith) void TalkToWitch(Player &player, Towner & /*witch*/) { if (Quests[Q_MUSHROOM]._qactive != QUEST_NOTAVAIL) { - if (Quests[Q_MUSHROOM]._qactive == QUEST_INIT && player.TryRemoveInvItemById(IDI_FUNGALTM)) { + if (Quests[Q_MUSHROOM]._qactive == QUEST_INIT && RemoveInventoryItemById(player, IDI_FUNGALTM)) { Quests[Q_MUSHROOM]._qactive = QUEST_ACTIVE; Quests[Q_MUSHROOM]._qlog = true; Quests[Q_MUSHROOM]._qvar1 = QS_TOMEGIVEN; @@ -420,7 +420,7 @@ void TalkToWitch(Player &player, Towner & /*witch*/) } if (Quests[Q_MUSHROOM]._qactive == QUEST_ACTIVE) { if (Quests[Q_MUSHROOM]._qvar1 >= QS_TOMEGIVEN && Quests[Q_MUSHROOM]._qvar1 < QS_MUSHGIVEN) { - if (player.TryRemoveInvItemById(IDI_MUSHROOM)) { + if (RemoveInventoryItemById(player, IDI_MUSHROOM)) { Quests[Q_MUSHROOM]._qvar1 = QS_MUSHGIVEN; QuestDialogTable[TOWN_HEALER][Q_MUSHROOM] = TEXT_MUSH3; QuestDialogTable[TOWN_WITCH][Q_MUSHROOM] = TEXT_NONE; @@ -435,12 +435,12 @@ void TalkToWitch(Player &player, Towner & /*witch*/) } } if (Quests[Q_MUSHROOM]._qvar1 >= QS_MUSHGIVEN) { - if (player.HasItem(IDI_BRAIN)) { + if (HasInventoryItemWithId(player, IDI_BRAIN)) { Quests[Q_MUSHROOM]._qmsg = TEXT_MUSH11; InitQTextMsg(TEXT_MUSH11); return; } - if (player.HasItem(IDI_SPECELIX)) { + if (HasInventoryItemWithId(player, IDI_SPECELIX)) { InitQTextMsg(TEXT_MUSH12); Quests[Q_MUSHROOM]._qactive = QUEST_DONE; AllItemsList[IDI_SPECELIX].iUsable = true; /// BUGFIX: This will cause the elixir to be usable in the next game @@ -456,7 +456,7 @@ void TalkToWitch(Player &player, Towner & /*witch*/) void TalkToBarmaid(Player &player, Towner & /*barmaid*/) { - if (!player._pLvlVisited[21] && player.HasItem(IDI_MAPOFDOOM)) { + if (!player._pLvlVisited[21] && HasInventoryItemWithId(player, IDI_MAPOFDOOM)) { Quests[Q_GRAVE]._qactive = QUEST_ACTIVE; Quests[Q_GRAVE]._qlog = true; Quests[Q_GRAVE]._qmsg = TEXT_GRAVE8; @@ -492,7 +492,7 @@ void TalkToHealer(Player &player, Towner &healer) } } if (Quests[Q_MUSHROOM]._qactive == QUEST_ACTIVE) { - if (Quests[Q_MUSHROOM]._qvar1 >= QS_MUSHGIVEN && Quests[Q_MUSHROOM]._qvar1 < QS_BRAINGIVEN && player.TryRemoveInvItemById(IDI_BRAIN)) { + if (Quests[Q_MUSHROOM]._qvar1 >= QS_MUSHGIVEN && Quests[Q_MUSHROOM]._qvar1 < QS_BRAINGIVEN && RemoveInventoryItemById(player, IDI_BRAIN)) { SpawnQuestItem(IDI_SPECELIX, healer.position + Displacement { 0, 1 }, 0, 0); InitQTextMsg(TEXT_MUSH4); Quests[Q_MUSHROOM]._qvar1 = QS_BRAINGIVEN; @@ -515,7 +515,7 @@ void TalkToStoryteller(Player &player, Towner & /*storyteller*/) { auto &betrayerQuest = Quests[Q_BETRAYER]; if (!gbIsMultiplayer) { - if (betrayerQuest._qactive == QUEST_INIT && player.TryRemoveInvItemById(IDI_LAZSTAFF)) { + if (betrayerQuest._qactive == QUEST_INIT && RemoveInventoryItemById(player, IDI_LAZSTAFF)) { InitQTextMsg(TEXT_VILE1); betrayerQuest._qlog = true; betrayerQuest._qactive = QUEST_ACTIVE; @@ -582,7 +582,7 @@ void TalkToFarmer(Player &player, Towner &farmer) switch (quest._qactive) { case QUEST_NOTAVAIL: case QUEST_INIT: - if (player.HasItem(IDI_RUNEBOMB)) { + if (HasInventoryItemWithId(player, IDI_RUNEBOMB)) { InitQTextMsg(TEXT_FARMER2); quest._qactive = QUEST_ACTIVE; quest._qvar1 = 1; @@ -615,7 +615,7 @@ void TalkToFarmer(Player &player, Towner &farmer) NetSendCmdQuest(true, quest); break; case QUEST_ACTIVE: - InitQTextMsg(player.HasItem(IDI_RUNEBOMB) ? TEXT_FARMER2 : TEXT_FARMER3); + InitQTextMsg(HasInventoryItemWithId(player, IDI_RUNEBOMB) ? TEXT_FARMER2 : TEXT_FARMER3); break; case QUEST_DONE: InitQTextMsg(TEXT_FARMER4); @@ -635,14 +635,14 @@ void TalkToFarmer(Player &player, Towner &farmer) void TalkToCowFarmer(Player &player, Towner &cowFarmer) { - if (player.TryRemoveInvItemById(IDI_GREYSUIT)) { + if (RemoveInventoryItemById(player, IDI_GREYSUIT)) { InitQTextMsg(TEXT_JERSEY7); return; } auto &quest = Quests[Q_JERSEY]; - if (player.TryRemoveInvItemById(IDI_BROWNSUIT)) { + if (RemoveInventoryItemById(player, IDI_BROWNSUIT)) { SpawnUnique(UITEM_BOVINE, cowFarmer.position + Direction::SouthEast); InitQTextMsg(TEXT_JERSEY8); quest._qactive = QUEST_DONE; @@ -652,7 +652,7 @@ void TalkToCowFarmer(Player &player, Towner &cowFarmer) return; } - if (player.HasItem(IDI_RUNEBOMB)) { + if (HasInventoryItemWithId(player, IDI_RUNEBOMB)) { InitQTextMsg(TEXT_JERSEY5); quest._qactive = QUEST_ACTIVE; quest._qvar1 = 1; @@ -721,7 +721,7 @@ void TalkToGirl(Player &player, Towner &girl) { auto &quest = Quests[Q_GIRL]; - if (quest._qactive != QUEST_DONE && player.TryRemoveInvItemById(IDI_THEODORE)) { + if (quest._qactive != QUEST_DONE && RemoveInventoryItemById(player, IDI_THEODORE)) { InitQTextMsg(TEXT_GIRL4); CreateAmulet(girl.position, 13, true, false); quest._qlog = false; diff --git a/test/inv_test.cpp b/test/inv_test.cpp index 156eede86..b0f718b91 100644 --- a/test/inv_test.cpp +++ b/test/inv_test.cpp @@ -192,7 +192,7 @@ TEST(Inv, RemoveSpdBarItem) } // Test removing a scroll from the inventory -TEST(Inv, RemoveScroll_inventory) +TEST(Inv, RemoveCurrentSpellScroll_inventory) { clear_inventory(); @@ -203,13 +203,13 @@ TEST(Inv, RemoveScroll_inventory) MyPlayer->InvList[0]._iMiscId = IMISC_SCROLL; MyPlayer->InvList[0]._iSpell = SPL_FIREBOLT; - RemoveScroll(*MyPlayer); + RemoveCurrentSpellScroll(*MyPlayer); EXPECT_EQ(MyPlayer->InvGrid[0], 0); EXPECT_EQ(MyPlayer->_pNumInv, 0); } // Test removing a scroll from the belt -TEST(Inv, RemoveScroll_belt) +TEST(Inv, RemoveCurrentSpellScroll_belt) { // Clear the belt for (int i = 0; i < MAXBELTITEMS; i++) { @@ -221,7 +221,7 @@ TEST(Inv, RemoveScroll_belt) MyPlayer->SpdList[3]._iMiscId = IMISC_SCROLL; MyPlayer->SpdList[3]._iSpell = SPL_FIREBOLT; - RemoveScroll(*MyPlayer); + RemoveCurrentSpellScroll(*MyPlayer); EXPECT_TRUE(MyPlayer->SpdList[3].isEmpty()); }