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

Use KHR_materials_transmission in FlightHelmet #34

Closed
wants to merge 1 commit into from

Conversation

mrdoob
Copy link
Contributor

@mrdoob mrdoob commented Oct 17, 2023

Every engine seem to be handling alphaMode: BLEND differently (used in the glasses):

Screenshot 2023-10-17 at 4 28 19 PM

KHR_materials_transmission seems to have better alignment so I propose updating this model with the extension.

/cc @elalish

@andreasplesch
Copy link
Contributor

On the other hand, the model shows off well what is possible without using extensions. An appropriate response may include addressing what the root cause of the differing interpretations is as the sample assets serve mostly this purpose.

@bghgary
Copy link

bghgary commented Oct 20, 2023

In Babylon.js, we turn on reflections by default because that's what most people expect. It can be turned off to be spec-conformant. We probably should enable this flag in the model viewer test code.

@elalish
Copy link
Contributor

elalish commented Oct 23, 2023

Yeah, the root cause of the differing interpretations is spec non-conformance because people wanted to do glass with alpha, which isn't correct, and hence why the transmission extension was added. I think this model should definitely be updated, because using alpha for glass is simply incorrect. @bghgary would you like to send a PR to update Babylon in our fidelity tests?

@elalish
Copy link
Contributor

elalish commented Nov 10, 2023

It appears I have the permissions, so I'm going to go ahead and merge this unless someone gives me a good reason not to. Is that okay @DRx3D?

@mrdoob
Copy link
Contributor Author

mrdoob commented Nov 13, 2023

Unable to fix these checks. Second attempt: #48

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.

4 participants