Skip to content

GLTFLoader: add arbitrary property animation through KHR_animation_pointer support#24108

Closed
hybridherbst wants to merge 3 commits intomrdoob:devfrom
needle-tools:khr-animation-pointer
Closed

GLTFLoader: add arbitrary property animation through KHR_animation_pointer support#24108
hybridherbst wants to merge 3 commits intomrdoob:devfrom
needle-tools:khr-animation-pointer

Conversation

@hybridherbst
Copy link
Copy Markdown
Contributor

@hybridherbst hybridherbst commented May 23, 2022

The following PRs are rolled into this one and ideally are merged first:

Description

This PR adds support for KHR_animation_pointer. We wanted to put this out early for discussion. There's a good amount of refactoring to be done (especially figuring out how to do property binding in a clean/extensible way, and how to deal with animated cameras/lights/materials that three might duplicate in certain cases).

three.js fork with built drag-and-drop viewer can be found here:
https://needle.tools/three.js/examples/?q=loader_mu#webgl_loader_multiple

Some sample models are here:
KhronosGroup/glTF-Sample-Models#353

To quickly test, just download some of the GLB files from the sample-models fork and drop them into the three-js fork above :)

Known Limitations

  • If lights or cameras are used on multiple nodes, three.js duplicates those. This currently isn't supported here. When animating lights/cameras is wanted, each node should have a unique light/camera.
  • Three.js only allows texture transforms on map and aoMap, not on other properties. This means they can be animated but don't have an effect. Custom shaders can still use those properties, so I think it's valuable to still import their animations (this is incomplete right now).
  • Materials might be duplicated depending on what features a particular mesh has (with/without vertex colors, normals, tangents). Tracks would need to be duplicated in this case as well to point to those new materials.

Next steps
Before doing the remaining implementation (which mostly consists of mapping glTF JSON Pointer strings to three.js property names), I think a couple of points are worth discussing:

  • currently PropertyBinding.findNode is monkey patched to support materials and node uuids. It may be that custom extensions need to have callbacks into this (e.g. KHR_animation_pointer wants to resolve materials). I'm unsure if findNode itself should be extended to support more cases or a callback mechanism be introduced (I'd prefer the callback).
  • lights currently don't work, getDependency doesn't have 'lights' right now (another candidate for a callback for findNode).
  • naming-wise it's a bit weird that pendingNodes may now contain other things than nodes (e.g. materials). Should we rename this to pendingDependencies?
  • given that at some point three.js may also want to export with KHR_animation_pointer, I'm unsure of where all the path and property remapping should happen. I guess for now import is good to tackle first to find all edge cases and then later talk about export.
  • some properties in three.js work differently - e.g. Light.penumbra is calculated from inner and outer spot angle. Not sure what the right approach would be during animation. Precalculating this as array seems rather complicated (inner and outer spot could have separate keyframe times etc.).
  • Light.angle also needs to be converted from rad to deg, which can probably be done when creating the Track array.

cc @donmccurdy @elalish @mrdoob and probably others :) I'm really looking forward to having support for this, opens up many possibilities.

Babylon and Gestaltor seem to have also started implementations.

This contribution is funded by Needle and prefrontal cortex

@mrdoob mrdoob added this to the r??? milestone May 24, 2022
@mrdoob
Copy link
Copy Markdown
Owner

mrdoob commented May 24, 2022

The name of the extension is kind of confusing.
At first I thought it was a way of making gltf elements react to the pointer.

@hybridherbst
Copy link
Copy Markdown
Contributor Author

hybridherbst commented May 24, 2022

Fully agree – it has been renamed a couple of times already and the name hasn't necessarily gotten better.
In case a good name comes to mind for "mostly arbitrary glTF property animation", please comment here:

@hybridherbst hybridherbst changed the title GLTFLoader: add KHR_animation_pointer support GLTFLoader: add arbitrary property animation through KHR_animation_pointer support May 24, 2022
Copy link
Copy Markdown
Collaborator

@donmccurdy donmccurdy left a comment

Choose a reason for hiding this comment

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

🎉 Thanks for opening this PR @hybridherbst! We'd likely want to wait on merging anything until the extension is ratified, or nearly so. It's helpful to see how this might look for discussion in the meantime.

The features required by KHR_animation_pointer are pretty complex to support (nothing wrong with this PR, it is just a very complex extension in practice) and so I'll flag a few likely issues in comments below.

