Split AnimationTarget into two components

[greeble-dev]

Objective

Add flexibility by refactoring AnimationTarget into two separate components. This will smooth the path for future animation features.

Background

bevy_animation animates entities by assigning them AnimationTarget components:

struct AnimationTarget {
    player: Entity,
    id: AnimationTargetId,
}

• player: Entity links to an entity that contains an AnimationPlayer component. An AnimationPlayer plays AnimationClip assets.
• id: AnimationTargetId identifies which tracks in an AnimationClip apply to the target entity.

When loading a glTF these components are automatically created. They can also be created manually.

Problem

The two parts of AnimationTarget often go together but sometimes would be better separated:

1. I might want to calculate the AnimationTargetId first, but not link it up to an AnimationPlayer until later (see 🧪 Bone attachments #18262 for an example).
2. I might want to use AnimationTargetId but not use AnimationPlayer - maybe I've got a different component that plays AnimationClips.

In theory player could be left as Entity::PLACEHOLDER, but that's messy and will trigger a warning in animate_targets.

Solution

This PR splits AnimationTarget into two components:

1. AnimationTargetId is just the original struct with a component derive.
2. AnimationPlayerTarget is a new unit struct (Entity).

I'm not convinced AnimationPlayerTarget is a good name, but it does fit the usual source/target naming for entity relationships. AnimationPlayerRef was another candidate.

AnimationPlayerTarget could be a relationship target, but there would be a performance cost from making AnimationPlayer a relationship source. Maybe it's still a good idea, but that's probably best left to another PR.

Performance

Profiled on many_foxes - difference was negligible.

Testing

Examples animated_mesh, animated_transform, animated_ui, animation_masks, eased_motion, scene_viewer.

Future

If this PR lands then I'll probably file a follow up that adds more flexibility to the the glTF loader creation of AnimationTargetId and AnimationPlayer. This will help #18262 and enable some other features.

[viridia]

How about AnimationPlayerRef or AnimationPlayerId?

[greeble-dev]

How about AnimationPlayerRef or AnimationPlayerId?

AnimationPlayerRef makes sense to me, but I suspect it might get pushback as the relationship system tends to use "target".

AnimationPlayerId I'm not so keen on - I read "SomethingId" as "this component identifies the entity it's contained in".

[viridia]

The name "target" is used at a meta-level: no relation components have the name "target" in them.

Some alternative names:

• ActivePlayer
• ActiveAnimationPlayer
• AnimatedBy

[greeble-dev]

AnimatedBy does seem the most relationship-y. The one wrinkle is whether it's polymorphic - does it link to an entity which can have any kind of animation driver component? Or does it link specifically to an entity with an AnimationPlayer component? If it's the former then seems fine. If it's the latter (which is the current situation) then it feels a bit imprecise.

I guess AnimatedByAnimationPlayer is a precise but ugly option.

[alice-i-cecile]

I like the core idea, and would actually be keen to swap this to a relation. Both directions feel useful to me.

AnimatedBy feels like the right terminology: I think we can clarify further in the docs.

[greeble-dev]

I've changed AnimationPlayerTarget to AnimatedBy.

I also tried relationships but ran into a problem - entities can't target themselves:

WARN bevy_ecs::relationship: The bevy_animation::AnimatedBy(1v0) relationship on entity 1v0 points to itself. The invalid bevy_animation::AnimatedBy relationship has been removed.

This is needed by the glTF importer, which puts the AnimationPlayer on the root of the hierarchy that's animated. Seems like it could also be a common pattern elsewhere - animated_transform is one example.

I have a distant memory of this limitation being debated on Discord or Github, but I haven't been able to find a reference.

Maybe one solution is that an entity with both an AnimationPlayer and an AnimationTargetId component will assume it's animated by that player, even without a AnimatedBy? Sounds a bit magical but also kinda convenient? But would require some icky changes to animate_targets. I think exploring that is a bit much for this PR.

Diff for reference:

 #[derive(Clone, Copy, Component, Reflect, Debug)]
 #[reflect(Component, Clone)]
+#[relationship(relationship_target = AnimationPlayer)]
 pub struct AnimatedBy(#[entities] pub Entity);

-#[derive(Component, Default, Reflect)]
+#[derive(Component, Default, Reflect)]
 #[reflect(Component, Default, Clone)]
+#[relationship_target(relationship = AnimatedBy)]
 pub struct AnimationPlayer {
     active_animations: HashMap<AnimationNodeIndex, ActiveAnimation>,
+    #[relationship]
+    animated_entities: Vec<Entity>,
 }

[alice-i-cecile]

I also tried relationships but ran into a problem - entities can't target themselves:

We could probably relax this restriction in some way, but we shouldn't tackle this here. I'm content with the design of this for now, but don't have the animation experience to give you a proper review.