godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
89.37k stars 20.24k forks source link

Swapped Axes in OpenSimplexNoise Images #30356

Closed poberlohr closed 4 years ago

poberlohr commented 5 years ago

Godot version: v3.1.stable.official

Issue description:

In a small expirement I created a NoiseTexture using OpenSimplexNoise and added it to the scene as background image. Then I tried to query the noise value at a specific position, but it didn't match with what was shown on the screen.

It took me a while to figure out, that OpenSimplexNoise.get_image() swaps the x- and y-axis:

for (int i = 0; i < p_height; i++) {
    for (int j = 0; j < p_width; j++) {
        float v = get_noise_2d(i, j); // this should be get_noise_2d(j, i);
// or maybe use x and y instead of j and i

See open_simplex_noise.cpp

Steps to reproduce:

var noise = OpenSimplexNoise()
var image = noise.get_image(1024, 1024)
image.lock()

var x = 700
var y = 400
var valueFromNoise = noise.get_noise_2d(x, y)
var pixelColor = image.get_pixel(x, y)
var valueFromNoiseImage = pixelColor.r * 2 - 1 # maps [0,1] to [-1,1]

if abs(valueFromNoise - valueFromNoiseImage) > 0.01:
    print("Reproduced issue: ", valueFromNoise, " vs ", valueFromNoiseImage)

Minimal reproduction project:

NoiseImageFlippedAxesDemo.zip

Remarks: It is of course easy to work around this issue by just calling noise.get_noise_2d(y, x) instead, but that's something one has to figure out first. What I don't know is, if fixing the order in open_simplex_noise.cpp would cause some side-effects for other projects. Alternatively the docs should just mention this issue, that would be helping too.

Anutrix commented 5 years ago

I think you are right but wouldn't it break compatibility?

poberlohr commented 5 years ago

It definitely could, especially if someone already calls noise.get_noise_2d(y, x) because it works that way.

The safest solution I can think of is introducing a fixed version of the function using a different name (or signature) and marking the existing one as deprecated.

What about passing a Vector2 instead of separate x- and y-values? That should work in c++, but how does this translate to GDScript?

Anutrix commented 5 years ago

Maybe add a breaks-compatibility tag?