const pathStart = path.substring( 0, pathIndex );
targetProperty = path.substring( pathIndex );

// TODO implement all properties and/or find a better way to have a good mapping here
Copy link
Copy Markdown
Collaborator

@donmccurdy donmccurdy May 24, 2022

Choose a reason for hiding this comment

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

Supporting all properties may be a non-goal for three.js I think. Especially as animated extension properties come into the mix. We can try to support whichever properties seem helpful to three.js users, and are not overly complex to include.

No need to change these in the draft PR now, but a few things to flag:

  • alphaCutoff animation can trigger shader recompilation
  • we have very limited support for UV transforms, all textures except aoMap must share the same transform. I wonder if we should avoid animating those properties until that is improved.
  • metallicFactor seems unlikely to be animated much?

targetProperty = 'morphTargetInfluences';
break;

// TODO matrix
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think animation is required to target TRS properties, not a matrix?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good question. @UX3D-nopper is /matrix intentionally included in KHR_animation_pointer?

targetProperty = 'far';
break;
case 'aspect':
break;
Copy link
Copy Markdown
Collaborator

@donmccurdy donmccurdy May 24, 2022

Choose a reason for hiding this comment

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

Similar to above, will have to call camera.updateProjectionMatrix() after any of these changes.

I'm not sure what I think about animation of the aspect ratio. glTF lacks any meaningful way to express "let the camera inherit the aspect ratio from the application", animating this property kind of seems like stacking blocks on top of something that is already not working properly.

if ( useExtension )
nodeDependency = this.getAnimationPointerDependency( target );
else
nodeDependency = this.getDependency( 'node', name );
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

One of the larger hurdles on this extension will be that glTF definitions do not translate 1:1 into three.js objects —

  • Materials will be cloned depending on the context in which they're used. The same glTF material applied to (1) points, (2) lines, (3) mesh with vertex colors, (4) mesh with vertex normals, (5) mesh with vertex tangents, will result in 5x copies of the material.
  • Lights and Cameras will be cloned if they're attached at >1 Node.
  • Probably similar for KHR_audio if that gets ratified?

Copy link
Copy Markdown
Contributor Author

@hybridherbst hybridherbst May 31, 2022

Choose a reason for hiding this comment

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

Do you have an idea on how to tackle these in the context of this extension? I'd assume that when they are split, the correct additional tracks would need to be created.

I first thought one could be keeping some importUUID around, which could then be used during findNode to map back to objects, but I don't think one track can target multiple objects (by design).

parts[ 2 ] = node.uuid;
animationPointerPropertyPath = parts.join( '.' );
if ( animationPointerDebug )
console.log( node, inputAccessor, outputAccessor, target, animationPointerPropertyPath );
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The trouble with targeting animation to the UUID is that many users will clone the model before attaching one or more copies to their scene. The cloned objects will have new UUIDs, and so the same animation clips cannot target them. We prefer to target based on unique names for that reason, but glTF doesn't require unique names. And in this case (see above) we may need to animate multiple objects, even if only a single source glTF definition was animated. 😕

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Switched to node.name ?? node.uuid, similar to other places.

@takahirox
Copy link
Copy Markdown
Collaborator

Hi, thanks for working on it.

I didn't look into the spec and change deeply yet, but let me share my rough thoghts so far.

  • If the extension doesn't greatly fit to Three.js API, it may be good to consider partial support
  • If the extension may not be so popular, the implementation can be too hacky, or the extension doesn't fit to Three.js API well, it may be good to make the extension handler as a plugin and release at an external repository like this

};

// HACK monkey patching findNode to ensure we can map to other types required by KHR_animation_pointer.
const find = PropertyBinding.findNode;
Copy link
Copy Markdown
Collaborator

@takahirox takahirox May 25, 2022

Choose a reason for hiding this comment

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

I don't think monkey patching is a good idea. Maybe good to consider either

  1. Update the core findNode
  2. Give up the perfect extension support and consider partial feature support that the existing findNode can handle
  3. If we really need monkey patching, it may be good to do it only if KHR_animation_pointer extension is used

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The existing findNode can't handle most of what animation_pointer requires unfortunately. I agree that it would be better to update findNode as mentioned in my original post, looking forward to ideas which parts should move there. Ideally there's a callback that for example extensions can tap into.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I changed the implementation to only monkey patch if the extension is actually used.


