Add SCNAnimationPlayer support for GLTF animations.

[lenkawell]

Add SCNAnimationPlayers for each GLTF animation so the loaded SCNScene is like scenes loaded from .scn, .dae and .usdz files.

[lenkawell]

Is this pull request usable, or is there something I should do to fix it? I've been using it in our Beta app for a few months and it seems to work nicely and provides compatibility with SceneKit's handling of animations in .scn and .usdz models.

[lenkawell]

Here's a movie of a simple scene that includes .scn, .usdz and .glb animated models. The .glb models are loaded with the latest GLTFKit2 plus this animation pull request. https://user-images.githubusercontent.com/1200965/166303081-51ee7a7e-12e5-477f-ac3b-e8614d127f08.mov

[warrenm]

Just now getting around to reviewing this seriously. I have several misgivings about this PR as authored.

First, it's not orthogonal to the existing GLTFSCNAnimation API. It supports a different subset of animation types and is used in a different way. In particular, it sets animation players on nodes at load-time, but there's no way to later discover those animations by name or the nodes to which they apply.

I appreciate that there are some scenarios where a well-known node (often the root of the armature/skeleton) acts as the Keeper of the Animations, but if there isn't, a user needs to visit the whole node hierarchy to discover animation players. I'd like to unify this with the existing API somehow. Explicitly, I'd like to make it possible to look up animation players by animation name on an asset.

Second, indiscriminately renaming nodes so that they can be targeted with KVC syntax makes it impossible to search for those nodes using the names serialized in the glTF asset, which is both a breaking change and a potential cause of user confusion. Specifically, it broke the Project Polly model, which has dots in node names. This is a perfectly reasonable configuration, and DCCs like Blender often use dots in node names to distinguish right/left counterparts.

Furthermore, using KVC-style animation key paths for all keyframe animations breaks the existing animation mechanism, since GLTFSCNAnimation expects animation key paths to refer directly to their target's properties rather than being prefixed with "/".

I respect and appreciate that these changes helped you meet the goals of your own project, but as written, I can't incorporate them. Having said that, there are some issues with the existing animation system that these changes brought to my attention, so I'll try to get around to making some improvements soon. I hope down the line we can find a solution for animation lookup and playback that is useful to an even broader audience.

[lenkawell]

I agree with you that the method of finding animations by enumerating the nodes in the graph is not a great design in SceneKit. Unfortunately it's also the only technique that I can find to use when it loads .scn, .usd, .usdz, etc. scene files. SceneKit used to have somewhat more elegant way to do it (entry methods on SCNSceneSource), but when they added SCNAnimationPlayers, it seems to me like they simply stopped supporting the previous method.

So while it's ugly, it's compatible with how I see Model I/O and SceneKit doing it for the other types of scene files. Our app is therefore able to use the same animation handling code regardless of the type of model scene file, now including GLTF/GLB. A GLTF-specific method would not be a great solution for us, especially considering the amount of animation code we have.

I also agree with you that renaming the nodes so they are KVC compatible is ugly, but again, I believe it's required to reasonably use the animations. An app like ours makes significant use of that KVC animation handling. I agree it's bad when nodes in the scene graph are renamed from what the designer picked, but again unfortunately, I have not found a way around it. I think most importers, including if I'm not mistaken, Apple's RealityConverter, rename the nodes to make them fully useable in RealityKit and SceneKit.

Maybe I'm not understanding how SceneKit animations are supposed to be used, but I've not found any documentation or sample code that seems to have anything different from what we're doing.

[warrenm]

I believe this PR is superseded by 049bf60. I have adopted a lot of your ideas around renaming nodes so they're compatible with animation key-path syntax and using SCNAnimationPlayer. Scene sources now have an array of GLTFSCNAnimations, each of which holds the original name of the animation and allows iterating animations in the order they appear in the glTF asset.

Thanks for the contribution and discussions, and for your patience. I hope this change isn't too disruptive if you choose to adopt it downstream.

[lenkawell]

I've tested this and it seems to work OK in our app. It took me a while to get it working though because I was expecting the SCNAnimationPlayers to be not just in the GLTFSCNSceneSource animations array but also (optionally?) added to nodes in the scene like SceneKit does for other asset files and RealityConverter does for GLTF to USDZ conversion.

I added code like this to our app and it seems to work like other asset file types:

// Add animation players for any animations in the scene. Add to first node with a skeleton or to the root node. var skeletonNode: SCNNode? = nil scene.rootNode.enumerateHierarchy({node, stop in if let skeleton = node.skinner?.skeleton {skeletonNode = skeleton; stop.pointee = true}}) let playerNode = skeletonNode ?? scene.rootNode for animation in sceneSource.animations { playerNode.addAnimationPlayer(animation.animationPlayer, forKey: animation.name) }