From c1f3dcb7056fcc4186e840717f2bd2c5b3944160 Mon Sep 17 00:00:00 2001 From: ephphatha Date: Tue, 19 Sep 2023 23:52:03 +1000 Subject: [PATCH] Use safer versions of random number functions instead of GenerateRnd --- Source/effects.cpp | 10 ++-------- Source/engine/palette.cpp | 2 +- Source/items.cpp | 32 +++++++++++++++----------------- Source/levels/drlg_l1.cpp | 2 +- Source/levels/drlg_l3.cpp | 8 ++++---- Source/levels/drlg_l4.cpp | 2 +- Source/levels/town.cpp | 2 +- Source/missiles.cpp | 22 +++++++++++----------- Source/towners.cpp | 14 +------------- test/random_test.cpp | 25 +++++++++++++++++++++++++ 10 files changed, 62 insertions(+), 57 deletions(-) diff --git a/Source/effects.cpp b/Source/effects.cpp index 2fc7bb536..788b8ac77 100644 --- a/Source/effects.cpp +++ b/Source/effects.cpp @@ -1026,8 +1026,6 @@ void PlaySfxPriv(TSFX *pSFX, bool loc, Point position) _sfx_id RndSFX(_sfx_id psfx) { - int nRand; - switch (psfx) { case PS_WARR69: case PS_MAGE69: @@ -1037,8 +1035,7 @@ _sfx_id RndSFX(_sfx_id psfx) case LS_ACID: case IS_MAGIC: case IS_BHIT: - nRand = 2; - break; + return PickRandomlyAmong({ psfx, static_cast<_sfx_id>(psfx + 1) }); case PS_WARR14: case PS_WARR15: case PS_WARR16: @@ -1046,13 +1043,10 @@ _sfx_id RndSFX(_sfx_id psfx) case PS_ROGUE14: case PS_MAGE14: case PS_MONK14: - nRand = 3; - break; + return PickRandomlyAmong({ psfx, static_cast<_sfx_id>(psfx + 1), static_cast<_sfx_id>(psfx + 2) }); default: return psfx; } - - return static_cast<_sfx_id>(psfx + GenerateRnd(nRand)); } void PrivSoundInit(uint8_t bLoadMask) diff --git a/Source/engine/palette.cpp b/Source/engine/palette.cpp index 43bc05c25..b3575dcb1 100644 --- a/Source/engine/palette.cpp +++ b/Source/engine/palette.cpp @@ -230,12 +230,12 @@ void LoadRndLvlPal(dungeon_type l) return; } - int rv = GenerateRnd(4) + 1; if (l == DTYPE_CRYPT) { LoadPalette("nlevels\\l5data\\l5base.pal"); return; } + int rv = RandomIntBetween(1, 4); char szFileName[27]; if (l == DTYPE_NEST) { if (!*sgOptions.Graphics.alternateNestArt) { diff --git a/Source/items.cpp b/Source/items.cpp index 944c6c16e..6e6ad6c67 100644 --- a/Source/items.cpp +++ b/Source/items.cpp @@ -4215,13 +4215,13 @@ void SpawnSmith(int lvl) constexpr int PinnedItemCount = 0; int maxValue = 140000; - int maxItems = 20; + int maxItems = 19; if (gbIsHellfire) { maxValue = 200000; - maxItems = 25; + maxItems = 24; } - int iCnt = GenerateRnd(maxItems - 10) + 10; + int iCnt = RandomIntBetween(10, maxItems); for (int i = 0; i < iCnt; i++) { Item &newItem = smithitem[i]; @@ -4283,9 +4283,8 @@ void SpawnWitch(int lvl) constexpr std::array<_item_indexes, MaxPinnedBookCount> PinnedBookTypes = { IDI_BOOK1, IDI_BOOK2, IDI_BOOK3, IDI_BOOK4 }; int bookCount = 0; - const int pinnedBookCount = gbIsHellfire ? GenerateRnd(MaxPinnedBookCount) : 0; - const int reservedItems = gbIsHellfire ? 10 : 17; - const int itemCount = GenerateRnd(WITCH_ITEMS - reservedItems) + 10; + const int pinnedBookCount = gbIsHellfire ? RandomIntLessThan(MaxPinnedBookCount) : 0; + const int itemCount = RandomIntBetween(10, gbIsHellfire ? 24 : 17); const int maxValue = gbIsHellfire ? 200000 : 140000; for (int i = 0; i < WITCH_ITEMS; i++) { @@ -4461,9 +4460,9 @@ void SpawnHealer(int lvl) { constexpr int PinnedItemCount = 2; constexpr std::array<_item_indexes, PinnedItemCount + 1> PinnedItemTypes = { IDI_HEAL, IDI_FULLHEAL, IDI_RESURRECT }; - const int itemCount = GenerateRnd(gbIsHellfire ? 10 : 8) + 10; + const int itemCount = RandomIntBetween(10, gbIsHellfire ? 19 : 17); - for (int i = 0; i < 20; i++) { + for (int i = 0; i < sizeof(healitem) / sizeof(healitem[0]); i++) { Item &item = healitem[i]; item = {}; @@ -4849,15 +4848,14 @@ void RechargeItem(Item &item, Player &player) if (item._iCharges == item._iMaxCharges) return; - int r = GetSpellStaffLevel(item._iSpell); - r = GenerateRnd(player.getCharacterLevel() / r) + 1; + int rechargeStrength = RandomIntBetween(1, player.getCharacterLevel() / GetSpellStaffLevel(item._iSpell)); do { item._iMaxCharges--; if (item._iMaxCharges == 0) { return; } - item._iCharges += r; + item._iCharges += rechargeStrength; } while (item._iCharges < item._iMaxCharges); item._iCharges = std::min(item._iCharges, item._iMaxCharges); @@ -4923,12 +4921,12 @@ bool ApplyOilToItem(Item &item, Player &player) switch (player._pOilType) { case IMISC_OILACC: if (item._iPLToHit < 50) { - item._iPLToHit += GenerateRnd(2) + 1; + item._iPLToHit += RandomIntBetween(1, 2); } break; case IMISC_OILMAST: if (item._iPLToHit < 100) { - item._iPLToHit += GenerateRnd(3) + 3; + item._iPLToHit += RandomIntBetween(3, 5); } break; case IMISC_OILSHARP: @@ -4943,7 +4941,7 @@ bool ApplyOilToItem(Item &item, Player &player) } break; case IMISC_OILSKILL: - r = GenerateRnd(6) + 5; + r = RandomIntBetween(5, 10); item._iMinStr = std::max(0, item._iMinStr - r); item._iMinMag = std::max(0, item._iMinMag - r); item._iMinDex = std::max(0, item._iMinDex - r); @@ -4964,7 +4962,7 @@ bool ApplyOilToItem(Item &item, Player &player) break; case IMISC_OILFORT: if (item._iMaxDur != DUR_INDESTRUCTIBLE && item._iMaxDur < 200) { - r = GenerateRnd(41) + 10; + r = RandomIntBetween(10, 50); item._iMaxDur += r; item._iDurability += r; } @@ -4975,12 +4973,12 @@ bool ApplyOilToItem(Item &item, Player &player) break; case IMISC_OILHARD: if (item._iAC < 60) { - item._iAC += GenerateRnd(2) + 1; + item._iAC += RandomIntBetween(1, 2); } break; case IMISC_OILIMP: if (item._iAC < 120) { - item._iAC += GenerateRnd(3) + 3; + item._iAC += RandomIntBetween(3, 5); } break; default: diff --git a/Source/levels/drlg_l1.cpp b/Source/levels/drlg_l1.cpp index b518b31ae..9faaa2e32 100644 --- a/Source/levels/drlg_l1.cpp +++ b/Source/levels/drlg_l1.cpp @@ -372,7 +372,7 @@ void FillFloor() if (dungeon[i][j] != Floor || Protected.test(i, j)) continue; - int rv = GenerateRnd(3); + int rv = RandomIntLessThan(3); if (rv == 1) dungeon[i][j] = Floor22; else if (rv == 2) diff --git a/Source/levels/drlg_l3.cpp b/Source/levels/drlg_l3.cpp index f809451b3..a4ed1ceb7 100644 --- a/Source/levels/drlg_l3.cpp +++ b/Source/levels/drlg_l3.cpp @@ -757,8 +757,8 @@ void CreateBlock(int x, int y, int obs, int dir) int x2; int y2; - int blksizex = GenerateRnd(2) + 3; - int blksizey = GenerateRnd(2) + 3; + int blksizex = RandomIntBetween(3, 4); + int blksizey = RandomIntBetween(3, 4); if (dir == 0) { y2 = y - 1; @@ -1102,10 +1102,10 @@ void River() if (dungeon[rx][ry] == 7) { dircheck = 0; if (dir < 2) { - river[2][riveramt] = GenerateRnd(2) + 17; + river[2][riveramt] = PickRandomlyAmong({ 17, 18 }); } if (dir > 1) { - river[2][riveramt] = GenerateRnd(2) + 15; + river[2][riveramt] = PickRandomlyAmong({ 15, 16 }); } river[0][riveramt] = rx; river[1][riveramt] = ry; diff --git a/Source/levels/drlg_l4.cpp b/Source/levels/drlg_l4.cpp index 76a040c35..1400c1722 100644 --- a/Source/levels/drlg_l4.cpp +++ b/Source/levels/drlg_l4.cpp @@ -846,7 +846,7 @@ void Substitution() if (FlipCoin(10)) { uint8_t c = dungeon[x][y]; if (L4BTYPES[c] == 6 && !Protected.test(x, y)) { - dungeon[x][y] = GenerateRnd(3) + 95; + dungeon[x][y] = PickRandomlyAmong({ 95, 96, 97 }); } } } diff --git a/Source/levels/town.cpp b/Source/levels/town.cpp index ab6b44cfb..5799f2994 100644 --- a/Source/levels/town.cpp +++ b/Source/levels/town.cpp @@ -221,7 +221,7 @@ void DrlgTPass3() dungeon[16][35] = 7; dungeon[17][35] = 7; for (int x = 36; x < 46; x++) { - FillTile(x, 78, GenerateRnd(4) + 1); + FillTile(x, 78, PickRandomlyAmong({ 1, 2, 3, 4 })); } } if (gbIsHellfire) { diff --git a/Source/missiles.cpp b/Source/missiles.cpp index 7ad2d3cf5..a5202a866 100644 --- a/Source/missiles.cpp +++ b/Source/missiles.cpp @@ -209,7 +209,7 @@ bool MonsterMHit(int pnum, int monsterId, int mindam, int maxdam, int dist, Miss if (!monster.isPossibleToHit() || monster.isImmune(t, damageType)) return false; - int hit = GenerateRnd(100); + int hit = RandomIntLessThan(100); int hper = 0; const Player &player = Players[pnum]; const MissileData &missileData = GetMissileData(t); @@ -1268,7 +1268,7 @@ void AddHorkSpawn(Missile &missile, AddMissileParameter ¶meter) void AddJester(Missile &missile, AddMissileParameter ¶meter) { MissileID spell = MissileID::Firebolt; - switch (GenerateRnd(10)) { + switch (RandomIntLessThan(10)) { case 0: case 1: spell = MissileID::Firebolt; @@ -1497,7 +1497,7 @@ void AddWarp(Missile &missile, AddMissileParameter ¶meter) void AddLightningWall(Missile &missile, AddMissileParameter ¶meter) { UpdateMissileVelocity(missile, parameter.dst, 16); - missile._miAnimFrame = GenerateRnd(8) + 1; + missile._miAnimFrame = RandomIntBetween(1, 8); missile._mirange = 255 * (missile._mispllvl + 1); switch (missile.sourceType()) { case MissileSource::Trap: @@ -1555,7 +1555,7 @@ void AddLightningBow(Missile &missile, AddMissileParameter ¶meter) dst += parameter.midir; } UpdateMissileVelocity(missile, dst, 32); - missile._miAnimFrame = GenerateRnd(8) + 1; + missile._miAnimFrame = RandomIntBetween(1, 8); missile._mirange = 255; if (missile._misource < 0) { missile.var1 = missile.position.start.x; @@ -1573,7 +1573,7 @@ void AddMana(Missile &missile, AddMissileParameter & /*parameter*/) int manaAmount = (GenerateRnd(10) + 1) << 6; for (int i = 0; i < player.getCharacterLevel(); i++) { - manaAmount += (GenerateRnd(4) + 1) << 6; + manaAmount += RandomIntBetween(1, 4) << 6; } for (int i = 0; i < missile._mispllvl; i++) { manaAmount += (GenerateRnd(6) + 1) << 6; @@ -1643,7 +1643,7 @@ void AddChargedBoltBow(Missile &missile, AddMissileParameter ¶meter) if (missile.position.start == dst) { dst += parameter.midir; } - missile._miAnimFrame = GenerateRnd(8) + 1; + missile._miAnimFrame = RandomIntBetween(1, 8); missile._mlid = AddLight(missile.position.start, 5); UpdateMissileVelocity(missile, dst, 8); missile.var1 = 5; @@ -1699,7 +1699,7 @@ void AddArrow(Missile &missile, AddMissileParameter ¶meter) const Player &player = Players[missile._misource]; if (HasAnyOf(player._pIFlags, ItemSpecialEffect::RandomArrowVelocity)) { - av = GenerateRnd(32) + 16; + av = RandomIntBetween(16, 47); } if (player._pClass == HeroClass::Rogue) av += (player.getCharacterLevel() - 1) / 4; @@ -1863,7 +1863,7 @@ void AddTeleport(Missile &missile, AddMissileParameter ¶meter) void AddNovaBall(Missile &missile, AddMissileParameter ¶meter) { UpdateMissileVelocity(missile, parameter.dst, 16); - missile._miAnimFrame = GenerateRnd(8) + 1; + missile._miAnimFrame = RandomIntBetween(1, 8); missile._mirange = 255; const WorldTilePosition position = missile._misource < 0 ? missile.position.start : Players[missile._misource].position.tile; missile.var1 = position.x; @@ -1913,7 +1913,7 @@ void AddLightningControl(Missile &missile, AddMissileParameter ¶meter) missile.var1 = missile.position.start.x; missile.var2 = missile.position.start.y; UpdateMissileVelocity(missile, parameter.dst, 32); - missile._miAnimFrame = GenerateRnd(8) + 1; + missile._miAnimFrame = RandomIntBetween(1, 8); missile._mirange = 256; } @@ -1923,7 +1923,7 @@ void AddLightning(Missile &missile, AddMissileParameter ¶meter) SyncPositionWithParent(missile, parameter); - missile._miAnimFrame = GenerateRnd(8) + 1; + missile._miAnimFrame = RandomIntBetween(1, 8); if (missile._micaster == TARGET_PLAYERS || missile.IsTrap()) { if (missile.IsTrap() || Monsters[missile._misource].type().type == MT_FAMILIAR) @@ -2611,7 +2611,7 @@ void AddChargedBolt(Missile &missile, AddMissileParameter ¶meter) if (missile.position.start == dst) { dst += parameter.midir; } - missile._miAnimFrame = GenerateRnd(8) + 1; + missile._miAnimFrame = RandomIntBetween(1, 8); missile._mlid = AddLight(missile.position.start, 5); UpdateMissileVelocity(missile, dst, 8); diff --git a/Source/towners.cpp b/Source/towners.cpp index e3c700d75..f33aeae95 100644 --- a/Source/towners.cpp +++ b/Source/towners.cpp @@ -701,19 +701,7 @@ void TalkToCowFarmer(Player &player, Towner &cowFarmer) break; case QUEST_HIVE_ACTIVE: if (!player._pLvlVisited[9] && player.getCharacterLevel() < 15) { - _speech_id qt = TEXT_JERSEY12; - switch (GenerateRnd(4)) { - case 0: - qt = TEXT_JERSEY9; - break; - case 1: - qt = TEXT_JERSEY10; - break; - case 2: - qt = TEXT_JERSEY11; - break; - } - InitQTextMsg(qt); + InitQTextMsg(PickRandomlyAmong({ TEXT_JERSEY9, TEXT_JERSEY10, TEXT_JERSEY11, TEXT_JERSEY12 })); break; } diff --git a/test/random_test.cpp b/test/random_test.cpp index f57b74e10..ed8a8bdc6 100644 --- a/test/random_test.cpp +++ b/test/random_test.cpp @@ -279,6 +279,31 @@ TEST(RandomTest, NegativeReturnValues) EXPECT_EQ(GenerateRnd(i), -8) << "Unexpected return value for a limit of " << i; } + // The following values are/were used throughout the code + SetRndSeed(1457187811); + EXPECT_EQ(GenerateRnd(37), -23) << "Unexpected return value for a limit of 37"; + + SetRndSeed(1457187811); + EXPECT_EQ(GenerateRnd(38), -12) << "Unexpected return value for a limit of 38"; + + SetRndSeed(1457187811); + // commonly used for dungeon megatile coordinates + EXPECT_EQ(GenerateRnd(40), -8) << "Unexpected return value for a limit of 40"; + + SetRndSeed(1457187811); + EXPECT_EQ(GenerateRnd(41), -9) << "Unexpected return value for a limit of 41"; + + SetRndSeed(1457187811); + EXPECT_EQ(GenerateRnd(52), -8) << "Unexpected return value for a limit of 52"; + + SetRndSeed(1457187811); + // commonly used for dungeon tile coordinates + EXPECT_EQ(GenerateRnd(80), -48) << "Unexpected return value for a limit of 80"; + + SetRndSeed(1457187811); + // commonly used for percentage rolls (typically in a context that 0 and -68 lead to the same outcome) + EXPECT_EQ(GenerateRnd(100), -68) << "Unexpected return value for a limit of 100"; + for (int i = 1; i < 32768; i *= 2) { SetRndSeed(1457187811); EXPECT_EQ(GenerateRnd(i), 0) << "Expect powers of 2 such as " << i << " to cleanly divide the int_min RNG value ";