let nodeDependency = undefined;
if ( useExtension )
nodeDependency = this.getAnimationPointerDependency( target );
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It may be good to consider the plugin system for handling a extension. It provides an extensibility while it keeps the simplicity for processing the glTF core in the loader unless the extension handler really needs to be in the loader core. If the current Plugin API doesn't fit to process the extension, please consider to propose a good new API for handling animation extension.

Copy link
Copy Markdown
Contributor Author

@hybridherbst hybridherbst May 31, 2022

Choose a reason for hiding this comment

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

Yes, as written in my original message that was the plan.
I now refactored it to be contained entirely in an animation, and added two new callbacks, see

@hybridherbst hybridherbst force-pushed the khr-animation-pointer branch from 9743045 to 7e0d1de Compare May 30, 2022 12:18
@hybridherbst
Copy link
Copy Markdown
Contributor Author

hybridherbst commented May 31, 2022

I believe the support added here is now "feature complete" for the core spec and all currently ratified extensions (minus some texture transforms that three doesn't support anyways). There's still open questions regarding the currently required use of PropertyBinding/monkey patching, and what to do when materials and/or lights/cameras are duplicated during import (and would thus require creating multiple tracks). Looking for input on these!

I refactored everything into a self-contained extension (GLTFAnimationPointerExtension), and had to add two new callbacks for it:

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

@hybridherbst
Copy link
Copy Markdown
Contributor Author

Made a separate PR with the required callbacks to allow for KHR_animation_pointer:

@hybridherbst
Copy link
Copy Markdown
Contributor Author

hybridherbst commented Oct 10, 2022

The PRs that would need to be merged so KHR_animation_pointer can at least be used as an external extension would be:

cc @donmccurdy @takahirox

@hybridherbst hybridherbst marked this pull request as ready for review October 10, 2022 10:04
@donmccurdy
Copy link
Copy Markdown
Collaborator

I think I would prefer that we exclude KHR_animation_pointer from the threejs repository unless/until it has started the ratification process with Khronos. I'm not aware if it's more than a proposal at this stage. Adding hooks or other extension API changes required to support the extension externally, as in the other PRs, would be welcome.

@hybridherbst
Copy link
Copy Markdown
Contributor Author

Understood. You are right that it's currently just a proposal, however, both Gestaltor and Babylon have already implemented it and there's multiple sample assets that I made. I'll ask what the blocker is for moving forward.


}

