godotengine / godot

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

Gradient.sample() behaving differently when passed a negative number, if created by script or loaded by file #96107

Open neoentest2 opened 2 weeks ago

neoentest2 commented 2 weeks ago

Tested versions

v4.2.rc2.official [1ba920fad]

System information

Godot v4.2.rc2 - Windows 11 Home - Vulkan (Forward+) - dedicated NVIDIA GeForce MX450 (NVIDIA; 32.0.15.5599) - 11th Gen Intel(R) Core(TM) i7-1165G7 @ 2.80GHz (8 Threads)

Issue description

Sampling a Gradient with a negative number will return Black if it is created by script, or the first color if loaded as resource (the latter being the expected/desirable outcome).

I know a Gradient shouldn't be sampled for negative values (that happened to my by accident, probably for a floating point approximation error). Maybe it's just undefined behavior, so I'm reporting this for nothing, but maybe it hides a bug/code problem of some kind.

Steps to reproduce

If you do that with a gradient loaded by file as resources, it will return the color at offset 0 instead

Minimal reproduction project (MRP)

image image

gradient_bug.zip

AThousandShips commented 2 weeks ago

Please test with a recent version like 4.2.2 or 4.3, you're on a pre-release for 4.2

Can confirm this on 4.4 [e3550cb20]

AThousandShips commented 2 weeks ago

This is not a bug, you are adding two new points to a gradient that already has two points, the default constructed gradient has two points, [ {0, Color.BLACK}, {1, Color.WHITE} ]

You need to instead do:

gradient_by_script.set_color(0, Color.RED)
gradient_by_script.set_color(1, Color.GREEN)

Or, before you add the points, do:

gradient_by_script.remove_point(0)
gradient_by_script.remove_point(0)

I don't know what would need to be documented further, but this isn't a bug but user error

neoentest2 commented 2 weeks ago

Please test with a recent version like 4.2.2 or 4.3, you're on a pre-release for 4.2

Ops, my bad; that slipped out of my mind.

This is not a bug, you are adding two new points to a gradient that already has two points, the default constructed gradient has two points, [ {0, Color.BLACK}, {1, Color.WHITE} ]

Oh, right, now that all makes sense. Should I change the title of this issue?


I don't know what would need to be documented further, but this isn't a bug but user error

At the moment, the only hint I could have that the constructor added 2 points in the documentation is the line

PackedColorArray colors = PackedColorArray(0, 0, 0, 1, 1, 1, 1, 1)

Which I think is not very legible. I could also have figured that out based on how a new Gradient resource is created in the editor, but still, I don't think things are ideal in this regard. What about adding something in the documentation of the Gradient::add_point() ? Something like:

Note: The Gradient constructor adds two points, [ {0, Color.BLACK}, {1, Color.WHITE} ]

AThousandShips commented 2 weeks ago

Note: The Gradient constructor adds two points, [ {0, Color.BLACK}, {1, Color.WHITE} ]

That isn't true though, it doesn't as such, it's just the default value

But I think the default value of a property should be the place to show what is going on, that is what it's for

neoentest2 commented 2 weeks ago

That isn't true though, it doesn't as such, it's just the default value

Got it, just saying that I think it should be stated more clearly in the Gradient documentation. I know that default values are shown, but

[ {0, Color.BLACK}, {1, Color.WHITE} ]

is way more clear then

[PackedColorArray]colors = PackedColorArray(0, 0, 0, 1, 1, 1, 1, 1)

It probably won't fool me again, but it gave me some troubles and I would like others not to experience them too.

AThousandShips commented 2 weeks ago

There are many places where default values aren't mentioned in the description, we could add a less specific description of just "initialized as a linear gradient from black to white" or something

I'm not sure people who miss this or don't check if there's any points already will be helped by the documentation spelling it out more directly