Fix incorrect Reinhard tonemap operator

[tracefree]

Fixes #93317

This PR changes the formula for applying the Reinhard tonemap operator to correctly use equation 4 in Reinhard's orginal 2002 paper and updates relevant documentation accordingly. For more details see the discussion in #93317.

Here are some screenshots of the TPS demo with and without the fix applied, taken at various whitepoint values:

With the fix

white = 6.0. This is the intended setting for the scene.

white = 1.0

white = 0.5

Without the fix (current master branch)

white = 6.0

white = 1.0

white = 0.5

[clayjohn]

You will also need to change the code for the Compatibility renderer here:

godot/drivers/gles3/shaders/tonemap_inc.glsl

Line 80 in e2fc6d3

return (p_white * color + color) / (color * p_white + p_white);

[tracefree]

I fail a Godot CPP test because I changed the enum name TONE_MAPPER_REINHARDT to TONE_MAPPER_REINHARD. Should I just revert that change? It's not essential to the bug fix, I just thought I'd fix the spelling while I'm at it, but I realize now that this would break compatibility with Godot CPP.

[clayjohn]

I fail a Godot CPP test because I changed the enum name TONE_MAPPER_REINHARDT to TONE_MAPPER_REINHARD. Should I just revert that change? It's not essential to the bug fix, I just thought I'd fix the spelling while I'm at it, but I realize now that this would break compatibility with Godot CPP.

Ya, you should revert it. It sucks to have a misspelled enum name. But it is more important to maintain compatibility then it is to correct a spelling error

[tracefree]

I reverted the enum name change, added comments linking to the paper in the code, and made the use of whitespace in the forumula given in the documentation consistent.

[allenwp]

Here's another comparison of before and after using some test scenes I just whipped up:

All images use Forward+ with whitepoint 6.0

Tonemap	Gradients (Linear Scale, No Debanding)	Hues (Exponential Scale, No Debanding)
Reinhard
Corrected Reinhard PR #93324

[clayjohn]

Sorry, I lost track of this after my last round of reviews. It looks good to me!

Just for posterity, I commented on the increase in instructions in a previous review. It makes sense for us to use this optimized version over the naive formula. But I am not concerned about the overall increase in instructions from using the correct formula for two reasons:

1. Its the correct formula, using an incorrect formula isn't a good optimization
2. This shader is texture read bound (especially if using glow, or FXAA etc.), so the extra instructions will be hidden by the usual latency hiding mechanisms.

It still makes sense to use the optimized form for two reasons:

1. Its better for mobile devices as it will use less battery (despite not running faster)
2. Its better if we end up copying this code in another part of the engine that may be more instruction-sensitive

[clayjohn]

Just since it has stagnated for awhile, do you mind doing an empty force push just to force the CI to run again?

[akien-mga]

I pushed a rebase for good measure as the base branch for this PR was 2500 commits behind master.

[akien-mga]

Thanks! And congrats for your first merged Godot contribution 🎉

[tracefree]

Thank you, and thanks to everyone who helped! :D

[MJacred]

@akien-mga: I believe it makes sense to cherrypick this PR?

[allenwp]

Although the old Reinhard implementation was clearly incorrect, it wasn’t necessarily problematic for games that used it. After this fix, lighting or colour adjustments may need to be re-tweaked for some games, so this kind of fix shouldn’t come through in a minor release; in my opinion it should only be introduced in a major release, like 4.4.0, when users are expecting these sorts of changes.