TSL: New color space name functions #29259
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
|
Thank you too :) |
| export const toOutputColorSpace = ( node, colorSpace = null ) => nodeObject( new ColorSpaceNode( nodeObject( node ), colorSpace, LinearSRGBColorSpace ) ); | ||
| export const toWorkingColorSpace = ( node, colorSpace = null ) => nodeObject( new ColorSpaceNode( nodeObject( node ), LinearSRGBColorSpace, colorSpace ) ); |
There was a problem hiding this comment.
These methods should not be conflated with outputColorSpace.
There was a problem hiding this comment.
@donmccurdy We need to be able to covert to log space in a shader, and then convert back.
export const workingToColorSpace = ( node, colorSpace = null ) => nodeObject( new ColorSpaceNode( nodeObject( node ), colorSpace, workingColorSpace ) );
export const colorSpaceToWorking = ( node, colorSpace = null ) => nodeObject( new ColorSpaceNode( nodeObject( node ), workingColorSpace, colorSpace ) );There was a problem hiding this comment.
toOutputColorSpace could be renamed to toColorSpace, but that begs the question: "from where?"
There was a problem hiding this comment.
These methods should not be conflated with
outputColorSpace.
toOutputColorSpace() will use renderer.outputColorSpace as default value, so I think there should be some representation. We have two base nomenclatures, which would be Output Color Space and Working Color Space in general terms.
There was a problem hiding this comment.
Another thing we could take advantage of from TSL is to inform(analyzing the input node) through a context which color space that node is inserted in the code flow, this would avoid redundant conversions. Similar to what we used in node.toTexture() recently.
There was a problem hiding this comment.
I'll look into adding workingToColorSpace and colorSpaceToWorking.
For toColorSpace ( targetColorSpace, sourceColorSpace ) we should use direct functions like these sRGBToLinearSRGB, linearSRGBTosRGB, ...
Just for us to think about, we shouldn't get used to string as a parameter, because that requires us to create a library to support it and that breaks tree-shaking.
If we have 4 different types of color-space between working/output, we will have to have 16 conversion functions and this growth is exponential for each additional color-space that we add here.
The future of renderer.outputColorSpace type could be a Node instead of string, this is one of the valid reasons for the current abstraction output/working names.
That's not a concern right now, I just wanted to clarify those points.
There was a problem hiding this comment.
@sunag I share your concern about the growing number of color space pairs, and their effect on tree-shaking. But I don't think I agree about using direct 1-to-1 functions or nodes for the conversions. For one thing, we have to import those functions, and call them conditionally, and if we have 4x4=16 functions it's going to be hard to ensure they're tree-shakeable for internal renderer usage.
A solution I'd prefer would be to continue using the predefined color space strings from WebGPU and CSS Color Module 4, but to allow users to define their own, and to import color spaces from three/addons. For example:
import { ColorManagement } from 'three';
import { Rec2100HLGColorSpace, Rec2100HLGImplementation } from 'three/addons/math/colorspaces/Rec2100HLG.js';
ColorManagement.addColorSpace( Rec2100HLGColorSpace, Rec2100HLGImplementation );Each definition would be similar to what you see in ColorManagement.js, but represented as data properties, not JavaScript, so it's easy to do conversions in JS, GLSL, or WGSL. There's a similar approach in nanocolor that I believe is well done: https://github.com/meshula/Nanocolor/blob/main/nanocolor.c#L150
Once registered, a color space could be used for any compatible purpose. Any color space conversions to/from a color space not registered with THREE.ColorManagement may be considered an error. We could probably drop Display P3 and Linear Display P3 from the core library.
There was a problem hiding this comment.
The conversion solution for direct functions is just the suggested function toColorSpace(), where it would be cleaner to do sRGBToLinearSRGB( a ) instead of a.toColorSpace( THREE.SRGBColorSpace, THREE.LinearSRGBColorSpace ) apart from the benefit of tree-shaking.
I don't think there was an appropriate solution in this sense for working/output so I created the PRs workingToColorSpace and colorSpaceToWorking following the suggestions.
I created a system based on this principle, it should already be functional. I think we can encourage users to add functions like a library to their projects instead of adding them all to the core, it would be a solution.
- StandardNodeLibrary
three.js/src/renderers/webgpu/nodes/StandardNodeLibrary.js
Lines 98 to 99 in f509e7b
This could be added externally with:
const nodeLibrary = renderer.nodes.library;
nodeLibrary.addColorSpace( linearSRGBTosRGB, getColorSpaceMethod( LinearSRGBColorSpace, SRGBColorSpace ) );
nodeLibrary.addColorSpace( sRGBToLinearSRGB, getColorSpaceMethod( SRGBColorSpace, LinearSRGBColorSpace ) );Looking at the code again, I would just make it simpler, e.g:
const nodeLibrary = renderer.nodes.library;
nodeLibrary.addColorSpace( linearSRGBTosRGB, THREE.LinearSRGBColorSpace, THREE.SRGBColorSpace );
nodeLibrary.addColorSpace( sRGBToLinearSRGB, THREE.SRGBColorSpace, THREE.LinearSRGBColorSpace );There was a problem hiding this comment.
I don't think we can ask users to foresee all possible source/target color space pairs, though. And with only a function/node defining a color space, it won't be possible to:
- Apply CPU-side calculations, like converting
new Color( 0x808080 )from sRGB to a user-defined working color space - Determine whether a texture in a user-defined color space requires unpacking with gl.unpackColorSpace or not. For this we need to be able to query, for a particular texture's color space, what are the associated transfer functions?
I think the workingToColorSpace and colorSpaceToWorking functions will fit into either solution well. But I would like to refactor THREE.ColorManagement a little, so that the node system can infer how to convert between two previously unknown color spaces based on their definitions (primaries, white point, transfer functions), without needing to have a conversion function/node registered in advance.
There was a problem hiding this comment.
I don't think we can ask users to foresee all possible source/target color space pairs, though. And with only a function/node defining a color space, it won't be possible to:
The idea of the user adding other ColorSpaces would be related to the colorspaces currently in effect in the project or in your library, not in a global sense.
It would be great to have your collaboration in this regard :)
Related issue: #29248 (comment)
Description
Description updated in this PR