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

added IBL Sheen support #23069

Merged
merged 2 commits into from
Dec 23, 2021
Merged

added IBL Sheen support #23069

merged 2 commits into from
Dec 23, 2021

Conversation

elalish
Copy link
Contributor

@elalish elalish commented Dec 22, 2021

Fixes #22359

@WestLangley @mrdoob

I finally did the math to curve fit the integrated BRDF for sheen so it can be applied to the IBL (filament and gltf sample viewer use a LUT stored in a texture instead). That approximation is pretty good. The really wild approximation is using our PMREM for the sheen lookup, since the sheen BRDF is very different from Trowbridge-Reitz (our base roughness model) and it's anisotropic. All the realtime renderers I've seen do this, but they sample their PMREM in different ways. Some choose to sample at the reflection vector and the sheen roughness value, but I don't think this is a very good choice (not that anything is very accurate). I opted for using our diffuse sample instead (the sheen BRDF is close to a tangent-plane circle for all roughness values, so this at least pulls light evenly from this circle, though also from above), and has the major advantage of not requiring an extra texture lookup. My way also ensures you don't get an image reflection when sheen roughness goes to zero, which may happen with the other approach.

I did my best to follow the style of the ClearCoat extension. I have an energy preservation term in there too that also came from my curve fitting. The sheen layer is the top layer, above clear coat, which seems reasonable, as it might represent a dusty surface.

Here are some example renders:
model-viewer-golden
model-viewer-golden

@elalish
Copy link
Contributor Author

elalish commented Dec 22, 2021

I added my analysis for sheen to the end of my PMREM analysis colab: (EDIT: working link now) https://drive.google.com/file/d/1T0D1VSyR4AllqIJTQAraEIzjlb5h4FKH/view?usp=sharing

Please excuse the state of my python; it was a bit of a rush and I should really brush up on numpy.

@WestLangley
Copy link
Collaborator

I did my best to follow the style of the ClearCoat extension.

That was the correct thing to do.

I have an energy preservation term in there too that also came from my curve fitting.

Since you are implementing your own sheen model and sheen energy preservation term -- something different than Dassault, glTF, or Filament -- this should be documented, and a link provided in the source code. This goes for PMREM, too.

The sheen layer is the top layer, above clear coat.

Actually, glTF spec calls for the sheen later to be under the clear coat layer. I think we should be glTF compliant.

@mrdoob mrdoob added this to the r136 milestone Dec 22, 2021
@elalish
Copy link
Contributor Author

elalish commented Dec 22, 2021

Actually, glTF spec calls for the sheen later to be under the clear coat layer. I think we should be glTF compliant.

News to me! Honestly that sounds like a spec error to me, but I'll follow up in the next WG call. The difference will be hard to notice in practice, but I'll fix it if it turns out I'm wrong. EDIT: okay, following up everyone does seem to agree on this, so I'll just fix it.

I'll add a link to my write-up.

@elalish elalish requested a review from WestLangley December 23, 2021 00:09
@mrdoob mrdoob merged commit d93296e into mrdoob:dev Dec 23, 2021
@mrdoob
Copy link
Owner

mrdoob commented Dec 23, 2021

Thanks!

@WestLangley
Copy link
Collaborator

outgoingLight = outgoingLight * ( 1.0 - 0.157 * sheen ) + sheenSpecular;

The 0.157 magic number has to be referenced with a link, too.

And for clarity, something like:

float sheenEnergyComp = 1.0 - 0.157 * max3( material.sheenColor );

outgoingLight = outgoingLight * sheenEnergyComp + sheenSpecular;

@WestLangley
Copy link
Collaborator

@mrdoob Can't you allow the review process to complete? What's the deal? Grrr...

@mrdoob
Copy link
Owner

mrdoob commented Dec 23, 2021

Ah, sorry! I'll wait until your approval next time 😅

@Mugen87 Mugen87 mentioned this pull request Dec 23, 2021
mrdoob added a commit that referenced this pull request Dec 23, 2021
@mrdoob
Copy link
Owner

mrdoob commented Dec 23, 2021

@WestLangley 820a605

@WestLangley
Copy link
Collaborator

@mrdoob As I said, a reference link is required -- perhaps the same one, or at least some explanation of where the magic number comes from.

This code cannot be maintained by others if the physical modeling methods are not documented.

/ping @elalish

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 not working in combination with hdr env?
3 participants