From 138f9375191e2fe72dcdd5281cb7261250531d20 Mon Sep 17 00:00:00 2001 From: obligaron Date: Sun, 25 Aug 2024 19:33:24 +0200 Subject: [PATCH] Fix infinite loop in unique item randomization --- Source/itemdat.h | 4 +- Source/items.cpp | 106 +++++++++++++++++++++++--------------- Source/items.h | 2 +- test/CMakeLists.txt | 1 + test/items_test.cpp | 120 ++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 190 insertions(+), 43 deletions(-) create mode 100644 test/items_test.cpp diff --git a/Source/itemdat.h b/Source/itemdat.h index 3a4cacee276..908864682fc 100644 --- a/Source/itemdat.h +++ b/Source/itemdat.h @@ -645,10 +645,10 @@ struct UniqueItem { ItemPower powers[6]; }; -extern std::vector AllItemsList; +extern DVL_API_FOR_TEST std::vector AllItemsList; extern std::vector ItemPrefixes; extern std::vector ItemSuffixes; -extern std::vector UniqueItems; +extern DVL_API_FOR_TEST std::vector UniqueItems; void LoadItemData(); diff --git a/Source/items.cpp b/Source/items.cpp index 8f7fe6345c2..8cd49c5daad 100644 --- a/Source/items.cpp +++ b/Source/items.cpp @@ -1419,19 +1419,25 @@ _item_indexes RndTypeItems(ItemType itemType, int imid, int lvl) }); } -_unique_items CheckUnique(Item &item, int lvl, int uper, int uidOffset = 0) +std::vector GetValidUniques(int lvl, unique_base_item baseItemId) { - if (GenerateRnd(100) > uper) - return UITEM_INVALID; - std::vector validUniques; for (int j = 0; j < static_cast(UniqueItems.size()); ++j) { if (!IsUniqueAvailable(j)) break; - if (UniqueItems[j].UIItemId == AllItemsList[item.IDidx].iItemId && lvl >= UniqueItems[j].UIMinLvl) { + if (UniqueItems[j].UIItemId == baseItemId && lvl >= UniqueItems[j].UIMinLvl) { validUniques.push_back(j); } } + return validUniques; +} + +_unique_items CheckUnique(Item &item, int lvl, int uper, int uidOffset = 0) +{ + if (GenerateRnd(100) > uper) + return UITEM_INVALID; + + auto validUniques = GetValidUniques(lvl, AllItemsList[item.IDidx].iItemId); if (validUniques.empty()) return UITEM_INVALID; @@ -3284,22 +3290,21 @@ void TryRandomUniqueItem(Item &item, _item_indexes idx, int8_t mLevel, int uper, if ((item._iCreateInfo & CF_UNIQUE) == 0 || item._iMiscId == IMISC_UNIQUE) return; - std::vector uids; + SetRndSeed(item._iSeed); + + // Get item base level, which is used in CheckUnique to get the correct valid uniques for the base item. + DiscardRandomValues(1); // GetItemAttrs + int blvl = GetItemBLevel(mLevel, item._iMiscId, onlygood, uper == 15); // Gather all potential unique items. uid is the index into UniqueItems. - for (size_t i = 0; i < UniqueItems.size(); ++i) { - const UniqueItem &uniqueItem = UniqueItems[i]; - // Verify the unique item base item matches idx. - bool isMatchingItemId = uniqueItem.UIItemId == AllItemsList[static_cast(idx)].iItemId; - // Verify itemLvl is at least the unique's minimum required level. - // mLevel remains unadjusted when arriving in this function, and there is no call to set iblvl until CheckUnique(), so we adjust here. - bool meetsLevelRequirement = ((uper == 15) ? mLevel + 4 : mLevel) >= uniqueItem.UIMinLvl; + auto validUniques = GetValidUniques(blvl, AllItemsList[static_cast(idx)].iItemId); + assert(!validUniques.empty()); + std::vector uids; + for (auto &possibleUid : validUniques) { // Verify item hasn't been dropped yet. We set this to true in MP, since uniques previously dropping shouldn't prevent further identical uniques from dropping. - bool uniqueNotDroppedAlready = !UniqueItemFlags[i] || gbIsMultiplayer; - - int uid = static_cast(i); - if (IsUniqueAvailable(uid) && isMatchingItemId && meetsLevelRequirement && uniqueNotDroppedAlready) - uids.emplace_back(uid); + if (!UniqueItemFlags[possibleUid] || gbIsMultiplayer) { + uids.emplace_back(possibleUid); + } } // If we find at least one unique in uids that hasn't been obtained yet, we can proceed getting a random unique. @@ -3324,45 +3329,66 @@ void TryRandomUniqueItem(Item &item, _item_indexes idx, int8_t mLevel, int uper, int32_t uidsIdx = std::max(0, GenerateRnd(static_cast(uids.size()))); // Index into uids, used to get a random uid from the uids vector. int uid = uids[uidsIdx]; // Actual unique id. + const UniqueItem &uniqueItem = UniqueItems[uid]; // If the selected unique was already generated, there is no need to fiddle with its parameters. if (item._iUid == uid) { - UniqueItemFlags[uid] = true; + if (!gbIsMultiplayer) { + UniqueItemFlags[uid] = true; + } return; } - const UniqueItem &uniqueItem = UniqueItems[uid]; - int targetLvl = 1; // Target level for reverse compatibility, since vanilla always takes the last applicable uid in the list. - - // Set target level. Ideally we use uper 15 to have a 16% chance of generating a unique item. - if (uniqueItem.UIMinLvl - 4 > 0) { // Negative level will underflow. Lvl 0 items may have unintended consequences. - uper = 15; - targetLvl = uniqueItem.UIMinLvl - 4; - } else { - uper = 1; - targetLvl = uniqueItem.UIMinLvl; + // Find our own id to calculate the offset in validUniques and check if we can generate a reverse-compatible version of the item. + int uidOffset = -1; + bool canGenerateReverseCompatible = true; + for (size_t i = 0; i < validUniques.size(); i++) { + if (validUniques[i] == uid) { + // Vanilla always picks the last unique, so the offset is calculated from the back of the valid unique list. + uidOffset = static_cast(validUniques.size() - i - 1); + } else if (uidOffset != -1 && UniqueItems[validUniques[i]].UIMinLvl <= uniqueItem.UIMinLvl) { + // Found an item with same or lower level as our desired unique after our unique. + // This means that we cannot possibly generate the item in reverse compatible mode and must rely on an offset. + canGenerateReverseCompatible = false; + } } + assert(uidOffset != -1); - // Amount to decrease the final uid by in CheckUnique() to get the desired unique. - const auto uidOffset = static_cast(std::count_if(UniqueItems.begin() + uid + 1, UniqueItems.end(), [&uniqueItem](UniqueItem &potentialMatch) { - return uniqueItem.UIItemId == potentialMatch.UIItemId && uniqueItem.UIMinLvl == potentialMatch.UIMinLvl; - })); + const Point itemPos = item.position; + if (canGenerateReverseCompatible) { + int targetLvl = 1; // Target level for reverse compatibility, since vanilla always takes the last applicable uid in the list. - Point itemPos = item.position; + // Set target level. Ideally we use uper 15 to have a 16% chance of generating a unique item. + if (uniqueItem.UIMinLvl - 4 > 0) { // Negative level will underflow. Lvl 0 items may have unintended consequences. + uper = 15; + targetLvl = uniqueItem.UIMinLvl - 4; + } else { + uper = 1; + targetLvl = uniqueItem.UIMinLvl; + } - // Force generate items until we find a uid match. - DiabloGenerator itemGenerator(item._iSeed); - do { + // Force generate items until we find a uid match. + DiabloGenerator itemGenerator(item._iSeed); + do { + item = {}; // Reset item data + item.position = itemPos; + // Set onlygood = true, to always get the required item base level for the unique. + SetupAllItems(*MyPlayer, item, idx, itemGenerator.advanceRndSeed(), targetLvl, uper, true, pregen); + } while (item._iUid != uid); + } else { + // Recreate the item with new offset, this creates the desired unique item but is not reverse compatible. + const int seed = item._iSeed; item = {}; // Reset item data item.position = itemPos; - SetupAllItems(*MyPlayer, item, idx, itemGenerator.advanceRndSeed(), targetLvl, uper, onlygood, pregen, uidOffset); - } while (item._iUid != uid); + SetupAllItems(*MyPlayer, item, idx, seed, mLevel, uper, onlygood, pregen, uidOffset); + item.dwBuff |= (uidOffset << 1) & CF_UIDOFFSET; + assert(item._iUid == uid); + } // Set item as obtained to prevent it from being dropped again in SP. if (!gbIsMultiplayer) { UniqueItemFlags[uid] = true; } - item.dwBuff |= (uidOffset << 1) & CF_UIDOFFSET; } void SpawnItem(Monster &monster, Point position, bool sendmsg, bool spawn /*= false*/) diff --git a/Source/items.h b/Source/items.h index 4fe6ca20ffc..9c2929af909 100644 --- a/Source/items.h +++ b/Source/items.h @@ -481,7 +481,7 @@ extern uint8_t ActiveItemCount; extern int8_t dItem[MAXDUNX][MAXDUNY]; extern bool ShowUniqueItemInfoBox; extern CornerStoneStruct CornerStone; -extern bool UniqueItemFlags[128]; +extern DVL_API_FOR_TEST bool UniqueItemFlags[128]; uint8_t GetOutlineColor(const Item &item, bool checkReq); bool IsItemAvailable(int i); diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 6e9a18972f2..ce9b73473da 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -24,6 +24,7 @@ set(tests drlg_l4_test effects_test inv_test + items_test math_test missiles_test pack_test diff --git a/test/items_test.cpp b/test/items_test.cpp new file mode 100644 index 00000000000..6ca2a6c46bb --- /dev/null +++ b/test/items_test.cpp @@ -0,0 +1,120 @@ +#include +#include + +#include "engine/random.hpp" +#include "items.h" +#include "player.h" +#include "spells.h" +#include "utils/str_cat.hpp" + +namespace devilution { +namespace { + +class ItemsTest : public ::testing::Test { +public: + void SetUp() override + { + Players.resize(1); + MyPlayer = &Players[0]; + for (auto &flag : UniqueItemFlags) + flag = false; + } + + static void SetUpTestSuite() + { + LoadItemData(); + LoadSpellData(); + gbIsSpawn = false; + gbIsMultiplayer = false; // to also test UniqueItemFlags + } +}; + +void GenerateAllUniques(bool hellfire, const size_t expectedUniques) +{ + gbIsHellfire = hellfire; + + std::mt19937 betterRng; + std::uniform_int_distribution dist(0, std::numeric_limits::max()); + + // Get seed for test run from time. If a test run fails, remember the seed and hardcode it here. + uint32_t testRunSeed = static_cast(time(nullptr)); + betterRng.seed(testRunSeed); + + std::set foundUniques; + + constexpr int max_iterations = 1000000; + int iteration = 0; + + for (int uniqueIndex = 0, n = static_cast(UniqueItems.size()); uniqueIndex < n; ++uniqueIndex) { + + if (!IsUniqueAvailable(uniqueIndex)) + continue; + + if (foundUniques.contains(uniqueIndex)) + continue; + + auto &uniqueItem = UniqueItems[uniqueIndex]; + + _item_indexes uniqueBaseIndex = IDI_GOLD; + for (std::underlying_type_t<_item_indexes> j = IDI_GOLD; j <= IDI_LAST; j++) { + if (!IsItemAvailable(j)) + continue; + if (AllItemsList[j].iItemId != uniqueItem.UIItemId) + continue; + if (AllItemsList[j].iRnd != IDROP_NEVER) + uniqueBaseIndex = static_cast<_item_indexes>(j); + break; + } + + if (uniqueBaseIndex == IDI_GOLD) + continue; // Unique not dropable (Quest-Unique) + + auto &baseItemData = AllItemsList[static_cast(uniqueBaseIndex)]; + + while (true) { + iteration += 1; + + if (iteration > max_iterations) + FAIL() << StrCat("Item ", uniqueItem.UIName, " not found in ", max_iterations, " tries with test run seed ", testRunSeed); + + Item testItem = {}; + testItem._iMiscId = baseItemData.iMiscId; + std::uniform_int_distribution dist(0, INT_MAX); + SetRndSeed(dist(betterRng)); + + int targetLvl = uniqueItem.UIMinLvl; + int uper = 1; + if (uniqueItem.UIMinLvl - 4 > 0) { + uper = 15; + targetLvl = uniqueItem.UIMinLvl - 4; + } + + SetupAllItems(*MyPlayer, testItem, uniqueBaseIndex, AdvanceRndSeed(), targetLvl, uper, true, false); + TryRandomUniqueItem(testItem, uniqueBaseIndex, targetLvl, uper, true, false); + SetupItem(testItem); + + if (testItem._iMagical != ITEM_QUALITY_UNIQUE) + continue; + + foundUniques.insert(testItem._iUid); + + if (testItem._iUid == uniqueIndex) + break; + } + } + + EXPECT_EQ(foundUniques.size(), expectedUniques) << StrCat("test run seed ", testRunSeed); +} + +TEST_F(ItemsTest, AllDiabloUniquesCanDrop) +{ + GenerateAllUniques(false, 79); +} + +TEST_F(ItemsTest, AllHellfireUniquesCanDrop) +{ + GenerateAllUniques(true, 99); +} + +} // namespace +} // namespace devilution