[3.3rc6] Values generated by simplex noise are different

[e344fde6bf]

Godot version:
3.3-rc6

OS/device including version:
Linux

Issue description:

The output of simplex noise has changed in 3.3rc6. Behaviour changed in this commit: da28904.

Steps to reproduce:

	var noise = OpenSimplexNoise.new()

	# Configure
	noise.seed = 0
	noise.octaves = 4
	noise.period = 20.0
	noise.persistence = 0.8

	# Sample
	print("%s: Values:" % Engine.get_version_info()["string"])
	print(noise.get_noise_2d(1.0, 1.0))
	print()

This outputs:

3.2.3-stable (custom_build): Values:
-0.141132

3.3-rc6 (official): Values:
0.160913

Minimal reproduction project:

simplex-noise-compat-break.zip

[akien-mga]

That's a bugfix, so I guess the value in 3.3 RC 6 is the new normal, and the value you had in 3.2.3 was wrong. CC @bruvzg

[bruvzg]

There was signed integer overflow in 3.2.3 (which is undefined behavior). Values for the specific seed are different, but behavior of the noise should be the same.

[e344fde6bf]

Yeah, I'm not doubting that this is a more correct implementation. However, it's a major breaking change that I found unexpected for a point release. For people making games with procedural level generation, this change would make level generation inconsistent between the two versions.

I know Godot expects some breaking changes between versions, so if this change makes the output consistent across the currently supported platforms then it probably makes sense to accept it in 3.3. However, if the behaviour was already consistent on the supported platforms (even if undefined behaviour in C/C++), then I think it should be delayed till 4.0 since this change would cause unnecessary friction for people who want to update to the new version.

If it is accepted, it should be documented in the release notes that simplex noise in 3.3 is not compatible with older versions.

[akien-mga]

I'll mention it in the CHANGELOG.md for 3.3 as a behavior change.

The previous logic was UB and would possibly behave differently on different platforms/compilers, so it was not consistent, even if it seems so on a given platform. Being UB, it could even change behavior when upgrading a given compiler to a new version.

The fix simply clarifies the intent by removing the UB - so if it makes the results different, it means that you were actually hitting a case where the UB leads to an unexpected (thus buggy) behavior. Now with 3.3 you can actually expect that your level generation will be consistent across platforms/compilers, which it wouldn't have been in 3.2.3.

[ttencate]

Looks like this wasn't added to the CHANGELOG.md after all?

[ttencate]

I think it'd be possible to restore the original behaviour¹, without reintroducing UB, at the expense of emulating the two's complement behaviour in this line:

    r = (int)((seedU + 31) % (i + 1));

It's in a tight loop of 256 iterations, but this function is only called upon creation of OpenSimplexNoise and in set_seed, so the performance penalty should be minimal.

---

¹ To the best of my knowledge, none of the platforms that Godot runs on do anything surprising upon signed integer overflow. They just wrap around in the two's complement manner. So the original behaviour probably was consistent across platforms already.

[Calinou]

Looks like this wasn't added to the CHANGELOG.md after all?

Apologies, I forgot about this. Feel free to open a pull request to add it to the 3.3 changelog on the 3.x branch 🙂

[akien-mga]

This was fixed by #48154.