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

MeshPhongMaterial: Assign color space of default specular value #25872

Closed

Conversation

donmccurdy
Copy link
Collaborator

Assigns a color space for the default specular value, to ensure that it behaves the same way with or without THREE.ColorManagement. Without color management, all inputs are assumed to be Linear-sRGB. With color management, hexadecimal colors are considered sRGB, so we need to specify exceptions.

Related issue:

@donmccurdy donmccurdy added this to the r152 milestone Apr 17, 2023
@@ -58,6 +58,7 @@

const material = new THREE.MeshPhongMaterial( {
color: 0xff0000,
specular: 0x111111,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I missed this when updating the example earlier in 3b5cbe0. Further changes to lighting would restore the original appearance too, but it was much easier just to keep the specular value, and has little bearing on the example.

@github-actions
Copy link

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
633.9 kB (157.1 kB) 633.9 kB (157.1 kB) +24 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.1 kB (103.1 kB) +11 B

@donmccurdy donmccurdy marked this pull request as draft April 17, 2023 21:17
@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Apr 17, 2023

Hm, let me take a closer look at this. I'm not sure why this PR breaks the webgl_camera_arrays screenshot while enabling color management in the first place (13397a8) did not.

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Apr 18, 2023

It seems best to skip this change. I'm still a bit confused about why, but perhaps we should think of Phong's .specular property as more like a factor than a color, like a three-channel version of MeshPhysicalMaterial's .specularIntensity... In any case, adding this Linear-to-sRGB conversion is increasing the default specular highlight in a way we probably do not want.

@donmccurdy donmccurdy closed this Apr 18, 2023
@WestLangley
Copy link
Collaborator

@donmccurdy

material.color is the diffuse reflectance of the material.

material.specular is the specular reflectance of the material.

They are both unit-less, and they are both colors.

I don't think it would make sense from a physical modeling standpoint for the two colors to be associated with different color spaces -- or for one to be a color, and the other not.

@donmccurdy
Copy link
Collaborator Author

@WestLangley I agree that we cannot consider the two to be associated with different color spaces in the renderer. I believe we are agreed that this PR can remain closed, then, even if my speculative reasons in #25872 (comment) are incorrect?

@WestLangley
Copy link
Collaborator

@donmccurdy Yes, I think this PR should remain closed.

@donmccurdy donmccurdy deleted the feat/phong-specular-color-correction branch April 19, 2023 17:28
@mrdoob mrdoob removed this from the r152 milestone Apr 20, 2023
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.

4 participants