Move loop-animation component to each object instead of scene

[keianhzo]

When two animation clips with the same name are targeted to different objects we are playing always the first one and the second one is never played.

This used to work fine in AFrame because we used to re-targe animations based on the object where loop-component was attached but with the bitECS migration we ignore the objects components and we attach one loop-animation component to the scene root so we can't re-target anymore as we have lost the reference of which objects play animations.

Note: This works fine in most cases but it fails when several objects are using the same animation name which seems to be pretty common (as I've seen this in different assets lately)

This PR addresses this issue keeping theloop-animation component attached to the original object so the animations can be correctly re-targeted.

As it's widely commented in the code, it makes sense to have only one loop-animation at the scene level that handles all the animations in the scene but that would require quite some refactoring to move the component to the scene level. What we could easily do is updating the Blender component to use the glTF animation index instead of the clip name as that one it's unique so we can at least do the component transform that we are doing and each clip would be correctly played in its related object. I'll open an issue to update the Blender component and start using that instead of the clip name.

Use this model for testing.
Frustum.glb.zip
Frustum.blend.zip

[takahirox]

Thinking whether we can convert from object-level loop-animation to scene-level loop-animation on the fly before parsing glTF content like #6121, or adding hack here.

https://github.com/mozilla/hubs/blob/051996a9e158a0bcff8c5ebb38590af92870fa3f/src/inflators/model.tsx#L141-L143
https://github.com/mozilla/hubs/blob/051996a9e158a0bcff8c5ebb38590af92870fa3f/src/inflators/model.tsx#L158-L167

As I wrote somewhere, I think scene-level is easier implement, more intuitive, and better maintainability and want to update loop-animation spec and Blender addon.

Honestly I'm very biased tho, I prefer scene-level loop-animation much.

[Exairnous]

Will a scene-level loop-animation handle objects dropped in-room correctly?

[keianhzo]

Will a scene-level loop-animation handle objects dropped in-room correctly?

Every dropped glTF is a scene and it will has it's own mixer to manage all the scene animations so it should work correctly.

Thinking whether we can convert from object-level loop-animation to scene-level loop-animation on the fly before parsing > glTF content like #6121, or adding hack here.

This will effectively be the same as you were doing just done at an earlier stage. I agree on that having a scene level loop-animation makes more sense but that would require some component redesigning to add some extra info. Now you can set different animation parameters like offset and timeScale so the scene component would need support to individually set those per animation.

Independently from where we place the component (scene or object) we should move the Blender component from using the animation name to using the index as now we have a problem with animations that use the same name as when we look for the animation among all the scene animations, we always pick same one as there is no way to tell which animation is the one targeted to a specific object so we end up re-targetting the first one to all the objects using that name. That would allow us to eventually simplify this:
https://github.com/mozilla/hubs/blob/31373a2c660e6a349a5548ff5de31bcdf1c9fce6/src/inflators/loop-animation.ts#L5-L19
and only use activeClipIndices for all.

[keianhzo]

If you feel this PR approach is correct for now we can focus on landing this and look into updating the architecture later to now delay the newLoader release anymore.

[takahirox]

I first want to understand the problem correctly that this PR want to resolve.

When two animation clips with the same name are targeted to different objects

Like this?

// glTF content
{
  "animations": [
    // Animation for node 1
    {
      "name": "foo_animation",
      "channels": [
        {
          "target": {
            "node": 1,
            ...
          },
          ...
        }
      ]
      ...
    },
    // Animation for node 2
    {
      "name": "foo_animation",
      "channels": [
        {
          "target": {
            "node": 2,
            ...
          },
          ...
        }
      ]
      ...
    }
  ],
  "nodes": [
    {
      "children": [1, 2]
    },
    {
      "mesh": 0,
      "extensions": {
        "MOZ_hubs_components": {
          "loop-animation": {
            "clip": "foo_animation",
            ...
          }
        }
      }
    },
    {
      "mesh": 1,
      "extensions": {
        "MOZ_hubs_components": {
          "loop-animation": {
            "clip": "foo_animation",
            ...
          }
        }
      }
    }
  ],
  ...
}

And the problem is that it is unclear what nodes[x]'s loop-animation.clip points to because loop-animation.clip is string and animations[n].names are not unique?

If so, honestly I still don't get how keeping loop-animation component at object (node) level allows retargetting... For example, in the example above, nodes[2] still doesn't know which one its loop-animation.clip points to, animations[0] or animations[1]?

[Exairnous]

@takahirox It depends. That's one way, another way has it combined together into one animation with multiple channels/samplers. Exporting multiple objects with the same action seems to result in what you have above, while exporting multiple tracks with the same name (for multiple objects) seems to result in the multiple channels/samplers. Here's a text based glTF file exported from Blender 3.6 to showcase this:
blender3.6-gltf-animation-export-test.zip

[keianhzo]

@takahirox When different objects have different animations with the same name in Blender they are all exported as different animations with same name in the loop-animation component clipproperty. (So the glTF structure that you mentioned in your last comment).

Then we look for the animation that matches the clip name here:
https://github.com/mozilla/hubs/blob/31373a2c660e6a349a5548ff5de31bcdf1c9fce6/src/bit-systems/loop-animation.ts#L33-L35

And we create the animation clips here:
https://github.com/mozilla/hubs/blob/31373a2c660e6a349a5548ff5de31bcdf1c9fce6/src/bit-systems/loop-animation.ts#L74-L80

As they all have the same name we always get the first AnimationClip from getActiveClips for all the animations. As the clip action is targeted to the scene object, we are effectively getting the same clip for each animation with the same name. As stated in the ThreeJS docs:

If an action fitting the clip and root parameters doesn't yet exist, it will be created by this method. Calling this method several times with the same clip and root parameters always returns the same clip instance.
So we are just running the the first clip.

With this PR we move the loop-animation component to the object level so when we call clipAction we retarget the animation with the same name (which tends to be the same animation) to all the different objects. This is what we are doing in AFrame.

As I mentioned before this behavior is NOT what we want, we don't want to re-target animations we want to run the right clip on the scene level mixer.

Giving this another thought another maybe simpler solution would be to add a pre-process stage to the GLTF loader and replace the "clip" field in the loop-animation component with the activeClipIndices field pointing to the animation indexes. We should only do that if the clipproperty exists as otherwise (Spoke exports for example) it would already being using indexes and that should be fine.

Let me know what you think and maybe we can take that approach better.

[takahirox]

Some comments, I'm confused...

1. As they all have the same name we always get the first AnimationClip from getActiveClips for all the animations.

Even if we move loop-animation to node (object) level, still the first clip is chosen, so, for example in #6153 (comment) animations[1] is for nodes[2] but even with node level loop-animation animations[0] is wrongly chosen, isn't it? Node level loop-animation doesn't resolve this wrong chosen clip problem, does it?

Do we assume that the same named animations have the same animation data? Is it a safe assumption?

2. With this PR we move the loop-animation component to the object level so when we call clipAction we retarget the animation with the same name (which tends to be the same animation) to all the different objects.

Honestly I still don't get why node level loop-animation allows animation retarget... In glTF animation target is specified as node and in Three.js animation target is specified as object uuid (more precisely in Three.js animation target can be specified as object name or uuid, but our gltf loader plugin forces to use uuid). In the Three.js clipAction target object is found with uuid. So, if we set optional root node to clipAction target object shouldn't be found (Assuming the original animation target object is not placed under the optional root node). What hack is used for animation retarget with node level loop-animation?

Update:

I found the hack. Here in Three.js the (optional) root node is used as animation target object if target object is unfound. If I'm right this is a hidden behavior so it may be dangerous to rely on it.

Actually this fallback is removed in the latest Three.js mrdoob/three.js#25329 I agree with removing the fallback as more straightforward Three.js API.

And probably animations may not be retargeted correctly in some cases. What I came up with so far are

Case 1: An animaton that targets multiple nodes will end up with retargetting only the optional root node.

// glTF content
"animations": [
  // animations[0] originally for nodes[2] and nodes[3]
  {
    "name": "foo_animation",
    // Multiple nodes can be targeted in channels
    "channels": [
      // channels[0] for nodes[2]
      {
        "target": {
          "node": 2,
          ...
        },
        ...
      },
      // channels[1] for nodes[3]
      {
        "target": {
          "node": 3,
          ...
        },
        ...
      }
    ]
    ...
  }
],
"nodes": [
  // nodes[0] expecting that animations[0].channels[0] is retargeted
  // to this node nodes[0] and animations[0].channels[1] is retargeted
  // to the child node nodes[1], but the both channels will be retargeted
  // to nodes[0] that is passed as optional root node to clipAction()
  {
    "mesh": 0,
    "children": [1],
    "extensions": {
      "MOZ_hubs_components": {
        "loop-animation": {
          "clip": "foo_animation",
          ...
        }
      }
    }
  },
  // nodes[1]
  {
    "mesh": 1,
  }
  ...
]

Case 2: If original target node is under the optional root node, the optional root node is not chosen as target node.

// glTF content
"animations": [
  // animations[0] originally for nodes[1]
  {
    "name": "foo_animation",
    "channels": [
      {
        "target": {
          "node": 1,
          ...
        },
        ...
      }
    ]
    ...
  }
],
"nodes": [
  // nodes[0], expecting that animations[0] is retargeted to this node
  // but in clipAction() it will be retarted to the original target node nodes[1]
  // because in clipAction() the target node is found by traversing the objects
  // from the optional root node. The original target node is found with uuid
  // so fallback (optional root node) won't be chosen as fallback.
  {
    "mesh": 0,
    "children": [1],
    "extensions": {
      "MOZ_hubs_components": {
        "loop-animation": {
          "clip": "foo_animation",
          ...
        }
      }
    }
  },
  // nodes[1], original animation target node
  {
    "mesh": 1,
    "extensions": {
      "MOZ_hubs_components": {
        "loop-animation": {
          "clip": "foo_animation",
          ...
        }
      }
    }
  },
  ...
]

Let me know what you think and maybe we can take that approach better.

I'm feeling more and more that the current loop-animation spec needs to be revisited as soon as possible because it may be problem prone. I prefer gltf pre-processing much. I will think how we can process properly next week.

[takahirox]

I guess perhaps we had assumption that animations that have the same name have the same animation data and animation target in a clip is only root node having loop-animation component. I'm not sure if it's a safe assumption but if we want to have backward compatilibity (for now) we may clone gltf.animation for each loop-animation node in the glTF preprocessing like

from

// glTF content
{
  "animations": [
    {
      "name": "foo_animation",
      "channels": [
        {
          "target": {
            "node": 0,
            ...
          },
          ...
        }
      ]
      ...
    },
    ...
  ],
  "nodes": [
    {
      "mesh": 0,
      "extensions": {
        "MOZ_hubs_components": {
          "loop-animation": {
            "clip": "foo_animation",
            ...
          }
        }
      }
    },
    {
      "mesh": 1,
      "extensions": {
        "MOZ_hubs_components": {
          "loop-animation": {
            "clip": "foo_animation",
            ...
          }
        }
      }
    }
  ],
  ...
}

to

// glTF content
{
  "animations": [
    {
      "name": "foo_animation",
      "channels": [
        {
          "target": {
            "node": 0,
            ...
          },
          ...
        }
      ]
      ...
    },
    {
       // Clone and rename
      "name": "foo_animation2",
      "channels": [
        {
          "target": {
            // Retarget to nodes[1]
            "node": 1,
            ...
          },
          ...
        }
      ]
      ...
    },
    ...
  ],
  "nodes": [
    {
      "mesh": 0,
      "extensions": {
        "MOZ_hubs_components": {
          "loop-animation": {
            "clip": "foo_animation",
            ...
          }
        }
      }
    },
    {
      "mesh": 1,
      "extensions": {
        "MOZ_hubs_components": {
          "loop-animation": {
            // Rename
            "clip": "foo_animation2",
            ...
          }
        }
      }
    }
  ],
  ...
}

The same animation data content are referred from original and cloned gltf.animations so probably additional memory cost is not a big deal. I would make a PR for this approach.

[takahirox]

Made a PR #6183

[keianhzo]

Closing in favor of #6183