From 3388aa9e58b3ac5001dd17615ed8c5b8abd6e720 Mon Sep 17 00:00:00 2001 From: ephphatha Date: Tue, 29 Jun 2021 00:31:18 +1000 Subject: [PATCH] Replace RNG calls used to initialise quest pools with instanced vanilla::RandomEngine This shows how the RandomEngine classes can be used to isolate pseudo-random generation logic. --- Source/engine/random.hpp | 35 ++++++++++++++++ Source/quests.cpp | 88 ++++++++++++++++++++++++---------------- Source/quests.h | 7 ++++ test/random_test.cpp | 17 +++++++- 4 files changed, 112 insertions(+), 35 deletions(-) diff --git a/Source/engine/random.hpp b/Source/engine/random.hpp index df7e09d3171d..f834bf0afcff 100644 --- a/Source/engine/random.hpp +++ b/Source/engine/random.hpp @@ -11,6 +11,8 @@ #include #include +#include "utils/stdcompat/optional.hpp" + namespace devilution { inline namespace randomV1 { @@ -246,12 +248,22 @@ class RandomEngine { */ const uint32_t initialState; + /** + * @brief Shows how many times the engine has advanced since being created + * @return The number of times the generator function has been called + */ + uint32_t GetCount() const + { + return engineCount; + } + /** * @brief Advances the engine and returns a random value * @return A random number in the range [0, 2^32) */ uint32_t operator()() { + engineCount++; return engine(); } @@ -341,11 +353,34 @@ class RandomEngine { return RandomLessThan(outcomes) == 0; } + /** + * @brief Returns one of a list of choices at random, where each option has equal chance of being selected. + * @tparam T A common type that all items share + * @param choices The list to choose from + * @return An optional containing one of the list chosen at random, may be empty to represent vanilla RNG bug cases + */ + template + std::optional RandomChoice(std::initializer_list choices) + { + int offset = RandomLessThan(choices.size()); + + if (offset < 0) { + return {}; + } + + return *(choices.begin() + offset); + } + private: /** * @brief A LCG using Borland C++ constants. */ std::linear_congruential_engine engine; + + /** + * @brief The number of times this engine has advanced since being created. + */ + uint32_t engineCount = 0; }; } // namespace vanilla diff --git a/Source/quests.cpp b/Source/quests.cpp index 9ad3fc3b21ba..f4512ce32813 100644 --- a/Source/quests.cpp +++ b/Source/quests.cpp @@ -86,30 +86,6 @@ const char *const questtrigstr[5] = { N_(/* TRANSLATORS: Quest Map*/ "A Dark Passage"), N_(/* TRANSLATORS: Quest Map*/ "Unholy Altar") }; -/** - * A quest group containing the three quests the Butcher, - * Ogden's Sign and Gharbad the Weak, which ensures that exactly - * two of these three quests appear in any single player game. - */ -int QuestGroup1[3] = { Q_BUTCHER, Q_LTBANNER, Q_GARBUD }; -/** - * A quest group containing the three quests Halls of the Blind, - * the Magic Rock and Valor, which ensures that exactly two of - * these three quests appear in any single player game. - */ -int QuestGroup2[3] = { Q_BLIND, Q_ROCK, Q_BLOOD }; -/** - * A quest group containing the three quests Black Mushroom, - * Zhar the Mad and Anvil of Fury, which ensures that exactly - * two of these three quests appear in any single player game. - */ -int QuestGroup3[3] = { Q_MUSHROOM, Q_ZHAR, Q_ANVIL }; -/** - * A quest group containing the two quests Lachdanan and Warlord - * of Blood, which ensures that exactly one of these two quests - * appears in any single player game. - */ -int QuestGroup4[2] = { Q_VEIL, Q_WARLORD }; void InitQuests() { @@ -160,16 +136,8 @@ void InitQuests() } if (!gbIsMultiplayer && sgOptions.Gameplay.bRandomizeQuests) { - vanilla::SetRndSeed(glSeedTbl[15]); - if (vanilla::GenerateRnd(2) != 0) - quests[Q_PWATER]._qactive = QUEST_NOTAVAIL; - else - quests[Q_SKELKING]._qactive = QUEST_NOTAVAIL; - - quests[QuestGroup1[vanilla::GenerateRnd(sizeof(QuestGroup1) / sizeof(int))]]._qactive = QUEST_NOTAVAIL; - quests[QuestGroup2[vanilla::GenerateRnd(sizeof(QuestGroup2) / sizeof(int))]]._qactive = QUEST_NOTAVAIL; - quests[QuestGroup3[vanilla::GenerateRnd(sizeof(QuestGroup3) / sizeof(int))]]._qactive = QUEST_NOTAVAIL; - quests[QuestGroup4[vanilla::GenerateRnd(sizeof(QuestGroup4) / sizeof(int))]]._qactive = QUEST_NOTAVAIL; + // Quests are set from the seed used to generate level 16. + InitialiseQuestPools(glSeedTbl[15], quests); } #ifdef _DEBUG if (questdebug != -1) @@ -191,6 +159,58 @@ void InitQuests() quests[Q_BETRAYER]._qvar1 = 2; } +void InitialiseQuestPools(uint32_t seed, QuestStruct quests[]) +{ + /** + * @brief To ensure saved games have the same quests available after saving and loading an RNG is created from + * the provided seed. + */ + vanilla::RandomEngine rng { seed }; + + /** + * @brief The quests Poison Water and Skeleton King share the same quest pool, only one of the two is available in + * a vanilla single player game. + */ + auto questPool1ID = rng.RandomChoice({ Q_PWATER, Q_SKELKING }); + quests[questPool1ID.value_or(Q_SKELKING)]._qactive = QUEST_NOTAVAIL; + + /** + * @brief The quests Butcher, Ogden's Sign, and Gharbad the Weak share the same quest pool. A rare bug (present in + * Diablo) allowed all quests to remain active but normally only two of these three quests appear in any single + * player game. + */ + auto questPool2ID = rng.RandomChoice({ Q_BUTCHER, Q_LTBANNER, Q_GARBUD }); + if (questPool2ID) + quests[*questPool2ID]._qactive = QUEST_NOTAVAIL; + + /** + * @brief The quests Halls of the Blind, the Magic Rock, and Valor share the same quest pool. As above normally only + * two of these three quests appear in any single player game. + */ + auto questPool3ID = rng.RandomChoice({ Q_BLIND, Q_ROCK, Q_BLOOD }); + if (questPool3ID) + quests[*questPool3ID]._qactive = QUEST_NOTAVAIL; + + /** + * @brief The quests Black Mushroom, Zhar the Mad, and Anvil of Fury share the same quest pool. As above normally + * only two of these three quests appear in any single player game. + */ + auto questPool4ID = rng.RandomChoice({ Q_MUSHROOM, Q_ZHAR, Q_ANVIL }); + if (questPool4ID) + quests[*questPool4ID]._qactive = QUEST_NOTAVAIL; + + /** + * @brief The quests Lachdanan and Warlord of Blood share the same quest pool. As above normally only one of these + * two quests appears in any single player game. + */ + auto questPool5ID = rng.RandomChoice({ Q_VEIL, Q_WARLORD }); + if (questPool5ID) + quests[*questPool5ID]._qactive = QUEST_NOTAVAIL; + + // There are no calls to the global RNG functions between now and the next call to vanilla::SetRndSeed + // To be absolutely safe we could call vanilla::SetRndSeed(rng.initialState) then vanilla::Discard(rng.GetCount()) +} + void CheckQuests() { if (gbIsSpawn) diff --git a/Source/quests.h b/Source/quests.h index cf097905f6da..4f1f418d7ae5 100644 --- a/Source/quests.h +++ b/Source/quests.h @@ -77,6 +77,13 @@ extern dungeon_type ReturnLvlT; extern int ReturnLvl; void InitQuests(); + +/** + * @brief Deactivates quests from each quest pool at random to provide variety for single player games + * @param seed The seed used to control which quests are deactivated + * @param quests The available quest list, this function will make some of them inactive when it returns +*/ +void InitialiseQuestPools(uint32_t seed, QuestStruct quests[]); void CheckQuests(); bool ForceQuests(); bool QuestStatus(int i); diff --git a/test/random_test.cpp b/test/random_test.cpp index 212397eefee3..ee1dd6d7026a 100644 --- a/test/random_test.cpp +++ b/test/random_test.cpp @@ -239,14 +239,20 @@ TEST(RandomTest, VanillaEngine) { uint32_t seed = ::testing::UnitTest::GetInstance()->random_seed(); auto engine1 = vanilla::RandomEngine(seed); + + engine1.RandomLessThan(0); + EXPECT_EQ(engine1.GetCount(), 0) << "vanilla::RandomEngines must not advance when RandomLessThan is called with an invalid limit"; + auto engine2 = vanilla::RandomEngine(seed); for (auto i = 0; i < 10; i++) ASSERT_EQ(engine1.RandomInt(), engine2.RandomInt()) << "vanilla::RandomEngines created from the same seed must generate the same sequence"; + EXPECT_EQ(engine1.GetCount(), 10) << "vanilla::RandomEngines must report how many times they have advanced"; + auto engine3 = vanilla::RandomEngine(engine1.initialState); + engine3.Discard(engine1.GetCount()); - engine3.Discard(10); ASSERT_EQ(engine3.RandomInRange(5, 24), engine1.RandomInRange(5, 24)) << "vanilla::RandomEngines created from the same seed must produce the same value at the same point in the sequence even if prior calls used different distributions"; @@ -260,6 +266,15 @@ TEST(RandomTest, VanillaEngine) ASSERT_EQ(engine4.RandomLessThan(6435), vanilla::GenerateRnd(6435)) << "vanilla::RandomEngines must generate the same sequence as the global RNG from the same starting seed"; } + + auto engine5 = vanilla::RandomEngine(988045466); + auto choice = engine5.RandomChoice({ -1, 1, 5, 2 }); + EXPECT_EQ(choice.has_value(), true) << "vanilla::RandomEngines must choose an option when generating a non-negative number"; + EXPECT_EQ(choice.value_or(4), 5) << "vanilla::RandomEngines must pick known values from a known seed"; + + choice = engine5.RandomChoice({ 2, 3, 4 }); + EXPECT_EQ(choice.has_value(), false) << "vanilla::RandomEngines must return an empty optional when generating a negative number"; + EXPECT_EQ(choice.value_or(-10), -10) << "vanilla::RandomEngines must return an empty optional when generating a negative number"; } TEST(RandomTest, V1Choices)