if ( targetId === null || isNaN( targetId ) ) {

Check warning

Code scanning / CodeQL

Comparison between inconvertible types

Variable 'targetId' cannot be of type null, but it is compared to [an expression](1) of type null.
@hybridherbst hybridherbst force-pushed the khr-animation-pointer branch 2 times, most recently from b7504d6 to 3e5ac4c Compare March 28, 2023 17:24
hybridherbst and others added 3 commits May 23, 2023 23:53
add some TODO comments, rename pointer path property
wip support for camera and light properties, added morph target support
formatting, fix alphaTest, add opacity track splitting
added some comments, added some material ext animation properties
fixed Ior > IOR, test switching to node names to avoid duplication problems (but incomplete yet, needs full paths)
fix: use "pointer" property instead of "path"
refactor: move to extension, add new callbacks
move remaining pieces into extension, rename methods
fix: orthographic/perspective camera, pbrMetallicRoughness prefix, clearcoat/sheen/specular extensions
rm change on PATH_PROPERTIES
only patch PropertyBindings if KHR_animation_pointer is actually used
fix: fall back to original findNode implementation earlier if special cases don't yield results
introduce animationPointerResolver to allow configuring KHR_animation_pointer from outside
fix: glTF camera fov is in radians, not degrees
fix: use QuaternionKeyframeTrack instead of ColorKeyframeTrack if property ends with .quaternion
use extension hooks
@GitHubDragonFly
Copy link
Copy Markdown
Contributor

@hybridherbst just as an FYI, on my end I have dared using your GLTF Loader version from this PR in my GLTF Viewer and it seems to be working fine for the Animate All The Things example that you have available for testing.

Whenever you might get back to dealing with this arbitrary extension, and in addition to the warning listed by the bot above, there is one other thing to correct related to CUBICSPLINE comparison on lines 1300 and 4921, where interpolation should be replaced with sampler.interpolation.

Khronos InterpolationTest example shows this correction as necessary to be fully functional.

@elalish
Copy link
Copy Markdown
Contributor

elalish commented Jan 8, 2024

The good news is the glTF interactivity WG is now pushing KHR_animation_pointer as a dependency for the interactivity extension. It should be going through the ratification process shortly, which will require implementations like this. Thanks @hybridherbst!

@hybridherbst
Copy link
Copy Markdown
Contributor Author

@GitHubDragonFly the version on our latest branch (2412881) should be better and lilkely already has that fix.

@elalish happy to hear – let me know when a good time would be to pick up this PR again and bring it up to speed with our latest changes.

@elalish
Copy link
Copy Markdown
Contributor

elalish commented Mar 2, 2024

@hybridherbst I think now is great - did you see that KHR_animation_pointer is going through ratification now? So there are already two implementations out there somewhere. Would be awesome to have in Three!

@mikeskydev
Copy link
Copy Markdown
Contributor

So, KHR_animation_pointer has been ratified for nearly a year! What is needed to get this over the line, or at least allow it to slot in neatly as a plugin?

@arpu
Copy link
Copy Markdown

arpu commented Jul 18, 2025

@hybridherbst anything we need to do to get this merged?

@takahirox
Copy link
Copy Markdown
Collaborator

Since the Animation Pointer extension has been ratified, I reviewed the changes again, but a mechanism that dynamically hacks the Three.js core code is still difficult to accept from a maintainability standpoint. As I proposed before, we should first update the Three.js core code and make the Three.js core Animation System more flexible so that it can handle a wider variety of properties across various object types, and make it possible for the Animation pointer plugin to support this extension in a simple way without hacking the Three.js core code; otherwise, I think adoption as a built-in plugin will be difficult.

I was also thinking about something a little different. This Animation Pointer extension can make the properties of various other extensions targets of animation, right? That led me to consider which plugin should manage the animation of the properties of such other extensions. Because how to apply a glTF extension on Three.js depends on its plugin, perhaps each extension’s plugin should handle the animation.

If we are to support the Animation Pointer extension as built-in, we should support animation only for the glTF core and for the extensions handled by the glTF loader built-in plugin, and we should provide a general mechanism, either in the Three.js Core or in this Animation pointer extension plugin, that makes it easy for third-party plugins to handle animation as well.

@Mugen87
Copy link
Copy Markdown
Collaborator

Mugen87 commented Aug 30, 2025

In the meanwhile, developers can use a third-party plugin @needle-tools/three-animation-pointer for GLTFLoader. It's demonstrated in a new example added via #31761.

@takahirox
Copy link
Copy Markdown
Collaborator

Yes, I had noticed that PR! It reminded me of this one, so I went back and reviewed it.

Now that we can handle the Animation Pointer extension by using that external library, and since making it a built-in plugin would probably require major changes from this PR, it might be best to delete this PR for now and submit a new one with the appropriate changes if it becomes necessary, if @hybridherbst has no objections.

@hybridherbst
Copy link
Copy Markdown
Contributor Author

since making it a built-in plugin would probably require major changes from this PR

This PR still contains mostly the changes that are now in the package. Of course if you feel its simpler to start a new PR we can do that. I'd still love to see proper support in three at some point, and yes, it means the animation capabilities need to be extended – the PR patches that in, but that could be core changes since it improves animation use in general.

@Mugen87
Copy link
Copy Markdown
Collaborator

Mugen87 commented Sep 1, 2025

it means the animation capabilities need to be extended – the PR patches that in, but that could be core changes since it improves animation use in general.

In this case, I suggest we start with a new PR that only extends the core so it's ready for the glTF extension. We can demonstrate the new capabilities independently of glTF in misc_animation_keys.

A second PR can then integrate the extension. Would that be okay for everyone? I believe having two PRs makes the code easier to review.

@takahirox
Copy link
Copy Markdown
Collaborator

I agree. Splitting the PRs for the Core Animation system and the glTF plugin will definitely make the review easier, and the Core Animation system should be reviewed thoroughly on its own.

Also, as noted above, the Animation Pointer plugin might need to provide a mechanism that allows Animation Pointer to be used for other extensions. Depending on how we support other extensions, it might be more appropriate to manage it as a third-party library rather than built-in, so the glTF plugin should also be reviewed carefully.

Splitting the PRs will make these considerations and discussions easier.

@Mugen87 Mugen87 closed this Sep 1, 2025
@Mugen87 Mugen87 modified the milestones: r???, r180 Sep 1, 2025
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.

9 participants