Skip to content
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

Commerce tone mapper #4495

Merged
merged 3 commits into from
Oct 5, 2023
Merged

Commerce tone mapper #4495

merged 3 commits into from
Oct 5, 2023

Conversation

elalish
Copy link
Collaborator

@elalish elalish commented Oct 4, 2023

This adds an experimental feature, as we're still soliciting feedback on the subtleties of the equation before it will become an official part of our API. The new feature is a tone mapping function designed for 3D Commerce applications as an alternative to our default, ACES. The Commerce tone mapper is designed to retain color accuracy as much as possible without producing noticeable artifacts in highlights (ACES and Linear/Clamped/No tone mapping both have poor color accuracy, though in different areas).

Note: the Commerce option has less contrast than ACES (intentionally), which may make it look "flat" in a side-by-side comparison with ACES. In general, higher contrast lighting should be used with Commerce tone mapping than with ACES to make up the difference. We may add a related option for our neutral lighting in the future, since the current default was designed for ACES.

This PR also adds an option for testing this in our Editor, as well as an additional model, Macbeth, that helps to show the color issues we're attempting to resolve. Macbeth includes unlit (and therefore un-tone-mapped) spheres with the same base color as the shiny and matte spheres to allow comparison.

ACES:
image
Commerce:
image

@elalish elalish self-assigned this Oct 4, 2023
@elalish elalish merged commit 4f10202 into master Oct 5, 2023
6 checks passed
@elalish elalish deleted the toneMapper branch October 5, 2023 16:58
@hybridherbst
Copy link
Contributor

I'm seeing a color shift on higher exposures; e.g. this model starts to become red-ish at exposure = 3 or higher:
(the editor only allows up to 2 right now)
van-veer-original-custom.zip

Exposure=1 Exposure=3 Exposure=10
image image image

Is this intended?

And an additional question: why is peak calculated with max(r, max(g,b)) instead of using a perceptual luminance value like float peak = dot(color, vec3(0.299, 0.587, 0.114)) or so?

@elalish
Copy link
Collaborator Author

elalish commented Oct 26, 2023

Thanks for the feedback! This appears intentional to me, since brown is also red-ish, just darker. With white light, PBR will only multiply your base color by a scalar factor and add some white - this tone mapper does the same: multiplies the color by a scalar factor and mixes toward white (desaturates for high brightness). Do you have any examples of what you would expect this color to look like when overexposed?

@elalish
Copy link
Collaborator Author

elalish commented Oct 26, 2023

As to perceptual luminance, I tried that route first and found it actually introduced a lot of problems. I finally realized it was because the purpose of a tone mapper is to fit into the sRGB unit cube, whose bounds do not have uniform perceptual luminance. That vector is useful when working in a luminance space, but I am not.

JL-Vidinoti pushed a commit to vidinoti/model-viewer that referenced this pull request Apr 22, 2024
* added commerce tonemapping

* added tone mapping selector to editor

* added Macbeth model to editor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants