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

ensure that indirect clear coat is shadowed by specular ambient occlusion #26941

Merged
merged 4 commits into from
Oct 12, 2023

Conversation

bhouston
Copy link
Contributor

@bhouston bhouston commented Oct 11, 2023

Related issue: #26939

When I fixed indirect sheen so that it is shadowed by ambient occlusion, I noticed that indirect clearcoat was similarly also not affected by ambient occlusion. So I have applied the same fix to clear coat as I did with sheen.

Caveat: My reuse of the specularOcclusion function is an approximation. I am not sure if it is appropriate for indirect specular, rather I think it was designed for direct specular. It is close anyhow and better than not occluding indirect clear coat.

This contribution is funded by Threekit

@github-actions
Copy link

github-actions bot commented Oct 11, 2023

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
651.2 kB (161.4 kB) 651.4 kB (161.4 kB) +200 B

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
443.9 kB (107.4 kB) 444.1 kB (107.4 kB) +200 B

@mrdoob mrdoob added this to the r158 milestone Oct 11, 2023
@mrdoob
Copy link
Owner

mrdoob commented Oct 11, 2023

/ping @elalish @WestLangley

@@ -10,7 +10,14 @@ export default /* glsl */`

float dotNV = saturate( dot( geometryNormal, geometryViewDir ) );

reflectedLight.indirectSpecular *= computeSpecularOcclusion( dotNV, ambientOcclusion, material.roughness );
float specularOcclusion = computeSpecularOcclusion( dotNV, ambientOcclusion, material.roughness );
Copy link
Contributor

Choose a reason for hiding this comment

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

This should use the clearcoat roughness instead, so I think you need to call the function twice.

@mrdoob mrdoob merged commit 661c6bb into mrdoob:dev Oct 12, 2023
19 checks passed
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