Add is_equal_approx/is_zero_approx for floats to shading language

[Chaosus]

Adds the support for these two functions to Godot Shading Language. If the user does not use them - they will not be added to the code and thus will not be compiled by the internal system compiler (main @reduz claim about adding custom functions).

This should fix (partly, only for floats): godotengine/godot-proposals#6870

[Calinou]

If the user does not use them - they will not be added to the code and thus will not be compiled by the internal system compiler (main @reduz claim about adding custom functions).

@paddy-exe may be interested in this system, since it could allow adding helper functions to the text-based shader language similar to the ones we already have in VisualShader.

[Calinou]

Tested locally (rebased on top of master 6758a7f), it works as expected.

Code looks good to me at a glance.

[Calinou]

(partly, only for floats)

This likely depends on godotengine/godot-proposals#2808, unless we're OK with using methods that have different names instead (which is inconsistent with how GDScript does it).

[paddy-exe]

Code looks good to me 👍🏻

[bitsawer]

I think this can be pretty useful, although as Calinou noted type overloading (for float, vec2, vec3 and vec4) would be great to add at some point as many similar GLSL functions understand overloading and these will be kind of special cases that don't. I usually use custom epsilon comparison values tuned for each purpose, but I think rough comparisons can be useful in many cases.

Another minor issue is this kind of "GDScriptification" of the shader language, as is_equal_approx() name and behavior is from GDScript. I don't personally mind at all, but for example with #71919 I discussed with @clayjohn and he mentioned that as main worry (if I remember right, feel free to correct me). I think asking reduz about it was the previous conclusion about it.

But generally, I don't mind adding this. Especially if we want to add function overlading at some point. Plus the system used for generating code only if used can be pretty useful for other things, too.

[Chaosus]

Another minor issue is this kind of "GDScriptification" of the shader language, as is_equal_approx() name and behavior is from GDScript.

Yeah, so either we assign a name from https://docs.godotengine.org/en/stable/tutorials/shaders/shaders_style_guide.html#naming-conventions (is_equal_approx) or make them like GLSL built-ins (isEqualApprox).

[bitsawer]

I personally like is_equal_approx() more in this case instead of isEqualApprox(), the snake-case naming hints it's a Godot specific addition and it's also more familiar to GDScript users. But some people might think it's inconsistent with other GLSL built-in fuctions and the Godot / GLSL function divide might not matter to most people (if they will even be aware of it), they will just see a conflicting naming conventions. Kind of a bikeshedding here, I can understand both arguments.

[QbieShay]

Hey 👋 I have commented under the original proposal, but I was wondering if we can put our brain together to try and make a bit of golfing and find code that can be inlined perhaps? Shouldn't it be better than using a function and binary operators? I proposed this for vectors length(abs(curr_color - oldColor)) > 0.01)

[Calinou]

Hey 👋 I have commented under the original proposal, but I was wondering if we can put our brain together to try and make a bit of golfing and find code that can be inlined perhaps? Shouldn't it be better than using a function and binary operators? I proposed this for vectors length(abs(curr_color - oldColor)) > 0.01)

This is likely significantly slower, as length() needs to perform sqrt() operations and can't early-abort if one of the components is not equal. I'd check the generated shader assembly from equivalent GLSL code to make sure.

[QbieShay]

But the GPU has dedicated hardware for that i think ..? not sure :D

[Calinou]

But the GPU has dedicated hardware for that i think ..? not sure :D

No matter how fast the GPU is, more instructions still take more time to run than fewer instructions 🙂

[QbieShay]

Hah, I wouldn't bet on that because GPUs are freaky. At the same time, I don't have enough hard evidence to support that and I don't think it's enough of an issue to be worthy more discussion ^^ no more opposition from my side

[clayjohn]

To add to Bitsawer's comment. I do feel weird about adding GDScript functionality into GDShaders, I think for two reasons:

1. It will increase demand for other GDScript functionality
2. It adds significant complexity to our shader pre-compiler which is already turning into an expensive and difficult to maintain area of the engine.

That being said this particular change is rather simple and does not add much complexity. So I think it is worth evaluating on its own. In any case, I think it would be beneficial to discuss this at the next rendering meeting so we can start to put together a clear idea about the future of GDShaders in respect to adding GDScript functionality.

[clayjohn]

We discussed this in the Rendering meeting on September 6. Those present in the meeting felt that adding this function into GDShaders is problematic as it mixed GLSL and GDScript paradigms, and doesn't actually solve the issue of users not understanding floating point precision.

In the meeting we agreed that a better solution would be to detect usages of equal and not-equal operators with floats on either side and present the user with a clear warning in the editor

[Chaosus]

Alright, then this should be closed. A warning is already been present for the users.

[Calinou]

We discussed this in the Rendering meeting on September 6. Those present in the meeting felt that adding this function into GDShaders is problematic as it mixed GLSL and GDScript paradigms, and doesn't actually solve the issue of users not understanding floating point precision.

Could we have a built-in EPSILON constant to make approximate comparisons easier, so you don't have to define it manually every time you need it?

[QbieShay]

I think writing < EPSILON or < 0.001 is free enough