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

Switch to the metal roughness model for glTF. #921

Merged
merged 9 commits into from
Nov 29, 2022
Merged

Switch to the metal roughness model for glTF. #921

merged 9 commits into from
Nov 29, 2022

Conversation

ikeough
Copy link
Contributor

@ikeough ikeough commented Nov 28, 2022

BACKGROUND:
When integrating the three gpu path tracer in Hypar, I noticed that materials were overly reflective. After opening gkjohnson/three-gpu-pathtracer#310, I was informed that the extension we have used previously, KHR_materials_pbrSpecularGlossiness is being "archived" in favor of newer extensions.

DESCRIPTION:
This PR updates our glTF serialization to support KHR_materials_specular and KHR_materials_ior, with conversions as noted in https://github.com/donmccurdy/glTF-Transform/blob/d77ca6a12c5b56efa1b6594806450dd38df19b25/packages/functions/src/metal-rough.ts

TESTING:
Run a test and export a glTF file. The results should look very similar. I've exported several glTF's and run them through Don McCurdy's viewer and they pass all validation and look correct. I've updated the Materials example to indicated roughness and specularity.

Here it is in Don McCurdy's viewer (latest three):
image

Here it is in Hypar (note: different HDRI environments are used)
image

FUTURE WORK:

  • The Material class should be updated with properties that align with the metallic roughness model.

REQUIRED:

  • All changes are up to date in CHANGELOG.md.

This change is Reviewable

@andrewheumann
Copy link
Member

This looks good to me! Have we tested on Hypar's 3d viewer / do we know if our existing GLTF loader already supports these extensions out of the box? We shouldn't merge this until that's verified + tested, so we don't break our ability to rely on alphas for hypar function development.

@ikeough
Copy link
Contributor Author

ikeough commented Nov 28, 2022

Good question. Threejs shows support for these extensions already: https://threejs.org/docs/#examples/en/loaders/GLTFLoader. So as long as we stay current with three, it should work. I have tried the drag and drop gltf workflow on Hypar and things look fine which is expected even without these extensions because it defaults to the metallness-roughness workflow. What I need to do is build a test with lots of shiny and matte stuff.

Copy link
Member

@andrewheumann andrewheumann left a comment

Choose a reason for hiding this comment

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

It looks like this support is already in the version of three we are using, so I'm OK to proceed w/ this one, but more tests would be valuable regardless!

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained

@ikeough ikeough merged commit 5a742bb into master Nov 29, 2022
@ikeough ikeough deleted the roughness branch November 29, 2022 20:31
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.

None yet

2 participants