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

rollRange under certain circumstances very likely to return the same result #750

Closed
felher opened this issue Jul 13, 2024 · 8 comments · Fixed by #770
Closed

rollRange under certain circumstances very likely to return the same result #750

felher opened this issue Jul 13, 2024 · 8 comments · Fixed by #770
Assignees
Labels

Comments

@felher
Copy link

felher commented Jul 13, 2024

The default java.util.Random handles calls to nextInt(x) where x is a power of two specially.

In Indigo, this leads to the fact that when you do dice.rollRange(1, 4) at the start of every FrameTick, you are overwhelmingly likely to get the same number as before.

Possible workaround: call rollRange(1, 4) (or with any other number) and discard the first return value. Successive calls seem to yield more random values.

hobnob added a commit that referenced this issue Oct 7, 2024
Tests now run 3 lots of 200,000,000 rolls of dice and check the
distribution of the results in order to test the assertion made in #750
@hobnob hobnob self-assigned this Oct 8, 2024
@hobnob hobnob linked a pull request Oct 28, 2024 that will close this issue
@davesmith00000
Copy link
Member

@felher, @hobnob has a PR open against this, if you're interested. I don't believe he managed to replicate the issue:

#770

I wonder if it's a difference in random behaviour due to the difference in platform? Or was something missed?

@JD557
Copy link

JD557 commented Oct 28, 2024

I wonder if it's a difference in random behaviour due to the difference in platform? Or was something missed?

The problem might be fixed (I didn't test it) but I think you did miss something,

The issue is not on the distribution of random values (e.g. calling dice.rollRange(1, 4) multiple times) but on the distribution of the first random value (e.g. calling Dice.diceSidesN(4, scala.util.Random.nextLong()).rollRange(1, 4) multiple times)

@felher
Copy link
Author

felher commented Nov 1, 2024

@davesmith00000 @hobnob
wow, cool, thanks for your work!

I'm not completely sure whether that fixes the issue. @JD557 is right that consecutive random numbers from the same generator should work, but it seems that every frame creates the RNG anew, and the first random number from a dice with 4 sides seems to be highly correlated to the next first random number.

How should I best test the fix? Checkout the repo, publishLocal and test against that? Or is there a snapshot published?
I did dig around a bit and found an old test of mine: https://scribble.ninja/u/felher/cmbyvskqdvghhlgeecbuzibjpug

The first line tells you the number of times the first random number for a frame tick was equal to first number of the last frame tick. The second line tells how many times the second number was equal to the first in the same frame tick. The second number is at 25%, which seems correct given a 4-sided dice. The first number is 90%, though.

@hobnob
Copy link
Member

hobnob commented Nov 7, 2024

How should I best test the fix? Checkout the repo, publishLocal and test against that?

Yup, that's how you would test this fix 🙂

The issue is not on the distribution of random values (e.g. calling dice.rollRange(1, 4) multiple times) but on the distribution of the first random value (e.g. calling Dice.diceSidesN(4, scala.util.Random.nextLong()).rollRange(1, 4) multiple times)

I wonder if it's worth checking the seed you're passing here.. each time you call Dice.diceSidesN(4, scala.util.Random.nextLong()) you're creating a new dice with a new seed. But actually you would be better off creating a single dice this way (or using another semi-random seed) and then rolling it multiple times

@hobnob
Copy link
Member

hobnob commented Nov 7, 2024

@JD557 I've just updated my unit tests to test the distribution of Dice.diceSidesN(4, scala.util.Random.nextLong()).rollRange(1, 4) and can confirm that this returns a uniform distribution of numbers each time, even when called multiple times

@hobnob
Copy link
Member

hobnob commented Nov 7, 2024

Of course - that doesn't address the over-weighting of the same number each frame, but I'm hoping the fix in #770 will do that 🤞

hobnob added a commit that referenced this issue Nov 8, 2024
Tests now run 3 lots of 200,000,000 rolls of dice and check the
distribution of the results in order to test the assertion made in #750
@davesmith00000
Copy link
Member

Hi All,

We're going to merge the PR against this soon, because it will fix the problem. However, it moves the behaviour of the engine slightly away from the way it's supposed to work. @hobnob and I have discussed this at great length!

That being the case, it's very likely that Dice will change again when I look to introduce the notion of 'services': #783

davesmith00000 pushed a commit that referenced this issue Nov 8, 2024
Tests now run 3 lots of 200,000,000 rolls of dice and check the
distribution of the results in order to test the assertion made in #750
davesmith00000 pushed a commit that referenced this issue Nov 8, 2024
Tests now run 3 lots of 200,000,000 rolls of dice and check the
distribution of the results in order to test the assertion made in #750
@davesmith00000
Copy link
Member

As per my previous comment:

This PR deprecates FrameContext in favour of a new Context type, that splits up long lived side effecting processes from frame-to-frame data / tools that are meant to be referentially transparent.

As part of that work, random values will now be available in two ways:

// As previously, Dice instances are created per frame and seeded on the running time, but Dice is now backed by the super simple but good enough (I hope) Xorshift algorithm.
ctx.frame.dice.roll

// References a long running (~whole game) scala.util.Random instance with a non-zero initial seed.
ctx.services.random.nextInt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants