godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.17k stars 98 forks source link

Add `is_equal_approx()` and `is_zero_approx()` functions to the shader language #6870

Closed Calinou closed 8 months ago

Calinou commented 1 year ago

Describe the project you are working on

The Godot editor :slightly_smiling_face:

Describe the problem or limitation you are having in your project

Due to floating-point precision errors, two floating-point numbers should not be compared with == or !=. Instead, a ranged comparison with a certain epsilon value should be used. (The optimal epsilon value depends on how far away the numbers are from 0.0 and the float datatype precision.)

This often causes shaders to break in unexpected ways, even if the shader editor now warns about floating-point comparisons. See https://www.reddit.com/r/godot/comments/13e2ufj/shader_not_working_in_godot_40/ for an example of this occurring in an user-written shader.

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

Add is_equal_approx() and is_zero_approx() functions to the shader language.

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

One hurdle is that the shader language doesn't support function overloading, so you can't have a single function that accepts different argument types. This would require either prefixing the function with a type (e.g. color_is_equal_approx()) or implementing function overloading in the Godot shader language.

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

Yes, but for a color comparison, you need this much code:

if (
    curr_color.r >= oldColor.r - 0.001 && curr_color.r <= oldColor.r + 0.001 &&
    curr_color.g >= oldColor.g - 0.001 && curr_color.g <= oldColor.g + 0.001 &&
    curr_color.b >= oldColor.b - 0.001 && curr_color.b <= oldColor.b + 0.001 && 
    curr_color.a >= oldColor.a - 0.001 && curr_color.a <= oldColor.a + 0.001
) {
    // Colors are approximately equal.
}

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

This is about the improving shader language's usability.

QbieShay commented 1 year ago

Could you do length(abs(curr_color - oldColor)) > 0.01)

Daniel-The-Fox commented 11 months ago

Maybe the above example code could be added to the docs for the time being until this proposal (or similar) is implemented? :)

AThousandShips commented 11 months ago

There already is, I don't think we need to add the detail about doing it for each component, that should be obvious from the existing detail

Why would you need to add:

const float EPSILON = 0.0001;
if (foo.x >= bar.x - EPSILON && foo.x <= bar.x + EPSILON &&
    foo.y >= bar.y - EPSILON && foo.y <= bar.y + EPSILON)

To the documentation as well?

Daniel-The-Fox commented 11 months ago

I don't understand why adding such a useful and often asked / misunderstood topic as comparing colors when coding a shader is "not needed", but okay...

AThousandShips commented 11 months ago

How is it a far step to extrapolate that?

Also if we would add en example it would be:

const float EPSILON = 0.0001;
if (dot(col_a - col_b, col_a - col_b) <= EPSILON * EPSILON)

Alternatively for less performant but simpler version:

const float EPSILON = 0.0001;
if (length(col_a - col_b) <= EPSILON)

That I can see being added

Daniel-The-Fox commented 11 months ago

So what does the following built-in function do?

bvec_typeΒ equalΒ (vec_type x, vec_type y) Bool vector comparison on == int/uint/float vectors.

From the description, I would've expected it could serve the task here, as it also states "float".

AThousandShips commented 11 months ago

It compares exactly, and each element, that's not what this proposal is about

It returns, for example: bvec2(foo.x == bar.x, foo.y == bar.y)

For that please open an issue in the documentation repo for the unclear description πŸ™‚, unrelated to this

Daniel-The-Fox commented 11 months ago

It compares exactly, and each element, that's not what this proposal is about

It returns, for example: bvec2(foo.x == bar.x, foo.y == bar.y)

For that please open an issue in the documentation repo for the unclear description πŸ™‚, unrelated to this

Ah! Okay, now I get it... πŸ˜† Thanks.

Daniel-The-Fox commented 11 months ago

How is it a far step to extrapolate that?

Also if we would add en example it would be:

const float EPSILON = 0.0001;
if (dot(col_a - col_b, col_a - col_b) < EPSILON * EPSILON)

Alternatively for less performant but simpler version:

const float EPSILON = 0.0001;
if (length(col_a - col_b) < EPSILON)

That I can see being added

Unfortunately, this is not working for me when trying to replace specific colors in an AnimatedSprite2D via a color shader:

shader_type canvas_item;
const float EPSILON = 0.0001;

uniform vec4 paper_color_old: source_color = vec4(0.0, 0.0, 0.0, 1.0);
uniform vec4 paper_color_new: source_color = vec4(1.0, 1.0, 1.0, 1.0);

