Fix RNG state not being restorable

[MidZik]

In the current code, it is not possible to save and restore an RNG state after it has produced any random numbers.

This PR fixes that by exposing the RNG state through the state property.

This change also breaks compatibility for the sake of standardizing the RNG API: the seed property was removed, and a seed method was added in its place. It wouldn't make sense that the state of an RNG was accessed through its seed property, since the seed is only used to initialize the RNG. It also makes sense for seed to be a method, since initializing an RNG from arbitrary input is called seeding it.

Finally, the rand_seed global function has been removed where possible. Seeding and state have been properly separated, so it doesn't make sense to "return a new seed", and since the input expects a seed, returning the state to use as the next seed wouldn't make much sense either. rand_seed is kept in VisualScript because removing it would be a migration nightmare, so it has been marked as deprecated.

Edit: Updated to match latest implementation.

Added last_seed property that will return the last value the RNG was seeded with.

[clayjohn]

How does this improve workflow for users?

To me, this looks like you have changed the name of seed to state and removed some functionality of the seed. If I am misunderstanding something, please correct me.

[MidZik]

The old code had no way to set pcg.state directly, the only way to "set" it was through the seeding function. Retrieving the state and putting through the seeding function will not return the RNG to its old state, so there needs to be a way to set pcg.state directly, which this PR adds.

Here are test snippets that demonstrate the issue/fix:

Old

var rng = RandomNumberGenerator.new()
print("result 1: ", rng.randi())
var saved_state = rng.seed
print("result 2 before restore state: ", rng.randi())
rng.seed = saved_state
print("result 2 after restore state:  ", rng.randi())

