Add ability to restore RandomNumberGenerator state

[Xrayez]

Closes #44031 (CC @timothyqiu, @Chaosus).
Note that I haven't done the revert here as in #44037.
Based on #35764 (only the changes pertaining to RNG class are retained).

• added state as a property to restore internal state of RNG;
• get_seed() returns last seed used to initialize the state rather than the current state now.

Added a test suite with a snippet from #44031, with test cases for restoring the sequence from resetting the state and the seed.

Resetting the seed is enough to achieve reproducible sequence of random numbers, but the state can be restored directly if you want to reproduce the same numbers at different times (given the same seed), at least that's how I understand the problem, just see the test cases for concrete usage examples.

The state property should only be set to values returned by the same property, else the RNG quality will likely suffer.

Additionally, randomize() could also return the new seed, but since get_seed() is functional now, this might not be necessary.

Compatibility breakage is minimal, and the only breaking change is that get_seed() will correctly return the seed, and not the previous state.

[akien-mga]

Looks great! Thanks for the fix and the test suite.

[akien-mga]

@timothyqiu @avencherus Could either of you confirm that this addresses your use case?

[Chaosus]

Tested - Seems worked fine

[ghost]

Yeah it works fine as far as I could tell. Storing the seed and state value and setting the seed and state back to these values allows the generation to carry on where it left off.

func _ready():
	var r = RandomNumberGenerator.new()

	r.seed = 123

	r.randf()
	r.randf()

	var s = r.state
	print(r.randf())

	r.seed = 124
	print(r.randf())

	r.seed = 123
	r.state = s

	print(r.randf())

Results in:

0.98391
0.209895
0.98391

[Xrayez]

Restoring the seed is not even required, you can restore the state alone, it's only required to set the seed once to make sure that initial state is properly setup in the first place (unlike setting state manually, which is easy to mess up). In fact, I think you can even implement your own seeding function without even having to use seed for that, possibly by extending RandomNumberGenerator class (most of the stuff is already initialized upon instantiation of the class, it initializes the RNG automatically with a DEFAULT_SEED anyway).

That said, there's a reason why #35764 proposed to have seed() to be a function over a property, but it seems like people prefer a property for this (and I kind of think the same, because separating this into seed() and get_last_seed() would only make it even more confusing to a user). In either case, that's something which might be worth a dedicated PR, if needed.

[ghost]

I'm not sure exactly what the criteria is for the setter/getter to be represented with properties. Would be curious if there is a guideline to this.

[timothyqiu]

IIUC, "seed" is a term to describe what initialized the random sequence. But it's previously used as a mixture of both RNG initializer and RNG state.

Separating into seed() and get_last_seed() has the advantage of term-compatibility. But it breaks the compilation anyway.

Adding a separate state property removes the need to overload seed as RNG state. Seed is now just the random sequence initializer which won't be changed by generating random numbers, so there is no such thing as "last" seed. I think it's good to correct the meaning of the term.

[MidZik]

I'd just like to restate one idiosyncrasy of this class as-is.

If you want to make an exact copy of an RNG, you have to copy over the right properties in the right order:

var rng_a = RandomNumberGenerator.new()
var rng_b = RandomNumberGenerator.new()
rng_a.seed = 50
rng_b.seed = 100

rng_a.randf()
rng_b.randf()

# Possible mistake 1: only setting the state.
# rng_a will now return the same numbers as rng_b, but their seed values are different.
# rng_a's seed value has no relation to its current state
rng_a.state = rng_b.state
print(rng_a.seed == rng_b.seed) # false!
print(rng_a.state == rng_b.state) # true

# Possible mistake 2: setting seed after the state.
# both RNGs have the same seed, but rng_a has been
# reset to the beginning of the stream due to the side effect of setting seed
rng_a.state = rng_b.state
rng_a.seed = rng_b.seed
print(rng_a.seed == rng_b.seed) # true
print(rng_a.state == rng_b.state) # false! 

# The only correct way:
rng_a.seed = rng_b.seed
rng_a.state = rng_b.state
print(rng_a.seed == rng_b.seed) # true
print(rng_a.state == rng_b.state) # true

Although the above example uses two separate RNG objects, the same steps must be followed when saving or loading the RNG state elsewhere, such as to/from a save file. Not everyone will care about restoring both the seed and the state of an RNG, but those that do will need to keep in mind that setting the seed property has side effects. One of the reason's I'm against seed being a property.

One possible solution, which is similar how the full-featured C++ PCG headers do it, is to make state return some object that encapsulates both the seed and the internal state. For example, it could return a string: "<seed> <state>". And when the state is set, it will set both the seed and the state at the same time, assuming it was passed in a valid string. Then there is no worry about about doing things in the right order, as you just worry about a single property. And, as a benefit, the seed and internal state can never be "out-of-sync" with one another.

[Xrayez]

