Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename hint_albedo for sRGB -> Linear color conversion in shaders #2323

Closed
kb173 opened this issue Feb 20, 2021 · 15 comments · Fixed by godotengine/godot#60803
Closed

Rename hint_albedo for sRGB -> Linear color conversion in shaders #2323

kb173 opened this issue Feb 20, 2021 · 15 comments · Fixed by godotengine/godot#60803
Assignees
Labels
breaks compat Proposal will inevitably break compatibility topic:shaders topic:3d
Milestone

Comments

@kb173
Copy link

kb173 commented Feb 20, 2021

Describe the project you are working on

A landscape visualization using a variety of custom shaders; this is not project-specific though.

Describe the problem or limitation you are having in your project

As is evident in the comments of the original issue for this and other posts such as godotengine/godot#11381 or https://godotdevelopers.org/forum/discussion/19049/custom-splatmap-shader-does-not-work-correctly-in-3-0-solved, many people (myself included) are initially confused by the sRGB -> Linear color space conversion in shaders, which is done as follows:

uniform sampler2D tex : hint_albedo;

The updated docs do make this clearer, but a hint_ still isn't where many people would intuitively search for when the 'colors look wrong'. In my experience, throughout the engine, 'hints' are usually something that make development easier, e.g. by providing better autocompletion or by exposing something to the editor in a more convenient way. It's unusual for a hint to be required for producing a correct output.

In order to make shaders slightly more approachable, I propose renaming this to something that is more obvious to newcomers.

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

Rename the keyword. Simply uniform sampler2D tex : albedo; would already be better in my opinion; something like uniform sampler2D tex : color_srgb; may be even more intuitive. This would be easy to do, but it'd break compatibility.

I suppose this could also be handled completely differently, but it's just a minor issue with no need for a big restructure.

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

uniform sampler2D tex : albedo;
// or
uniform sampler2D tex : color_srgb;

These are just some of my ideas, maybe someone else can come up with something better.

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

The default behavior will still be no conversion.

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

The functionality of converting a texture from sRGB to Linear is already in the engine, this is just a rename of that.

@itsybitesyspider
Copy link

I'd like to elaborate a little as I seem to have triggered this proposal. This problem occurs from the point of view of someone who is familiar with linear and sRGB color spaces and graphics programming, but is new to writing non-visual shaders in Godot. In many graphics pipelines an sRGB hinting mechanism as this does not exist and won't be anticipated.

Terminology issues:

"hint" - problematic in the implication that the effect is trifling and not worth my time to ponder.
"albedo" - problematic in that not all sRGB color data is albedo and not all albedo data is in sRGB.
"color" - problematic when used alone because it's too vague
"color/data" - this is what's used the drop down in the VisualShader editor and is super apparent as a suspect in any color correction issue. However it loses clarity of meaning in a text shader where both options are not adjacent.
"linear/gamma/sRGB" - any of these terms, even if used poorly, immediately alert the user that the item is related to color correction.

This proposal was needed because a change to the keyword would break old projects. Therefore we need a discussion. I suggest the following:

(1) New keywords (whatever is finally chosen by the bikeshed senate) be introduced that have the same behavior.
(2) The documentation be updated to use the new keyword.
(3) The visual shader be updated to output the new keyword.
(4) The tool should raise a warning that the old keyword is deprecated.
(5) Leave a TODO so someone might delete the old keyword sometime in 2030.

@Calinou
Copy link
Member

Calinou commented Feb 20, 2021

(1) New keywords (whatever is finally chosen by the bikeshed senate) be introduced that have the same behavior.

We'd rather have only one way to achieve a given result, rather than multiple ways to achieve the same result. Personally, I would name the hint just srgb or color_srgb.

(5) Leave a TODO so someone might delete the old keyword sometime in 2030.

If we change this keyword before Godot 4.0 is released, we can remove the old keyword for 4.0 🙂
This is why I added the 4.0 milestone to this proposal.

@Jummit

This comment has been minimized.

@itsybitesyspider
Copy link

If the lead developers are comfortable with breaking old projects over this, I have no objection. I just thought that was the point of the discussion.

@itsybitesyspider
Copy link

We probably want to bikeshed the entire list, though, since they will want to have an internally consistent set of names:

hint_color, for example, probably implies that vec4 color uniforms are also sRGB. But I actually don't know (which is the problem).

Type Hint Description
vec4 hint_color Used as color
int, float hint_range(min,max [,step] ) Used as range (with min/max/step)
sampler2D hint_albedo Used as albedo color, default white
sampler2D hint_black_albedo Used as albedo color, default black
sampler2D hint_normal Used as normalmap
sampler2D hint_white As value, default to white.
sampler2D hint_black As value, default to black
sampler2D hint_aniso As flowmap, default to right.

@rainlizard
Copy link

A few more victims:

https://godotengine.org/qa/35289/unusual-color-ramping-on-shadermaterial-textures-help

hint_albedo confused newbies
Total victim count... 15+

