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

Adding explanation to seeded rng used in examples #12593

Merged

Conversation

andristarr
Copy link
Contributor

Objective

Solution

  • Added/updated a universally worded comment to all seeded rng instances in our examples.

@@ -48,6 +48,8 @@ fn setup(
));

// cubes

// Make it play out the same way every time with deterministic results. We do this for testing purposes.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Make it play out the same way every time with deterministic results. We do this for testing purposes.
// Make it deterministic for testing purposes.

I personally don't like "make it play out" as a phrase. I feel like determinism is a common enough term, or at least easily googled, so it should be enough to just say that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@BD103 BD103 added the C-Examples An addition or correction to our examples label Mar 20, 2024
Copy link
Member

@BD103 BD103 left a comment

Choose a reason for hiding this comment

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

Thanks!

@Bluefinger
Copy link
Contributor

If we are emphasising that the seeded rng is being used for deterministic purposes, then we kinda shouldn't be using StdRng in the first place. StdRng is documented as being deterministic in output, but not portable, so there is no assurance that the output will be the same on different platforms or different versions of rand. And if we are testing across different platforms, we'd then expect the output to match across all platforms. For further reference from the rand documentation, their documented expectations for portability/determinism.

Given rand minor versions might in the future change the underlying algorithm for StdRng, end users might find their tests breaking if they used StdRng in their tests after basing them on our examples. Better to instead encourage using the algorithm crates directly if they want to have that extra assurance in determinism. Currently, rand_chacha is what is the underlying algorithm for StdRng, specifically ChaCha12Rng. For game dev purposes, ChaCha8Rng would be a better fit to provide higher throughput at the expense of some entropy quality (though it will still be far better than what is probably required).

@andristarr
Copy link
Contributor Author

So this PR should be abandoned I assume?

@Bluefinger
Copy link
Contributor

Bluefinger commented Mar 21, 2024

So this PR should be abandoned I assume?

Up to you. Personally, I would just merge this as an easy docs change (since the comments in the PR are correct with intent), then follow-up with the more extensive changes.

@andristarr
Copy link
Contributor Author

Merge it from my opinion then as well!

@matiqo15 matiqo15 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 21, 2024
@@ -48,6 +48,8 @@ fn setup(
));

// cubes

// Make it deterministic for testing purposes.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Make it deterministic for testing purposes.
// We're seeding the PRNG here to make this example deterministic for testing purposes.
// This isn't strictly required in practical use unless you need your app to be deterministic.

This may need to be reflected on the other examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you want this exact wording for all instances? I feel its a little bit verbose.

Copy link
Member

@james7132 james7132 Mar 24, 2024

Choose a reason for hiding this comment

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

It is, but explaining the full reasoning is probably for the best.

@BD103
Copy link
Member

BD103 commented Mar 23, 2024

Can this PR update from main, now that #12644 is merged? (There should be a button on Github, or you can run git merge main after pulling the main branch locally.) We've switched to ChaCha8Rng, so I want to make sure there are not any conflicts.

@andristarr
Copy link
Contributor Author

Merged main in, it should is up to date now!

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 25, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 25, 2024
@james7132
Copy link
Member

@alice-i-cecile I think the above comment about rewording it should be addressed before merging. This is otherwise a bit light on why it's being done.

@alice-i-cecile alice-i-cecile removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 25, 2024
@matiqo15 matiqo15 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 26, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 26, 2024
Merged via the queue into bevyengine:main with commit d39ab55 Mar 26, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Examples An addition or correction to our examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Explain the usage of seeded RNG in examples
6 participants