-
-
Notifications
You must be signed in to change notification settings - Fork 35.5k
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
GLTFLoader: Implement Materials Variants extension handler as plugin #20789
Conversation
examples/js/loaders/GLTFLoader.js
Outdated
|
||
gltf.scenes[ i ].userData.variants = variants; | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I save variants in scene.userData.variants
, too, because they can be used for future glTF exporter support. Otherwise, variants parameter may need to be separately passed to the exporter.
examples/js/loaders/GLTFLoader.js
Outdated
|
||
gltf.userData.variants = variants; | ||
|
||
gltf.userData.selectVariant = function selectVariant ( variantName ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taking scene
as a parameter instead of using gltf.scenes
would be another option.
Sounds OK to implement the variant example using the plugin API, if it's not much more complicated for users to understand when reading the example. But I'd prefer that the plugin itself remain in the example, and not ship as part of GLTFLoader... I think it's important that GLTFLoader returns something that behaves like a normal THREE.Group hierarchy, and having GLTFLoader return functions like The before/after root hooks sound like good simple additions. Is the |
Thanks for sharing your opinion. It makes sense to me. I made import { GLTFLoader } from './jsm/loaders/GLTFLoader.js';
import GLTFMaterialsVariantsExtension from './jsm/loaders/gltf_plugins/KHR_materials_variants.js';
const loader = new GLTFLoader().register(parser => {
return new GLTFMaterialsVariantsExtension(parser);
});
loader.load( 'MaterialsVariantsShoe.gltf', async gltf => {
scene.add(gltf.scene);
const variants = gltf.userData.variants;
await gltf.userData.selectVariant(variants[1]);
render();
}); If users want their own variant select function, they don't register the handler and they can write their own function with the data under the
The loader saves the parameters for unknown extensions. If the Another solution for saving the parameters of known extension may be adding invokeAll |
I'm interested in this for model-viewer as well (I already added variants support based on your example). However, I need the ability to export those variants as well with GLTFExporter, which currently fails because though it retains the variants I think I could implement variants export now using |
Ah, just saw #20842, which sounds promising! |
gltf.userData.selectVariant(variants[1]); Adding methods to What do you guys think about creating a |
I am pretty nervous about this... I would prefer that GLTFLoader produced normal three.js objects. From my perspective it's OK if we don't support
Note that GLTFLoader does not process materials until they're needed for something, either by an object in the scene graph, or direct request by an external call to
I don't think I quite understand how |
@donmccurdy Fair, after reviewing the code a bit more I was mistaken about what |
Yeah, it reminds me of that I agree with
I'm thinking of placing an unofficial function |
Adding variants code in |
Technically the variants aren't defined at the glTF scene level, but at the glTF root level. Therefore I would propose putting |
Ah, sorry... |
How about adding import GLTFMaterialsVariantsExtension from './jsm/loaders/gltf_plugins/KHR_materials_variants.js';
const loader = new GLTFLoader();
loader.register(parser => {
return new GLTFMaterialsVariantsExtension(parser);
});
loader.load('model.gltf', async gltf => {
scene.add(gltf.scene);
const variants = gltf.userData.variants; // ['variantName0', 'variantName1', ...]
await GLTFMaterialsVariantsExtension.selectVariant(gltf.scene, variants[0], true /* doTraverse */);
render();
}); In this PR The A problem is serialization from/to json wouldn't work for material variants extension because Three.js json serialization doesn't serialize Three.js objects/references in |
If we need to change something to support exporting variants, we can do that on its own, too. |
I personally think yes as I wrote in the PR comment. The code in
On the other hand, I also can understand Don's feeling that |
b3a06d9
to
550165d
Compare
I made a PR #21207 for Regarding |
#21207 looks good to me, thanks. I do still prefer that GLTFLoader should return plain three.js objects, and not glTF-specific methods or subclasses. Both are hard to serialize or clone, and increase the public API surface and maintenance responsibilities related to GLTFLoader. |
I wrote the https://github.com/takahirox/three-gltf-plugins Let's consider moving them to the Three.js repository if I (or we) come up with an idea to make the extension great fit to plain Three.js APIs and structures. BTW I guess @elalish may be interested in the exporter plugin? |
Nice work @takahirox! Do you want to put a link next to the extensions listed in https://threejs.org/docs/#examples/en/loaders/GLTFLoader? |
That sounds good. I would do soon, after I have post-surgery health checkup today. |
Opened a PR for the doc #21261 I would be happy if you review. And I also pleased if you comment for #19359 (comment) because the suggestion can simplify my materials variants plugin. |
Related PR: #20690
Description
glTF Materials variants extension example has been added in #20690. It's a nice example indeed.
Currently
KHR_materials_variants
extension handler is implemented inexamples/webgl_loader_gltf_variants.html
. Handling the extension without the loader change would be good energy savings. And as Don mentioned, no variants abstraction in Three.js. So handling in tools or apps side would sound reasonable.On the other hands, there are some concerns. Users who want to use the extension need to copy and paste the extension hander from the example to their applications. Or they need to be aware of the extension specification, learn the parser API, and write their own similar handlers.
Suggestion
What do you think of providing basic variant select function from the loader and implementing the handler as plugin? And also we keep holding the extension information in
.userData
for users want to write their variant select function? (And they can be used when exporting.)I tried it and open a PR as draft. I want you folks to review and to give the feedbacks especially about
API
Another option would be adding variant select helper static function as
GLTFLoader.Utils.selectVariant
or somewhere else.Plugin API change
Add invokeAll
beforeRoot()
andafterRoot(result)
hook points. They are especially good for handling extensions under the glTF root. I assume very basic use cases arebeforeRoot
for hacking gltf definitions andafterRoot
for hackinig generated objects.KHR_materials_variants
extension handler uses onlyafterRoot
but I thinkbeforeRoot
would be useful, too. I actually needed it when I tried VRM loader. Regarding the performance overhead, one hook point each before and after entire glTF file parse so I haven't measured the performance yet but I think it won't be big response time overhead.Add
addExtensionsToUserData
parameter to plugin. (Better name idea is welcome.) If it'strue
, extension information are saved underfoo.userData
as we do for unknown extensions.I think these additions would be worth even if we will end up concluding we won't implement the extension handler as plugin.
/cc @donmccurdy