Reinhard Tonemapping uses an incorrect formula, leading to too bright images

Tested versions

Reproducible in: Every version of Godot released after October 2018

System information

Godot v4.2.2.stable unknown - Arch Linux #1 ZEN SMP PREEMPT_DYNAMIC Wed, 12 Jun 2024 20:16:55 +0000 - X11 - Vulkan (Forward+) - dedicated NVIDIA GeForce GTX 1080 Ti (nvidia; 550.90.07) - AMD Ryzen 5 2600X Six-Core Processor (12 Threads)

Issue description

Godot's Reinhard tonemapping mode does not follow any of the formulas given in Reinhard's original 2002 paper: https://www-old.cs.utah.edu/docs/techreports/2002/pdf/UUCS-02-001.pdf

There are two valid formulas, commonly known as the Reinhard formula (equation 3 in the paper)

color / (color + 1)

and the "modified" or "extended" Reinhard formula which includes a white point (equation 4 in the paper):

color * (1 + color/(white * white)) / (color + 1)

Godot's implementation can be found here. It is given by

(white * color + color) / (color * white + white)

As has been uncovered by @allenwp on Rocket Chat in this thread, this was most likely intended to be a mathematically equivalent simplification of Reinhard's modified formula. However, there are three problems with it that probably snuck their way in during several refactors of the code.

Missing square on the `white` parameter

The first one I noticed is that the white parameter is not being squared. white is supposed to be the (float) value of the luminance above which color values should be mapped to white. (See e.g. this blog post by @64 for an explanation of Reinhard tonemapping and the role of the white point). Since Godot does not apply the squaring it loses its meaning and instead becomes an arbitrary parameter influencing the brightness of the image, making it harder to produce the same look across different pieces of software with the same parameters.

Incorrect simplification of the formula

The second one is the main problem. Ignoring the non-squared white parameter for a moment, a correct simplification of equation 4 in the paper should look like this:

(white * color + color * color) / (color * white + white)

Notice that second term in the numerator is squared, unlike in Godot's implementation.

Not applied to luminance

This one is not actually a problem per se but a matter of taste. In Reinhard's paper, the tonemapping operator is applied only to the luminance of the pixel (again see here for an explanation), whereas in Godot it's applied to each color channel individually. There is a valid reason to do this, which is to avoid hue shifting due to clamping of individual color channels (see the paragraph "Luminance only" towards the bottom of this blog post by @johnhable for a more detailed explanation. It is talking about Filmic tonemapping but the argument should hold for Reinhard as well). It can also be argued that by not operating on luminance this method leads to even stronger hue shifting in other scenarios however. Which version you prefer is a matter of artistic choice, so I don't consider this a bug. I just mention it here since solving the main issue will break compatibility anyway, so if there is any argument to be made to switch to the luminance-only model now is the last chance to do so. But I think it would be better to simply offer both options (like bevy does for example) - if there is any desire to go in this direction I'll open a feature proposal.

Wrong documentation

Finally, not a problem with the formula itself but with its documentation: It claims to apply the unmodified Reinhard formula to the image, which is not true.

Conclusion

@allenwp put together an interactive comparison of the tonemapping curves from Reinhard's paper and the different versions that have existed in Godot over the years which you can check out here: https://www.desmos.com/calculator/fbjhifxcpm. Notice how the current implementation matches none of Reinhard's formulas and leads to brighter images.

Fixing this issue will break compatibility with existing projects that have fine-tuned values for the Reinhard tonemapper. It is not my place to judge if it is worth doing so, but I've been told that it is Godot's priority to match other software when it comes to tonemapping to make it easier for artists to create consistent looking assets. I will submit a PR with the fix soon (not including a switch to the luminance only model, unless there is consensus that Godot should follow it).

It is also worth mentioning that the correct modified Reinhard formula with the default white parameter of 1.0 is identical to linear tonemapping with extra steps, which I will try to make clear in the updated documentation. (According to Reinhard the white point should be set to the maximum luminance of any pixel on the screen to preserve all details, but automating this would require a new buffer storing this value in addition to the average screen luminance which I think is outside the scope of this bug).

EDIT: Fixed a typo in the modified Reinhard formula

Steps to reproduce

Open a scene with colors in HDR, add a WorldEnvironment node, add an Environment, and select "Reinhard" mode under Tonemap.

Minimal reproduction project (MRP)

