Skip to content

Add support for white parameter to AgX tonemapper#101559

Closed
allenwp wants to merge 1 commit into
godotengine:masterfrom
allenwp:agx-add-white-param
Closed

Add support for white parameter to AgX tonemapper#101559
allenwp wants to merge 1 commit into
godotengine:masterfrom
allenwp:agx-add-white-param

Conversation

@allenwp
Copy link
Copy Markdown
Contributor

@allenwp allenwp commented Jan 14, 2025

Not cherry-pickable to 4.3, as AgX is only in 4.4.

Usage

In addition to resolving issues with the Mobile renderer, the white parameter can be used to give higher contrast to AgX by decreasing both exposure and white. This approach is higher quality than using the Environment's contrast adjustment and less also less computationally expensive.

Forward+ with no white parameter (16 white) Forward+ with white parameter (adjusted exposure and white)
image image

Consistent rendering between Mobile and Forward+

Because the Mobile renderer only provides values to the tonemapper up to 2.0, you can achieve the same look with AgX between the Mobile and Forward+ renderers by setting the white paramter to 2.0 or less. This makes it easy to use AgX on a cross-platform game that must use the Mobile renderer on some platforms, but uses the Forward+ renderer on others.

TODO

☑ Fix issue that sometimes occurs when white <= 1.0 and Compatibility renderer. (seems to be a local build issue...)
☑ Optional: Limit editor's range of White parameter to prevent value of 0.0.

@allenwp allenwp requested review from a team as code owners January 14, 2025 21:27
@Calinou Calinou added this to the 4.x milestone Jan 14, 2025
@AThousandShips AThousandShips changed the title Added support for white parameter to AgX tonemapper Add support for white parameter to AgX tonemapper Jan 14, 2025
@allenwp allenwp force-pushed the agx-add-white-param branch from 9ac3e81 to 04ca7d3 Compare January 15, 2025 02:18
Copy link
Copy Markdown
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, there's an issue in Compatibility if White is exactly 1.0:

image

The whitepoint also acts strangely with values lower than 1.0. This issue doesn't occur in Forward+.

Regardless, I was trying to tweak values to get a less contrasted result that looks more like Forward+, but I didn't find much success.

In Forward+, this is what AgX looks like with a whitepoint of 16.0 in Forward+:

Screenshot 2025-01-15 173018

I've tried all tonemapping methods with various exposures/whitepoints and couldn't get a similar result in Compatibility. The closest was actually Linear with the exposure set to a value around 1.6, but it had all the typical issues of blown out highlights. It's possible that getting truly close to a Forward+-like appearance in Compatibility will require godotengine/godot-proposals#4731, but I expect this would make adjustments a lot slower on low-end devices.

PS: We should probably prevent whitepoint values of exactly 0.0 in the inspector, as such as value is invalid for a whitepoint and generally leads to strange results with all tonemappers (pure black image or artifacts all over the image). Limiting the lowest value to 0.01 would avoid this - I've tested that value with all tonemappers and no rendering issues occur.

Testing project: test-tonemap-hdri.zip

@allenwp
Copy link
Copy Markdown
Contributor Author

allenwp commented Jan 15, 2025

The whitepoint also acts strangely with values lower than 1.0. This issue doesn't occur in Forward+.

Values lower than 1.0 will result in negative white parameters being passed to the shader. A value of exactly 1.0 will result in a value of 0.0 being passed to the shader. Seems I’ll need to do some digging into why this might sometimes be a problem with Compatibility.

Originally I had the log2 calculation happening in the shader and I thought it worked fine for all ranges of white in Compatibility, so maybe the problematic behaviour is specific to passing the zero/negative values as a parameter to the shader.

@allenwp
Copy link
Copy Markdown
Contributor Author

allenwp commented Jan 15, 2025

Tested locally, there's an issue in Compatibility if White is exactly 1.0:

Strange, I can't reproduce using the latest build artifact from this PR:

image

Godot v4.4.beta (a934213ba) - Windows 10 (build 19045) - Multi-window, 1 monitor - OpenGL 3 (Compatibility) - NVIDIA GeForce GTX 980 Ti (NVIDIA; 31.0.15.4665) - Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz (8 threads)

