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

Fixes to the new SDL_rand capability #10063

Merged
merged 5 commits into from
Jun 19, 2024
Merged

Conversation

JKaniarz
Copy link
Contributor

  • Switched PRNG constants
  • Fixed auto-initialization so it won't break PRNG reproducibility
  • Added SDL_rand_float and SDL_rand_n to the API
  • Updated code doc-comments
  • Removed SDL_rand_r.

Description

I ran hundreds of core-hours of statistical testing to make sure the LCG constants are not terrible. These 32-bit parameters perform as well as a full 64-bit implementation.

I'm unsure about changing SDL_rand_n to Uint32. Sint32 is probably what users want, but there's gotchas when n is negative. But are people going to miss the 'U' and pass negative numbers anyway? Should switch to Sint32 and add some special logic?

Feel free to revert the one commit that removes SDL_rand_r if you think we really need it. I feel it's too advanced for what we're offering and it would force users to bring their own rand_n and rand_float. (or require us to make SDL_rand_r_n()...)

I also updated the caution statements to explicitly mention things this is not suitable for. I know we say no guarantees about quality, but this implantation is better than many std::rand() implementations. Way better than @slouken's 'demo code only' requirement. This is a good enough solution for 99% of single-player games. Should we soften the warning a bit?

Existing Issue(s)

@slouken slouken merged commit 16e69cb into libsdl-org:main Jun 19, 2024
39 checks passed
@slouken
Copy link
Collaborator

slouken commented Jun 19, 2024

I went ahead and merged this. Can you look at @madebr's issue in #10062?

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.

None yet

2 participants