Texture: Add Texture.DEFAULT_COLOR_SPACE

[linbingquan]

Related issue: #XXXX

Description

Add Texture.DEFAULT_COLORSPACE variable.
In some cases, reduce the amount of duplicate code.

[github-actions]

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev	Filesize PR	Diff
674.5 kB (167.1 kB)	674.5 kB (167.2 kB)	+46 B

🌳 Bundle size after tree-shaking

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

Filesize dev	Filesize PR	Diff
453.8 kB (109.6 kB)	453.9 kB (109.6 kB)	+45 B

[donmccurdy]

Interesting idea! Let's use the DEFAULT_COLOR_SPACE (three words) for consistency with .colorSpace. Note that we'd also need to update the loaders...

three.js/examples/jsm/loaders/GLTFLoader.js

Lines 3374 to 3378 in ed211d8

	if ( colorSpace !== undefined ) {
	texture.colorSpace = colorSpace;
	}

... otherwise they'll assume the default is NoColorSpace, and fail to set up non-color textures (like .normalMap) correctly. It would also be important to clarify... is this meant to be the default for all textures when not otherwise specified? Or the default for all color textures? If you set Texture.DEFAULT_COLOR_SPACE = THREE.DisplayP3ColorSpace, would you expect loaders to ...

• (A) assign color textures Display P3 (from the DEFAULT_COLOR_SPACE), and non-color textures NoColorSpace
• (B) assign color textures sRGB (because it's convention in most model formats), and non-color textures NoColorSpace
• (C) assign all textures DEFAULT_COLOR_SPACE
• (D) ... something else?

I'm leaning toward (A).

[linbingquan]

Thank you for your response.

Interesting idea! Let's use the DEFAULT_COLOR_SPACE (three words) for consistency with .colorSpace.

Done.

Note that we'd also need to update the loaders...

TBH, I don't know which loaders should be updated...

I've seen examples with quite a bit of duplicate code like: texture.colorSpace = THREE.SRGBColorSpace;

I found three loaders(EXRLoader, KTX2Loader and USDZLoader) that reference NoColorSpace.

[donmccurdy]

I've seen examples with quite a bit of duplicate code like: texture.colorSpace = THREE.SRGBColorSpace;

Personally I would recommend that we keep the examples this way, perhaps unless they're intentionally using a wide gamut like Display P3, which is only experimentally supported right now. I worry that if we 'simplify' the examples by doing Texture.DEFAULT_COLOR_SPACE = SRGBColorSpace we're going to lead users down a more confusing path, where other textures (like normal maps, aoMaps, metal/rough maps, transmission, ...) don't work properly, presenting weird and confusing issues. Whereas if the color texture has the wrong color space, it's considerably more obvious what is wrong.

We'll need to check all the loaders in #23283, if they support textures, but it doesn't need to be in this PR. The goal would be to make sure the loaders always specify the .colorSpace property, using DEFAULT_COLOR_SPACE (or whatever the format dictates) for color textures, and NoColorspace for non-color textures.

/cc @gkjohnson @WestLangley

[gkjohnson]

Generally I'm very against these kinds of editable global flags since they make function behavior less predictable and less reliable. They just pass on the verbosity to dependent projects. Ie if I have a project and I need loaded or processed data to be set to NoColorSpace in some other dependency then I need to do the following every time:

const originalDefaultColorSpace = Texture.DEFAULT_COLOR_SPACE;
Texture.DEFAULT_COLOR_SPACE = NoColorSpace;

// load or generate the data

Texture.DEFAULT_COLOR_SPACE = originalDefaultColorSpace;

It also sets projects to break more easily. For example if someone is using three-gpu-pathtracer or the mesh bvh shader textures and they've set the default texture color space to sRGB then suddenly all my path tracer data textures break because they're implicitly set to the wrong color space.

I think some examples of what this is actually helping should be provided so we can determine if this is actually addressing an issue and discuss other options. Without knowing more it should be very simple for end users to create utility functions that set the appropriate color space.

[WestLangley]

I understand the motivation behind this PR, but I think the implications of this change would be too confusing.

[linbingquan]

To be honest, I didn't think much of it, and after reading some feedback, I found that adding Texture.DEFAULT_COLOR_SPACE was confusing. Because it involves other things like loaders and other libs dependencie on existing Texture.colorSpace paramater. I feel this PR can be close.

[donmccurdy]

Ok, I believe you are right – thanks @gkjohnson, @WestLangley, and @linbingquan!