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

[rollerozxa]

Fixes #14088

This PR fixes the tonemapping effect and makes it look like it used to before bloom being added, by moving it above the "linear colorspace" conversion and reverting the changes made in applyToneMapping.

It also moves the saturation effect outside of ENABLE_TONE_MAPPING so it's possible to use it without requiring tone mapping to be enabled. I don't know why it was made like that and it still works when moving it outside of the preprocessor blocks. If that should be moved into a separate PR then let me know.

To do

This PR is Ready for Review.

How to test

Test with tonemapping enabled, see that it looks like it used to. (See #14088 (comment))

Also test such that other shader effects such as saturation aren't messed up because of the change (to test the saturation effect, you can install the Saturation Adjustment mod)

[grorp]

Conceptually, I agree with both of your changes. I think it would be good to split them into two commits.

However, I think that applying tonemapping and saturation before exposure and bloom doesn't make sense. Tonemapping should stay at the end of the "second stage" shader. Maybe you could move tonemapping after

minetest/client/shaders/second_stage/opengl_fragment.glsl

Lines 125 to 126 in 16c2247

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

instead?

[rollerozxa]

Sounds good. I'll move it, split the PR into two commits and mark it as NOSQUASH.

[rollerozxa]

Done. I also edited lua_api.md to remove the mention of saturation requiring tonemapping. Hopefully I've re-applied everything correctly.

[rubenwardy]

cc @lhofhansl

[rubenwardy]

From @x2048

I think saturation control should be moved out of the current ENABLE_TONE_MAPPING option because of how broken that option is :)

What ENABLE_TONE_MAPPING does today is simply selecting between two specific tone mapping operators, but tone mapping control in broad sense is wider and includes exposure control, color tint, tone mapping operator, saturation, brightness, contrast and output gamma correction plus everything I forgot. I think all of these parameters must be exposed to the game developers and many of the adjustable by players, and there should be no mode where they are disabled. Some combinations of the parameters could give the "unprocessed" look of the picture, in the style of 3D games from 90s.

Regarding the tone mapping operators, Minetest supports two right now:

* Trivial Clamp operator when ENABLE_TONE_MAPPING is false and

* Uncharted 2 operator when ENABLE_TONE_MAPPING is true.

You can see that tone mapping happens all the time, and what the option chooses is merely the operator function. We could have more operators in the future:

* Reinhard, Extended Reinhard, Reinhard-Jodie

* ACES

* Custom parametric curves

* Custom camera response functions via LUT

[lhofhansl]

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.

[lhofhansl]

Tried it. Looks good to me.
I think we can improve Tonemapping in the future, but let's restore what it was for now.

[sfan5]

it still has no effect if shaders are disabled entirely

[grorp]

Please rebase.

[lhofhansl]

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

[rubenwardy]

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

[lhofhansl]

Fair enough.

[HybridDog]

This commit has changed the tone mapping function.

In the old code, the EOTF was executed at the beginning in applyToneMapping, so the tone mapping was applied in linearised sRGB at that time, too.
However, instead of an OETF with the exponent 1.0/2.2, in the end it applied the exponent 1.0/1.6.

In the current code, the EOTF is executed before calling applyToneMapping and the OETF with the exponent 1.0/2.2 is applied after calling applyToneMapping.
The current code also has a different exposureBias value.

So, if I understand it correctly, to make the tone mapping work like it did before, only two changes in applyToneMapping should be required:

• A pow(color.rgb, vec3(2.2 / 1.6)) at the end
• Setting the exposureBias to 5.5 instead of 2.0

However, there is low contrast, so I may be wrong:
master, no tone mapping:

master, with tone mapping:

master, with tone mapping and the above-mentioned changes applied:

[lhofhansl]

@HybridDog I agree. But is better than before. Let's get this in, and then we can refine.

@rollerozxa Can you please rebase? :)

[rollerozxa]

will do, sorry for the delay