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
base: master
Are you sure you want to change the base?
Introduce xoshiro RNG to generate dungeon seeds #7030
Conversation
There was a problem hiding this 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.
2754e84
to
ebdad6e
Compare
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]; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
devilutionX/Source/dvlnet/base_protocol.h
Lines 364 to 365 in ad452cb
if (gameData->size != sizeof(GameData)) | |
return {}; |
There was a problem hiding this comment.
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
There was a problem hiding this 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
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. |
0e3f76c
to
7d29e83
Compare
Implements the technique for mitigating overlapping random number sequences described in #6438 (comment).
Summary of changes:
GenerateSeed()
toGenerateRandomNumber()
GenerateSeed()
GameData::dwSeed
toGameData::gameSeed
std::chrono
to generate itThe 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