godotengine / godot-docs

Godot Engine official documentation
https://docs.godotengine.org
Other
3.91k stars 3.2k forks source link

Accessing COLOR as input variable breaks shader unintuitively #5545

Open SyntaxPartnership opened 5 years ago

SyntaxPartnership commented 5 years ago

Godot version: 3.1 Alpha 2

OS/device including version: Windows 7 Professional Service Pack 1

Issue description: Shader turns entire sprite white.

Tried both of the following shader coding:

shader_type canvas_item;

//Set target colors.
uniform vec4 target_color_1 : hint_color;
uniform vec4 target_color_2 : hint_color;
uniform vec4 target_color_3 : hint_color;

//Set replacement colors.
uniform vec4 replace_color_1 : hint_color;
uniform vec4 replace_color_2 : hint_color;
uniform vec4 replace_color_3 : hint_color;

void fragment() {
    if (COLOR == target_color_1)
    {
        COLOR = replace_color_1
    }
    if (COLOR == target_color_2)
    {
        COLOR = replace_color_2
    }
    if (COLOR == target_color_3)
    {
        COLOR = replace_color_3
    }
}

And:

shader_type canvas_item;
uniform float threshold : hint_range(0,1) = 0.01;
uniform vec4 T0:hint_color = vec4(0.058823, 0.058823, 0.058823, 1);
uniform vec4 T1:hint_color = vec4(0.254902, 0.254902, 0.254902, 1);
uniform vec4 T2:hint_color = vec4(0.45098, 0.45098, 0.45098, 1);

uniform vec4 R0 : hint_color;
uniform vec4 R1 : hint_color;
uniform vec4 R2 : hint_color;

void fragment() {
    vec3 c = COLOR.rgb;

    if (abs(T0.r-c.r) <= threshold && abs(T0.g-c.g) <= threshold && abs(T0.b-c.b) <= threshold) {COLOR.rgb = R0.rgb;}
    if (abs(T1.r-c.r) <= threshold && abs(T1.g-c.g) <= threshold && abs(T1.b-c.b) <= threshold) {COLOR.rgb = R1.rgb;}
    if (abs(T2.r-c.r) <= threshold && abs(T2.g-c.g) <= threshold && abs(T2.b-c.b) <= threshold) {COLOR.rgb = R2.rgb;}

}

test2 (Ignore the green pixel.)

Steps to reproduce: Run app.

DavidSichma commented 5 years ago

COLOR is intended to be ~only~ primary used as an output. Related godotengine/godot#23179.

Try vec4 col = texture(TEXTURE, UV);.

EDIT: COLOR is actually the output of the vertex shader. Unless you use vertex colors this will be white.

akien-mga commented 5 years ago

This is indeed expected as mentioned by @DavidSichma. Yet many users seem to be hit by this, so we should see if we can improve the UX, e.g. by getting a compilation error when trying to read COLOR before it has been assigned.

clayjohn commented 5 years ago

@akien-mga In the fragment shader COLOR is both an input and output. It is the vertex color output from the vertex shader into the fragment shader and it is the output color from the fragment shader. So it would not make sense to throw a compilation error when it is used. I think the best thing to do is just document COLOR better and explain that it does not automatically read from the texture.

akien-mga commented 4 years ago

CC @Chaosus

Chaosus commented 4 years ago

By default, vertex color is always white so comparison like if (COLOR == target_color_1) doesn't make much sense. More commonly, If you need to replace a texture pixel color in a fragment shader you should use texture(sampler, UV) instead COLOR...

Chaosus commented 4 years ago

@akien-mga I don't understand what unintuitively and wrong in this behavior. Do you want me to create a PR to warn users about this? Of course, I could :)

clayjohn commented 4 years ago

As I said above, I don't think there should be an editor warning. I think this is just a documentation issue.

Chaosus commented 4 years ago

Phew... Then I think so too. It's rather hard to implement such warn system anyways 😄

akien-mga commented 2 years ago

Transferred to godot-docs as this seems to be a documentation issue.

From a quick look at https://docs.godotengine.org/en/stable/tutorials/shading/shading_reference/canvas_item_shader.html#vertex-built-ins, I'd say that it might still not be crystal clear that one can't read from COLOR in the vertex function (it's specified as inout, without particular caveat).