-
-
Notifications
You must be signed in to change notification settings - Fork 35.6k
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
MeshNormalNodeMaterial: Convert packed normal to working color space #30590
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
diffuseColor.assign( vec4( directionToColor( transformedNormalView ), opacityNode ) ); | ||
// By convention, a normal packed to RGB is in sRGB color space. Convert it to working color space. | ||
|
||
diffuseColor.assign( toWorkingColorSpace( vec4( directionToColor( transformedNormalView ), opacityNode ), SRGBColorSpace ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind removing the SRGBColorSpace
from that line. toWorkingColorSpace()
has just one parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops... but can you explain how it converts to working color space without knowing what color space to convert from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function assumes the given color node is in output color space.
There is an alternative TSL function that is more flexible in that regard:
three.js/src/nodes/display/ColorSpaceNode.js
Lines 160 to 169 in 3bb6e70
/** | |
* TSL function for converting a given color node from the given color space to the current working color space. | |
* | |
* @tsl | |
* @function | |
* @param {Node} node - Represents the node to convert. | |
* @param {string} colorSpace - The source color space. | |
* @returns {ColorSpaceNode} | |
*/ | |
export const colorSpaceToWorking = ( node, colorSpace ) => nodeObject( new ColorSpaceNode( nodeObject( node ), colorSpace, WORKING_COLOR_SPACE ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function (
toWorkingColorSpace
) assumes the given color node is in output color space.
Then it should be called toWorkingColorSpaceFromOutputColorSpace
.
Is "output color space" the renderer's output color space?... I guess that is true...
Then the inverse method should be toOutputColorSpaceFromWorkingColorSpace
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are fine, since the source and destination are clear -- although I prefer using the more specific terms: targetColorSpace
and sourceColorSpace
.
three.js/src/nodes/display/ColorSpaceNode.js
Line 158 in 972f726
export const workingToColorSpace = ( node, colorSpace ) => nodeObject( new ColorSpaceNode( nodeObject( node ), WORKING_COLOR_SPACE, colorSpace ) ); |
three.js/src/nodes/display/ColorSpaceNode.js
Line 169 in 972f726
export const colorSpaceToWorking = ( node, colorSpace ) => nodeObject( new ColorSpaceNode( nodeObject( node ), colorSpace, WORKING_COLOR_SPACE ) ); |
//
These are also fine, for the same reason -- although I would rename them. and adhere to the same nomenclature as above: workingToColorSpace( color, targetColorSpace )
and colorSpaceToWorking( color, sourceColorSpace )
.
three.js/src/math/ColorManagement.js
Lines 79 to 89 in 972f726
fromWorkingColorSpace: function ( color, targetColorSpace ) { | |
return this.convert( color, this.workingColorSpace, targetColorSpace ); | |
}, | |
toWorkingColorSpace: function ( color, sourceColorSpace ) { | |
return this.convert( color, sourceColorSpace, this.workingColorSpace ); | |
}, |
//
But I think these should be removed, as is it not clear from the name what they do, and it is not clear to me why they are needed.
three.js/src/nodes/display/ColorSpaceNode.js
Line 137 in 972f726
export const toOutputColorSpace = ( node ) => nodeObject( new ColorSpaceNode( nodeObject( node ), WORKING_COLOR_SPACE, OUTPUT_COLOR_SPACE ) ); |
three.js/src/nodes/display/ColorSpaceNode.js
Line 147 in 972f726
export const toWorkingColorSpace = ( node ) => nodeObject( new ColorSpaceNode( nodeObject( node ), OUTPUT_COLOR_SPACE, WORKING_COLOR_SPACE ) ); |
/ping @gkjohnson
It seems a few examples using |
I have been unable to successfully update screenshots for some time now, unfortunately. |
Fixes #30575.
Also #28831 (comment).
By convention, the packed normal, when interpreted as a color, is assumed to be in sRGB color space. It must be converted to linear working color space.