Skip to content

MeshNormalNodeMaterial: Convert packed normal to working color space #30590

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Feb 24, 2025
Merged

MeshNormalNodeMaterial: Convert packed normal to working color space #30590

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fb8e248
Convert packed normal to working color space
WestLangley Feb 23, 2025
bdd802a
Convert packed normal to working color space
WestLangley Feb 23, 2025
90bd55c
Clean up
WestLangley Feb 23, 2025
9ba526c
Update screenshots
WestLangley Feb 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/materials/nodes/MeshNormalNodeMaterial.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@
import { directionToColor } from '../../nodes/utils/Packing.js';
import { materialOpacity } from '../../nodes/accessors/MaterialNode.js';
import { transformedNormalView } from '../../nodes/accessors/Normal.js';
import { float, vec4 } from '../../nodes/tsl/TSLBase.js';
import { float, vec4, toWorkingColorSpace } from '../../nodes/tsl/TSLBase.js';
import { SRGBColorSpace } from '../../constants.js';

import { MeshNormalMaterial } from '../MeshNormalMaterial.js';


const _defaultValues = /*@__PURE__*/ new MeshNormalMaterial();

/**
Expand Down Expand Up @@ -54,7 +56,9 @@

const opacityNode = this.opacityNode ? float( this.opacityNode ) : materialOpacity;

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 ) );
Copy link
Collaborator

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.

Copy link
Collaborator Author

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?

Copy link
Collaborator

@Mugen87 Mugen87 Feb 23, 2025 •
edited
Loading

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 ) );

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

I haven't used nodes much yet but I tend to agree that more explicit functions are better and using functions that assume information about color spaces can make code unclear.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Mugen87 When you get a chance, can you think about this a bit? 🙏

All we need are these two methods: workingToColorSpace() and colorSpaceToWorking() -- in both ColorSpaceNode.js and ColorManagement.js. This avoids having different nomenclatures for the same thing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll try to revisit this issue when the JSDoc migration has been finished.


}

Expand Down
Loading