three.js icon indicating copy to clipboard operation
three.js copied to clipboard

GLTFLoader: add arbitrary property animation through KHR_animation_pointer support

Open hybridherbst opened this issue 2 years ago • 11 comments

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

  • #24252
  • #24193
  • #24603

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: https://github.com/KhronosGroup/glTF-Sample-Models/pull/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.

  • https://github.com/BabylonJS/Babylon.js/issues/12544

This contribution is funded by Needle and prefrontal cortex

hybridherbst avatar May 23 '22 15:05 hybridherbst

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.

mrdoob avatar May 24 '22 07:05 mrdoob

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:

  • https://github.com/KhronosGroup/glTF/pull/2147

hybridherbst avatar May 24 '22 08:05 hybridherbst

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

takahirox avatar May 25 '22 08:05 takahirox

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 avatar May 31 '22 23:05 hybridherbst

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

  • https://github.com/mrdoob/three.js/pull/24193

hybridherbst avatar Jun 04 '22 21:06 hybridherbst

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

  • #24252
  • #24193

cc @donmccurdy @takahirox

hybridherbst avatar Oct 10 '22 10:10 hybridherbst

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.

donmccurdy avatar Oct 10 '22 20:10 donmccurdy

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.

hybridherbst avatar Oct 10 '22 21:10 hybridherbst

@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.

GitHubDragonFly avatar Jan 08 '24 21:01 GitHubDragonFly

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!

elalish avatar Jan 08 '24 22:01 elalish

@GitHubDragonFly the version on our latest branch (24128818278145748255e7ed8fb6ff083ec9da6b) 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.

hybridherbst avatar Jan 09 '24 22:01 hybridherbst

@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!

elalish avatar Mar 02 '24 00:03 elalish