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

ColorManagement: Add .getLuminanceCoefficients #28880

Merged

Conversation

donmccurdy
Copy link
Collaborator

@donmccurdy donmccurdy commented Jul 16, 2024

Follow-up to #28861. Adds a method to ColorManagement, to produce luminance coefficients for a given color space. Currently supports sRGB, Linear sRGB, Display P3, and Linear Display P3. For sRGB and Linear sRGB I used the (more common?) 4-decimal precision, 0.2126, 0.7152, 0.0722, simply or consistency with OCIO, Blender, and ASC specifications. I expect the difference will be ~zero, but if we see E2E failures as a result, I can switch back. I believe it's fair to call these either "luma coefficients" or "luminance coefficients", I don't have a preference other than internal consistency.

Certain post-processing effects were using luminance coefficients from Rec. 601, which we do not otherwise support. I've corrected those coefficients.

One notable exception on ColorManagement's use, common.glsl.js contains hard-coded luminance coefficients. Ideally they'd be based on the working color space, but I'm unsure if it's a good idea to put a new global uniform into this shader chunk. Suggestions welcome. In terms of working toward support for wide-gamut PBR rendering, it might be more practical to focus on WebGPURenderer for now. Fixed. ✅

Finally, note that while the luminance coefficients are the same for Linear sRGB vs. sRGB, and for Linear Display P3 vs. Display P3, the dot product gives different results with different meanings given linear vs. non-linear components.

/cc @Mugen87 @WestLangley

Copy link

github-actions bot commented Jul 16, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
683.5 kB (169.2 kB) 683.9 kB (169.3 kB) +397 B

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
460.7 kB (111.1 kB) 461.1 kB (111.2 kB) +397 B

@WestLangley
Copy link
Collaborator

I believe it's fair to call these either "luma coefficients" or "luminance coefficients", I don't have a preference other than internal consistency.

Can we please use luninanceCoefficients and stay away from luma? luninanceCoeffs is OK, too, if you want.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 16, 2024

Ideally they'd be based on the working color space, but I'm unsure if it's a good idea to put a new global uniform into this shader chunk. Suggestions welcome.

I would say yes so ideally components like LuminosityHighPassShader could directly use the luminance() function from commons.glsl. That should make the code more readable.

When thinking about it, we can have a solution without a uniform at all if you update WebGLProgram instead. You can embed the coefficients directly in the fragment shader similar to the tone mapping and color space logic. Use getTexelEncodingFunction() as a reference.

@donmccurdy
Copy link
Collaborator Author

Can we please use luninanceCoefficients and stay away from luma? luninanceCoeffs is OK, too, if you want.

Ok, works for me. 👍🏻

You can embed the coefficients directly in the fragment shader similar to the tone mapping and color space logic.

Will look into this – thanks!

Moving this PR to 'draft' as I address feedback.

@donmccurdy donmccurdy marked this pull request as draft July 16, 2024 15:45
@donmccurdy donmccurdy marked this pull request as ready for review July 17, 2024 02:05
@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Jul 17, 2024

I believe all feedback has been addressed, moving the luminance function to WebGLProgram works well. ✅

@donmccurdy donmccurdy merged commit 4727942 into mrdoob:dev Jul 17, 2024
12 checks passed
@donmccurdy donmccurdy deleted the feat-colormanagement-getLuminanceCoefficients branch July 17, 2024 14:02
vanruesc added a commit to pmndrs/postprocessing that referenced this pull request Jul 28, 2024
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.

3 participants