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

Using vec4 values in canvas_item shaders appear washed out when 2D HDR is active. #84989

Closed
JohnPandich opened this issue Nov 16, 2023 · 19 comments · Fixed by #92444
Closed

Using vec4 values in canvas_item shaders appear washed out when 2D HDR is active. #84989

JohnPandich opened this issue Nov 16, 2023 · 19 comments · Fixed by #92444

Comments

@JohnPandich
Copy link

Godot version

4.2 beta 6

System information

Linux Mint, Forward+, ntel© Xeon© CPU E5-2650 v4 @ 2.20GHz × 12, 64gb ram

Issue description

When using a canvas item shader, vec4 values appear washed out when 2D HDR is activated. Anecdotally, glows seem to appear more bland and white than in 3.x when I would expect them to be more "dynamic". This leads me to believe there is an issue with 2D HDR's sRGB -> linear conversion.

With 2D HDR:
image

With out 2D HDR:
image

Steps to reproduce

To reproduce:

Enable "HDR 2D" in Rendering -> Viewport settings
Create a Sprite2D in a scene
Add a shader to that Sprite2D's material with a vec4 color that is not black or white, but in the mid range.
Set the COLOR in the fragment fragment function to a value from the vec4.
To view this issue in the "minimal reproduction project", open the scene named "repro.tscn" and activate/deactivate 2D HDR.

Minimal reproduction project

ColorRepro.zip

@JohnPandich
Copy link
Author

I can confirm that this is also in 4.2 RC 1

@Calinou
Copy link
Member

Calinou commented Nov 21, 2023

I can confirm this on 4.2.rc1 (Linux, GeForce RTX 4090 with NVIDIA 535.129.03). This also occurs with vec3 (colors that don't have an alpha component).

Using the Canvas background mode in Environment and playing with tonemap properties doesn't yield the expected appearance, so the issue is somewhere else.

A Sprite2D with icon.svg retains the exact same color for the Godot blue (#478cbf) regardless of whether HDR 2D is enabled.

Edit: Disregard the accidental close/reopen above.

@h0lley
Copy link

h0lley commented Nov 25, 2023

Ran into this issue as well when I needed to compare colors in a shader.

Textures passed into the main texture property of e.g. Sprite2D appear to remain untouched, while textures and/or colors passed via uniforms and even shader constants are converted; appear brighter.

here's some illustrations:

image

the shader displays the very same texture differently depending on whether its passed via uniform or taken from Sprite2D's texture property.

Colors passed via uniform and even shader constants are affected, too:

image

when trying to compare the two supposedly identical colors in the shader (for instance via distance()) then they are off.

edit:

clayjohn just explained to me on rocket chat that we are supposed to use the source_color hint on sampler2D uniforms, and that indeed solves the problem for passing textures. however just like OP my original application is with passing a Color and then the issue does occur despite the source_color hint:

image

my specs:
Godot v4.2.rc1.official [ad72de5]
Ubuntu 23.10
Kernel 6.5.0-13-generic
NVIDIA GeForce GTX 970
Driver 535.129.03

@darksylinc
Copy link
Contributor

darksylinc commented Nov 25, 2023

This is exactly an sRGB vs linear issue. I color-picked the comparisons pictures posted by OP and the difference was exactly the sRGB conversion.

I'm not familiar enough with the whole pipeline in 2D rendering to fix this issue, and what Godot should be doing (probably what users expect? isn't that a bit subjective? Maybe a checkbox to toggle automatic conversion).

In case someone wants to take a stab at this problem, the conversion routines (which should happen in the CPU so they're sent already converted to the GPU) are here.

Also these are all the same problem: #82351 #82154 #56634

@JohnPandich
Copy link
Author

I believe that any viewport running with HDR 2D enabled should do automatic conversion for all "source_color" shader parameters. Do any of the folks with more experience working in engine code see a flaw in that simple conclusion?

@clayjohn
Copy link
Member

clayjohn commented Jan 11, 2024

I believe that any viewport running with HDR 2D enabled should do automatic conversion for all "source_color" shader parameters. Do any of the folks with more experience working in engine code see a flaw in that simple conclusion?

I don't see in flaw in your conclusion. You describe how it's supposed to work. That's why this is labelled as a bug.

@darksylinc
Copy link
Contributor

I believe that any viewport running with HDR 2D enabled should do automatic conversion for all "source_color" shader parameters. Do any of the folks with more experience working in engine code see a flaw in that simple conclusion?

Nope, that's how it's supposed to work. That's why this is labelled as a bug.

Clay: your wording is confusing. You're making it sound like you're in disagreement with what JohnPadlich said but reading it carefully you're both in agreement.

@clayjohn
Copy link
Member

I believe that any viewport running with HDR 2D enabled should do automatic conversion for all "source_color" shader parameters. Do any of the folks with more experience working in engine code see a flaw in that simple conclusion?

Nope, that's how it's supposed to work. That's why this is labelled as a bug.

Clay: your wording is confusing. You're making it sound like you're in disagreement with what JohnPadlich said but reading it carefully you're both in agreement.

Oops, updated my wording to be more clear that "nope" means "I don't see a flaw in your conclusion"

@JohnPandich
Copy link
Author

JohnPandich commented Jan 15, 2024

I have done an initial dive into approaching this issue and I am including my notes in this ticket. My goal was to see where Sampler2D's "source_color" was accounted for and how this translated into a linear_to_sRGB conversion.

  1. shader_language.cpp (@ line 8681 at the time of this update) includes the following code which marks a Sampler2D has having the "source_color" hint:
    `
    case TK_HINT_SOURCE_COLOR: {
    if (type != TYPE_VEC3 && type != TYPE_VEC4 && !is_sampler_type(type)) {
    _set_error(vformat(RTR("Source color hint is for '%s', '%s' or sampler types only."), "vec3", "vec4"));
    return ERR_PARSE_ERROR;
    }

    							if (is_sampler_type(type)) {
    								if (uniform.use_color) {
    									_set_error(vformat(RTR("Duplicated hint: '%s'."), "source_color"));
    									return ERR_PARSE_ERROR;
    								}
    								uniform.use_color = true; // <- HERE IS THE LINE OF CODE (8281)
    							} else {
    								new_hint = ShaderNode::Uniform::HINT_SOURCE_COLOR;
    							}
    						} break;
    

`

  1. shader_compiler.cpp (@ line 597 at the time of this update) transfers this information to a texture object
    `
    if (SL::is_sampler_type(uniform.type)) {
    for (int j = 0; j < STAGE_MAX; j++) {
    r_gen_code.stage_globals[j] += ucode;
    }

     			GeneratedCode::Texture texture;
     			texture.name = uniform_name;
     			texture.hint = uniform.hint;
     			texture.type = uniform.type;
     			texture.use_color = uniform.use_color;  // <- HERE IS THE LINE OF CODE (597)
     			texture.filter = uniform.filter;
     			texture.repeat = uniform.repeat;
     			texture.global = uniform.scope == ShaderLanguage::ShaderNode::Uniform::SCOPE_GLOBAL;
     			texture.array_size = uniform.array_size;
     			if (texture.global) {
     				r_gen_code.uses_global_textures = true;
     			}
    
     			r_gen_code.texture_uniforms.write[uniform.texture_order] = texture;
     		} else {
    

`

  1. Finally, in material_storage.cpp (@ line 918 at the time of this update), this flag is checked and
    an sRGB version of the texture is returned if "rd_texture_srgb.is_valid()" is true
    `
    bool srgb = p_use_linear_color && p_texture_uniforms[i].use_color;

     	for (int j = 0; j < textures.size(); j++) {
     		TextureStorage::Texture *tex = TextureStorage::get_singleton()->get_texture(textures[j]);
    
     		if (tex) {
     			rd_texture = (srgb && tex->rd_texture_srgb.is_valid()) ? tex->rd_texture_srgb : tex->rd_texture; // <- HERE IS THE LINE OF CODE (918)
    

`

  1. texture_storage.cpp has some logic to determine if srgb is appropriate which could be useful in analyzing when addressing this issue for vec3 and vec4 values.

Unfortunately I do not have a clear idea on how to approach this yet. It seems as though it would be relatively straight forward to mark source_color for vec3 and vec4 uniforms and to do a linear_to_srgb conversion in shader_compiler._dump_node_code storing 2 values, 1 converted and one unconverted. The more difficult problem to address is how to propagate HDR 2D information when rendering and to actually choose the appropriate value. Any suggestions by someone with more experience in engine code is greatly appreciated.

@JohnPandich
Copy link
Author

On my next dive I will look into how uniforms are updated via GDScript's "set_shader_parameter" utilize that logic to find a way to inject if HDR 2D is being used via the material. (Hopefully in the next few months.)

@darthLeviN
Copy link
Contributor

Just wanted to report something similar. To my understanding source_color is not performing srgb -> linear conversion for vec4 if HDR 2D is enabled.

@produno
Copy link

produno commented Feb 7, 2024

Is there any current workaround for this? I would love to update my steam screenshots and i am planning to create my initial trailer soon so would like my game to look as pretty as possible.

I tried adjusting colour conversion but it didn't work.

@JohnPandich
Copy link
Author

@produno As a work around you can add a 1 pixel texture of the color and use that. Best of luck.

@ahalldenabberton
Copy link

demo

I'm assuming this is going to get fixed in some later update to Godot, but in the meantime, if there are other people like me looking to use HDR 2D for its glow effects while trying to avoid messing up the colors on your 2D shaders, the following code's worked for me:

shader_type canvas_item;

uniform vec4 targetColor: source_color;
uniform bool convertLinearToSRGB = true;

float linearToSRGB(float input) {
    if(input < 0.040449907){
		return input/12.92;
	}else{
		return pow(((input+0.055)/1.055),2.4);
	}
}

void fragment() {
	if (! convertLinearToSRGB){
		COLOR.rgb = targetColor.rgb;
	}else{
		vec3 convertedColor = vec3(linearToSRGB(targetColor.r),linearToSRGB(targetColor.g),linearToSRGB(targetColor.b));
		COLOR.rgb = convertedColor;
	}
}

@produno
Copy link

produno commented Apr 8, 2024

I'm assuming this is going to get fixed in some later update to Godot, but in the meantime, if there are other people like me looking to use HDR 2D for its glow effects while trying to avoid messing up the colors on your 2D shaders, the following code's worked for me:

This doesn't work for me as can be seen here.

This is with HDR off.
HDRoff

This is with HDR on using the code above.
HDRon

The textures that use the shader are way too dark.

@Dowsley
Copy link
Contributor

Dowsley commented May 12, 2024

@Calinou do we have any fix for this on the current snapshot of godot 4.3, or at least a planned fix for the upcoming beta? I skimmed through the changelog of the former but couldn't find anything.

@Dowsley
Copy link
Contributor

Dowsley commented May 12, 2024

I'm assuming this is going to get fixed in some later update to Godot, but in the meantime, if there are other people like me looking to use HDR 2D for its glow effects while trying to avoid messing up the colors on your 2D shaders, the following code's worked for me:

Thank you, this worked as a workaround.

@produno
Copy link

produno commented May 13, 2024

@Calinou do we have any fix for this on the current snapshot of godot 4.3, or at least a planned fix for the upcoming beta? I skimmed through the changelog of the former but couldn't find anything.

It's not fixed in 4.3 dev 6. I have not seen anything indicating it will be fixed for 4.3.

@clayjohn
Copy link
Member

@Calinou do we have any fix for this on the current snapshot of godot 4.3, or at least a planned fix for the upcoming beta? I skimmed through the changelog of the former but couldn't find anything.

It's not fixed in 4.3 dev 6. I have not seen anything indicating it will be fixed for 4.3.

When a fix is merged, this issue report will be closed. As long as this is open, the issue remains unfixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.