Remove EntityPath and use Entity as a direct id

[the10thWiz]

Remove EntityPath

Right now, only the animation code uses EntityPath, and it feels like a hacky solution. Rather, by re-ordering the gltf loading code, AnimationClips can actually reference the underlying entity by Entity.

This also significantly improved performance, since it skips looking up by entity path every frame, as well as avoiding calculating the entity paths while loading.

================

Original version

Objective

Looking up an entity by name is somewhat difficult, and only done within bevy_animation. The actual code is difficult to read, and outside crates also have a need to lookup entities by name. To facilitate an easier interface, I've created a NameLookup type (which implements SystemParam), and provided a few methods for looking up entities.

Motivation

I'm currently working on a Inverse Kinematics solver for Bevy, which requires the ability to lookup bones in an armature by name (in the same way an animation does).

Solution

I've provided an implementation of a NameLookup type, which implements SystemParam.

The interface is roughly:

impl NameLookup {
    pub fn lookup_any(&self, path: &EntityPath) -> Result<Entity>;
    pub fn lookup(&self, root: Entity, path: &EntityPath) -> Result<Entity>;
}

This also avoids the need to use loop labels (sort of a goto), be refactoring into multiple methods.

There is a note in the current code that lookup by EntityPath can be optimized for better performance. By refactoring it into an independent method, we isolate the lookup itself, and allow benchmarking & testing of the lookup code independent of the rest of the animation logic. Because this is also a public type, external crates can also benefit from performance improvements. Finally, this is also a unified place to implement caching (if possible), to avoid repeated lookups.

[the10thWiz]

I'm not sure if animations is the right place to put this, since it might belong in core or ecs (given that is could be used more generally, even if animations aren't enabled).

[alice-i-cecile]

Yep, I'd put this in core :)

[mockersf]

I'm not sure if animations is the right place to put this, since it might belong in core or ecs (given that is could be used more generally, even if animations aren't enabled).

In or next to https://github.com/bevyengine/bevy/blob/main/crates/bevy_core/src/name.rs please!

I would also like a few tests on those functions.

[the10thWiz]

After some analysis, I think it might be possible to remove EntityPath altogether. My initial assumption was that the EntityPath matched the gltf format, but after looking into the format, it doesn't appear to be the case. Rather, by reordering some code within the GLTF file loader, it's actually possible for AnimationClip to store the actual Entity id, rather than the path. This also provides support for animating nodes without names (since we don't need them). This also removes the need for external crates to lookup by path, since AnimationClip would provide the actual id, rather than the path.

[alice-i-cecile]

Rather, by reordering some code within the GLTF file loader, it's actually possible for AnimationClip to store the actual Entity id, rather than the path. This also provides support for animating nodes without names (since we don't need them). This also removes the need for external crates to lookup by path, since AnimationClip would provide the actual id, rather than the path.

Fantastic, I like the sounds of that direction. Can you make sure to update the PR description and title to match the actual work done in the PR when you're ready?

[mockersf]

it's actually possible for AnimationClip to store the actual Entity id, rather than the path

That's actually not possible because an AnimationClip can be applied to several entity hierarchies, which is why we are using path with names.

The scenario you're describing would only work for an animation on only one scene, but would break if one was to spawn the same scene multiple time and reuse the same animation. Also, it's often the case to distribute animations and models in different gltf files, in which case there are no knowledge shared of entities.

[the10thWiz]

it's actually possible for AnimationClip to store the actual Entity id, rather than the path

That's actually not possible because an AnimationClip can be applied to several entity hierarchies, which is why we are using path with names.

The scenario you're describing would only work for an animation on only one scene, but would break if one was to spawn the same scene multiple time and reuse the same animation. Also, it's often the case to distribute animations and models in different gltf files, in which case there are no knowledge shared of entities.

I'm not sure how to distribute animations and models between multiple gltf files, especially since the gltf format uses indicies internally. I guess maybe you could duplicate the bones into a separate files, but I'm not sure how common this actually is.

I'm not a fan of the current name based approach, and I don't think applying the same animation to several entity hierarchies is necessarily worth while. That being said, it is something I'm going to have think about.

Spawning the same scene multiple times is a major issue though. I see things like SkinnedMesh work because they are components (so they can update to point at the new entities). It might be interesting to move the AnimationPlayer to be on the SceneInstance, which holds the internal references within the scene. This is going to require some more thought. This might also protect against applying an animation clip to the wrong player, which currently just results in a console warning.

Overall, I don't like the name approach, and I would like to come up with a better option. I'm going to mark this as a draft for now, but I'd like to tackle this.

[mockersf]

You should look at https://github.com/bevyengine/rfcs/pull/49 (and https://github.com/bevyengine/rfcs/pull/51 while you're there)

[the10thWiz]

After careful reflection, I've chosen to revert back to my original goal with this PR, and open a new PR when and if I have a more complete solution. For now, I'm looking at contributing to the discussion around the RFCs listed above.

[mockersf]

CI seems to be lost with your force pushes. Could you push a new commit to trigger it? And put the PR out of draft if you're happy enough with its state 🙂

[the10thWiz]

CI seems to be lost with your force pushes. Could you push a new commit to trigger it? And put the PR out of draft if you're happy enough with its state 🙂

At this point, I'm still looking into writing tests, so it's not quite ready. I think the actual issue is that my branch is out of date, and CI is does a merge with master before running tests.

[james7132]

@the10thWiz do you have the time to drive this PR to completion? This would be super useful in simplifying our animation code, and I'd like to see it completed. If not, we can mark this PR up for adoption, your commit history and credit to you will be retained.

[james7132]

Removing the Animation label since animation is no longer reliant on name lookups after #11707.