Tonemapping effect does not do much anymore

[rollerozxa]

I was looking through some of my old screenshots, and noticed how much brighter and vibrant they are with tonemapping compared to looking at Minetest with tonemapping enabled right now. For example, take this screenshot from 2020 of my old city world with tonemapping enabled:

I still have the world, so I booted it up on Minetest 5.8.0 with tonemapping enabled and tried to get into the same position as the old screenshot:

It looks dull! Compared to the old tonemapping, it feels like there's a darkening filter applied over the image, there is not as much contrast if you look at e.g. the stone brick texture. There's not much difference when tonemapping is enabled compared to when it is disabled now.

Looking at a git blame for the tonemapping code, it led me to the commit which added bloom shaders. It both changed the applyToneMapping function and enclosed it in some bloom related code, having the tonemapping effect be affected by the "translate to linear colorspace" (unsure what this means) line. I made these quick and dirty changes to the second_stage shader in an attempt to revert to the previous effect, reverting the changes in applyToneMapping and putting the call above "translate to linear colorspace":

diff --git a/client/shaders/second_stage/opengl_fragment.glsl b/client/shaders/second_stage/opengl_fragment.glsl
index 928e408e2..6818ccdb7 100644
--- a/client/shaders/second_stage/opengl_fragment.glsl
+++ b/client/shaders/second_stage/opengl_fragment.glsl
@@ -61,13 +61,15 @@ 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;
+	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);
 }
 
 vec3 applySaturation(vec3 color, float factor)
@@ -92,6 +94,8 @@ void main(void)
 	vec4 color = texture2D(rendered, uv).rgba;
 #endif
 
+	color = applyToneMapping(color);
+
 	// translate to linear colorspace (approximate)
 	color.rgb = pow(color.rgb, vec3(2.2));
 
@@ -114,7 +118,7 @@ void main(void)
 	if (uv.x > 0.5 || uv.y > 0.5)
 #endif
 	{
-#if ENABLE_TONE_MAPPING
+#if 0
 		color = applyToneMapping(color);
 		color.rgb = applySaturation(color.rgb, saturation);
 #endif

The resulting image looks almost identical to the old tonemapping that you could see in the first image. Vibrant and bright:

So what?

I am unsure if this change that was made as a part of the bloom commit is a bug or if it was intentional to nerf the effect caused by tonemapping, and since x2048 is no longer around it would be hard to know for sure. Personally I consider the change to be a bug and should be fixed by making the effect look like it used to (basically by doing what my diff above does, but keeping the saturation effect intact), but I'm sure someone who thought the old tonemapping was too bright would consider it to be an intentional change.

[numberZero]

I’d consider that a bug. Minetest doesn’t use any HDR rendering so that so-called “tone mapping” is just a fancy visual effect, and it should do whatever it did, i.e. make dull MTG textures shine.

if it was intentional to nerf the effect caused by tonemapping

Totally unlikely. The intention was likely to apply filters in the linear colorspace as that’s where they’re typically defined (not that that makes much sense with Minetest’s lighting model). Tone mapping however, was apparently dependent on working in the non-linear sRGB(-like) colorspace (regardless of what colorspace the original UC2 function was designed for).

[lhofhansl]

I agree with @numberZero . That's a bug we should fix.

@rollerozxa Feel like creating a PR?

[rollerozxa]

Will do. I've basically got the fix ready already.

While I was at it I was looking at why the saturation effect requires tonemapping to be enabled and realised there is absolutely no reason it requires tonemapping to be enabled to work (it was just put behind a preprocessor block like that for some reason)

[HybridDog]

A saturation-increasing effect usually requires tonemapping, or at least a proper gamut clipping, so that out-of-gamut colours are mapped well to colours which can be displayed:
https://bottosson.github.io/posts/gamutclipping/#tulip

However, for this the tonemapping should support input colour values less than 0 and it should be applied after the saturation increase and not before, like it's done in Minetest.

I think tonemapping should not be applied in the sRGB colour space, so personally I prefer an adjustment of the tonemapping parameters instead, which could have a similar effect to switching colour spaces.

[numberZero]

I think tonemapping should not be applied in the sRGB colour space, so personally I prefer an adjustment of the tonemapping parameters instead, which could have a similar effect to switching colour spaces.

That doesn’t matter IMO. It’s just a visual effect.