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

Material: Infer texture color space (WIP) #25857

Closed

Conversation

donmccurdy
Copy link
Collaborator

Related issue:

Description

Updates THREE.MeshBasicMaterial and THREE.MeshStandardMaterial to infer the color space of assigned textures. This is possible based on a couple of assumptions:

  1. Textures assigned to material slots like .map and .emissiveMap MUST contain color data
  2. When a texture containing color data has .colorSpace === THREE.NoColorSpace (default), THREE.NoColorSpace can safely be interpreted as "unknown" or "unspecified"
  3. In that case, we can infer sRGB vs. Linear-sRGB based on the texture's .type property

There are limitations to this approach:

  • We cannot infer wide-gamut color spaces like Display P3, they must be specified
  • We cannot infer color spaces for NodeMaterial when a texture is connected only indirectly to a material input
  • We cannot infer color spaces for ShaderMaterial

Just an idea — I don't yet feel confident enough about this to include it in r152 with the other color management changes.

@donmccurdy donmccurdy added this to the r??? milestone Apr 17, 2023
@github-actions
Copy link

github-actions bot commented Apr 17, 2023

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
633.9 kB (157.1 kB) 634.2 kB (157.2 kB) +372 B

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
425.1 kB (103 kB) 425.3 kB (103.1 kB) +174 B

src/materials/Material.js Fixed Show fixed Hide fixed
src/materials/Material.js Fixed Show fixed Hide fixed
@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Apr 17, 2023

E2E tests fail on one example — webgl_nodes_loader_gltf_iridescence. Because getters are not enumerable, NodeMaterial.fromMaterial( material ) runs into trouble iterating over material properties. That could be solved in a few ways, but I'll wait for initial feedback on the PR before working on a fix.

@LeviPesin
Copy link
Contributor

Should be fixed by #25861.

@gkjohnson
Copy link
Collaborator

gkjohnson commented Apr 18, 2023

Just an idea — I don't yet feel confident enough about this to include it in r152 with the other color management changes.

I think I like the intent - I'm just not sure if I like the side effect of the material modifying the color space field on the Texture. Is this something the renderer can do based on the material field + texture combo? I assume no due to implicit hardware texture colorspace conversion.

The other limitations make me hesitant to support this, though.

We cannot infer wide-gamut color spaces like Display P3, they must be specified

With more asset color spaces starting to become relevant part of me is wondering how far this idea of users not needing to know about color spaces will be able to go 😅

@sunag sunag closed this in #25861 Apr 18, 2023
@Mugen87 Mugen87 reopened this Apr 18, 2023
@mrdoob
Copy link
Owner

mrdoob commented Apr 20, 2023

  1. When a texture containing color data has .colorSpace === THREE.NoColorSpace (default), THREE.NoColorSpace can safely be interpreted as "unknown" or "unspecified"

In that case... Wouldn't it be more consistent to use null for that case and remove the NoColorSpace constant?

@mrdoob
Copy link
Owner

mrdoob commented Apr 20, 2023

I think I like the intent - I'm just not sure if I like the side effect of the material modifying the color space field on the Texture. Is this something the renderer can do based on the material field + texture combo?

Indeed. Is this something the renderer could do instead? 🤔

@donmccurdy
Copy link
Collaborator Author

In that case... Wouldn't it be more consistent to use null for that case and remove the NoColorSpace constant?

I'm happy with either of these. The value of the NoColorSpace constant is currently an empty string, ''.

/cc @WestLangley


I think I like the intent - I'm just not sure if I like the side effect of the material modifying the color space field on the Texture. Is this something the renderer can do based on the material field + texture combo?

Indeed. Is this something the renderer could do instead? 🤔

My thought was that we probably don't want the renderer having to know about every possible material, any more than it already does... if that isn't a concern then yes, I imagine the renderer could also do this.

I see it as an advantage that the texture's .colorSpace property gets filled in when missing, though. I worry that other things (e.g. export) will become harder if textures are floating around in applications without that information.


With more asset color spaces starting to become relevant part of me is wondering how far this idea of users not needing to know about color spaces will be able to go

One thing I see in tools like OpenColorIO (used by Blender, Unreal, Substance, ...) is the concept of color space "roles". You don't necessarily assign every texture its color space, but might define in a config that the color space of all base color maps is X. For our purposes a similar idea could be:

THREE.ColorManagement.textureColorSpace = DisplayP3ColorSpace;

But still, yes, the user will certainly need to do some configuration to have wide-gamut color.

@WestLangley
Copy link
Collaborator

In that case... Wouldn't it be more consistent to use null for that case and remove the NoColorSpace constant?

Thinking... Do we need to explicitly differentiate between "not color data" and "unspecified color space", or can null suffice for both. 🤔

With more asset color spaces starting to become relevant part of me is wondering how far this idea of users not needing to know about color spaces will be able to go

I'm inclined to agree. I don't think the renderer should try to be so smart, lest it become too smart by half.

@donmccurdy
Copy link
Collaborator Author

Do we need to explicitly differentiate between "not color data" and "unspecified color space", or can null suffice for both.