void fragment() {
    vec4 curr_pixel_color = texture(TEXTURE, UV);
    if (length(curr_pixel_color - paper_color_old) < EPSILON) {
        COLOR = paper_color_new;
    } else {
        COLOR = curr_pixel_color;
    }
}

I guess I'm somehow getting overwhelmed by all the vector math...? πŸ˜†

AThousandShips commented 11 months ago

This code is valid but you might not be having the right epsilon, it's probably a very small value for this, since the maximum difference is 4

(Also you need to use three backtics, I fixed it for you but remember in the future πŸ™‚)

But this is out of scope for this, I'd suggest if you can't get this code to work to ask in the other community channels πŸ™‚

Edit: Also try this and see:

shader_type canvas_item;
const float EPSILON = 0.0001;

uniform vec4 paper_color_old: source_color = vec4(0.0, 0.0, 0.0, 1.0);
uniform vec4 paper_color_new: source_color = vec4(1.0, 1.0, 1.0, 1.0);

void fragment() {
    vec4 curr_pixel_color = texture(TEXTURE, UV);
    if (length(curr_pixel_color - paper_color_old) < EPSILON) {
        curr_pixel_color = paper_color_new;
    }
    COLOR = curr_pixel_color;
}

Otherwise you can try:

shader_type canvas_item;
const float EPSILON = 0.0001;

uniform vec4 paper_color_old: source_color = vec4(0.0, 0.0, 0.0, 1.0);
uniform vec4 paper_color_new: source_color = vec4(1.0, 1.0, 1.0, 1.0);

bool is_zero_approx_float(float p_arg) {
    return (p_arg >= -EPSILON) && (p_arg <= EPSILON);
}

bool is_zero_approx_vec4(vec4 p_vec) {
    return is_zero_approx_float(p_vec.x) && is_zero_approx_float(p_vec.y) && is_zero_approx_float(p_vec.z) && is_zero_approx_float(p_vec.w);
}

void fragment() {
    vec4 curr_pixel_color = texture(TEXTURE, UV);
    if (is_zero_approx_vec4(curr_pixel_color - paper_color_old)) {
        curr_pixel_color = paper_color_new;
    }
    COLOR = curr_pixel_color;
}

No harm in declaring your own methods for this for shorthand

Daniel-The-Fox commented 11 months ago

This looks promising and clean, many thanks already. πŸ‘πŸ» I’ll give it a try tomorrow and get back to you here again. πŸ‘‹πŸ»

How do you come up with the correct Epsilon? My vector math skills aren’t up to speed anymore… 😞

Once it works, this would make a great addition for the docs… 😎

Daniel-The-Fox commented 11 months ago

Unfortunately, the new code is not working as expected as well. However, when I manually set the paper_color_old to the color white (vec4(1.0, 1.0, 1.0, 1.0)) the white color gets replaced by the paper_color_new chosen.

I used the source_color type hint as explained in the docs in order to have a color picker in the shader parameters section in the Inspector. I noticed that there are different kinds of color pickers. Do they make a difference in terms of what values (i.e. int vs. float) they capture and send to the shader script? πŸ€”

Unfortunately, there's no print in the shader language. Otherwise I could debug it more easily...

PS: I really like your 2nd example code and will stick to that! πŸ˜ƒ

Daniel-The-Fox commented 11 months ago

Ok, so I tweaked the code snippet from @AThousandShips (Many thanks again! πŸ‘πŸ»πŸ˜ƒ) provided here and my graphics and the following shader is now working for me. It's probably not in a quality state that can be directly added to the docs as an example how to replace colors, but maybe it can serve as a basis for someone ending up here after a Google search or for a future example in the Godot docs. πŸ€” πŸ˜„

I added all the insights I got from this thread as well as while googling and trial & error for a solution that works for me. πŸ˜„ As stated above already, this is probably not in the scope of an improvement proposal, but it's totally related to the initial I believe. In addition, the majority of tutorials I found on replacing colors via a shader do color / float comparison via a simple ==! 😱 I wonder how and why that works out for them, but well....

At least I learned a bunch here on my way! Thanks for that! πŸ˜„ So here goes my shader:

