Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce xoshiro RNG to generate dungeon seeds #7030

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

StephenCWills
Copy link
Member

Implements the technique for mitigating overlapping random number sequences described in #6438 (comment).

Summary of changes:

  • Introduce the xoshiro128++ RNG
  • Rename GenerateSeed() to GenerateRandomNumber()
  • Use xoshiro128++ to reimplement GenerateSeed()
  • Rename GameData::dwSeed to GameData::gameSeed
  • Use 64 bits for the game seed and use std::chrono to generate it
  • Update logic for generating dungeon seeds using xoshiro128++ instead of LCG

The 32-bit overload for xoshiro128plusplus::seed() is intended to provide a way to reseed xoshiro128++ using a dungeon seed. This will become necessary to create a deterministic sequence of seeds for monsters, objects, and items during dungeon generation. It feels a bit like falling back on old habits to use this random number generator to seed itself, but the SplitMix32 implementation makes it work.

This replaces #6438
This resolves #6261

Source/engine/random.hpp Outdated Show resolved Hide resolved
Source/engine/random.hpp Outdated Show resolved Hide resolved
Source/engine/random.hpp Outdated Show resolved Hide resolved
Source/engine/random.hpp Outdated Show resolved Hide resolved
Source/engine/random.hpp Outdated Show resolved Hide resolved
Source/engine/random.hpp Outdated Show resolved Hide resolved
@AJenbo AJenbo requested a review from ephphatha March 18, 2024 00:41
ephphatha
ephphatha previously approved these changes Mar 18, 2024
Copy link
Contributor

@ephphatha ephphatha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this implementation (I was looking at using xoshiro128** myself but couldn't think of a good initial use case :D), a few nits and the only real concern is whether we want to give the option of using the old dungeon table logic for people who want to stay close to the original game.

Regarding "It feels a bit like falling back on old habits to use this random number generator to seed itself, but the SplitMix32 implementation makes it work." the flaw with the old logic is that it's using a single set of parameters for the LCG to seed itself, so all sequences are overlapping. This implementation solves that since you're using three different families of function (SplitMix > xoshiro > LCG) to seed each other.

You could even use the global xoshiro128plusplus generator to spawn instanced xoshiro128plusplus generators by making use of the jump() function, the purpose of that is to advance the state far enough that the sequences will not overlap in their usage lifetime.

Real excited about potential future improvements too. It would be amazing to see instances of xoshiro128++ used for monster AI, that'd let multiplayer games get away from constantly reseeding the global RNG in an attempt to sync monster behaviour.

Source/engine/random.hpp Outdated Show resolved Hide resolved
Source/multi.cpp Outdated Show resolved Hide resolved
Source/msg.cpp Show resolved Hide resolved
Source/multi.cpp Outdated Show resolved Hide resolved
Source/multi.h Outdated
@@ -23,7 +23,7 @@ struct Player;
struct GameData {
int32_t size;
/** Used to initialise the seed table for dungeon levels so players in multiplayer games generate the same layout */
uint32_t dwSeed;
uint32_t gameSeed[4];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm this will move the version and programid which will cause issues for older versions trying to determin what client it is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking it would fail before getting to that point, because the size changed.

if (gameData->size != sizeof(GameData))
return {};

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that means that they won't see it rather then seeing that there are clients running other versions. Not critical, but not ideal. Might be worth updating that code, and make sure that we in the future only append to this structure.

Looks like gamelist will have a similar issue: https://github.com/diasurgical/devilutionx-gamelist/blob/main/main.cpp#L270

Source/engine/random.hpp Outdated Show resolved Hide resolved
Source/engine/random.cpp Outdated Show resolved Hide resolved
Source/engine/random.hpp Outdated Show resolved Hide resolved
Source/engine/random.hpp Outdated Show resolved Hide resolved
ephphatha
ephphatha previously approved these changes Mar 20, 2024
Copy link
Contributor

@ephphatha ephphatha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RNG implementation looks good to me, if we find a platform that implements std::random_device as a SplitMix64 generator seeded with the current unix timestamp then I guess we gotta revisit how the global rng gets seeded :D

@StephenCWills
Copy link
Member Author

Assuming this PR makes it into 1.6, don't forget to merge diasurgical/devilutionx-gamelist#27 and update the Discord bot at some point before the release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Finding two identical items in the same session
3 participants