godotengine / godot

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

Editor interpreting unsigned `uvec` shader uniform parameters as signed #92064

Open xkisu opened 4 months ago

xkisu commented 4 months ago

Tested versions

Reproducible in v4.2.1-stable

System information

Godot v4.2.1.stable - macOS 14.4.1 - Vulkan (Forward+) - integrated Apple M1 - Apple M1 (8 Threads)

Issue description

The editor is refusing to let me specify integers over the 32-bit signed max for unsigned shader vector parameters. When I try to specify one over the signed max (i.e. an ARGB hexidecimal color) the editor reverts it to the signed 32-bit max. The max of an unsigned 32-bit integer field should be 4294967295, but as you can see with the steps below it's capped at the signed 32-bit limit of 2147483647.

I'm not sure if this is a wider problem with Godot using the signed Vector4i type for uvec4 shader types (which is a problem of it's own), or if this is just an issue with how the editor is interpreting the parameter limits. image

image

Steps to reproduce

  1. Create a Shader with a uvec2, uvec3, or uvec4 parameter (example using my shader):
    
    // color_ramp.gd
    shader_type canvas_item;

uniform uvec4 a_span; // [spanX, y, color0, color1]

varying vec4 v_rampColor;

mediump vec4 unpack_color_int(uint color) { // Format is ARGB uint r = (color >> 16u) & 0xffu; uint g = (color >> 8u) & 0xffu; uint b = (color >> 0u) & 0xffu; uint a = (color >> 24u) & 0xffu;

return vec4(float(r), float(g), float(b), float(a)) / 255.0f;

}

void vertex() { v_rampColor = unpack_color_int((VERTEX_ID & 1) == 0 ? a_span.z : a_span.w); }

void fragment() { COLOR = v_rampColor; }


2. Attach the shader to a canvas item.
3. Attempt to set the Z or W parameters to a uint32 hex value, i.e. `4278255360` (`0xff00ff00`).
![image](https://github.com/godotengine/godot/assets/28452108/aee3783a-ac39-44fb-abc3-f26500cdd582)
4. Notice that after hitting enter, the value reverts to the **signed** 32-bit integer max.
![image](https://github.com/godotengine/godot/assets/28452108/39235d1c-3491-4daa-8b2e-02afcc78a417)

### Minimal reproduction project (MRP)

[unsigned_shader_parameter_issue.zip](https://github.com/godotengine/godot/files/15358931/unsigned_shader_parameter_issue.zip)
xkisu commented 4 months ago

I just realized this does work correctly if I pass the signed decimal conversion equivalent to the fields (don't know why I didn't think to try that before), but I still feel like it should be possible to enter the unsigned values directly for an unsigned field - it feels like erroneous behaviour to have to use signed integers to set an unsigned field. image

AThousandShips commented 4 months ago

Thank you for reporting, consolidating in:

Which has been solved on latest branch

See there for more details, if you think something was missed about this and it's not the same issue, please comment here and it can be reopened

xkisu commented 4 months ago

Thank you for reporting, consolidating in:

Which has been solved on latest branch

See there for more details, if you think something was missed about this and it's not the same issue, please comment here and it can be reopened

It looks like #89436 does fix it for uint parameters, but doesn't for uvec* parameter and the merged changes added a todo comment for them. Would be good to also fix it for uvec parameters otherwise they experience the same behaviour.

https://github.com/godotengine/godot/blob/bd2300d77a6008167043f23fd91bcc562cde0a19/servers/rendering/shader_language.cpp#L4139

AThousandShips commented 4 months ago

True forgot that detail, then this will be fixed by: