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

ShadersChunk: Only use clearcoat chunks when clearcoat > 0 #22405

Merged
merged 5 commits into from
Aug 24, 2021
Merged

Conversation

mrdoob
Copy link
Owner

@mrdoob mrdoob commented Aug 24, 2021

Description

Currently MeshPhysicalMaterial computed shader includes clearcoat chunks even when material.clearcoat is 0. This PR solves that and does some additional clean up.

@mrdoob mrdoob requested a review from WestLangley August 24, 2021 10:45
@mrdoob mrdoob added this to the r132 milestone Aug 24, 2021
@mrdoob mrdoob requested a review from Mugen87 August 24, 2021 10:46
Copy link
Collaborator

@WestLangley WestLangley left a comment

Choose a reason for hiding this comment

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

yay!

todo = todo - 1;

Comment on lines +198 to +201
clearcoat: useClearcoat,
clearcoatMap: useClearcoat && !! material.clearcoatMap,
clearcoatRoughnessMap: useClearcoat && !! material.clearcoatRoughnessMap,
clearcoatNormalMap: useClearcoat && !! material.clearcoatNormalMap,
Copy link
Owner Author

Choose a reason for hiding this comment

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

@WestLangley Should we do something like this for transmission too? If so... what parameters depend on material.transmision > 0?

Copy link
Collaborator

Choose a reason for hiding this comment

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

transmission: <float>,
transmissionMap: new THREE.Texture( <Image> ),

thickness: <float>,
thicknessMap: new THREE.Texture( <Image> ),
attenuationDistance: <float>,
attenuationTint: <Color>,

Copy link
Owner Author

Choose a reason for hiding this comment

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

Excellent! I'll do that on my next PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But actually, transmission and volume are separate extensions. The last four are for volume.

Copy link
Owner Author

@mrdoob mrdoob Aug 24, 2021

Choose a reason for hiding this comment

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

Oh! But... in the context of our shader chunks, do thickness and attenuation affect the material if transmission is 0?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Related: You can do something like this for sheenTint too, so the version will be incremented when sheenTint is changed from black to not-black.

Would love to see that. That would make the physical exclusive properties more consistent.

Copy link
Owner Author

Choose a reason for hiding this comment

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

So then the logic would be like this?

if ( transmision > 0 ) {

    transmission: <float>
    transmissionMap: new THREE.Texture( <Image> )

    if ( thickness > 0 ) {

        thickness: <float>
        thicknessMap: new THREE.Texture( <Image> )
        attenuationDistance: <float>
        attenuationTint: <Color>

    }

}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, something like that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There should be USE_TRANSMISSION and USE_VOLUME.

It is up to you if USE_TRANSMISSION is used to mean "transmission_without_volume" -- a.k.a. thin surface only.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh! But... in the context of our shader chunks, do thickness and attenuation affect the material if transmission is 0?

No.

@mrdoob mrdoob marked this pull request as ready for review August 24, 2021 14:13
@mrdoob mrdoob merged commit a4a931b into dev Aug 24, 2021
@mrdoob mrdoob deleted the clearcoat branch August 24, 2021 15:10
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