From 20190ee6873502a17e5cc093e5419bdbc29c05dc Mon Sep 17 00:00:00 2001 From: Andrew James Date: Sun, 27 Jun 2021 18:37:09 +1000 Subject: [PATCH] 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 | 19 +++++++++++-------- Source/engine.cpp | 10 +++++----- Source/engine.h | 2 +- Source/items.h | 2 ++ Source/loadsave.cpp | 12 ++++++------ Source/monster.h | 6 ++++-- Source/msg.h | 4 ++++ Source/multi.h | 3 ++- Source/objects.h | 6 +++++- Source/towners.cpp | 2 +- Source/towners.h | 1 + test/random_test.cpp | 12 ++++++++---- 12 files changed, 50 insertions(+), 29 deletions(-) diff --git a/Source/diablo.cpp b/Source/diablo.cpp index 85dbe435a..f76888f09 100644 --- a/Source/diablo.cpp +++ b/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(); diff --git a/Source/engine.cpp b/Source/engine.cpp index 7c615caaa..a7a08bafe 100644 --- a/Source/engine.cpp +++ b/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(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(sglGameSeed)); } uint32_t GetLCGEngineState() diff --git a/Source/engine.h b/Source/engine.h index 9b5fbb018..94e0cf47c 100644 --- a/Source/engine.h +++ b/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(); diff --git a/Source/items.h b/Source/items.h index 28e37017f..375b6c004 100644 --- a/Source/items.h +++ b/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; diff --git a/Source/loadsave.cpp b/Source/loadsave.cpp index 2485e8d30..c7d62d732 100644 --- a/Source/loadsave.cpp +++ b/Source/loadsave.cpp @@ -592,8 +592,8 @@ static void LoadMonster(LoadHelper *file, int i) file->skip(4); // Unused pMonster->position.last.x = file->nextLE(); pMonster->position.last.y = file->nextLE(); - pMonster->_mRndSeed = file->nextLE(); - pMonster->_mAISeed = file->nextLE(); + pMonster->_mRndSeed = file->nextLE(); + pMonster->_mAISeed = file->nextLE(); file->skip(4); // Unused pMonster->_uniqtype = file->nextLE(); @@ -718,7 +718,7 @@ static void LoadObject(LoadHelper *file, int i) pObject->_oTrapFlag = file->nextBool32(); pObject->_oDoorFlag = file->nextBool32(); pObject->_olid = file->nextLE(); - pObject->_oRndSeed = file->nextLE(); + pObject->_oRndSeed = file->nextLE(); pObject->_oVar1 = file->nextLE(); pObject->_oVar2 = file->nextLE(); pObject->_oVar3 = file->nextLE(); @@ -1649,8 +1649,8 @@ static void SaveMonster(SaveHelper *file, int i) file->skip(4); // Unused file->writeLE(pMonster->position.last.x); file->writeLE(pMonster->position.last.y); - file->writeLE(pMonster->_mRndSeed); - file->writeLE(pMonster->_mAISeed); + file->writeLE(pMonster->_mRndSeed); + file->writeLE(pMonster->_mAISeed); file->skip(4); // Unused file->writeLE(pMonster->_uniqtype); @@ -1764,7 +1764,7 @@ static void SaveObject(SaveHelper *file, int i) file->writeLE(pObject->_oTrapFlag ? 1 : 0); file->writeLE(pObject->_oDoorFlag ? 1 : 0); file->writeLE(pObject->_olid); - file->writeLE(pObject->_oRndSeed); + file->writeLE(pObject->_oRndSeed); file->writeLE(pObject->_oVar1); file->writeLE(pObject->_oVar2); file->writeLE(pObject->_oVar3); diff --git a/Source/monster.h b/Source/monster.h index cdfb9545c..16b52da80 100644 --- a/Source/monster.h +++ b/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; diff --git a/Source/msg.h b/Source/msg.h index 422148179..b2f571bb2 100644 --- a/Source/msg.h +++ b/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; diff --git a/Source/multi.h b/Source/multi.h index dbb554967..8285c004b 100644 --- a/Source/multi.h +++ b/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; diff --git a/Source/objects.h b/Source/objects.h index cb822b2c4..797d213c6 100644 --- a/Source/objects.h +++ b/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; diff --git a/Source/towners.cpp b/Source/towners.cpp index ea03e274f..da0fd05ab 100644 --- a/Source/towners.cpp +++ b/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; diff --git a/Source/towners.h b/Source/towners.h index ee6623d37..d4fa0cf78 100644 --- a/Source/towners.h +++ b/Source/towners.h @@ -45,6 +45,7 @@ struct TownerStruct { byte *_tNAnim[8]; std::unique_ptr _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; diff --git a/test/random_test.cpp b/test/random_test.cpp index f95fb39ee..2aad9e89f 100644 --- a/test/random_test.cpp +++ b/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::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::min()) << "Invalid distribution"; SetRndSeed(3604671459U); // yields 0 ASSERT_EQ(AdvanceRndSeed(), 0) << "Invalid distribution";