Isolate shrine/quest pool RNG from global RNG state

Source/engine/random.hpp

@@ -10,9 +10,132 @@
 #include <algorithm>
 #include <cstdint>
 #include <initializer_list>
+#include <random>
 
 namespace devilution {
 
+class DiabloGenerator {
+private:
+	/** Borland C/C++ psuedo-random number generator needed for vanilla compatibility */
+	std::linear_congruential_engine<uint32_t, 0x015A4E35, 1, 0> lcg;
+
+public:
+	/**
+	 * @brief Set the state of the RandomNumberEngine used by the base game to the specific seed
+	 * @param seed New engine state
+	 */
+	DiabloGenerator(uint32_t seed)
+	{
+		lcg.seed(seed);
+	}
+
+	/**
+	 * @brief Advance the global RandomNumberEngine state by the specified number of rounds
+	 *
+	 * Only used to maintain vanilla compatibility until logic requiring reproducable random number generation is isolated.
+	 * @param count How many values to discard
+	 */
+	void discardRandomValues(unsigned count)
+	{
+		lcg.discard(count);
+	}
+
+	/**
+	 * @brief Generates a random non-negative integer (most of the time) using the vanilla RNG
+	 *
+	 * This advances the engine state then interprets the new engine state as a signed value and calls std::abs to try
+	 * discard the high bit of the result. This usually returns a positive number but may very rarely return -2^31.
+	 *
+	 * This function is only used when the base game wants to store the seed used to generate an item or level, however
+	 * as the returned value is transformed about 50% of values do not reflect the actual engine state. It would be more
+	 * appropriate to use GetLCGEngineState() in these cases but that may break compatibility with the base game.
+	 *
+	 * @return A random number in the range [0,2^31) or -2^31
+	 */
+	int32_t advanceRndSeed()
+	{
+		const int32_t seed = static_cast<int32_t>(lcg());
+		// since abs(INT_MIN) is undefined behavior, handle this value specially
+		return seed == std::numeric_limits<int32_t>::min() ? std::numeric_limits<int32_t>::min() : std::abs(seed);
+	}
+
+	/**
+	 * @brief Generates a random integer less than the given limit using the vanilla RNG
+	 *
+	 * If v is not a positive number this function returns 0 without calling the RNG.
+	 *
+	 * Limits between 32768 and 65534 should be avoided as a bug in vanilla means this function always returns a value
+	 * less than 32768 for limits in that range.
+	 *
+	 * This can very rarely return a negative value in the range (-v, -1] due to the bug in AdvanceRndSeed()
+	 *
+	 * @see AdvanceRndSeed()
+	 * @param v The upper limit for the return value
+	 * @return A random number in the range [0, v) or rarely a negative value in (-v, -1]
+	 */
+	int32_t generateRnd(int32_t v)
+	{
+		if (v <= 0)
+			return 0;
+		if (v <= 0x7FFF) // use the high bits to correct for LCG bias
+			return (advanceRndSeed() >> 16) % v;
+		return advanceRndSeed() % v;
+	}
+
+	/**
+	 * @brief Generates a random boolean value using the vanilla RNG
+	 *
+	 * This function returns true 1 in `frequency` of the time, otherwise false. For example the default frequency of 2
+	 * represents a 50/50 chance.
+	 *
+	 * @param frequency odds of returning a true value
+	 * @return A random boolean value
+	 */
+	bool flipCoin(unsigned frequency)
+	{
+		// Casting here because GenerateRnd takes a signed argument when it should take and yield unsigned.
+		return generateRnd(static_cast<int32_t>(frequency)) == 0;
+	}
+
+	/**
+	 * @brief Picks one of the elements in the list randomly.
+	 *
+	 * @param values The values to pick from
+	 * @return A random value from the 'values' list.
+	 */
+	template <typename T>
+	const T pickRandomlyAmong(const std::initializer_list<T> &values)
+	{
+		const auto index { std::max<int32_t>(generateRnd(static_cast<int32_t>(values.size())), 0) };
+
+		return *(values.begin() + index);
+	}
+
+	/**
+	 * @brief Generates a random non-negative integer
+	 *
+	 * Effectively the same as GenerateRnd but will never return a negative value
+	 * @param v upper limit for the return value
+	 * @return a value between 0 and v-1 inclusive, i.e. the range [0, v)
+	 */
+	inline int32_t randomIntLessThan(int32_t v)
+	{
+		return std::max<int32_t>(generateRnd(v), 0);
+	}
+
+	/**
+	 * @brief Randomly chooses a value somewhere within the given range
+	 * @param min lower limit, minumum possible value
+	 * @param max upper limit, either the maximum possible value for a closed range (the default behaviour) or one greater than the maximum value for a half-open range
+	 * @param halfOpen whether to use the limits as a half-open range or not
+	 * @return a randomly selected integer
+	 */
+	inline int32_t randomIntBetween(int32_t min, int32_t max, bool halfOpen = false)
+	{
+		return randomIntLessThan(max - min + (halfOpen ? 0 : 1)) + min;
+	}
+};
+
 /**
  * @brief Set the state of the RandomNumberEngine used by the base game to the specific seed
  * @param seed New engine state

Source/objects.cpp

@@ -1375,9 +1375,7 @@ void AddPedestalOfBlood(Object &pedestalOfBlood)
 
 void AddStoryBook(Object &storyBook)
 {
-	SetRndSeed(glSeedTbl[16]);
-
-	storyBook._oVar1 = GenerateRnd(3);
+	storyBook._oVar1 = (glSeedTbl[16] >> 16) % 3;
 	if (currlevel == 4)
 		storyBook._oVar2 = StoryText[storyBook._oVar1][0];
 	else if (currlevel == 8)
@@ -2233,7 +2231,7 @@ void OperatePedestal(Player &player, Object &pedestal, bool sendmsg)
 	}
 }
 
-void OperateShrineMysterious(Player &player)
+void OperateShrineMysterious(DiabloGenerator &rng, Player &player)
 {
 	if (&player != MyPlayer)
 		return;
@@ -2243,7 +2241,7 @@ void OperateShrineMysterious(Player &player)
 	ModifyPlrDex(player, -1);
 	ModifyPlrVit(player, -1);
 
-	switch (static_cast<CharacterAttribute>(GenerateRnd(4))) {
+	switch (static_cast<CharacterAttribute>(rng.generateRnd(4))) {
 	case CharacterAttribute::Strength:
 		ModifyPlrStr(player, 6);
 		break;
@@ -2265,7 +2263,7 @@ void OperateShrineMysterious(Player &player)
 	InitDiabloMsg(EMSG_SHRINE_MYSTERIOUS);
 }
 
-void OperateShrineHidden(Player &player)
+void OperateShrineHidden(DiabloGenerator &rng, Player &player)
 {
 	if (&player != MyPlayer)
 		return;
@@ -2295,7 +2293,7 @@ void OperateShrineHidden(Player &player)
 			}
 			if (cnt == 0)
 				break;
-			int r = GenerateRnd(NUM_INVLOC);
+			int r = rng.generateRnd(NUM_INVLOC);
 			if (player.InvBody[r].isEmpty() || player.InvBody[r]._iMaxDur == DUR_INDESTRUCTIBLE || player.InvBody[r]._iMaxDur == 0)
 				continue;
 
@@ -2422,7 +2420,7 @@ void OperateShrineReligious(Player &player)
 	InitDiabloMsg(EMSG_SHRINE_RELIGIOUS);
 }
 
-void OperateShrineEnchanted(Player &player)
+void OperateShrineEnchanted(DiabloGenerator &rng, Player &player)
 {
 	if (&player != MyPlayer)
 		return;
@@ -2439,7 +2437,7 @@ void OperateShrineEnchanted(Player &player)
 	if (cnt > 1) {
 		int spellToReduce;
 		do {
-			spellToReduce = GenerateRnd(maxSpells) + 1;
+			spellToReduce = rng.generateRnd(maxSpells) + 1;
 		} while ((player._pMemSpells & GetSpellBitmask(static_cast<SpellID>(spellToReduce))) == 0);
 
 		spell = 1;
@@ -2471,12 +2469,12 @@ void OperateShrineEnchanted(Player &player)
 	InitDiabloMsg(EMSG_SHRINE_ENCHANTED);
 }
 
-void OperateShrineThaumaturgic(const Player &player)
+void OperateShrineThaumaturgic(DiabloGenerator &rng, const Player &player)
 {
 	for (int j = 0; j < ActiveObjectCount; j++) {
 		Object &object = Objects[ActiveObjects[j]];
 		if (object.IsChest() && object._oSelFlag == 0) {
-			object._oRndSeed = AdvanceRndSeed();
+			object._oRndSeed = rng.advanceRndSeed();
 			object._oSelFlag = 1;
 			object._oAnimFrame -= 2;
 		}
@@ -2640,15 +2638,15 @@ void OperateShrineHoly(const Player &player)
 	InitDiabloMsg(EMSG_SHRINE_HOLY);
 }
 
-void OperateShrineSpiritual(Player &player)
+void OperateShrineSpiritual(DiabloGenerator &rng, Player &player)
 {
 	if (&player != MyPlayer)
 		return;
 
 	for (int8_t &itemIndex : player.InvGrid) {
 		if (itemIndex == 0) {
 			Item &goldItem = player.InvList[player._pNumInv];
-			MakeGoldStack(goldItem, 5 * leveltype + GenerateRnd(10 * leveltype));
+			MakeGoldStack(goldItem, 5 * leveltype + rng.generateRnd(10 * leveltype));
 			player._pNumInv++;
 			itemIndex = player._pNumInv;
 
@@ -2746,14 +2744,14 @@ void OperateShrineGlimmering(Player &player)
 	InitDiabloMsg(EMSG_SHRINE_GLIMMERING);
 }
 
-void OperateShrineTainted(const Player &player)
+void OperateShrineTainted(DiabloGenerator &rng, const Player &player)
 {
 	if (&player == MyPlayer) {
 		InitDiabloMsg(EMSG_SHRINE_TAINTED1);
 		return;
 	}
 
-	int r = GenerateRnd(4);
+	int r = rng.generateRnd(4);
 
 	int v1 = r == 0 ? 1 : -1;
 	int v2 = r == 1 ? 1 : -1;
@@ -2949,14 +2947,14 @@ void OperateShrineSolar(Player &player)
 	RedrawEverything();
 }
 
-void OperateShrineMurphys(Player &player)
+void OperateShrineMurphys(DiabloGenerator &rng, Player &player)
 {
 	if (&player != MyPlayer)
 		return;
 
 	bool broke = false;
 	for (auto &item : player.InvBody) {
-		if (!item.isEmpty() && FlipCoin(3)) {
+		if (!item.isEmpty() && rng.flipCoin(3)) {
 			if (item._iDurability != DUR_INDESTRUCTIBLE) {
 				if (item._iDurability > 0) {
 					item._iDurability /= 2;
@@ -2980,7 +2978,7 @@ void OperateShrine(Player &player, Object &shrine, SfxID sType)
 
 	CloseGoldDrop();
 
-	SetRndSeed(shrine._oRndSeed);
+	DiabloGenerator rng(shrine._oRndSeed);
 	shrine._oSelFlag = 0;
 
 	PlaySfxLoc(sType, shrine.position);
@@ -2989,10 +2987,10 @@ void OperateShrine(Player &player, Object &shrine, SfxID sType)
 
 	switch (shrine._oVar1) {
 	case ShrineMysterious:
-		OperateShrineMysterious(player);
+		OperateShrineMysterious(rng, player);
 		break;
 	case ShrineHidden:
-		OperateShrineHidden(player);
+		OperateShrineHidden(rng, player);
 		break;
 	case ShrineGloomy:
 		OperateShrineGloomy(player);
@@ -3011,10 +3009,10 @@ void OperateShrine(Player &player, Object &shrine, SfxID sType)
 		OperateShrineReligious(player);
 		break;
 	case ShrineEnchanted:
-		OperateShrineEnchanted(player);
+		OperateShrineEnchanted(rng, player);
 		break;
 	case ShrineThaumaturgic:
-		OperateShrineThaumaturgic(player);
+		OperateShrineThaumaturgic(rng, player);
 		break;
 	case ShrineFascinating:
 		OperateShrineCostOfWisdom(player, SpellID::Firebolt, EMSG_SHRINE_FASCINATING);
@@ -3038,7 +3036,7 @@ void OperateShrine(Player &player, Object &shrine, SfxID sType)
 		OperateShrineCostOfWisdom(player, SpellID::ChargedBolt, EMSG_SHRINE_SACRED);
 		break;
 	case ShrineSpiritual:
-		OperateShrineSpiritual(player);
+		OperateShrineSpiritual(rng, player);
 		break;
 	case ShrineSpooky:
 		OperateShrineSpooky(player);
@@ -3062,7 +3060,7 @@ void OperateShrine(Player &player, Object &shrine, SfxID sType)
 		OperateShrineGlimmering(player);
 		break;
 	case ShrineTainted:
-		OperateShrineTainted(player);
+		OperateShrineTainted(rng, player);
 		break;
 	case ShrineOily:
 		OperateShrineOily(player, shrine.position);
@@ -3086,7 +3084,7 @@ void OperateShrine(Player &player, Object &shrine, SfxID sType)
 		OperateShrineSolar(player);
 		break;
 	case ShrineMurphys:
-		OperateShrineMurphys(player);
+		OperateShrineMurphys(rng, player);
 		break;
 	}
 

Source/quests.cpp

@@ -298,24 +298,24 @@ void InitQuests()
 
 void InitialiseQuestPools(uint32_t seed, Quest quests[])
 {
-	SetRndSeed(seed);
-	quests[PickRandomlyAmong({ Q_SKELKING, Q_PWATER })]._qactive = QUEST_NOTAVAIL;
+	DiabloGenerator rng(seed);
+	quests[rng.pickRandomlyAmong({ Q_SKELKING, Q_PWATER })]._qactive = QUEST_NOTAVAIL;
 
 	// using int and not size_t here to detect negative values from GenerateRnd
-	int randomIndex = GenerateRnd(sizeof(QuestGroup1) / sizeof(*QuestGroup1));
+	int randomIndex = rng.generateRnd(sizeof(QuestGroup1) / sizeof(*QuestGroup1));
 
 	if (randomIndex >= 0)
 		quests[QuestGroup1[randomIndex]]._qactive = QUEST_NOTAVAIL;
 
-	randomIndex = GenerateRnd(sizeof(QuestGroup2) / sizeof(*QuestGroup2));
+	randomIndex = rng.generateRnd(sizeof(QuestGroup2) / sizeof(*QuestGroup2));
 	if (randomIndex >= 0)
 		quests[QuestGroup2[randomIndex]]._qactive = QUEST_NOTAVAIL;
 
-	randomIndex = GenerateRnd(sizeof(QuestGroup3) / sizeof(*QuestGroup3));
+	randomIndex = rng.generateRnd(sizeof(QuestGroup3) / sizeof(*QuestGroup3));
 	if (randomIndex >= 0)
 		quests[QuestGroup3[randomIndex]]._qactive = QUEST_NOTAVAIL;
 
-	randomIndex = GenerateRnd(sizeof(QuestGroup4) / sizeof(*QuestGroup4));
+	randomIndex = rng.generateRnd(sizeof(QuestGroup4) / sizeof(*QuestGroup4));
 
 	// always true, QuestGroup4 has two members
 	if (randomIndex >= 0)