Update random number functions to avoid IB (#2226)

* Update random number functions to avoid IB

Most calls to set seed were using uint32_t already, there were a few variables which were better served by having their type changed from signed to unsigned. The one exception is ItemStruct::_iSeed. This is an identifier that also happens to be used as a seed occasionally so a comment was added documenting this exception.

Includes suggested style changes for Source/towners.h; Source/towners.cpp; Source/msg.h; Source/multi.h

Source/diablo.cpp

@@ -73,10 +73,13 @@ namespace devilution {
 SDL_Window *ghMainWnd;
 DWORD glSeedTbl[NUMLEVELS];
 dungeon_type gnLevelTypeTbl[NUMLEVELS];
-int glEndSeed[NUMLEVELS];
-int glMid1Seed[NUMLEVELS];
-int glMid2Seed[NUMLEVELS];
-int glMid3Seed[NUMLEVELS];
+
+// Used for debugging level generation
+uint32_t glEndSeed[NUMLEVELS];
+uint32_t glMid1Seed[NUMLEVELS];
+uint32_t glMid2Seed[NUMLEVELS];
+uint32_t glMid3Seed[NUMLEVELS];
+
 int MouseX;
 int MouseY;
 bool gbGameLoopStartup;
@@ -1560,19 +1563,19 @@ void LoadGameLevel(bool firstflag, lvl_entry lvldir)
 		if (leveltype != DTYPE_TOWN) {
 			if (firstflag || lvldir == ENTRY_LOAD || !myPlayer._pLvlVisited[currlevel] || gbIsMultiplayer) {
 				HoldThemeRooms();
-				glMid1Seed[currlevel] = GetRndSeed();
+				glMid1Seed[currlevel] = GetLCGEngineState();
 				InitMonsters();
-				glMid2Seed[currlevel] = GetRndSeed();
+				glMid2Seed[currlevel] = GetLCGEngineState();
 				IncProgress();
 				InitObjects();
 				InitItems();
 				if (currlevel < 17)
 					CreateThemeRooms();
 				IncProgress();
-				glMid3Seed[currlevel] = GetRndSeed();
+				glMid3Seed[currlevel] = GetLCGEngineState();
 				InitMissiles();
 				InitDead();
-				glEndSeed[currlevel] = GetRndSeed();
+				glEndSeed[currlevel] = GetLCGEngineState();
 
 				if (gbIsMultiplayer)
 					DeltaLoadLevel();

Source/engine.cpp

@@ -21,7 +21,7 @@
 namespace devilution {
 
 /** Current game seed */
-int32_t sglGameSeed;
+uint32_t sglGameSeed;
 
 /**
  * Specifies the increment used in the Borland C/C++ pseudo-random.
@@ -242,7 +242,7 @@ int CalculateWidth2(int width)
  * @brief Set the RNG seed
  * @param s RNG seed
  */
-void SetRndSeed(int32_t s)
+void SetRndSeed(uint32_t s)
 {
 	sglGameSeed = s;
 }
@@ -253,8 +253,8 @@ void SetRndSeed(int32_t s)
  */
 int32_t AdvanceRndSeed()
 {
-	sglGameSeed = (RndMult * static_cast<uint32_t>(sglGameSeed)) + RndInc;
-	return abs(sglGameSeed);
+	sglGameSeed = (RndMult * sglGameSeed) + RndInc;
+	return GetRndSeed();
 }
 
 /**
@@ -263,7 +263,7 @@ int32_t AdvanceRndSeed()
  */
 int32_t GetRndSeed()
 {
-	return abs(sglGameSeed);
+	return abs(static_cast<int32_t>(sglGameSeed));
 }
 
 uint32_t GetLCGEngineState()

Source/engine.h

@@ -264,7 +264,7 @@ Direction GetDirection(Point start, Point destination);
  */
 int CalculateWidth2(int width);
 
-void SetRndSeed(int32_t s);
+void SetRndSeed(uint32_t s);
 int32_t AdvanceRndSeed();
 int32_t GetRndSeed();
 uint32_t GetLCGEngineState();

Source/items.h

@@ -171,6 +171,7 @@ enum icreateinfo_flag2 {
 constexpr int ItemAnimWidth = 96;
 
 struct ItemStruct {
+	/** Randomly generated identifier */
 	int32_t _iSeed;
 	uint16_t _iCreateInfo;
 	enum item_type _itype;
@@ -219,6 +220,7 @@ struct ItemStruct {
 	int16_t _iPLLight;
 	int8_t _iSplLvlAdd;
 	bool _iRequest;
+	/** Unique item ID, used as an index into UniqueItemList */
 	int _iUid;
 	int16_t _iFMinDam;
 	int16_t _iFMaxDam;

Source/loadsave.cpp

@@ -592,8 +592,8 @@ static void LoadMonster(LoadHelper *file, int i)
 	file->skip(4); // Unused
 	pMonster->position.last.x = file->nextLE<int32_t>();
 	pMonster->position.last.y = file->nextLE<int32_t>();
-	pMonster->_mRndSeed = file->nextLE<int32_t>();
-	pMonster->_mAISeed = file->nextLE<int32_t>();
+	pMonster->_mRndSeed = file->nextLE<uint32_t>();
+	pMonster->_mAISeed = file->nextLE<uint32_t>();
 	file->skip(4); // Unused
 
 	pMonster->_uniqtype = file->nextLE<uint8_t>();
@@ -718,7 +718,7 @@ static void LoadObject(LoadHelper *file, int i)
 	pObject->_oTrapFlag = file->nextBool32();
 	pObject->_oDoorFlag = file->nextBool32();
 	pObject->_olid = file->nextLE<int32_t>();
-	pObject->_oRndSeed = file->nextLE<int32_t>();
+	pObject->_oRndSeed = file->nextLE<uint32_t>();
 	pObject->_oVar1 = file->nextLE<int32_t>();
 	pObject->_oVar2 = file->nextLE<int32_t>();
 	pObject->_oVar3 = file->nextLE<int32_t>();
@@ -1649,8 +1649,8 @@ static void SaveMonster(SaveHelper *file, int i)
 	file->skip(4); // Unused
 	file->writeLE<int32_t>(pMonster->position.last.x);
 	file->writeLE<int32_t>(pMonster->position.last.y);
-	file->writeLE<int32_t>(pMonster->_mRndSeed);
-	file->writeLE<int32_t>(pMonster->_mAISeed);
+	file->writeLE<uint32_t>(pMonster->_mRndSeed);
+	file->writeLE<uint32_t>(pMonster->_mAISeed);
 	file->skip(4); // Unused
 
 	file->writeLE<uint8_t>(pMonster->_uniqtype);
@@ -1764,7 +1764,7 @@ static void SaveObject(SaveHelper *file, int i)
 	file->writeLE<uint32_t>(pObject->_oTrapFlag ? 1 : 0);
 	file->writeLE<uint32_t>(pObject->_oDoorFlag ? 1 : 0);
 	file->writeLE<int32_t>(pObject->_olid);
-	file->writeLE<int32_t>(pObject->_oRndSeed);
+	file->writeLE<uint32_t>(pObject->_oRndSeed);
 	file->writeLE<int32_t>(pObject->_oVar1);
 	file->writeLE<int32_t>(pObject->_oVar2);
 	file->writeLE<int32_t>(pObject->_oVar3);

Source/monster.h

@@ -166,8 +166,10 @@ struct MonsterStruct { // note: missing field _mAFNum
 	uint8_t _mint;
 	uint32_t _mFlags;
 	uint8_t _msquelch;
-	int _mRndSeed;
-	int _mAISeed;
+	/** Seed used to determine item drops on death */
+	uint32_t _mRndSeed;
+	/** Seed used to determine AI behaviour/sync sounds in multiplayer games? */
+	uint32_t _mAISeed;
 	uint8_t _uniqtype;
 	uint8_t _uniqtrans;
 	int8_t _udeadval;

Source/msg.h

@@ -236,6 +236,10 @@ struct TCmdPItem {
 	uint8_t y;
 	uint16_t wIndx;
 	uint16_t wCI;
+	/**
+	 * Item identifier
+	 * @see ItemStruct::_iSeed
+	 */
 	int32_t dwSeed;
 	uint8_t bId;
 	uint8_t bDur;

Source/multi.h

@@ -22,7 +22,8 @@ enum event_type : uint8_t {
 
 struct GameData {
 	int32_t size;
-	int32_t dwSeed;
+	/** Used to initialise the seed table for dungeon levels so players in multiplayer games generate the same layout */
+	uint32_t dwSeed;
 	uint32_t programid;
 	uint8_t versionMajor;
 	uint8_t versionMinor;

Source/objects.h

@@ -36,7 +36,11 @@ struct ObjectStruct {
 	bool _oTrapFlag;
 	bool _oDoorFlag;
 	int _olid;
-	int _oRndSeed;
+	/**
+	 * Saves the absolute value of the engine state (typically from a call to AdvanceRndSeed()) to later use when spawning items from a container object
+	 * This is an unsigned value to avoid implementation defined behaviour when reading from this variable.
+	 */
+	uint32_t _oRndSeed;
 	int _oVar1;
 	int _oVar2;
 	int _oVar3;

Source/towners.cpp

@@ -49,7 +49,7 @@ void InitTownerInfo(int i, const TownerInit &initData)
 	towner._ttype = initData.type;
 	towner.position = initData.position;
 	towner.talk = initData.talk;
-	towner._tSeed = AdvanceRndSeed();
+	towner._tSeed = AdvanceRndSeed(); // TODO: Narrowing conversion, tSeed might need to be uint16_t
 
 	dMonster[towner.position.x][towner.position.y] = i + 1;
 

Source/towners.h

@@ -45,6 +45,7 @@ struct TownerStruct {
 	byte *_tNAnim[8];
 	std::unique_ptr<byte[]> _tNData;
 	byte *_tAnimData;
+	 /** Used to get a voice line and text related to active quests when the player speaks to a town npc */
 	int16_t _tSeed;
 	/** Tile position of NPC */
 	Point position;

test/random_test.cpp

@@ -19,16 +19,20 @@ TEST(RandomTest, RandomEngineParams)
 	// Starting from a seed of 0 means the multiplicand is dropped and the state advances by increment only
 	AdvanceRndSeed();
 	ASSERT_EQ(GetLCGEngineState(), increment) << "Increment factor is incorrect";
-	AdvanceRndSeed();
 
 	// LCGs use a formula of mult * seed + inc. Using a long form in the code to document the expected factors.
+	AdvanceRndSeed();
 	ASSERT_EQ(GetLCGEngineState(), (multiplicand * 1) + increment) << "Multiplicand factor is incorrect";
 
-	// C++11 defines the default seed for a LCG engine as 1, we've had 1 call since then so starting at element 2
+	// C++11 defines the default seed for a LCG engine as 1. The ten thousandth value is commonly used for sanity checking
+	// a sequence, so as we've had one round since state 1 we need to discard another 9999 values to get to the 10000th state.
+	// This loop has an off by one error, so test the 9999th value as well as 10000th
 	for (auto i = 2; i < 10000; i++)
 		AdvanceRndSeed();
-
 	uint32_t expectedState = 3495122800U;
+	ASSERT_EQ(GetLCGEngineState(), expectedState) << "Wrong engine state after 9999 invocations";
+	AdvanceRndSeed();
+	expectedState = 3007658545U;
 	ASSERT_EQ(GetLCGEngineState(), expectedState) << "Wrong engine state after 10000 invocations";
 }
 
@@ -38,7 +42,7 @@ TEST(RandomTest, AbsDistribution)
 	// This relies on undefined behaviour when called on std::numeric_limits<int_t>::min(). See C17 7.22.6.1
 	// The current behaviour is this returns the same value (the most negative number of the type).
 	SetRndSeed(1457187811); // yields -2147483648
-	ASSERT_EQ(AdvanceRndSeed(), -2147483648) << "Invalid distribution";
+	ASSERT_EQ(AdvanceRndSeed(), std::numeric_limits<int32_t>::min()) << "Invalid distribution";
 	SetRndSeed(3604671459U); // yields 0
 	ASSERT_EQ(AdvanceRndSeed(), 0) << "Invalid distribution";