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 sheen is shadowed by specular ambient occlusion. #26940

Merged
merged 2 commits into from
Oct 12, 2023

Conversation

bhouston
Copy link
Contributor

@bhouston bhouston commented Oct 11, 2023

Fixed #26939

Description

This fixes sheen so that that when it is lit by indirect light (IBL) it will be shadowed by ambient occlusion.

Before:
Screenshot 2023-10-10 at 5 01 25 PM

After:
Screenshot 2023-10-10 at 8 26 38 PM

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 sheen.

Fun fact: I think that clear coat has the same bug. I will make a PR for occluding indirect clearcoat separately.

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 kB (161.3 kB) 651.2 kB (161.4 kB) +179 B

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
443.7 kB (107.3 kB) 443.9 kB (107.4 kB) +179 B

@mrdoob
Copy link
Owner

mrdoob commented Oct 11, 2023

/ping @elalish @WestLangley

@WestLangley
Copy link
Collaborator

Related: #8107.

And, the paper describing the specular occlusion term:
https://seblagarde.files.wordpress.com/2015/07/course_notes_moving_frostbite_to_pbr_v32.pdf

@bhouston
Copy link
Contributor Author

Thanks @WestLangley! I remembered wrong, the specular occlusion is for indirect specular. Thus I think this is mostly correct.

Review is still appreciated though!

Copy link
Contributor

@elalish elalish left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good to me. I should have thought of this when I added support.

@mrdoob mrdoob merged commit 9113c04 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.

Sheen from IBL isn't being affected by AO
4 participants