N/A

Great work! This is a really excellent investigation into the history of our "Reinhard" tonemapper implementation.

It sounds like there may be value in standardizing on equation 4 so Godot behaves more similarly to other applications. But before making that change we need to evaluate:

What other applications are doing (are they all using equation 4?). I've certainly seen equation 3 out in the wild as often as equation 4, but we should check with other applications that users will more commonly compare Godot to (Blender, 3DS Max, Substance, Unreal, Unity, ThreeJS)
The actual visual difference in realistic scenes (to ensure we aren't going to break compatibility too severely).

Note, I don't think compatibility is a huge issue as I suspect very few people are using the reinhard tonemapping when the other options pretty much look better in every scenario. Accordingly, matching other applications is likely more valuable.

It would also be interesting to find out where our version came from. It was likely adapted from another open source project. In the past I have had good luck copying old code like that (from when it was first added to Godot) and searching in Github to find other projects using the same code.

GLES 2 used "equation 4" from the original Reinhard paper verbatim, including basing off of luminance instead of per-channel.

It's pretty obvious that GLES 3 also intended to use "equation 4" from the original Reinhard paper, but applying the curve on each colour channel independently instead of mapping to luminance. Unfortunately, there was a typo and the white parameter was not squared. This is the root issue at hand here.

From a solely historical perspective, the correct fix to this issue would be to go back to the original GLES 3 formula, but correctly square the white parameter. This is the fix that should have been implemented in #21436 back in 2018, but was not, likely because the easy-to-find typo had been obscured by a previous mathematical simplification.

I think the GLES 3 change to apply Reinhard to each colour channel independently was a good change for reasons described in John Hable's blog post that is linked in the original issue. This blog post, along with the historical change made in GLES 3, suggest that it is a good idea to keep it being applied to individual colour channels, rather than changing it back to the GLES 2 way of applying to luminance.

color / (color + 1) will result in a drastically different image than the current incorrect implementation in most cases. I believe this would be most similar to setting the white parameter to infinity, which is likely not how most existing projects are configured. (If any projects use it, which as mentioned, might be zero.)

@clayjohn I checked Blender's implementation and it uses equation three, i.e. the unmodified Reinhard formula. Perhaps we could add a toggle to the tonemapper settings, something like "enable manual white point"? Then users can choose between equation 4 and 3 (which is effectively the same as 4 with white going to infinity).

Also, is there a standard set of Godot scenes that are commonly used to compare effects?

@allenwp

GLES 2 used "equation 4" from the original Reinhard paper verbatim, including basing off of luminance instead of per-channel.

I must be missing something, in your link, GLES2 and GLES3 appear to use the same formula (except GLES3 uses per-channel instead of luminance. In both cases white is not squared.

GLES2:

lp = ( lp * ( 1.0 + ( lp / ( white) ) ) ) / ( 1.0 + lp );
color.rgb*=lp;

GLES3:

color.rgb = ( color.rgb * ( 1.0 + ( color.rgb / ( white) ) ) ) / ( 1.0 + color.rgb );

I checked Blender's implementation and it uses equation three, i.e. the unmodified Reinhard formula. Perhaps we could add a toggle to the tonemapper settings, something like "enable manual white point"? Then users can choose between equation 4 and 3 (which is effectively the same as 4 with white going to infinity).

Hmmm, I don't think that is a good idea. We need to be really careful about exposing new parameters that users will never use. Its really easy to suffer from the "death by 1000 cuts".

What I meant above is we should look at multiple other pieces of software and see if there is a common standard. We could consider changing if, for example, everyone used either equation 3 or equation 4.

Also, is there a standard set of Godot scenes that are commonly used to compare effects?

No. Most people just use the TPS demo.

I must be missing something, in your link, GLES2 and GLES3 appear to use the same formula (except GLES3 uses per-channel instead of luminance. In both cases white is not squared.

GLES2:
lp = ( lp * ( 1.0 + ( lp / ( white) ) ) ) / ( 1.0 + lp );
color.rgb*=lp;
GLES3:
color.rgb = ( color.rgb * ( 1.0 + ( color.rgb / ( white) ) ) ) / ( 1.0 + color.rgb );

My mistake @clayjohn! Thanks for correcting me. I misread the USE_AUTOWHITE part which multiplies white by white_mult to be squaring white, but clearly that's not the case.

godotengine / godot