-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Switch to portable RNG in examples #12644
Conversation
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
fe6bc58
to
8ec18ae
Compare
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.
Looks good! Thank you :)
material_rng: StdRng::seed_from_u64(42), | ||
velocity_rng: StdRng::seed_from_u64(42), | ||
transform_rng: StdRng::seed_from_u64(42), | ||
color_rng: ChaCha8Rng::seed_from_u64(42), |
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.
ChaCha8Rng
implements Clone
I think, so if we want, we could just clone a single instance over and over if they are all using the same seed though. Buuuut, it seems like a bit of a code smell to reuse the same seed for multiple RNG instances, even if an example. If we want things to be deterministic with generated seeds, we could make use of a single ChaCha8Rng::seed_from_u64(42)
, and then everything else gets ChaCha8Rng::from_rng(&mut rng).unwrap()
.
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.
The documentation says the use of unwrap
is discouraged, do you think it's fine here or shall I try to use a match
or something? (I'm very new to Rust, so apologies, I'll have more questions than usual!)
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.
ChaCha8Rng
implementsClone
I think, so if we want, we could just clone a single instance over and over if they are all using the same seed though. Buuuut, it seems like a bit of a code smell to reuse the same seed for multiple RNG instances, even if an example. If we want things to be deterministic with generated seeds, we could make use of a singleChaCha8Rng::seed_from_u64(42)
, and then everything else getsChaCha8Rng::from_rng(&mut rng).unwrap()
.
I disagree with cloning in this case. I think it will make reading the code more difficult, since the variable will need to be created beforehand. As for changing the seeds, it may be good practice in an actual game but changing the seeds now will mess up the screenshot tests. I don't think it's worth the change.
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.
The documentation says the use of
unwrap
is discouraged, do you think it's fine here or shall I try to use amatch
or something? (I'm very new to Rust, so apologies, I'll have more questions than usual!)
For OS/Hardware sources which can fail, then yes, you shouldn't unwrap()
. But for deterministic pseudorandom sources where you are in full control of the seeding (such as with ChaCha8Rng
), then usage of the Rng source will never fail. Not all entropy sources are fallible, only when they interface hardware/OS APIs can failures happen. If a PRNG algorithm can fail, then it is not deterministic in output.
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 disagree with cloning in this case. I think it will make reading the code more difficult, since the variable will need to be created beforehand. As for changing the seeds, it may be good practice in an actual game but changing the seeds now will mess up the screenshot tests. I don't think it's worth the change.
Then perhaps we need some comments here to explain that a bit more clearly. But yeah, if we have already a bunch of screenshot test suites in place, we shouldn't change that unless absolutely necessary.
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.
Looks good to me! This is an important issue for any multiplayer games, so it's definitely worth calling out in the examples.
Objective
Fixes issue #12613 - the RNG used in examples is deterministic, but its implementation is not portable across platforms. We want to switch to using a portable RNG that does not vary across platforms, to ensure certain examples play out the same way every time.
Solution
Replace all occurences of
rand::rngs::StdRng
withrand_chacha::ChaCha8Rng
, as recommended in issue #12613Changelog
rand_chacha
as a new dependency (controversial?)rand::rngs::StdRng
withrand_chacha::ChaCha8Rng