-
-
Notifications
You must be signed in to change notification settings - Fork 35.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AgX Tone Mapping: Add Required Gamut Mapping #27413
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
const float AgxMaxEv = 4.026069; // log2(pow(2, LOG2_MAX) * MIDDLE_GRAY) | ||
// LOG2_MIN = -10.0 | ||
// LOG2_MAX = +6.5 | ||
// MIDDLE_GRAY = 0.18 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
10 stops below middle gray. These comments help clarify the formulas that follow.
// Gamut mapping. Simple clamp for now. | ||
color = clamp( color, 0.0, 1.0 ); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't done in our other color conversion functions (see P3 to Linear sRGB conversion). Is it something we should add there, as well?
That aside - is it important that the values be clamped to in-gamut after this conversion? It's very likely for the Linear sRGB values to come into this function with values far greater that 1.0 and some effects such as screen space bloom depend on those larger-than 1 values being output to the render target.
edit
I see now that AgX is clamped, as well. I guess I'm in part confused by this line in your original post:
This is required when mapping from the larger Rec.2020 gamut to the smaller Rec.709 gamut.
Is it important that we clamp that values after the color space conversion or after tone mapping? In tone mapping we seem to be clamping these values. In other color space conversions we are not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't done in our other color conversion functions (see P3 to Linear sRGB conversion). Is it something we should add there, as well?
I prefer to think of the color conversion functions themselves as open-domain-to-open-domain. But certainly any particular invocation of the functions needs to decide whether to clamp, gamut map, or keep the full domain.
... some effects such as screen space bloom depend on those larger-than 1 values being output to the render target
I'm trying hard to encourage users to put HDR effects like bloom before tone-mapping. See forum discussion. But yes, a lot of people have done this. My preference would mainly be consistency, whichever result that means. It looks like our existing tone mappers mostly do clamp before color space conversion.
The OETF which follows requires code values in [0, 1] currently. This PR must be merged with r160. |
Happy to make a .1 release with this once everyone is happy with the PR 👌 |
Can you please elaborate? I'm happy to have this PR merged based on consistency with the other tonemapping functions but I'd like to understand what you're suggesting and the reasoning for it. I assuming the OETF after tone mapping you're referring to is the color space conversion step that happens in the <colorspace_fragment> shader block (ie p3 to linear srgb, etc). Is that right? If that's the case there are plenty of other scenarios where colors are being converted without being clamped (like when tone mapping isn't used and in the Color class). Are you suggesting that this is incorrect, as well? |
If you are asking "why do we not clamp elsewhere", then that is a separate discussion unrelated to this PR. Certainly, when we map from a smaller gamut to a larger one, clamping is not required. When we map from a larger gamut to a smaller one, some form of gamut mapping is appropriate, unless the app accommodates out-of-gamut values. Clamping is the simplest form of gamut mapping. The output of tone mapping is a code value between 0 and the maximum code value the monitor supports. The AgX tone mapping algorithm -- and all of the current three.js tone mapping algorithms -- only support LDR monitors. Hence, the peak code value is 1. At least, that is my understanding. It is true, the monitor hardware likely clamps inputs to [0, 1] anyway, but the clamping here is just a stand-in for gamut mapping. |
Random question before it's too late... Would be okay to name the constant from After some searches I was unable to find the meaning of |
Not sure if that was directed at me. AgX is a pseudo-chemical term for the class of silver halides used in creative chemical film. Ag being silver, and the X as a holder for any of the other elements that are combined with it. It originated as an experiment in picture formation that focused on the purity dimension in relation to our visual cognition’s underlying mechanic to scise a picture and parse it into meaningful mental assemblies. |
Thank you for the explanations!
I originally named it "AGXToneMapping" but all the resources online referred to it as AgX tone mapping. I feel like for the sake of consistency with the rest of the ecosystem we should retain the capitalization as-is. |
This PR adds a gamut mapping step to the AgX tone mapping algorithm. This is required when mapping from the larger Rec.2020 gamut to the smaller Rec.709 gamut. The following image highlights out-of-gamut pixels, which are now avoided.
It remains to determine if the simple
clamp()
introduced in this PR is adequate for our purposes.