The reason I went to this effort is because akien and reduz godotengine/godot#24992 (comment) have given the impression that they believe things are fine as they are, which diminishes the chances of this proposal being successful.

@clayjohn
Copy link
Member

clayjohn commented May 1, 2022

We probably want to bikeshed the entire list, though, since they will want to have an internally consistent set of names:

I think that is a good idea. I suggest the following renames/additions

  1. hint_albedo -> source_srgb:
    This clarifies that the source image/color is in sRGB space and needs to be converted to linear space before being sampled in the shader. (This hint will likely be ignored in the OpenGL3 renderer)
  2. Get rid of hint_albedo_black:
    as it becomes unessecary as it is just source_srgb and hint_black
  3. hint_color:
    change the behaviour to just specify that this uses a color value (so a color picker is used), but don't do sRGB to linear conversion as is done now.

@Calinou
Copy link
Member

Calinou commented May 5, 2022

We discussed this in a proposal meeting and agreed to the following changes:

  • hint_albedosource_color
  • hint_black/hint_whitehint_default_black/hint_default_white

@Chaosus
Copy link
Member

Chaosus commented May 5, 2022

@Calinou and what doing with a hint_black_albedo ?

@Chaosus Chaosus self-assigned this May 5, 2022
@Calinou
Copy link
Member

Calinou commented May 5, 2022

@Calinou and what doing with a hint_black_albedo ?

I think hint_black_albedo can still be removed, as a black sRGB color is identical to a black linear color.

@golddotasksquestions
Copy link

@Calinou @clayjohn

I really don't understand this name change. The issue people are having seems to be with texture color space conversion.

If the intention was to make things more intuitive, this is my opinion a catastrophic failure.

As a newcomer just as much as a seasoned Godot user, when I to define a uniform color to show up as a color-pickable color-button in the Inspector, I think of hints. Why? because a hint is what this is! After the colon I give the editor a hint on how to interpret the value after the =

"hint" is what we have in the export variables in GDScript too. They fulfill the exact same purpose. "hint" is what the other shader uniform hints are called too. For this reason they still have the word "hint_" as prefix.

image

I spend the bigger part of my life with graphics and I still don't understand what "source" could possibly mean in the context of a vec4 color
I really don't understand why "source_color" is any better, or easier to guess than "hint_color" for a vec3 or vec4 color.
I don't understand why anyone here would think why this inconsistency makes Godot shader easier to use.

Using "source_color" instead "hint_color" for vec3 vec4 color-pickable color-button uniforms does absolutely nothing at all to help users with the srgb color space texture issue. All this does is to introduce more inconsistency and making hints harder to guess (therefore less intuitive!)

@clayjohn
Copy link
Member

clayjohn commented Oct 3, 2022

@golddotasksquestions the problem lies in the fact that the hint_color wasn't actually a hint (like the other hints are). hint_color did 2 things, it hinted to the inspector that a color picker should be used and it ensured that proper sRGB->linear conversions were done on the color. The name "hint" implies that it just provides context to the inspector, but we weren't respecting that.

This was a very common source of error as the comments in this proposal highlight.

I understand that getting used to the new name is frustrating for users like yourself who have already figured out the nuance of stuff like this in 3.x. But for new users it is very important that we are clear and consistent and we don't hide important functionality behind unintuitive names.

I'm confident that you will get used to the new name with time.

@golddotasksquestions
Copy link

golddotasksquestions commented Oct 4, 2022

@clayjohn Thanks for the explanation, very much appreciated! But it seems I have actually not at all figured out those nuances. I have reread the new documentation a few times now, and tried to understand what you mean here in your last comment, I understood the first part, but I still don't understand the second part of what hint_color supposedly did.

I am aware about srgb-liniar color space conversion when it comes to texture, but how is this relevant when using hint_color uniform vec3 vec4 (so no sampler2D)?

What's even more confusing for me now is how "hint_" prefix is used for sampler2D uniforms ...

@clayjohn
Copy link
Member

clayjohn commented Oct 4, 2022

I am aware about srgb-liniar color space conversion when it comes to texture, but how is this relevant when using hint_color uniform vec3 vec4 (so no sampler2D)?

It does the same thing, it ensures that the color is in the appropriate color space when read in the shader. For spatial shaders it ensures that the color is linear while for 2D shaders it ensures that the color is in sRGB.

What's even more confusing for me now is how "hint_" prefix is used for sampler2D uniforms ...

For sampler2D uniforms the hint_ prefix tells the engine what the texture will be used for so the engine can either provide a good default or expose it nicely in the editor (like exposing a slider for range etc.) But the hint_ does not change the value that you read from the texture

@golddotasksquestions
Copy link

Thanks! I think I now understand how this name change came to be, but I still think it's a terrible choice and failure at making naming more consistent and therefore easier to remember and more intuitive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaks compat Proposal will inevitably break compatibility topic:shaders topic:3d
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

9 participants