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

Color: Improve documentation about color management #29545

Merged
merged 3 commits into from
Oct 3, 2024

Conversation

donmccurdy
Copy link
Collaborator

Currently the THREE.Color docs suggest that RGB values cannot extend beyond the range [0, 1], leading to some confusion as in this thread. I've added a more detailed introduction to the class, along with some information about handling of color spaces, as a lighter/easier alternative to the full color management guide.

@WestLangley
Copy link
Collaborator

Currently the THREE.Color docs suggest that RGB values cannot extend beyond the range [0, 1].

And that is correct. material.color represents a reflectance, which is unit-less, and between 0 and 1.

This is why we have .intensity properties.

The intensity is where the units come from, such as radiance (luminance), irradiance (illuminance), or radiant intensity (luminous intensity).

In the case of lights, light.color serves as an attenuating mask, which is also unit-less, and between 0 and 1.

//

In the case of MeshBasicMaterial, the inline comments are not correct. The .color property is not emissive.

The .color property attenuates the environment map and light map, if present. Since the reflected light cannot exceed the incident light, .color cannot exceed 1.

MeshBasicMaterial should include an emissive property and an emissiveIntensity property. The emissive color does not have to match the material color.

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Oct 3, 2024

Hm, admittedly I much prefer to use MeshBasicMaterial as a stand-in for an unlit material, which of course we do not actually have... but as it is, yes, perhaps it's better not to use MeshBasicMaterial as my example. A custom ShaderMaterial could be a better example. I would strongly rather not continue adding new shading features to MeshBasicMaterial, I would (if anything) remove them and make it unlit, but I know that's also impossible as a large breaking change.

But I do still think we need to avoid saying THREE.Color RGB values cannot extend beyond the range [0, 1], this isn't true except in particular contexts where THREE.Color might be used. For example, using a THREE.Color instance with out-of-gamut sRGB components as input to THREE.ColorManagement for the purpose of converting it to in-gamut P3 would be valid.

@donmccurdy
Copy link
Collaborator Author

If I remove the paragraph beginning “Minimum and maximum values...”, do you feel the rest is OK? I see that part will need some care.

@WestLangley
Copy link
Collaborator

@WestLangley said

MeshBasicMaterial should include an emissive property and an emissiveIntensity property. The emissive color does not have to match the material color.

I really think that is the proper thing to do, if we are to be intellectually honest.

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Oct 3, 2024

We could alternatively use MeshStandardMaterial#emissive for cases like this:

function createAreaLightMaterial( intensity ) {
const material = new MeshBasicMaterial();
material.color.setScalar( intensity );
return material;
}

I think a core problem is that MeshBasicMaterial's support for any type of lighting is a surprise to most users when/if they discover it, we are trying to align the material intellectually to a concept that almost no one expects it to represent. Or that is my impression; perhaps I misjudge users' perceptions of the material!

In any case, I think I have removed the messier parts of this PR now, please let me know if you see other issues, thanks again!

@donmccurdy donmccurdy changed the title Color: Improve documentation about RGB range and color spaces Color: Improve documentation about color management Oct 3, 2024
@WestLangley
Copy link
Collaborator

I think a core problem is that MeshBasicMaterial's support for any type of lighting is a surprise to most users when/if they discover it

The features such as light map support, and env map support, were included so MeshBasicMaterial could be a stand-in on mobile in the early days of three.js. I agree that MeshBasicMaterial is a mash-up of sorts.

If you want to think about a new MeshUnlitMaterial, I think that could be illustrative.

@donmccurdy
Copy link
Collaborator Author

Even that I suppose is challenging, what would it mean for an "unlit" material to have RGB values >1... questions for another time. 😅 Thank you for improving this PR!

@donmccurdy donmccurdy merged commit ea86688 into mrdoob:dev Oct 3, 2024
3 checks passed
@donmccurdy donmccurdy deleted the docs/color-rgb-range branch October 3, 2024 21:45
@mrdoob mrdoob added this to the r170 milestone Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants