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

Viewer material variants #15920

Merged
merged 6 commits into from
Dec 2, 2024
Merged

Conversation

ryantrem
Copy link
Member

@ryantrem ryantrem commented Dec 2, 2024

This PR adds support for material variants along with default UI in the new Viewer.

ViewerMaterialVariants.mov
  • Updates the KHR_materials_variants extension to have a callback option that "returns" a "controller" (available variants + selected variant). KHR_materials_variants already has both instance and static functions for getting material variants and selecting a material variant, but this would require importing the extension, and we don't want the extension included in the bundle statically (we only want to pay the bundle cost of the extension when it is actually used). This "controller callback" is kind of a new pattern for glTF extensions using the newish options support. Another way to do this would be to have a higher level callback option for when a specific loader plugin is created (analogous to OnPluginActivatedObservable) and when a specific glTF extension is created (analogous to onExtensionLoadedObservable) and use the extension instance directly. I don't really like this because to me it seems like a complex API (in the same way the static observables are complex and often a sticking point for users). Another option yet would be to have some kind of extensible output object in the same way options are an extensible input object. For example, AssetContainer could have an extras property (or something like that) where the underlying AssetExtras type could be extended by loader plugins and glTF extensions. Then you might do something like (const variants = assetContainer.extras.materialVariants and assetContainer.extras.selectedMaterialVariant = someVariant). That would be a bigger change though, and I'm not sure how we would make something like this work across the different loader functions. I think it could make sense for loadAssetContainerAsync, but maybe not the other loader functions. Thoughts @bghgary?
  • Viewer uses the "controller" returned by KHR_materials_variants to expose a simple material variants API.
  • Viewer element exposes material variants and also provides default UI (variant selection).
  • A bit of refactoring in the viewer element to make it easy to have any combination of toolbar controls (animation, material variants, hotspots) and put a vertical separator between each one.

@ryantrem ryantrem requested review from sebavan and bghgary December 2, 2024 17:12
@bjsplat
Copy link
Collaborator

bjsplat commented Dec 2, 2024

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

Copy link
Contributor

@bghgary bghgary left a comment

Choose a reason for hiding this comment

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

Discussed offline

@ryantrem ryantrem enabled auto-merge (squash) December 2, 2024 21:48
@bjsplat
Copy link
Collaborator

bjsplat commented Dec 2, 2024

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@bjsplat
Copy link
Collaborator

bjsplat commented Dec 2, 2024

@bjsplat
Copy link
Collaborator

bjsplat commented Dec 2, 2024

@bjsplat
Copy link
Collaborator

bjsplat commented Dec 2, 2024

@ryantrem ryantrem merged commit 587b99d into BabylonJS:master Dec 2, 2024
13 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.

4 participants