(I also don't know what you mean by strange behaviour with white parameter below zero, so maybe a screenshot would help...)

@Calinou
Copy link
Copy Markdown
Member

Calinou commented Jan 15, 2025

(I also don't know what you mean by strange behaviour with white parameter below zero, so maybe a screenshot would help...)

I meant when it's equal to 0.0 (you can't set it below 0.0 with the inspector).

@allenwp
Copy link
Copy Markdown
Contributor Author

allenwp commented Jan 15, 2025

(I also don't know what you mean by strange behaviour with white parameter below zero, so maybe a screenshot would help...)

I meant when it's equal to 0.0 (you can't set it below 0.0 with the inspector).

Whoops! I wrote that incorrectly — I’m sorry! Let me try again:

The whitepoint also acts strangely with values lower than 1.0. This issue doesn't occur in Forward+.

I don’t know what you mean by acting strangely with values lower than 1.0. Would you be able to post a screenshot of this behaviour?

@allenwp
Copy link
Copy Markdown
Contributor Author

allenwp commented Jan 16, 2025

Tested locally, there's an issue in Compatibility if White is exactly 1.0:
...
The whitepoint also acts strangely with values lower than 1.0. This issue doesn't occur in Forward+.

Ah-ha! I have been able to reproduce this on my end. Thanks, @Calinou -- I'll dig into this issue and see if I can work around it.

@allenwp allenwp force-pushed the agx-add-white-param branch from 04ca7d3 to 95854d2 Compare January 16, 2025 17:42
@allenwp
Copy link
Copy Markdown
Contributor Author

allenwp commented Jan 16, 2025

Tested locally, there's an issue in Compatibility if White is exactly 1.0:
...
The whitepoint also acts strangely with values lower than 1.0. This issue doesn't occur in Forward+.

Ah-ha! I have been able to reproduce this on my end. Thanks, @Calinou -- I'll dig into this issue and see if I can work around it.

On my end, this issue appears to be a build-related issue that has nothing to do with this PR. Would you mind deleting all .gen.h files from drivers\gles3 and its subfolders and rebuilding, then let me know if that fixes the problem for you? Additionally, the build artifact from this PR should also work, as it doesn't suffer from this build issue....

@allenwp allenwp force-pushed the agx-add-white-param branch from 95854d2 to d411ccf Compare January 16, 2025 19:40
@Calinou
Copy link
Copy Markdown
Member

Calinou commented Jan 16, 2025

On my end, this issue appears to be a build-related issue that has nothing to do with this PR. Would you mind deleting all .gen.h files from drivers\gles3 and its subfolders and rebuilding, then let me know if that fixes the problem for you? Additionally, the build artifact from this PR should also work, as it doesn't suffer from this build issue....

I've done a clean build and can't reproduce the issue.

@allenwp
Copy link
Copy Markdown
Contributor Author

allenwp commented Jan 17, 2025

I've done a clean build and can't reproduce the issue.

Thanks for the review!

Unfortunately, this approach does have some downsides. The sigmoid contrast curve seems to be designed with a white value of ~16.29. A lower white value introduces not only higher contrast, but also a different relative scaling of channels, which means hue shift can pop up in some extreme cases like this one:

image

(Note the lower white value produces a larger hue shift towards blue as the color approaches black.)

It's possible that the matrices were also designed with a specific white value in mind as well, but I don't yet understand that part of AgX, so I can't comment on it yet.

For this reason, I don't think this PR is a perfect "silver bullet" solution... Let me know if anyone has other ideas, but otherwise this might be the best way to deal with the limited dynamic range issue that the mobile renderer experiences.

@allenwp allenwp force-pushed the agx-add-white-param branch from d411ccf to 31c2d49 Compare January 20, 2025 17:18
Fixes godotengine#101558
Also limits white parameter from being set lower than 0.01 in the editor to prevent log2(0.0).
@allenwp
Copy link
Copy Markdown
Contributor Author

allenwp commented Jan 20, 2025

Rebased to include #101515.

@allenwp allenwp marked this pull request as draft January 22, 2025 13:27
@allenwp
Copy link
Copy Markdown
Contributor Author

allenwp commented Jan 22, 2025

I’m looking more into the sigmoid contrast curve right now and how it might adjust in relation to different max exposures. While I look into this, I’m marking this PR as a draft.

@allenwp
Copy link
Copy Markdown
Contributor Author

allenwp commented Feb 4, 2025

AgX is designed to have a middle grey value of 0.18. This means that the film-like contrast curve uses a linear input value of 0.18 as its inflection point: a 0.18 input will always result in 0.18 output, regardless of the white/min/max exposure settings of the tonemapper. This piece of code is the critical line that ensures that the x_pivot (inflection point of the curve) is always at a value of middle grey (0.18). This same line exists in Troy's original AgX configuration.

The 0.18 middle grey was likely chosen because this is an ACES standard that is used in a lot of HDR processing. Here are some links that help demonstrate the 0.18 middle grey value in AgX was not an arbitrary choice:

https://github.com/imageworks/OpenColorIO-Configs/blob/master/aces_1.0.3/python/aces_ocio/colorspaces/aces.py (0.18 used as a middle grey)
https://community.acescentral.com/t/is-mid-gray-nit-setting-in-st2084-based-on-0-18-in-scene-linear-not-18-mid-gray/3531
https://community.acescentral.com/t/ue5-ssta-input-unit/5245
https://github.com/colour-science/aces-dev/blob/86284e2f145a89e3612f05ec7ea5a3e9d92cc779/documents/LaTeX/S-2014-003/appendixC.tex#L28

This PR incorrectly adjusts the middle grey value as the white parameter is changed, which results in lower white values becoming very bright:

16.0 white 2.0 white
image image

Closing as superseded by #102425, which correctly implements white parameter support.

@allenwp allenwp closed this Feb 4, 2025
@AThousandShips AThousandShips removed this from the 4.x milestone Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AgX has low dynamic range output with Mobile renderer

3 participants