godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.13k stars 69 forks source link

Expose `CMP_EPSILON` constant to the scripting API #3565

Open Calinou opened 2 years ago

Calinou commented 2 years ago

Related to https://github.com/godotengine/godot-proposals/issues/3285.

Describe the project you are working on

The Godot editor :slightly_smiling_face:

Describe the problem or limitation you are having in your project

To ensure consistent results, floating-point comparison should not be performed using == or !=. Instead, is_equal_approx() or is_zero_approx() should be used. However, this is only suitable when checking for equality. When checking whether a value is greater than or lower than a specified number, it's best to use an epsilon approach:

We have a CMP_EPSILON constant defined in C++, but it's not exposed to scripting or shaders (as is done with PI, TAU, …).

Describe the feature / enhancement and how it helps to overcome the problem or limitation

The constant could be exposed as EPSILON to require less typing.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

The EPSILON constant should be added to GDScript, VisualScript and shader language implementation.

If this enhancement will not be used often, can it be worked around with a few lines of script?

Yes, as the epsilon constant can be declared in a script as follows:

const EPSILON = 0.00001

func _ready():
    if 2.0 + 3.0 < 5.0 + EPSILON:
        print("Lower than 5; this should never happen.")

For shaders, const float EPSILON = 0.00001; can be used in global scope as well.

Is there a reason why this should be core and not an add-on in the asset library?

This is about improving convenience when comparing floating-point values "the intended way".

aaronfranke commented 2 years ago

One concern that I have about this proposal is that the current value may not be optimal. 32-bit floats have about 6.9 decimal digits of mantissa/precision, so 1e-6 is a good epsilon value that will give about 3 bits of error tolerance. However, the current value is 0.00001 which is 1e-5. Changing this value to 1e-6 (0.000001) would likely silently break any part of the engine that expects the current very high tolerance for floating-point errors, so I haven't proposed doing it yet, but if we expose this constant then I would prefer we make this change first.

Xrayez commented 2 years ago

The constant could be exposed as EPSILON to require less typing.

Could this be confused with machine epsilon? Yet it's too specific to create confusion I guess.

One concern that I have about this proposal is that the current value may not be optimal.

If the current value is kept, is it important to still have access to internal CMP_EPSILON? For instance, some geometry functions use CMP_EPSION but a developer would use exposed EPSILON value in GDScript using the result of those functions, which may lead to inconsistency in results, and even bugs.

MajorMcDoom commented 8 months ago

I would like to raise another use case for this I've encountered that isn't a comparison of any kind, which is to find the biggest possible number that is still smaller than another number. i.e.: var b := a - EPSILON

IMO, the only meaningful epsilon is the one relating to floating point representation (machine epsilon). Anything else is ultimately arbitrary and use-case specific, no matter how currently ubiquitous it is in the code-base. It's a magic number that has a non-zero chance to become a liability at some point in the future.

Whatever the final decision though, I do think we'd benefit from more clear documentation and more specific namings.

aaronfranke commented 8 months ago

@MajorMcDoom No single constant can accomplish what you describe. The step size between 2 and the biggest number above 2 is twice as big as the step size between 1 and the biggest number above 1.

MajorMcDoom commented 8 months ago

@MajorMcDoom No single constant can accomplish what you describe. The step size between 2 and the biggest number above 2 is twice as big as the step size between 1 and the biggest number above 1.

Ahhhh, yes. I'm assuming you meant "below 2" and "below 1", but you are totally right. Not sure how I overlooked that the steps are not regular for floating point, but please disregard my comment. :)

Chaosus commented 2 months ago

The @clayjohn conclusion is to not include this built-in to shaders:

I don't think this will be very useful for shaders where float is not guaranteed to be 32 bits. Floats already have the problem that an epsilon value is only useful for a given range, but with shaders you have to factor in devices too. I.e. epsilon values are only useful for a given range for a given set of devices. IMO exposing this is likely to cause more harm than benefit for users.

Epsilon values should really be use-case specific when the user knows what range of values they are operating in.