godotengine
Add `srgb_color` shader hint.
Sometimes shaders want to accept a color as input that is always in the srgb color space. This adds a hint to vec3/vec4 to enable the color picker popup for those vectors but disable color space conversion.
@QbieShay This is something that has come up a few times. I thought there was a proposal, but all I can find is https://github.com/godotengine/godot-proposals/issues/2323
Basically, people want to use the color picker, but they want to keep colors in sRGB space. When using the source_color hint, Godot will automatically convert the color to linear in 3D shaders and in 2D shaders when using HDR. So we need a hint that tells Godot to use a color picker without giving Godot permission to do color space operations
Discussed this in the rendering meeting. It's clear to me now why the feature is wanted, but we discussed a different API:
- Support for
source_colorshould be extended to vec3 - This new hint should work together with source color, and just disable the unwanted operation.
For example
uniform vec3 albedo_color: source_color // shows color picker, performs conversion
unifomr vec3 albedo_color1: source_color, disable_color_conversion // shows color picker, but disables extra logic
uniform vec3 albedo_color2: disable_color_conversion // behaves like the previous one
This expands using color for vec3 (which is something that we should have already) and add an extra option, allows for compact syntax for advanced users, so everyone should be happy with this solution ^^
NOTE: I called it "disable color conversion" but we can find something better. But it should still be clear.
NOTE: I called it "disable color conversion" but we can find something better. But it should still be clear.
I like it, but its a bit long? Maybe no_color_conversion ?
Judging by the existing uniform hints and render modes, the most idiomatic name here is either color_conversion_disable or color_conversion_disabled.
color_conversion_disable matches existing hints more closely, but there is only one existing example: repeat_disable. color_conversion_disabled matches the multiple examples of render modes (cull_disabled, depth_test_disabled, specular_disabled, etc.). My preference personally is color_conversion_disabled.
The name is a little on the long side, but we already have some verbose hints like hint_normal_roughness_texture or alpha_to_coverage_and_one, so it's probably fine.
disable_color_conversion is not idiomatic because the order is wrong. To use an analogy with properties, idiomatically the "property name" comes first, then the "property value".
We discussed again in the rendering meeting and have settled on color_conversion_disabled as suggested by tetrapod. The one difference from Qbie's suggestion is that we agree that it should be an error to use it by itself
I'm not sure I follow your comments @clayjohn .
When the parser sees color_conversion_disabled it checks that the existing hint is HINT_SOURCE_COLOR and then replaces it with HINT_COLOR_CONVERSION_DISABLED since there can only be one hint set. So the conditions do trigger.
I'm not sure I follow your comments @clayjohn .
When the parser sees color_conversion_disabled it checks that the existing hint is HINT_SOURCE_COLOR and then replaces it with HINT_COLOR_CONVERSION_DISABLED since there can only be one hint set. So the conditions do trigger.
Oh, interesting. I see what you mean. Each hint overwrites the previous one. In cases where multiple hints are valid we have edge case handling. I really don't like that approach, but its not your fault. Its best to stick with the patterns that are there.
That just leaves my comment about the code completion.
I'm not sure I follow your comments @clayjohn . When the parser sees color_conversion_disabled it checks that the existing hint is HINT_SOURCE_COLOR and then replaces it with HINT_COLOR_CONVERSION_DISABLED since there can only be one hint set. So the conditions do trigger.
Oh, interesting. I see what you mean. Each hint overwrites the previous one. In cases where multiple hints are valid we have edge case handling. I really don't like that approach, but its not your fault. Its best to stick with the patterns that are there.
That just leaves my comment about the code completion.
Yeah, I don't like it either, but It didn't seem worth refactoring into a bitset or something just for this.
