Replace RNG calls used to initialise quest pools with instanced vanil…

…la::RandomEngine

This shows how the RandomEngine classes can be used to isolate pseudo-random generation logic.

Source/engine/random.hpp

@@ -11,6 +11,8 @@
 #include <initializer_list>
 #include <random>
 
+#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 <typename T>
+	std::optional<T> RandomChoice(std::initializer_list<T> 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<uint32_t, 0x015A4E35, 1, 0> engine;
+
+	/**
+	 * @brief The number of times this engine has advanced since being created.
+	*/
+	uint32_t engineCount = 0;
 };
 } // namespace vanilla
 

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)

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);

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)