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

Add AgX tone mapping #4615

Merged
merged 6 commits into from
Dec 29, 2023
Merged

Add AgX tone mapping #4615

merged 6 commits into from
Dec 29, 2023

Conversation

elalish
Copy link
Collaborator

@elalish elalish commented Dec 29, 2023

This adds AgX tone mapping as an option, mostly for comparison and because it's now easy. It has some great improvements over ACES, but for e-commerce it's still far too desaturated. I've also added it as an option in our editor, and made Commerce the editor's default, in preparation for it becoming MV's default in our next major release (which won't be this one). I also added an example to make comparison easy. FYI @donmccurdy @mrdoob.

@elalish elalish self-assigned this Dec 29, 2023
@donmccurdy
Copy link
Contributor

donmccurdy commented Dec 29, 2023

Two notes (feel free to integrate or ignore them).

ACES is a film industry standard that is widely used...

I think this might be unnecessarily generous to ACES Filmic. While various ACES-authored standards are widely used in the film industry for working with imagery, I do not know that the ACES Filmic Tone Mapping is one of them. How it became prevalent in realtime, I'm not sure, but would be curious to learn.

[AgX] has the same film-centric drawback of significant desaturation.

Note that introduction of Looks providing (among other things) contrast choices will be a high priority, and I consider it an essential part of supporting AgX. Not sure if Looks are factored into your evaluation?

@elalish
Copy link
Collaborator Author

elalish commented Dec 29, 2023

Thanks for the info! I'll try to clean up the language a bit. I don't know anything about Looks - what can they do? A tone mapper is already an arbitrary function, so if AgX encompasses another arbitrary function called a Look, then I'm not sure what AgX specifies. Any info you have would be appreciated.

@elalish
Copy link
Collaborator Author

elalish commented Dec 29, 2023

@donmccurdy In particular, if you know of any way to construct a "Look" that would make AgX similar to my Commerce tone mapper, that would be very interesting.

@elalish elalish merged commit 2a217a7 into master Dec 29, 2023
5 checks passed
@elalish elalish deleted the agX branch December 29, 2023 04:14
@donmccurdy
Copy link
Contributor

I'll borrow some phrasing from Romain

... a bigger problem I think is that the tone mapper is often used as a look. Instead the tone mapper should do as little as possible (compress the dynamic range, hopefully with knobs) but the look should come from color grading.

I think AgX and Commercial are alike in trying to do less, and that's a good thing.

Troy has described the default Look from AgX this way:

... the default is an attempt to pack high picture signal density, which means that folks will probably want to push the “contrast” ever so slightly; it’s impossible to go the other way.

In the case of AgX, Looks provide some simple color grading presets. I'm unsure, so far, what the pros/cons of having such presets are, compared to exposing a full color grading API.

As for constructing a Look with less desaturation in AgX, I suspect that the "Punchy" or higher contrast looks would move in that direction, but we haven't added them to three.js yet.

@elalish
Copy link
Collaborator Author

elalish commented Jan 8, 2024

Gotcha, so more of a semantic distinction. Is there a "standard" way to apply contrast, or is that curve another black art like tone mapping?

@donmccurdy
Copy link
Contributor

donmccurdy commented Jan 8, 2024

There exists, at least, an "easy" way:

https://github.com/mrdoob/three.js/blob/208f5e48adb750e15c4fb5167f492b414d8bd5bf/examples/jsm/shaders/BrightnessContrastShader.js#L46-L50

AgX does this a bit differently, see the approximation in google/filament#7236.

Whatever the color grading operation, the decision about when to apply it is important. From Real-time Rendering, 4th Ed:

When real-time applications first adopted color grading, the display-referred approach was predominant. However, the scene-referred approach has since been gaining traction due to its higher visual quality.

In other words, I believe it's better if color grading is applied within or before tone mapping, and not tacked on at the end.

With AgX, the Look is applied to a log encoding, before encoding to the output color space. I don't understand the practice of tone mapping and color grading design much beyond that, I'm afraid.

@elalish
Copy link
Collaborator Author

elalish commented Jan 8, 2024

Yeah, that's looking pretty black art 🥲. I wonder if applying the Punchy look would be a good default, or at least if you know your output is sRGB? Looks simple enough.

@donmccurdy
Copy link
Contributor

Would need criteria to define "a good default", I think. I'm inclined toward using 'base contrast', as Blender does, as the starting point for new projects. But perhaps knowing the default would be replacing ACES Filmic in existing projects could change the situation. And I'm less in touch with the e-commerce discussions.

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

* update tone map selector in editor

* updated MacbethBalls

* use commerce tone mapper in more examples

* address feedback
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.

2 participants