So far I haven't been able to think of a case where we'd need both.


Ok, it sounds like inferring texture color space doesn't currently belong in the material or renderer classes, so I'm happy to close out the PR for now. 👍

@donmccurdy donmccurdy closed this Apr 21, 2023
@donmccurdy donmccurdy deleted the feat/infer-texture-colorspace branch April 21, 2023 14:18
@WestLangley
Copy link
Collaborator

Do we need to explicitly differentiate between "not color data" and "unspecified color space", or can null suffice for both?

So far I haven't been able to think of a case where we'd need both.

If that is the case, I agree texture.colorSpace can default to null, and the NoColorSpace constant can be removed.

@donmccurdy
Copy link
Collaborator Author

@mrdoob mrdoob removed this from the r??? milestone Apr 27, 2023
@donmccurdy
Copy link
Collaborator Author

An alternative to this PR... how do others feel about adding support for inferring texture color space from naming conventions? To my understanding some naming conventions are common in game engines, though I don't know if they're used for color space inferrence.

I'm not sure I'd want that on by default, but it should be easy enough to support an API, either in ColorManagement or TextureLoader, by which users can register their own naming convention —

THREE.TextureLoader.setColorSpaceRules({
  '_diffuse': THREE.SRGBColorSpace,
  '_emissive': THREE.SRGBColorSpace,
  '_occlusion': THREE.NoColorSpace,
  '_metalness': THREE.NoColorSpace,
  '_roughness': THREE.NoColorSpace,
});

Color spaces would be applied by TextureLoader, if a suffix matches the texture's filename. Model loaders can (and already do) override color space themselves.

Related:

@gkjohnson
Copy link
Collaborator

gkjohnson commented Nov 1, 2023

I don't know if I love relying on filenames 😅 I might have mentioned this in another issue - but it would be good to know what the expected file format options are for p3 or other wide-gamut color content. In the case of PNG it will be ambiguous and using _diffuse as an identifier to determine color space will be incorrect when wide gamut is used. Do we know Unity or Unreal or Blender recommend for HDR content? This webkit page on wide gamut images uses PNG to display wide gamut images.

Depending on what the problem we're trying to solve here is we might consider adding another loader like ColorTextureLoader or VisualTextureLoader that just tries to set the color to an appropriate color space for color map images. This might feel more obvious to users who are having trouble with these "loose" texture files as you've called them.

But overall I think what you recommend in w3c/ColorWeb-CG#112 is the best option, though it will probably be awhile before it's available.

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Nov 1, 2023

Mainly the problem I'm trying to solve is that users don't know which textures should use which color space (or NoColorSpace). Even if we provided a complete list of recommendations, applying the color spaces correctly to every texture every time is verbose, e.g. in R3F it's something like...

const map = useTexture('path/to/diffuse.png', (texture) => {
  texture.colorSpace = THREE.SRGBColorSpace;
  return texture;
}); 

...it would be good to know what the expected file format options are for p3 or other wide-gamut color content

I think that's a strength of filename conventions over the original PR here. If the user is authoring wide-gamut content they can simply adjust the map:

THREE.TextureLoader.setColorSpaceRules({
  '_diffuse': THREE.DisplayP3ColorSpace,
  '_emissive': THREE.DisplayP3ColorSpace,
  '_occlusion': THREE.NoColorSpace,
  '_metalness': THREE.NoColorSpace,
  '_roughness': THREE.NoColorSpace,
});

I don't necessarily think we should provide any default naming convention – much less one that encompasses wide-gamut color spaces. But I'm looking for methods users can set up once, or copy/paste from an example, rather than thinking about it on every texture load. :/

I'm not optimistic about w3c/ColorWeb-CG#112 solving this completely. Even if the feature is added, non-color maps often carry sRGB ICC profiles, and detecting SRGBColorSpace vs. NoColorSpace may remain hard. That may provide enough signal to identify Display P3 images correctly though.

@gkjohnson
Copy link
Collaborator

Mainly the problem I'm trying to solve is that users don't know which textures should use which color space

At the least I think users may understand which colors are "color" data and which ones are not when they're loading them - maybe not? But if they do then providing a separate loader specifically designated for color textures that auto-assigns a more sensible color space may be more intuitive.

I don't necessarily think we should provide any default naming convention – much less one that encompasses wide-gamut color spaces. But I'm looking for methods users can set up once, or copy/paste from an example, rather than thinking about it on every texture load. :/

fwiw these are both something that can be built now as a three.js example module that extends TextureLoader. But either way it's not a silver bullet. Content and textures using different color spaces can still be loaded into a single scene.

I'm not optimistic about w3c/ColorWeb-CG#112 solving this completely. Even if the feature is added, non-color maps often carry sRGB ICC profiles, and detecting SRGBColorSpace vs. NoColorSpace may remain hard. That may provide enough signal to identify Display P3 images correctly though.

That may be right... Maybe more of a reason in that case to provide a texture loader specifically for color maps that tries to determine the right color space. And no-color-space textures would still use regular TextureLoader.

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.

6 participants