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

MeshPhysicalMaterial: .sheen -> .sheenTint #22381

Merged
merged 1 commit into from
Aug 21, 2021

Conversation

WestLangley
Copy link
Collaborator

@WestLangley WestLangley commented Aug 21, 2021

To accommodate upcoming changes to MeshPhysicalMaterial, .sheen is renamed to .sheenTint.

At a later time, additional properties will be added:

sheenTintMap
sheenRoughness
sheenRoughnessMap

This nomenclature is consistent with the properties .specularTint and .attenuationTint.

sheenTint is a THREE.Color. it is never null.

A new convenience method THREE.Color.isBlack() is added which can be used to test for zero values.

As per the glTF spec, the sheen layer is disabled when the color is black.

@WestLangley WestLangley added this to the r132 milestone Aug 21, 2021
@WestLangley
Copy link
Collaborator Author

Note: webgl_materials_physical_sheen.html needs to be rewritten. I have not modified it.

@mrdoob mrdoob merged commit 588bc49 into mrdoob:dev Aug 21, 2021
@mrdoob
Copy link
Owner

mrdoob commented Aug 21, 2021

Thanks!

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 21, 2021

sheenTint is a THREE.Color. it is never null.

#22379 (comment). I vote to keep null for optional shader features.

@WestLangley WestLangley deleted the dev_sheenTint branch August 21, 2021 11:16
@WestLangley
Copy link
Collaborator Author

sheenTint is a THREE.Color. it is never null.

... I vote to keep null for optional shader features.

We set optional material texture properties to null.

.emissive is a THREE.Color and it is not set to null; it is set to zero.

Same for sheenTint.

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 21, 2021

The difference is that emissive does not influence a GLSL define. That is true for sheenTint.

In other words changing the default value of emissive does not produce a shader permutation whereas sheenTint does.

@WestLangley
Copy link
Collaborator Author

That is not the user's concern.

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 21, 2021

Don't get me wrong, I'm fine with your alternative approach. I just though because of #17700 (comment) we want to use null for such use cases.

@mrdoob
Copy link
Owner

mrdoob commented Aug 24, 2021

Would it make sense to add a sheen (float) property that behaves like clearcoat and transmission?
It'd basically multiply sheenTint.

@WestLangley
Copy link
Collaborator Author

WestLangley commented Aug 24, 2021

Personally, I think clearcoat should be named clearcoatIntensity. Ditto for transmissionIntensity. They are not booleans.

EDIT: ...but I do not have a strongly feeling about it. It is OK as-is.

Would it make sense to add a sheen (float) property that behaves like clearcoat and transmission?
It'd basically multiply sheenTint.

Regarding sheen, we will be having

sheenTint
sheenTintMap
sheenRoughness
sheenRoughnessMap

Adding sheenIntensity seems natural IMO, but the glTF spec does not call for it.

EDIT: In Version 2022x of the Dassault spec, sheenIntensity has been removed -- consistent with the glTF spec.

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.

3 participants