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

WebGLRenderer: Add 'AgXPunchyToneMapping' #27618

Closed
wants to merge 3 commits into from

Conversation

donmccurdy
Copy link
Collaborator

@donmccurdy donmccurdy commented Jan 25, 2024

Related issue:

Description

Adds THREE.AgXPunchyToneMapping, as found in Blender and Filament. A more contrasty and saturated look, intended for sRGB displays. Intended to provide easier workflows to/from Blender, while retaining a more similar look to what users are currently getting from ACES Filmic.

Comparisons

From left to right: linear • agx • agx-punchy • aces-filmic.

Caution

In the screenshots below, the third column ("agx-punchy") is out of date based on comments in #27618 (comment). I'll update the screenshots soon, and would expect a boost in saturation for the entire column.

game_fps
keyframes
anisotropy
compressed
lights
sheen
ldraw
ground_envmap

Copy link

github-actions bot commented Jan 25, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
673.5 kB (167.2 kB) 674.4 kB (167.4 kB) +891 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev Filesize PR Diff
454.2 kB (110.1 kB) 455.1 kB (110.4 kB) +869 B

@donmccurdy donmccurdy requested a review from gkjohnson January 25, 2024 01:34
// on Blender's implementation using rec 2020 primaries
// https://github.com/google/filament/pull/7236
// Inputs and outputs are encoded as Linear-sRGB.
vec3 agxLook( vec3 color, uint look ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function appears to be missing chunks of code... Was that done on purpose?

(I am not referring to the "golden" chunk.)

Copy link
Collaborator Author

@donmccurdy donmccurdy Jan 25, 2024

Choose a reason for hiding this comment

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

Hm – you're correct, and it was not intended. I've added the missing factor, and the result is a significant increase in saturation.

Before

keyframes

After

keyframes_sat

I'll update the remaining screenshots tomorrow, if there is no other feedback to address before then.

Comment on lines +128 to +139
vec3 offset = vec3( 0.0 );
vec3 slope = vec3( 1.0 );
vec3 power = vec3( 1.0 );
float sat = 1.0;

if ( look == AGX_LOOK_PUNCHY ) {

slope = vec3( 1.0 );
power = vec3( 1.35, 1.35, 1.35 );
sat = 1.4;

}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be simplified -- or are you leaving this for future look options?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was leaving space for future looks, but I don't particularly feel that we need to ship the "Golden" look out of the box, and I'm not sure if the contrast-oriented looks are implemented in the same way. So, I'd be happy to simplify this if you prefer!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Up to you. :-)


}

const vec3 lw = vec3( 0.2126, 0.7152, 0.0722 );
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know this was copied from elsewhere, but these are not the correct luminance coefficients for BT.2020 primaries. I guess this can be addressed in a separate PR.

Copy link
Collaborator Author

@donmccurdy donmccurdy Jan 25, 2024

Choose a reason for hiding this comment

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

I don't mind taking the time on this PR. I believe we're working in a log space at this point, between the inset and outset matrices. My mental model, illustrated as (hopefully-intelligible?) pseudocode:

outdated
LINEAR_SRGB {
  # (1) apply shading

  LINEAR_REC2020 {
    AGX_LOG {
      # (2) apply sigmoid and look
    }
  }
  
  SRGB {
    # (3) output to drawing buffer
  }
}

This comment was marked as outdated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Who knows, but I would like to think the color space remains Rec.2020.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm... I hadn't yet noticed that these are BT.709 coefficients. Indeed, I don't understand why we'd be using BT.709 coefficients here, rather than BT.2020 coefficients (adapted to the log encoding or not).

These can be found in the iodite "minimal agx" blog post, on which Godot's and Filament's implementations are also based. I'm not comfortable enough reading Blender's implementation, or Troy's original, to know how OpenColorIO implements this step.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe these would be the luminance coefficients for Rec.2020.

0.2626983  0.6780088  0.0592929

Common sense tells me these would be the appropriate coefficients.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Before and after:

bt2020_luma_coeffs

