GLTFLoader: add hooks for animation

[hybridherbst]

Related issue:

• GLTFLoader: add arbitrary property animation through KHR_animation_pointer support #24108

Description

This PR adds callbacks for extensions to change how animation data from glTF files is parsed, in preparation for KHR_animation_pointer.

Two new callbacks are added:

• loadAnimationTargetFromChannel( channel )
• createAnimationTracks( node, inputAccessor, outputAccessor, sampler, target )

Additionally, getDependency('light') was missing - I can make a separate PR if wanted, it's currently part of this PR.

This contribution is funded by Needle

[hybridherbst]

@takahirox @donmccurdy let me know if you have feedback here :) this enables at least putting KHR_animation_pointer into takahirox' extensions repo but will also enable putting it into three.js directly if wanted.

[takahirox]

Thanks for working on it. Let me review when I have time.

[hybridherbst]

Looking forward to feedback here 👀

[takahirox]

I will try this weekend. Thanks for the patience.

Additionally, getDependency('light') was missing - I can make a separate PR if wanted, it's currently part of this PR.

Yes, separating it from this PR is good to me.

[takahirox]

What I first want to think of is whether we really need new fine grained hook points for animation because we already seem to have a coarse grained hook loadAnimation(). Would you mind if clarifying what the exisiting loadAnimation() hook doesn't resolve?

[hybridherbst]

@takahirox I recommend you take a look at how these hooks would be used from

• GLTFLoader: add arbitrary property animation through KHR_animation_pointer support #24108

With only coarse hooks, hundreds of lines of code would need to be duplicated. With animation_pointer, what changes is how the pointer is resolved, not how all the logic of waiting for buffers, dependencies, etc. works.

[hybridherbst]

Rebased this on dev and and removed the getDependency( 'light' ) part since that is now its own PR:

• GLTFLoader: add getDependency( type, index ) implementation #24252

Looking forward to more feedback, we're eager to finally bring KHR_animation_pointer to three and this is a prerequisite :)

[donmccurdy]

@hybridherbst given my comment in #24108 (comment), and not wanting to provide the implementation of KHR_animation_pointer out of the box in three.js unless/until the extension appears close to ratification ... are the hooks here sufficient to allow you to implement KHR_animation_pointer externally? Or do the helper functions provided here need to be exposed on the parser instance?

Other example of external extensions, by @takahirox, here — https://github.com/takahirox/three-gltf-extensions#how-to-use

[hybridherbst]

@donmccurdy that's what I meant with my comment above yours :) if #24252 and #24193 are merged then the rest works as an external extension for now, and at least people can start using it.

[donmccurdy]

@hybridherbst sorry if i'm missing something: the getArrayFromAccessor and createCubicSplineTrackInterpolant functions appear to be private to the GLTFLoader.js module. They're used by #24108, and I can't see how an external extension would access them. Is that a problem?

[hybridherbst]

@donmccurdy sorry for the delay, finally got around to rebasing this branch. You're right that these two would need to be accessible for the KHR_animation_pointer extension to work "standalone". What would be the best way to do that? Moving them into GLTFLoader so that no extra methods need to be exposed?

[donmccurdy]

@hybridherbst I think we could make them methods on the internal GLTFParser class, the extension will have access to that. @takahirox does that sound OK?

[hybridherbst]

OK, moved them there and adjusted the KHR_animation_pointer branch too. Still have to test if it's now fully "externalizable" but looks good. Thanks!

[takahirox]

@donmccurdy

@takahirox does that sound OK?

It would sound ok to me. I might want to revisit how to expose such utility functions later tho (for example, do they really need to be the methods of the parser? Can't they be exported just as functions?).

By the way, what do you think of #24193 (comment) ?

I'm not sure if it is good to add new fine grained hooks just for removing duplicated code from a specific extension handler. If we keep doing it, the parser may end up with having a tons of fine grained hooks.

A few thoughts.

• Should the coarse grained hook (loadAnimation() in this case) be removed if fine grained hooks are really needed and we add them?
• To avoid duplicated code in an extension handler, is it worth to think of exposing utility functions that can be used from the extension handler instead of adding new fine grained hooks?

Any thoughts?

[donmccurdy]

@takahirox I don't have much preference on whether utility functions are members of the GLTFParser class, or separate exports. I tend to think of the parser as being the API we provide to extensions, and exports as being the API for end-users, but I'm fine with either here.

I agree it would be better if we could use coarser hooks (e.g. loadAnimation) instead of finer (createAnimationTracks, loadAnimationTargetFromChannel) from a maintainability perspective... @hybridherbst do you think that would be a little duplication, a lot of duplication, or just not possible?

[hybridherbst]

From my perspective I think the coarse hook can stay, there may be other uses for that - like someone entirely replacing how animation is loaded.

I do think KHR_animation_pointer is a bit special because it doesn't really change how animation works, just where it comes from and where it's targeted to, so it benefits from the finer hooks by not having to duplicate hundreds of lines of code. I think ultimately the granularity is kind of defined by how people define glTF extensions - so in the future there might (but not necessarily) be other attachment points needed. Hard to predict...

I had also commented on that here right after your question @takahirox: #24193 (comment)

[donmccurdy]

At least speaking for myself – GLTFLoader's complexity is already very near the limit of what we want to maintain as a loader in this repository. Each hook often needs to be called at a specific point in the loading process, so new hooks further limit how we can refactor the loader in the future. I think we are hoping to avoid adding more hooks, if the resulting duplication is modest enough.

[hybridherbst]

I agree with that point! I tried to find a good balance here between not introducing too many things and enabling a powerful external extension that would otherwise result in very-much-not-modest duplication. What is your suggestion?

[hybridherbst]

I'd really like to bring KHR_animation_pointer to three, even when just as external extension for now 🥲
https://twitter.com/babylonjs/status/1641485443375177728 (they're even using the sample files I made for demonstrating it in three.js!)

[hybridherbst]

@donmccurdy on my recent counting it would be ~300 lines of duplicated code, 80% of that in the current loadAnimation method. Do you find that acceptable? As someone potentially maintaining the GLTFAnimationPointer extension for the time being I must say that's not great...

With the callbacks above, the extension is ~400 lines and potentially merge-able; without them, the extension is ~750 lines and doesn't feel mergeable anymore as it duplicates so much code.

EDIT: Trying to actually decouple this and I'm running into more duplication. All the interpolators are also internal to GLTFLoader.js. That's another 100 lines.

[donmccurdy]

• cc GLTFLoader: Refactor animation methods to be accessible from extensions on parser #26126