Result: (notice that result 2 doesn't return the same number like it should)

result 1: 3161026589
result 2 before restore state: 2668139190
result 2 after restore state:  2393350234

New

var rng = RandomNumberGenerator.new()
print("result 1: ", rng.randi())
var saved_state = rng.state
print("result 2 before restore state: ", rng.randi())
rng.state = saved_state
print("result 2 after restore state:  ", rng.randi())

Result: (here, result 2 returns the same number both times, as expected)

result 1: 3161026589
result 2 before restore state: 2668139190
result 2 after restore state:  2668139190

[ghost]

Yeah it seems setting the seed currently doesn't set the seed. I kind of remember it working at one point, but may be a faulty memory.

@Chaosus

[Chaosus]

I don't know whether it makes sense (I need to restore my memory about RNG), I also think if we are migrating to 4.0 there is no better chance for a long time to remove/change deprecated functions at all.

[Faless]

Not an expert on RNG, but it seems that the problem is mixing up state and seed.
The seed is one thing, the state another one:
Specifically:

	var rng = RandomNumberGenerator.new()
	rng.seed = 42
	print("result 1 before restore state: ", rng.randi())
	print("result 2 before restore state: ", rng.randi())
	rng.seed = 42
	print("result 1 after restore state:  ", rng.randi())
	print("result 2 after restore state:  ", rng.randi())

will give you the expected result when resetting the seed:

result 1 before restore state: 492690617
result 2 before restore state: 1919685028
result 1 after restore state:  492690617
result 2 after restore state:  1919685028

The reason why you get bogus results, is that the RNG seems to change the seed after every rand call (which I think is wrong) thus, when you read the seed, you won't get the right one:

godot/core/math/random_pcg.h

Lines 80 to 84 in a7f49ac

	void randomize();
	_FORCE_INLINE_ uint32_t rand() {
	current_seed = pcg.state;
	return pcg32_random_r(&pcg);
	}

This does not mean that we don't want to be able to read/set the state, but the PR should not remove the seed (just fix it), because they are conceptually different things.

[ghost]

@Faless Is it correct to say the seed is the value for the initial state, and it wouldn't change as values are generated? And setting the seed is expected to set the state?

[MidZik]

@Faless Yes, you are correct that there was a mixup between state and seed. That mixup occurred in the original implementation. Setting and getting the seed property was actually settting and getting the state of the RNG. This lead to problems, such as #17664, because setting state to arbitrary values is expected to give bad performance. But this is also why saving and restoring the RNG state did work before, as @avencherus reports.

At some point, this issue was fixed by adding proper seeding. Setting the seed property seeded the RNG correctly. But getting the seed property still returned the state of the RNG, as before. These are, as you said, unrelated. You can't get the state, seed with it, and expect the RNG to return to its old state.

So in summary, these are my changes:

• Re-add the ability to set the state of the RNG through the state property.
• Move getting the state from the getter of seed to the getter of state, fixing nomenclature.
• Removed seed as a writable property, because setting it would have the side effect of changing state. Added it as a method instead, exactly because it has side effects.

It would be possible and valid to have seed as a read-only property, which only returns the value that was used to seed the RNG and only changes if the RNG is reseeded. If this functionality is desirable, I can change the code to instead have seed as a read-only property, and change the method name to reseed.

[Faless]

I don't understand the reason to remove the seed. It is expected (and desired) that setting the seed resets the state of the rng. I think it should just be specified in the docs if it's not already.

…

On Fri, Jan 31, 2020, 21:17 Matt Idzik ***@***.***> wrote: @Faless <https://github.com/Faless> Yes, you are correct that there was a mixup between state and seed. That mixup occurred in the original implementation. Setting and getting the seed property was actually settting and getting the state of the RNG. This lead to problems, such as #17664 <#17664>, because setting state to arbitrary values is expected to give bad performance. But this is also why saving and restoring the RNG state *did* work before, as @avencherus <https://github.com/avencherus> reports. At some point, this issue was fixed by adding proper seeding. Setting the seed property seeded the RNG correctly. But *getting* the seed property still returned the state of the RNG, as before. These are, as you said, unrelated. You can't get the state, seed with it, and expect the RNG to return to its old state. So in summary, these are my changes: - Re-add the ability to set the state of the RNG through the state property. - Move getting the state from the getter of seed to the getter of state, fixing nomenclature. - Removed seed as a writable property, because setting it would have the side effect of changing state. Added it as a method instead, exactly because it has side effects. It would be possible and valid to have seed as a read-only property, which only returns the value that was used to seed the RNG and only changes in the RNG is reseeded. If this functionality is desirable, I can change the code to instead have seed as a read-only property, and change the method name to reseed. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#35764?email_source=notifications&email_token=AAM4C3WKLSDGJ57CUQLIED3RASBO3A5CNFSM4KN7V66KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKP4BCQ#issuecomment-580894858>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAM4C3URTPL3DBV6NWNPDWDRASBO3ANCNFSM4KN7V66A> .

[MidZik]

It's bad design to allow seed to be set through a property. If seed == 42 and then you do seed = 42, the state ends up changing despite not technically changing the seed. This behavior is much better suited for a method.

[Faless]

I see, I get your point now, it's okay to remove the property, but I would leave the possibility to get the original seed. Something like `get_original_seed`.

…

On Fri, Jan 31, 2020, 21:48 Matt Idzik ***@***.***> wrote: It's bad design to allow seed to be set through a property. If seed == 42 and then you do seed = 42, the state ends up changing despite not technically changing the seed. This behavior is much better suited for a method. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#35764?email_source=notifications&email_token=AAM4C3X3SORGO5X2Y4YQX53RASFCHA5CNFSM4KN7V66KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKP6TYQ#issuecomment-580905442>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAM4C3VKDZ4YIZWSD2ZQR4TRASFCHANCNFSM4KN7V66A> .

[ghost]

@MidZik If seed==42, and someone for unexplained reasons wants to just set it to 42 again, and the property exposed will call the setter as well.

Why would seed = 42 be any different than set_seed(42) in GDScript? How does removing the property stop someone from setting the seed to the same value many times in a row?

Also, this is a fix that would be good to have cherry picked to 3.2 and possibly older versions, but might not be if it breaks compatibly.

[Sslaxx]

Would it beneficial, if these changes were to go ahead, for randomize to return the seed value it "generates"?

[bojidar-bg]

Why would seed = 42 be any different than set_seed(42) in GDScript? How does removing the property stop someone from setting the seed to the same value many times in a row?

The issue is not about seed = 42 causing different effects every time it is called, but about seed = seed resetting the random number generator in a non-obvious way.

[rslepon]

If I may, I'd like to ask for this bug fix to be introduced into the earliest possible future version release, presumably 3.2.1. This feature was broken by version 3.2; it used to work up until version 3.1.2, The game I am developing is dependent on the ability to "go back in time", including the ability to restore the internal random number generators I use to their past states.

Here is a code snippet to further stress the point:

   var rng = RandomNumberGenerator.new()
   rng.randomize()
   var seed1: int
   var seed2: int

   # progress the state from its initial state
   prints("I0a", rng.randi())

   # remember the current seed (state) and generate some random numbers
   seed1 = rng.get_seed()
   prints("S1", seed1)
   prints("I1a", rng.randi())
   seed2 = rng.get_seed()
   prints("S2", seed2)
   prints("I2a", rng.randi())

   # try to restore the current seed (state) back to that saved in seed1 and repeat the random number generation
   rng.set_seed(seed1)
   seed1 = rng.get_seed()
   prints("S1", seed1)
   prints("I1a", rng.randi())
   seed2 = rng.get_seed()
   prints("S2", seed2)
   prints("I2a", rng.randi())

This code yields in version 3.2 something like (depending on randomize()):

I0a 1633962893

S1 1754456572831808779
I1a 529297481
S2 4481590325336486542
I2a 1163472799

S1 1754456572831808779
I1a 514962709
S2 -6734385598687245951
I2a 854767678

Note that despite S1 being the same, I1a, S2 and I2a differ between the first and second set (i.e. set_seed() does not restore current state).

On the other hand, this code yields in version 3.1.2 something like (depending on randomize()):

I0a 4143282578

S1 -3949835972124745630
I1a 669194603
S2 8963768025262493833
I2a 2612194481

S1 -3949835972124745630
I1a 669194603
S2 8963768025262493833
I2a 2612194481

Note that S1, I1a, S2 and I2a are identical in both sets (i.e. set_seed() does restore current state).

I urge you to consider releasing this bug fix as soon as possible.

[rslepon]

UPDATE:
In case anyone else is struggling with this, here is a possible workaround until the bug is fixed.

Whenever saving a state for future restoration, add the following line:
rng.set_seed(rng.get_seed())
where rng is your random number generator object.

This essentially resets the rng object to an initial state using the current seed, which is the same thing that happens upon a later restore, thus resulting in the same stream on save and restore.

That said, I am not sure what effect this workaround would have on the quality of the rng stream, so I'd consider it only a temporary workaround until the bug is fixed.

[MidZik]

Ok, I'll create a PR for 3.2 without breaking changes. It will just add the ability to get and set the state directly.

I will be modifying this PR to support getting the original seed, and I don't see any reason to not have randomize() return the seed it created either.

[MidZik]

I've completed the implementation as best as I could for this PR. It does not include returning a new seed from randomize(), since just getting last_seed immediately after calling randomize() accomplishes the same thing.

The last_seed property can be used to retrieve the last value used to seed the RNG. It is writable so that the user can save and restore it on their own if they need to do so.

Once this PR is merged I can work on the 3.2 backwards-compatible version.

[akien-mga]

Thanks for the work on those fixes! This is now superseded by #44089 where @MidZik is attributed as co-author.