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

MeshStandardMaterial: Remove roughness-dependent Fresnel from environment lighting #22308

Merged
merged 1 commit into from
Aug 11, 2021

Conversation

WestLangley
Copy link
Collaborator

Correction to #15644 and #16559.

The sample code in the paper is not consistent with the derivation in the paper.

The derivation uses specular reflectance F0, the code used fresnel F.

This discrepancy has been noted elsewhere.

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

Even with this change, CI passed for me. We'll see what happens... :-)

@mrdoob
Copy link
Owner

mrdoob commented Aug 11, 2021

Curious to see! 👀

@mrdoob mrdoob merged commit b2aeac3 into mrdoob:dev Aug 11, 2021
@mrdoob
Copy link
Owner

mrdoob commented Aug 11, 2021

Thanks!

@mrdoob
Copy link
Owner

mrdoob commented Aug 11, 2021

It's pretty subtle in general, but I can see the change in the helmet:

Before After
Screenshot 2021-08-11 at 10 56 04 Screenshot 2021-08-11 at 10 55 47

@elalish this will probably affect https://modelviewer.dev/fidelity/

@mrdoob
Copy link
Owner

mrdoob commented Aug 11, 2021

More noticeable in the FlightHelmet with Model Viewer's lighting setup:

Before After
Screenshot 2021-08-11 at 11 14 36 Screenshot 2021-08-11 at 11 13 01

@mrdoob
Copy link
Owner

mrdoob commented Aug 11, 2021

I think this should bring us closer to the other renderers 👏

Notice how bright the surface of the wood piece was before this change:

Screenshot 2021-08-11 at 11 16 53

Edit: After this change...

Screenshot 2021-09-17 13 46 19

@mrdoob
Copy link
Owner

mrdoob commented Aug 11, 2021

Also noticeable in Cerberus.obj:

Before After
Screenshot 2021-08-11 at 11 31 33 Screenshot 2021-08-11 at 11 32 16

@mrdoob
Copy link
Owner

mrdoob commented Aug 11, 2021

One more:

Before After
Screenshot 2021-08-11 at 11 44 06 Screenshot 2021-08-11 at 11 43 48

@mrdoob
Copy link
Owner

mrdoob commented Aug 11, 2021

It's surprising that the e2e ci hasn't noticed any change... 😶

@mrdoob
Copy link
Owner

mrdoob commented Aug 11, 2021

As in #16559 (comment), it's specially noticeable in the tubes behind the helmet:

Before After
Screenshot 2021-08-11 at 12 04 04 Screenshot 2021-08-11 at 12 03 52

@mrdoob
Copy link
Owner

mrdoob commented Aug 11, 2021

Looking good! ✨

@WestLangley WestLangley deleted the dev_environment_fresnel branch August 11, 2021 12:45
@elalish
Copy link
Contributor

elalish commented Aug 11, 2021

Thanks for the diligence, @WestLangley! I'm working on updating our fidelity tests, so I'll be able to give better feedback soon, but I agree with @mrdoob's analysis.

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