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

ColorAdjustmentNode: Correct lumaCoeffs. #28861

Merged
merged 1 commit into from
Jul 13, 2024
Merged

ColorAdjustmentNode: Correct lumaCoeffs. #28861

merged 1 commit into from
Jul 13, 2024

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Jul 13, 2024

Related issue: -

Description

This PR corrects the luma coefficients to the values previously used in WebGLRenderer.

Copy link

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
683.5 kB (169.2 kB) 683.5 kB (169.2 kB) +0 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) 460.7 kB (111.1 kB) +0 B

@@ -86,7 +86,7 @@ export const saturation = nodeProxy( ColorAdjustmentNode, ColorAdjustmentNode.SA
export const vibrance = nodeProxy( ColorAdjustmentNode, ColorAdjustmentNode.VIBRANCE );
export const hue = nodeProxy( ColorAdjustmentNode, ColorAdjustmentNode.HUE );

export const lumaCoeffs = vec3( 0.2125, 0.7154, 0.0721 );
export const lumaCoeffs = vec3( 0.2126729, 0.7151522, 0.0721750 ); // // assumes rgb is in linear color space with sRGB primaries and D65 white point
Copy link
Collaborator Author

@Mugen87 Mugen87 Jul 13, 2024

Choose a reason for hiding this comment

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

@WestLangley @donmccurdy Looking at https://en.wikipedia.org/wiki/Rec._709#Luma_coefficients, it seems 0.2125, 0.7154, 0.0721 are actually the more recent luma coefficients.

Should we update the formula in common.js/ WebGLRenderer instead?

Copy link
Collaborator

@WestLangley WestLangley Jul 13, 2024

Choose a reason for hiding this comment

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

I don't think Luma is the correct term. Luma is computed from components in sRGB color space -- not linear sRGB color space. I'd suggest renaming to LuminanceCoeffs -- if you want to retain that constant. /ping @Mugen87

https://en.wikipedia.org/wiki/Relative_luminance

Copy link
Collaborator

@donmccurdy donmccurdy Jul 13, 2024

Choose a reason for hiding this comment

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

I think it's more about consistency than correctness for our purposes – I've seen the 4-digit coefficients 0.2126, 0.7152, 0.0722 most often, that's closer to what we've historically used, Blender uses them, and that's also what the ASC CDL requires... I would be inclined not to update to the newer BT.709-1 coefficients until we observe other software relevant to our users doing the same.

Ideally (not necessarily in this PR, I could look at this later) the coefficients should be color space dependent, and could throw an error for anything but Linear-sRGB and sRGB.

const luminanceCoeffs = ColorManagement.getLuminanceCoefficients(
  ColorManagement.workingColorspace
);

My understanding would be that the coefficients are applicable to both sRGB and Linear-sRGB components, but produce different results: luma (for sRGB components) and relative luminance (for Linear-sRGB components)

Copy link
Collaborator Author

@Mugen87 Mugen87 Jul 13, 2024

Choose a reason for hiding this comment

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

I would be inclined not to update to the newer BT.709-1 coefficients until we observe other software relevant to our users doing the same.

Okay, in this case I merge so we are consistent with WebGLRenderer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

the coefficients should be color space dependent

Yes, that is why I included the comment in common.glsl.js:

float luminance( const in vec3 rgb ) {

	// assumes rgb is in linear color space with sRGB primaries and D65 white point

	const vec3 weights = vec3( 0.2126729, 0.7151522, 0.0721750 );

	return dot( weights, rgb );

}

I could look at this later

Great! These coefficients are simply the 2nd row in the conversion matrix from Linear-sRGB to XYZ color space, assuming a D65 white point.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

BTW: There is a shader which is in used by the unreal bloom pass which relies on even different values:

vec3 luma = vec3( 0.299, 0.587, 0.114 );
float v = dot( texel.xyz, luma );

These seems to be Rec. 601 luma luma coefficients. Would it make sense to change the code to the following so 709 is used? Related https://en.wikipedia.org/wiki/Luma_(video)#Rec._601_luma_versus_Rec._709_luma_coefficients

const v = luminance( texel.xyz );

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Mugen87 Mugen87 merged commit b999622 into mrdoob:dev Jul 13, 2024
12 checks passed
@Mugen87 Mugen87 added this to the r167 milestone Jul 21, 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