/*
 * This shader is based on code from AThousandShips at https://github.com/godotengine/godot-proposals/issues/6870
 * and the tutorial from Yep at https://www.youtube.com/watch?v=4qzi5fEJs4s
 *
 * Further insights got triggered while reading Godot Engine docs on comparing floats in Godot's shader language:
 * https://docs.godotengine.org/en/stable/tutorials/shaders/shader_reference/shading_language.html#flow-control
 *
 * Background info:
 * - Colors are vectors of floats in Godot (Red, Green, Blue, Alpha)!
 * - Depenging on your image, your screen and the compatibility mode you're using Godot in, 
 *   colors will differ in their floating point number representation, 
 *   even if you're seeing (or think you're seeing) the exact same color!
 *   For instance, see https://stackoverflow.com/q/75702333
 * - The basic idea is to have an image with up to 3 dummy colors to be replaced by pickable colors
 * - The max. 3 dummy colors should be either a
 *     o "full" red (R: 1.0, G: 0.0, B: 0.0) or
 *     o "full" green (R: 0.0, G: 1.0, B: 0.0) or
 *     o "full" blue (R: 0.0, G: 0.0, B: 1.0)
 * - Some image editors will not display values as 0.0 - 1.0, but as 0 - 255.
 *   In this case, use 0 for 0.0 and 255 for 1.0
 * - This makes color (vector) comparison much easier as floating point number errors don't come into effect as much
 * - Don't change the shader parameters "Color 1 Old" and "Color 2 Old" if not necessary!
 *   They're being displayed as pickable as a reference when picking "Color 1 New" and "Color 2 New"
 * - You probably shoudn't have to, but you can use EPSILON to tweak the tolerance for comparing colors, 
 *   if you really need to 
 * - Don't forget to set "Texture" -> "Filter" to "Nearest" in the Inspector when working with pixel art, 
 *   in order to prevent unwanted anti-aliasing
 * - Don't forget to enable "Local to Scene" in the shader's properties in the Inspector, 
 *   if you're planning to use multiple instances of the same shader with different color replacements
 * - You can use this shader in GDScript to set a different replacement color via something like: 
 *     var new_color = Color(
 *             randf_range(0.0, 1.0),
 *             randf_range(0.0, 1.0),
 *             randf_range(0.0, 1.0),
 *             1.0
 *     )
 *     sprite.material.set_shader_parameter("color_1_new", new_color)
 * - For details, see https://docs.godotengine.org/en/stable/classes/class_color.html
 */

shader_type canvas_item;

// Constants
const float EPSILON = 0.0001;

// Shader parameters
uniform vec4 color_1_old: source_color = vec4(0.0, 1.0, 0.0, 1.0);      // "full" green
uniform vec4 color_1_new: source_color = vec4(1.0, 1.0, 1.0, 1.0);      // white

uniform vec4 color_2_old: source_color = vec4(1.0, 0.0, 0.0, 1.0);      // "full" red
uniform vec4 color_2_new: source_color = vec4(0.0, 0.0, 0.0, 1.0);      // black

bool is_zero_approx_float(float p_arg) {
    return (
            (p_arg >= -EPSILON) &&
            (p_arg <= EPSILON)
    );
}

bool is_zero_approx_vec4(vec4 p_vec) {
    return (
            is_zero_approx_float(p_vec.x) &&
            is_zero_approx_float(p_vec.y) &&
            is_zero_approx_float(p_vec.z) &&
            is_zero_approx_float(p_vec.w)
    );
}

// This function is called on every pixel.
// See https://docs.godotengine.org/en/stable/tutorials/shaders/introduction_to_shaders.html
void fragment() {

    vec4 current_pixel_color = texture(TEXTURE, UV);
    vec4 new_pixel_color = current_pixel_color;

    // Don't change transparent pixels
    if (current_pixel_color.a > 0.0) {

        if (is_zero_approx_vec4(current_pixel_color - color_1_old)) {
            new_pixel_color = color_1_new;
        }

        if (is_zero_approx_vec4(current_pixel_color - color_2_old)) {
            new_pixel_color = color_2_new;
        }

        // Finally, update color of current pixel
        COLOR = new_pixel_color;
    }

}

PS: I wanted to define and use constants for the "full green" and "full red", but apparently you can't assign constants as default values to shader parameters (uniforms). πŸ™ˆ

PPS: I still believe that having a built-in function to compare colors would be a great addition and ease the life of (new) users a lot! 😎

Calinou commented 8 months ago

Closing, as the associated pull request was rejected as per https://github.com/godotengine/godot/pull/77239#issuecomment-1726409905.