@donmccurdy any opinion/objections here?

@hybridherbst the core specification states —

When node isn’t defined, channel SHOULD be ignored.

I'd support a change doing that, but the current PR causes the entire animation (and all channels it contains) to be ignored. Could we ignore just the affected channel?

I'm also fine with merging your other PRs instead, though I think there is an issue to be addressed in #24193 (comment).

That return was totally unintended, sorry for the delay – uses continue now.

Here's me loading one of our animation pointer sample models:

Regarding testing this more, I don't think a file with mixed animations (some animation pointer, some not) currently exists, but that may not be a blocker for this PR.

I think this change is good because target.node is optional in glTF core spec.

https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html#reference-animation-channel-target

@donmccurdy The spec describes that "When (target.node is) undefined, the animated object MAY be defined by an extension." If target.node is undefined and an extension defines it, should the extension be in extensionsRequired? Or not really because it is MAY not MUST?

The spec describes that "When (target.node is) undefined, the animated object MAY be defined by an extension." If target.node is undefined and an extension defines it, should the extension be in extensionsRequired? Or not really because it is MAY not MUST?

I think the extension itself could decide one way or another here, e.g. to mandate extensionsRequired for itself. But from a core spec perspective, the file is valid whether or not an extension provides the target.

An open question would be whether to return "null" AnimationClips, thus potentially breaking external code that assumes that whenever an animation is loaded an AnimationClip is returned, or returning a clip with no tracks:

I started to feel that it would be good to return a clip with tracks with empty target name.

If target.node is undefined, the target node is expected to be specified with a certain extension in general case.

Returning a clip with tracks with emptry target name will help the extension handlers like

// a certain extension plugin
async loadAnimation(animationIndex) {
  const animationDef = this.parser.json.animations[animationIndex];
  const extensionDef = animationDef.extensions[this.name];
  const clip = await this.parser.loadAnimation(animationIndex);
  for (const track of clip.tracks) {
    track.name = this._getTargetName(extensionDef) + track.name;
  }
  return clip;
}

Another option may be adding a new hook for animation target like getAnimationTarget() or getAnimationTargetName(). We may not want to add fine grained hooks because it may end up with having tons of fine grained hooks, but in this case it might be acceptable because the spec describes that the target node may be specified by a certain extension so not adding a new random hook point.

@takahirox the animation hooks that would be needed for KHR_animation_pointer are the ones in

• GLTFLoader: add hooks for animation #24193

I just didn't get to respond yet...

Rebased on latest, I think this PR was already approved - would be great to get it merged :)