I can't really tell the difference here, but I am confused, and I suspect there should be some accommodation for the log encoding. I've left a comment on the blog post asking about it.

Copy link
Collaborator Author

@donmccurdy donmccurdy Feb 5, 2024

Choose a reason for hiding this comment

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

@WestLangley I received a response to my question about the luminance coefficients on the Minimal AgX blog past, and did some further reading. The coefficients come from ASC CDL v1.2, which is a standardized representation of color grading looks. I haven't managed to find the actual text of ASC CDL v1.2, but the luminance coefficients are constant and based on Rec. 709 regardless of the working color space. This is not AgX-specific.

While very possibly more correct, the practical benefits of matching the coefficients to the working color space here are unclear to me. I do believe the log space encoding means we are not strictly dealing with Rec. 2020 here, either. The benefit of using Rec. 709 coefficients is the ability to import looks from other tools. I'm inclined to leave a comment about ASC-CDL v1.2 but to keep the coefficients of Rec. 709.

@hybridherbst
Copy link
Contributor

hybridherbst commented Feb 2, 2024

Could we, instead of shipping a specific AgX look, make the parameters adjustable, as they are intended to be according to Troy? Neither the "punchy" nor the "default" look satisfy all applications, and AgX is designed so that there are only a few, artist-adjustable, parameters exposed to control the look (offset, slope, power, saturation).

Instead of adding more enums for various looks I think exposing the parameters and then having e.g. the presets from Blender, or the "punchy" and "golden" examples, as available constants to feed into those parameters would be much more effective and more in line to how AgX is intended to be used.

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Feb 5, 2024

@hybridherbst The ASC CDL v1.2 definition of color grading transforms with [offset, slope, power, saturation] is new to me. Personally I don't know how to use those parameters (except saturation) other than copy/pasting them from existing code. Have you used these parameters before, and do you feel they are artist-friendly?

It's encouraging that the CDL parameters are not AgX-specific. If we could support the same color grading API across multiple compatible tone mappers, that would be great. Perhaps one of...

// (a)
renderer.setToneMappingCDL( slope, offset, power, saturation );

// (b)
renderer.setColorGradingCDL( slope, offset, power, saturation );

... and then we don't need to include "looks".

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Feb 6, 2024

That said ... a color grading API will often include more parameters than just these. See Filament's ColorGrading API for example. Such an API would involve either loading a LUT, or generating a LUT at runtime. I'm not sure we want to put all of this API surface into WebGLRenderer.

Maybe a compromise would be to include just the CDL parameters on WebGLRenderer, along with exposure. And then if we need a more advanced API, that can be added to the node system, or a post-processing pass to be applied before THREE.OutputPass.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 6, 2024

I'm not sure we want to put all of this API surface into WebGLRenderer.

WebGPURenderer and the node material makes it possible to implement more advanced tone mapping/color grading without introducing new methods on renderer level. E.g. ToneMappingNode could be enhanced with a new gradientNode property that allows definitions like shown in #27618 (comment).

I think it's okay if more advanced/specific features are only exclusively supported in WebGPURenderer.

@hybridherbst
Copy link
Contributor

I just think introducing AgX examples purely meant for learning, such as "Punchy" or "Golden", as tonemapper options in three.js doesn't make much sense – they're not "presets set in stone", they are provided for understanding.

Slope, offset, power, saturation are similar to the high-level parameters that are available in grading tools (Premiere, Davinci etc) – personally I find them intuitive.

I would very much welcome having that control for AgX outside of postprocessing passes, because these are not viable on mobile/VR. I think more complex LUT tonemapping methods can either be implemented via CustomTonemapping or as pass.

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Feb 6, 2024

Ok, let's try it — I believe it will be straightforward to support these custom looks (based on CDL parameters) in both AgX and ACES Filmic. Both tonemappers are designed to go through a log space where a custom look may be applied.

Our other tonemappers do not have that design feature, to my understanding. We could apply the color grading before tonemapping, but the results will be different. Or perhaps we only enable the CDL parameters for tonemappers designed to support looks. It may take some experimenting to decide.

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.

4 participants