Info: Unity's Random has a dedicated State class, and I see no seed property there (just state), but there's InitState method which is equivalent to seed().

If we want to play safe and prevent extra String/Array parsing, then I think we should just follow similar idea that the state has to be a class (such as RandomNumberGeneratorState), and make it truly serializable (extend Resource).

I think this should be discussed at GIP first to reach consensus, but I hope the changes themselves in this PR are unambiguous, but yeah I agree that it may be easy to do a mistake with such API, but then we're talking about adding extra layer of complexity to the class (which Godot's development philosophy might not tolerate). 😛

I think most people will still use just the seed. If you do need to use the state, I think there's a high chance that a developer will know about those side-effects.

[Xrayez]

I have updated the documentation to mention that setting the seed has a side effect of changing the state, with code snippets suggesting the correct order of initialization.

[Xrayez]

avencherus

I'm not sure exactly what the criteria is for the setter/getter to be represented with properties. Would be curious if there is a guideline to this.

MidZik

Not everyone will care about restoring both the seed and the state of an RNG, but those that do will need to keep in mind that setting the seed property has side effects. One of the reason's I'm against seed being a property.

There are cases where setting properties have certain "side effects" as well already in Godot, most prominently seen in Control nodes where margin/anchor properties are not meant to be modified directly in Container nodes, from docs:

Margins are often controlled by one or multiple parent Container nodes, so you should not modify them manually if your node is a direct child of a Container. Margins update automatically when you move or resize the node.

So I'm just saying that perhaps it's not that catastrophic, and that's just one of the things which can be properly documented instead of over-engineering the solution. I'm perfectly fine with current API myself.

[Xrayez]

I wrote a proposal to discuss this: godotengine/godot-proposals#1950.

[ghost]

It's been a few years since I last messed around with prng, and I'm lacking time to review Godot's implementation. Though I have this vague memory that the seed and state in some methods are one and the same thing. The seed would just conceptually be the starting point (what you're seeding the function with), and the math would just modify the value in a deterministic way.

If it's the same case with Godot's implementation, then maybe it's conceptually easier for most to split things apart to express these ideas with their own language and intents. Otherwise it requires some specialized knowledge here to have these expectations.

Wish I knew more here, as it'd probably be clearer to give some pseudo code instead.

At any rate, hope this doesn't get wound up in a debate about best practices or API purity. I'll use whatever you come up with. I'm only looking forward to a reliable way via GDScript to restore the sequences of the RNG class without having to deviate from the core code of the 3.2 branch.

[Xrayez]

It's been a few years since I last messed around with prng, and I'm lacking time to review Godot's implementation. Though I have this vague memory that the seed and state in some methods are one and the same thing. The seed would just conceptually be the starting point (what you're seeding the function with), and the math would just modify the value in a deterministic way.

I wouldn't fixate on the exact implementation, the interface could be agnostic. I think it's totally possible that some other RNG implementation has better distinction between initial state/seeding/current state mechanism.

At any rate, hope this doesn't get wound up in a debate about best practices or API purity. I'll use whatever you come up with. I'm only looking forward to a reliable way via GDScript to restore the sequences of the RNG class without having to deviate from the core code of the 3.2 branch.

Yes, I'd just merge this PR sooner than later because it does fix an existing bug with get_seed() returning the state rather than the seed, and having state property exposed won't hurt existing workflow in any way, and in fact this is required if you relied on get_seed() to return the state somehow, so this is one of the reasons why I decided to expose the state with this PR as well, so you're still able to replicate whatever workflow you're used to without breaking anything.

The test suite is hopefully correct so if anyone would like to remove the seed property, I wouldn't mind at all, especially since GDScript does have it as a method currently, I'm just saying that in order to proceed, it's better to merge this PR first, because what we discuss currently is outside the scope of this PR.

[akien-mga]

Yep sounds good to me.

Any concern on the impact this may have on compatibility for 3.2 users once cherry-picked?

[akien-mga]

Thanks!

[Xrayez]

Any concern on the impact this may have on compatibility for 3.2 users once cherry-picked?

Well I think technically it does break compatibility, but then users rely on the buggy behavior in 3.2 in that case... I'd say it's unlikely that a regular user would want to retrieve the seed, most of the time it's the other way. That said, I'm not sure whether this bug fix is that critical to be cherry-picked, but I wouldn't mind if this could be cherry-picked personally, I've never had to use it like that.

I usually store the seed somewhere else instead (upon initialization of RNG class). And I think it would be easy to fix this in projects as long as state is exposed too.

[ghost]

Any concern on the impact this may have on compatibility for 3.2 users once cherry-picked?

I think it's more of a compatibility concern for 3.1. This wouldn't close #44031 until it has been cherry-picked.

The unintended usage of seed to restore state isn't working anymore in 3.2. I haven't been doing the kind of testing that would expose this in a while, so I didn't noticed my RNG was broken until timothyqiu posted the issue.

At present 3.1 and master have ways to restore RNG state, but 3.2 does not.

[akien-mga]

I think it's more of a compatibility concern for 3.1.

Well the compatibility breakage from 3.1 to 3.2 was known and intentional, as it was fixing issues with the 3.1 RNG implementation.

Then #44031 was not intentional, but my question is whether fixing it will break compatibility between 3.2-3.2.3 and 3.2.4, e.g. for users who do manage to use the 3.2-3.2.3 RNG in a reproducible way, and expect this to stay compatible.
If there's no way to have any reproducible behavior in 3.2-3.2.3, then yes the cherry-pick of this fix should be fine.

[ghost]

users who do manage to use the 3.2-3.2.3 RNG in a reproducible way

Sorry, I don't think I understand. Is there a way to reproduce the RNG state in 3.2? Or do you mean some situation where the RNG is created again with a seed and they rely on the same results from there?

In the first case, I don't know, the only work around I can think of to that is to store the number of times you use an RNG and when making a new one use it that many times to get it to return to the desired state.

class RNG:
	
	var r = RandomNumberGenerator.new()
	var n = 0
	
	func _init(p_seed):
		r.seed = p_seed
		
	func rand_f():
		n += 1
		return r.randf()
		
	func get_uses():
		return n
		
	func restore(s, n):
		
		r.seed = s
		
		for i in n:
			r.randf()
		

func _ready() -> void:

	var r = RNG.new(100)

	printt(r.rand_f())
	printt(r.rand_f())
	printt(r.rand_f())
	
	var n = r.get_uses()
	
	print(n, " - Next 3.")
	printt(r.rand_f())
	printt(r.rand_f())
	printt(r.rand_f())
	
	
	r.restore(100, n)

	print("\nNumber restored, should see the last three.")
	printt(r.rand_f())
	printt(r.rand_f())
	printt(r.rand_f())

3.2.4.beta

0.549445
0.950485
0.453266
3 - Next 3.
0.1897
0.767313
0.195429

Number restored, should see the last three.
0.1897
0.767313
0.195429

Godot Engine v4.0.dev.custom_build.575f47e1b

0.549445
0.950485
0.453266
3 - Next 3.
0.1897
0.767313
0.195429

Number restored, should see the last three.
0.1897
0.767313
0.195429

In the other case, works the same as far as I can tell.

func _ready():
	
	var r = RandomNumberGenerator.new()
	r.seed = 100

	printt(r.randf())
	printt(r.randf())
	printt(r.randf())

	var r2 = RandomNumberGenerator.new()
	r2.seed = 100

	print()
	printt(r2.randf())
	printt(r2.randf())
	printt(r2.randf())

3.2.4.beta

0.549445
0.950485
0.453266

0.549445
0.950485
0.453266

Godot Engine v4.0.dev.custom_build.575f47e1b

0.549445
0.950485
0.453266

0.549445
0.950485
0.453266

Or something else?

[Xrayez]

I say cherry-picking this to 3.2 has little chance/impact of breaking existing ways of having reproducible results with RNG, in contrast with potentially breaking changes such as Variant fixes in 3.2 which lead to numerous regressions in the past... I had to update a few projects due to relying on incorrect behavior as a feature anyways.

That said, I think you'd still be able to do the workarounds even with a bug fix in this PR, including the above snippets.

[MidZik]

The cherry-picked version should only add the state property, and not change how get_seed works. It is possible that some users used get_seed in some fashion to make a saveable RNG. For example:

def reproducible_randf(rng):
    var result = rng.randf()
    rng.seed = rng.seed
    return result

The above workaround function lets a user get a random float from an RNG, and also puts the RNG into a state where you can save the seed and get reproducible results. At the expense of drastically reducing the RNG quality.

[ghost]

@MidZik Do you have some usage code to go with that? I couldn't figure out how to restore state with that function.

[MidZik]

@avencherus You just save and restore the seed property directly after calling that function.

var saved_seed = rng.seed

# at a later point in time
rng.seed = saved_seed

The important detail is that after calling rng.randf(), you can no longer save and restore the seed. But you can after setting the seed property. So the above function works around that by getting a random value and then seeding itself immediately after. The downside is that seeding an RNG from its own state can potentially give you horrible sequences of numbers somewhere down the line. For example, your RNG might end up in a state where it loops through the same 100 numbers over and over in the same order. Which isn't really random anymore.

[akien-mga]

So I'm still not sure about what should be done exactly to fix the issue in 3.2 without risk of upsetting existing users, as happens every single time we touch RNG... If anyone who understands the context is willing to do a PR for 3.2 and further discussion, that'd be great :)

[Xrayez]

As MidZik said, I think just exposing the state as a property should be enough without the risk of breaking anything in 3.2...

[akien-mga]

As MidZik said, I think just exposing the state as a property should be enough without the risk of breaking anything in 3.2...

Sounds good. I'd welcome a tested PR that does this :)