Remove conditionals for OOB reads that aren't hit in normal gameplay

Reworded and added comments explaining what would've happened and calling out the case that is still technically possible.

Source/quests.cpp

@@ -104,30 +104,6 @@ const char *const QuestTriggerNames[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 };
 
 /**
  * @brief There is no reason to run this, the room has already had a proper sector assigned
@@ -274,7 +250,7 @@ void InitQuests()
 	}
 
 	if (!UseMultiplayerQuests() && *sgOptions.Gameplay.randomizeQuests) {
-		// Quests are set from the seed used to generate level 16.
+		// Quests are set from the seed used to generate level 15.
 		InitialiseQuestPools(DungeonSeeds[15], Quests);
 	}
 
@@ -301,25 +277,22 @@ void InitialiseQuestPools(uint32_t seed, Quest quests[])
 	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 = rng.generateRnd(sizeof(QuestGroup1) / sizeof(*QuestGroup1));
-
-	if (randomIndex >= 0)
-		quests[QuestGroup1[randomIndex]]._qactive = QUEST_NOTAVAIL;
-
-	randomIndex = rng.generateRnd(sizeof(QuestGroup2) / sizeof(*QuestGroup2));
-	if (randomIndex >= 0)
-		quests[QuestGroup2[randomIndex]]._qactive = QUEST_NOTAVAIL;
+	if (seed == 988045466) {
+		// If someone starts a new game at 1977-12-28 19:44:42 UTC or 2087-02-18 22:43:02 UTC
+		//  vanilla Diablo ends up reading QuestGroup1[-2] here. Due to the way the data segment
+		//  is laid out (at least in 1.09) this ends up reading the address of the string
+		//  "A Dark Passage" and trying to write to Quests[<addr>*8] which lands in read-only memory.
+		// The proper result would've been to mark The Butcher unavailable but instead nothing happens.
+		rng.discardRandomValues(1);
+	} else {
+		quests[rng.pickRandomlyAmong({ Q_BUTCHER, Q_LTBANNER, Q_GARBUD })]._qactive = QUEST_NOTAVAIL;
+	}
 
-	randomIndex = rng.generateRnd(sizeof(QuestGroup3) / sizeof(*QuestGroup3));
-	if (randomIndex >= 0)
-		quests[QuestGroup3[randomIndex]]._qactive = QUEST_NOTAVAIL;
+	quests[rng.pickRandomlyAmong({ Q_BLIND, Q_ROCK, Q_BLOOD })]._qactive = QUEST_NOTAVAIL;
 
-	randomIndex = rng.generateRnd(sizeof(QuestGroup4) / sizeof(*QuestGroup4));
+	quests[rng.pickRandomlyAmong({ Q_MUSHROOM, Q_ZHAR, Q_ANVIL })]._qactive = QUEST_NOTAVAIL;
 
-	// always true, QuestGroup4 has two members
-	if (randomIndex >= 0)
-		quests[QuestGroup4[randomIndex]]._qactive = QUEST_NOTAVAIL;
+	quests[rng.pickRandomlyAmong({ Q_VEIL, Q_WARLORD })]._qactive = QUEST_NOTAVAIL;
 }
 
 void CheckQuests()

test/quests_test.cpp

@@ -31,16 +31,31 @@ TEST(QuestTest, SinglePlayerBadPools)
 	EXPECT_EQ(Quests[Q_SKELKING]._qactive, QUEST_NOTAVAIL) << "Skeleton King quest is deactivated with 'bad' seed";
 	ResetQuests();
 
+	// Having this seed for level 15 requires starting a game at 1977-12-28 07:44:42 PM or 2087-02-18 10:43:02 PM
+	// Given Diablo was released in 1996 and it's currently 2024 this will never have been naturally hit. It's not
+	//  possible to hit this case on a pre-NT kernel windows system but it may be possible on macos or winnt?
 	InitialiseQuestPools(988045466, Quests);
 	EXPECT_THAT(GetActiveFlagsForSlice({ Q_BUTCHER, Q_LTBANNER, Q_GARBUD }), ::testing::Each(QUEST_INIT)) << "All quests in pool 2 remain active with 'bad' seed";
 	ResetQuests();
 
+	// This seed can only be reached by editing a save file or modifying the game. Given quest state (including
+	//  availability) is saved as part of the game state there's no vanilla compatibility concerns here.
 	InitialiseQuestPools(4203210069U, Quests);
-	EXPECT_THAT(GetActiveFlagsForSlice({ Q_BLIND, Q_ROCK, Q_BLOOD }), ::testing::Each(QUEST_INIT)) << "All quests in pool 3 remain active with 'bad' seed";
+	// If we wanted to retain the behaviour that vanilla Diablo would've done we should instead deactivate
+	//  Quests[QuestGroup2[-2]]. This would hit QuestGroup1[1] (Ogden's Sign), however that's already marked
+	//  as unavailable with this seed due to the previous quest group's roll.
+	EXPECT_EQ(Quests[Q_LTBANNER]._qactive, QUEST_NOTAVAIL) << "Ogden's Sign should be deactivated with 'bad' seed";
+	EXPECT_EQ(Quests[Q_BLIND]._qactive, QUEST_NOTAVAIL) << "Halls of the Blind should also be deactivated with 'bad' seed";
 	ResetQuests();
 
+	// This seed can only be reached by editing a save file or modifying the game. Given quest state (including
+	//  availability) is saved as part of the game state there's no vanilla compatibility concerns here.
 	InitialiseQuestPools(2557708932U, Quests);
-	EXPECT_THAT(GetActiveFlagsForSlice({ Q_MUSHROOM, Q_ZHAR, Q_ANVIL }), ::testing::Each(QUEST_INIT)) << "All quests in pool 4 remain active with 'bad' seed";
+	// If we wanted to retain the behaviour that vanilla Diablo would've done we should instead deactivate
+	//  Quests[QuestGroup3[-2]]. This would hit QuestGroup2[1] (Magic Rock), however that's already marked
+	//  as unavailable with this seed due to the previous quest group's roll.
+	EXPECT_EQ(Quests[Q_ROCK]._qactive, QUEST_NOTAVAIL) << "Magic Rock should be deactivated with 'bad' seed";
+	EXPECT_EQ(Quests[Q_MUSHROOM]._qactive, QUEST_NOTAVAIL) << "Black Mushroom should also be deactivated with 'bad' seed";
 	ResetQuests();
 
 	InitialiseQuestPools(1272442071, Quests);