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

Make RNG portable #1104

Merged
merged 4 commits into from Feb 21, 2017

Conversation

Projects
None yet
4 participants
@scurest
Contributor

scurest commented Feb 19, 2017

The intention of this PR is to make random number generation totally portable by:

Removing rand and srand

Replacing rand() % a with GetRandomNumber(0, a-1) generally, but also

  • I changed this line which was probably a bug
  • rand() / (RAND_MAX / a + 1) was replaced with Utils::GetRandomNumber(0, a - 1) (is this right?)
  • I also added ChanceOf for the common case of getting a bool with a certain probability (this could also be used for the battle algorithms)

Removing uniform_int_distribution

The algorithm it uses isn't part of the standard.

My first idea was to copy the uniform_int_distribution from (say) GCC's stdlib here, but it's a lot of code, and it's complicated because it has to deal with RNGs with non-power-of-two ranges. We only need to consume random u32s and generate random i32 ranges, so I wrote a rejection sampler for just that case.

Hopefully I didn't get any of the math wrong (please check!).


The upshot is that the seed **should** determine the RNG sequence and the RNG should now be reproducible.

@Ghabry

Imo all random number changes look correct, just have 2 other small things.

Show outdated Hide outdated src/game_interpreter.cpp
@@ -736,7 +736,7 @@ bool Game_Interpreter::CommandControlVariables(RPG::EventCommand const& com) { /
int a, b;
a = max(com.parameters[5], com.parameters[6]);
b = min(com.parameters[5], com.parameters[6]);
value = rand() % (a-b+1)+b;
value = Utils::GetRandomNumber(b,a);

This comment has been minimized.

@Ghabry

Ghabry Feb 20, 2017

Member

Interestingly the help of the official english version (both 2k/2k3) says the interval is exclusive ("For example, if the range “0~100” is specified, the value used will be a random number above 0 but below 100.").
RPG Advocates translation says "between 0 and 100" and Dons version doesn't explain the interval at all.

But I verified that the interval is indeed inclusive, code is correct.

@Ghabry

Ghabry Feb 20, 2017

Member

Interestingly the help of the official english version (both 2k/2k3) says the interval is exclusive ("For example, if the range “0~100” is specified, the value used will be a random number above 0 but below 100.").
RPG Advocates translation says "between 0 and 100" and Dons version doesn't explain the interval at all.

But I verified that the interval is indeed inclusive, code is correct.

// 3. If it fell into the range of rem leftover numbers,
// reject it and go back to step 2.
uint32_t m = max + 1;
uint32_t rem = -m % m; // = 2^32 mod m

This comment has been minimized.

@Ghabry

Ghabry Feb 20, 2017

Member

Is that correct? -m % m is always 0.

@Ghabry

Ghabry Feb 20, 2017

Member

Is that correct? -m % m is always 0.

This comment has been minimized.

@scurest

scurest Feb 20, 2017

Contributor

Because m is unsigned, -m is equal to 2^32 - m here (cite) and 2^32 - m mod m = 2^32 mod m.

@scurest

scurest Feb 20, 2017

Contributor

Because m is unsigned, -m is equal to 2^32 - m here (cite) and 2^32 - m mod m = 2^32 mod m.

This comment has been minimized.

@Ghabry

Ghabry Feb 20, 2017

Member

Ah now I get it. Didn't knew that it's promoted to unsigned beforehand, interesting trick :D

@Ghabry

Ghabry Feb 20, 2017

Member

Ah now I get it. Didn't knew that it's promoted to unsigned beforehand, interesting trick :D

Show outdated Hide outdated src/utils.h
*/
bool ChanceOf(int32_t n, int32_t m);
/**
* Seeds the RNG used by GetRandomNumber

This comment has been minimized.

@Ghabry

Ghabry Feb 20, 2017

Member

I request a minor documentation change here: "used by GetRandomNumber and ChanceOf"

@Ghabry

Ghabry Feb 20, 2017

Member

I request a minor documentation change here: "used by GetRandomNumber and ChanceOf"

@Ghabry

This comment has been minimized.

Show comment
Hide comment
@Ghabry

Ghabry Feb 20, 2017

Member

I'm not sure if rand() / (RAND_MAX / a + 1) has an off-by-one error but your code does what was intended.

That line was definitely wrong. Just nobody ever noticed it, because the function is unused - our Auto Battle code currently ignores Skills.

Good to see GetRandomNumber/ChanceOf everywhere now to improve the randomness because our old code has a non-even randomness-distribution because most of the code only uses the incorrect rand() % number code :/.

Member

Ghabry commented Feb 20, 2017

I'm not sure if rand() / (RAND_MAX / a + 1) has an off-by-one error but your code does what was intended.

That line was definitely wrong. Just nobody ever noticed it, because the function is unused - our Auto Battle code currently ignores Skills.

Good to see GetRandomNumber/ChanceOf everywhere now to improve the randomness because our old code has a non-even randomness-distribution because most of the code only uses the incorrect rand() % number code :/.

@carstene1ns

Just a coding style problem: You seem to leave out the space between function arguments occasionally.
Function wise the replacements looks okay to me, however the GetRandomUnsigned() is above my head.

@Ghabry

Ghabry approved these changes Feb 20, 2017

Tested the randomness of the two new function and they give a nice distribution. So implementation appears correct.

@fdelapena fdelapena added this to the 0.5.1 milestone Feb 20, 2017

scurest added some commits Feb 5, 2017

Replace rand with Utils::GetRandomNumber
Completely removes old-style RNG. This improves reproducability of
runs with --replay-input and -seed (in small tests, the save files
only differ in the timestamp now).

ChanceOf was also added for the common case of needing a random bool.
Remove uniform_int_distribution
Because the algorithm is not part of the standard, the output of
uniform_int_distribution is not portable. Replace it with hand-rolled
code for GetRandomNumber.
Add GetRandomU32
The point is to document that GetRandomUnsigned depends on the range
of the RNG being [0, U32_MAX], which is true for MT19937, but might
not be if it ever changes.
@scurest

This comment has been minimized.

Show comment
Hide comment
@scurest

scurest Feb 21, 2017

Contributor

@carstene1ns Oh yeah, I do do that, don't I (when the arguments are short). Sorry! I amended the spaces in.

Contributor

scurest commented Feb 21, 2017

@carstene1ns Oh yeah, I do do that, don't I (when the arguments are short). Sorry! I amended the spaces in.

@Ghabry Ghabry merged commit 19e59ef into EasyRPG:master Feb 21, 2017

6 checks passed

Android (armeabi-v7a) Build finished.
Details
GNU/Linux Build finished.
Details
OSX Build finished.
Details
Windows (x64) Build finished.
Details
Windows (x86) Build finished.
Details
web Build finished.
Details

@scurest scurest deleted the scurest:rand branch Feb 21, 2017

Ghabry added a commit to libretro/easyrpg-libretro that referenced this pull request May 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment