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

[NOSQUASH] Fix tonemapping and apply saturation even if tonemapping is disabled #14109

Merged
merged 2 commits into from
Jan 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 13 additions & 9 deletions client/shaders/second_stage/opengl_fragment.glsl
Original file line number Diff line number Diff line change
Expand Up @@ -68,14 +68,17 @@ vec3 uncharted2Tonemap(vec3 x)

vec4 applyToneMapping(vec4 color)
{
const float exposureBias = 2.0;
color = vec4(pow(color.rgb, vec3(2.2)), color.a);
const float gamma = 1.6;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These constants come out of nowhere somehow.
In any case I looked up the original commit (9df79a4) that changed this behavior, and these constants were there before.
So that's good then as long as we're happy with the visual outcome - which I think we are.

const float exposureBias = 5.5;
color.rgb = uncharted2Tonemap(exposureBias * color.rgb);
// Precalculated white_scale from
//vec3 whiteScale = 1.0 / uncharted2Tonemap(vec3(W));
vec3 whiteScale = vec3(1.036015346);
color.rgb *= whiteScale;
return color;
return vec4(pow(color.rgb, vec3(1.0 / gamma)), color.a);
}
#endif

vec3 applySaturation(vec3 color, float factor)
{
Expand All @@ -84,7 +87,6 @@ vec3 applySaturation(vec3 color, float factor)
float brightness = dot(color, vec3(0.2125, 0.7154, 0.0721));
return mix(vec3(brightness), color, factor);
}
#endif

#ifdef ENABLE_DITHERING
// From http://alex.vlachos.com/graphics/Alex_Vlachos_Advanced_VR_Rendering_GDC2015.pdf
Expand Down Expand Up @@ -130,20 +132,22 @@ void main(void)
color = applyBloom(color, uv);
#endif


color.rgb = clamp(color.rgb, vec3(0.), vec3(1.));

// return to sRGB colorspace (approximate)
color.rgb = pow(color.rgb, vec3(1.0 / 2.2));

#ifdef ENABLE_BLOOM_DEBUG
if (uv.x > 0.5 || uv.y > 0.5)
#endif
{
#if ENABLE_TONE_MAPPING
color = applyToneMapping(color);
color.rgb = applySaturation(color.rgb, saturation);
#endif
}

color.rgb = clamp(color.rgb, vec3(0.), vec3(1.));

// return to sRGB colorspace (approximate)
color.rgb = pow(color.rgb, vec3(1.0 / 2.2));
color.rgb = applySaturation(color.rgb, saturation);
}

#ifdef ENABLE_DITHERING
// Apply dithering just before quantisation
Expand Down
5 changes: 2 additions & 3 deletions doc/lua_api.md
Original file line number Diff line number Diff line change
Expand Up @@ -8028,9 +8028,8 @@ child will follow movement and rotation of that bone.
* Passing no arguments resets lighting to its default values.
* `light_definition` is a table with the following optional fields:
* `saturation` sets the saturation (vividness; default: `1.0`).
values > 1 increase the saturation
values in [0,1) decrease the saturation
* This value has no effect on clients who have the "Tone Mapping" shader disabled.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it still has no effect if shaders are disabled entirely

* values > 1 increase the saturation
* values in [0,1] decrease the saturation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually that was meant to read [0,1). In math ')' denotes the end of an interval excluding the end value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please write that out, don't rely on math notation like that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough.

* `shadows` is a table that controls ambient shadows
* `intensity` sets the intensity of the shadows from 0 (no shadows, default) to 1 (blackness)
* This value has no effect on clients who have the "Dynamic Shadows" shader disabled.
Expand Down