From a17b3961f129a584671f694390a0dcc47e127065 Mon Sep 17 00:00:00 2001
From: Patrick Walton <pcwalton@mimiga.net>
Date: Sun, 4 Feb 2024 16:51:46 -0800
Subject: [PATCH 01/27] Rework animation to be done in two phases.

This eliminates all the unsafe code, increases parallelism, and reduces
the number of queries to just two.
---
 crates/bevy_animation/Cargo.toml |   1 +
 crates/bevy_animation/src/lib.rs | 783 +++++++++++++++++--------------
 2 files changed, 437 insertions(+), 347 deletions(-)

diff --git a/crates/bevy_animation/Cargo.toml b/crates/bevy_animation/Cargo.toml
index f2d5da4cd7dbf..50f97e30d3116 100644
--- a/crates/bevy_animation/Cargo.toml
+++ b/crates/bevy_animation/Cargo.toml
@@ -13,6 +13,7 @@ keywords = ["bevy"]
 bevy_app = { path = "../bevy_app", version = "0.12.0" }
 bevy_asset = { path = "../bevy_asset", version = "0.12.0" }
 bevy_core = { path = "../bevy_core", version = "0.12.0" }
+bevy_log = { path = "../bevy_log", version = "0.12.0" }
 bevy_math = { path = "../bevy_math", version = "0.12.0" }
 bevy_reflect = { path = "../bevy_reflect", version = "0.12.0", features = [
   "bevy",
diff --git a/crates/bevy_animation/src/lib.rs b/crates/bevy_animation/src/lib.rs
index 5771e2e9da510..1b6875d58855c 100644
--- a/crates/bevy_animation/src/lib.rs
+++ b/crates/bevy_animation/src/lib.rs
@@ -3,20 +3,22 @@
 mod animatable;
 mod util;
 
-use std::ops::{Add, Deref, Mul};
+use std::ops::{Add, Mul};
 use std::time::Duration;
 
-use bevy_app::{App, Plugin, PostUpdate};
+use bevy_app::{App, Plugin, PostUpdate, Update};
 use bevy_asset::{Asset, AssetApp, Assets, Handle};
 use bevy_core::Name;
 use bevy_ecs::prelude::*;
+use bevy_ecs::system::SystemState;
 use bevy_hierarchy::{Children, Parent};
+use bevy_log::error;
 use bevy_math::{FloatExt, Quat, Vec3};
 use bevy_reflect::Reflect;
 use bevy_render::mesh::morph::MorphWeights;
 use bevy_time::Time;
 use bevy_transform::{prelude::Transform, TransformSystem};
-use bevy_utils::{tracing::warn, HashMap};
+use bevy_utils::{EntityHashMap, EntityHashSet, HashMap};
 
 #[allow(missing_docs)]
 pub mod prelude {
@@ -153,6 +155,12 @@ pub struct AnimationClip {
     duration: f32,
 }
 
+#[derive(Clone, Component)]
+pub struct AnimationTarget {
+    root: Entity,
+    path: EntityPath,
+}
+
 impl AnimationClip {
     #[inline]
     /// [`VariableCurve`]s for each bone. Indexed by the bone ID.
@@ -228,7 +236,6 @@ struct PlayingAnimation {
     /// Note: This will always be in the range [0.0, animation clip duration]
     seek_time: f32,
     animation_clip: Handle<AnimationClip>,
-    path_cache: Vec<Vec<Option<Entity>>>,
     /// Number of times the animation has completed.
     /// If the animation is playing in reverse, this increments when the animation passes the start.
     completions: u32,
@@ -242,7 +249,6 @@ impl Default for PlayingAnimation {
             elapsed: 0.0,
             seek_time: 0.0,
             animation_clip: Default::default(),
-            path_cache: Vec::new(),
             completions: 0,
         }
     }
@@ -479,174 +485,186 @@ impl AnimationPlayer {
     }
 }
 
-fn entity_from_path(
-    root: Entity,
-    path: &EntityPath,
-    children: &Query<&Children>,
-    names: &Query<&Name>,
-    path_cache: &mut Vec<Option<Entity>>,
-) -> Option<Entity> {
-    // PERF: finding the target entity can be optimised
-    let mut current_entity = root;
-    path_cache.resize(path.parts.len(), None);
-
-    let mut parts = path.parts.iter().enumerate();
-
-    // check the first name is the root node which we already have
-    let Some((_, root_name)) = parts.next() else {
-        return None;
-    };
-    if names.get(current_entity) != Ok(root_name) {
-        return None;
+pub fn update_animation_targets(
+    mut commands: Commands,
+    candidate_targets: Query<
+        (
+            Entity,
+            &Parent,
+            Option<&Children>,
+            &Name,
+            Option<&AnimationTarget>,
+        ),
+        (
+            Or<(With<Parent>, With<Children>)>,
+            Or<(Changed<Parent>, Changed<Children>, Changed<Name>)>,
+            Without<AnimationPlayer>,
+        ),
+    >,
+    players: Query<(
+        Entity,
+        &mut AnimationPlayer,
+        Option<Ref<Children>>,
+        Ref<Name>,
+    )>,
+) {
+    // Get ready to mark all dirty entities. Start with all targets that were
+    // modified, as well as any children of animation players that were
+    // modified.
+    //
+    // TODO: Make this a Local.
+    let mut worklist: Vec<Entity> = candidate_targets.iter().map(|row| row.0).collect();
+    for (_, _, kids, name) in players.iter() {
+        if let Some(kids) = kids {
+            if name.is_changed() || kids.is_changed() {
+                worklist.extend(kids.iter().map(|kid| *kid));
+            }
+        }
     }
 
-    for (idx, part) in parts {
-        let mut found = false;
-        let children = children.get(current_entity).ok()?;
-        if let Some(cached) = path_cache[idx] {
-            if children.contains(&cached) {
-                if let Ok(name) = names.get(cached) {
-                    if name == part {
-                        current_entity = cached;
-                        found = true;
-                    }
-                }
+    // Iteratively mark all descendants of dirty entities as dirty themselves.
+    // At the end of this process, `dirty_entities` will consist of a forest.
+    let mut dirty_entities: EntityHashSet<Entity> = EntityHashSet::default();
+    while let Some(entity) = worklist.pop() {
+        if dirty_entities.contains(&entity) {
+            continue;
+        }
+        if let Ok((_, _, Some(kids), _, _)) = candidate_targets.get(entity) {
+            worklist.extend(kids.iter().map(|&kid| kid));
+        }
+        dirty_entities.insert(entity);
+    }
+
+    // Find the roots of the trees in the forest.
+    worklist = dirty_entities
+        .iter()
+        .filter(|&&entity| {
+            !candidate_targets
+                .get(entity)
+                .is_ok_and(|(_, parent, _, _, _)| dirty_entities.contains(&parent.get()))
+        })
+        .cloned()
+        .collect();
+
+    // Compute new `AnimationTarget` components.
+    let mut new_targets: EntityHashMap<Entity, AnimationTarget> = EntityHashMap::default();
+    while let Some(entity) = worklist.pop() {
+        let (_, parent, kids, name, _) = candidate_targets
+            .get(entity)
+            .expect("Shouldn't have dirtied an entity that wasn't a target");
+
+        // Figure out the parent path. First, if we've already computed the path
+        // of the parent, just use that.
+        let mut new_target = AnimationTarget::new();
+        if let Some(parent_target) = new_targets.get(&parent.get()) {
+            new_target = parent_target.clone();
+        }
+
+        // Otherwise, if this is a root of a tree in the forest, we know the
+        // parent's `AnimationTarget` is up to date, so fetch that.
+        if new_target.path.is_empty() {
+            if let Ok((_, _, _, _, Some(parent_target))) = candidate_targets.get(parent.get()) {
+                new_target = parent_target.clone();
             }
         }
-        if !found {
-            for child in children.deref() {
-                if let Ok(name) = names.get(*child) {
-                    if name == part {
-                        // Found a children with the right name, continue to the next part
-                        current_entity = *child;
-                        path_cache[idx] = Some(*child);
-                        found = true;
-                        break;
+
+        // If we still didn't find an `AnimationTarget` for the parent, we must be an
+        // immediate child of an animation player. Handle that case.
+        if new_target.path.is_empty() {
+            match players.get(parent.get()) {
+                Ok((_, _, _, name)) => {
+                    new_target = AnimationTarget {
+                        root: parent.get(),
+                        path: EntityPath::from(name.clone()),
                     }
                 }
+                Err(_) => {
+                    error!(
+                        "Couldn't determine an appropriate path for {:?}. Is the tree malformed?",
+                        entity
+                    );
+                    break;
+                }
             }
         }
-        if !found {
-            warn!("Entity not found for path {:?} on part {:?}", path, part);
-            return None;
+
+        // Push our own name onto the path and record it.
+        new_target.path.parts.push(name.clone());
+        new_targets.insert(entity, new_target);
+
+        // Enqueue children.
+        if let Some(kids) = kids {
+            worklist.extend(kids);
         }
     }
-    Some(current_entity)
-}
 
-/// Verify that there are no ancestors of a given entity that have an [`AnimationPlayer`].
-fn verify_no_ancestor_player(
-    player_parent: Option<&Parent>,
-    parents: &Query<(Has<AnimationPlayer>, Option<&Parent>)>,
-) -> bool {
-    let Some(mut current) = player_parent.map(Parent::get) else {
-        return true;
-    };
-    loop {
-        let Ok((has_player, parent)) = parents.get(current) else {
-            return true;
-        };
-        if has_player {
-            return false;
-        }
-        if let Some(parent) = parent {
-            current = parent.get();
-        } else {
-            return true;
-        }
+    // Finally, update the `AnimationTarget` components.
+    for (entity, animation_target) in new_targets.drain() {
+        commands.entity(entity).insert(animation_target);
     }
 }
 
-/// System that will play all animations, using any entity with a [`AnimationPlayer`]
-/// and a [`Handle<AnimationClip>`] as an animation root
-#[allow(clippy::too_many_arguments)]
 pub fn animation_player(
     time: Res<Time>,
-    animations: Res<Assets<AnimationClip>>,
-    children: Query<&Children>,
-    names: Query<&Name>,
-    transforms: Query<&mut Transform>,
-    morphs: Query<&mut MorphWeights>,
-    parents: Query<(Has<AnimationPlayer>, Option<&Parent>)>,
-    mut animation_players: Query<(Entity, Option<&Parent>, &mut AnimationPlayer)>,
+    animation_clips: Res<Assets<AnimationClip>>,
+    mut players: Query<&mut AnimationPlayer>,
 ) {
-    animation_players
-        .par_iter_mut()
-        .for_each(|(root, maybe_parent, mut player)| {
-            update_transitions(&mut player, &time);
-            run_animation_player(
-                root,
-                player,
-                &time,
-                &animations,
-                &names,
-                &transforms,
-                &morphs,
-                maybe_parent,
-                &parents,
-                &children,
-            );
-        });
+    for mut player in players.iter_mut() {
+        update_transitions(&mut player, &time);
+
+        let paused = player.paused;
+        if paused {
+            continue;
+        }
+
+        // Update the main animation.
+        update_animation(&mut player.animation, paused, &time, &animation_clips);
+
+        // Update transition animations.
+        for transition in &mut player.transitions {
+            update_animation(&mut transition.animation, paused, &time, &animation_clips)
+        }
+    }
 }
 
-#[allow(clippy::too_many_arguments)]
-fn run_animation_player(
-    root: Entity,
-    mut player: Mut<AnimationPlayer>,
-    time: &Time,
-    animations: &Assets<AnimationClip>,
-    names: &Query<&Name>,
-    transforms: &Query<&mut Transform>,
-    morphs: &Query<&mut MorphWeights>,
-    maybe_parent: Option<&Parent>,
-    parents: &Query<(Has<AnimationPlayer>, Option<&Parent>)>,
-    children: &Query<&Children>,
+pub fn animate_targets(
+    world: &mut World,
+    mut system_state: Local<
+        Option<
+            SystemState<(
+                Query<&AnimationPlayer, Without<AnimationTarget>>,
+                Query<EntityMut, With<AnimationTarget>>,
+            )>,
+        >,
+    >,
 ) {
-    let paused = player.paused;
-    // Continue if paused unless the `AnimationPlayer` was changed
-    // This allow the animation to still be updated if the player.elapsed field was manually updated in pause
-    if paused && !player.is_changed() {
-        return;
+    if system_state.is_none() {
+        *system_state = Some(SystemState::new(world));
     }
 
-    // Apply the main animation
-    apply_animation(
-        1.0,
-        &mut player.animation,
-        paused,
-        root,
-        time,
-        animations,
-        names,
-        transforms,
-        morphs,
-        maybe_parent,
-        parents,
-        children,
-    );
-
-    // Apply any potential fade-out transitions from previous animations
-    for AnimationTransition {
-        current_weight,
-        animation,
-        ..
-    } in &mut player.transitions
-    {
-        apply_animation(
-            *current_weight,
-            animation,
-            paused,
-            root,
-            time,
-            animations,
-            names,
-            transforms,
-            morphs,
-            maybe_parent,
-            parents,
-            children,
-        );
-    }
+    // This elaborate setup using `resource_scope` is a workaround for the fact
+    // that `EntityMut` queries lock out resources. This is a known bug in Bevy.
+    world.resource_scope(|world, clips: Mut<Assets<AnimationClip>>| {
+        let (players, mut targets) = system_state.as_mut().unwrap().get_mut(world);
+        targets.par_iter_mut().for_each(|mut target_entity| {
+            let target = target_entity.get::<AnimationTarget>().unwrap();
+            let Ok(player) = players.get(target.root) else {
+                error!(
+                    "Couldn't find the animation player for {:?}",
+                    target_entity.id()
+                );
+                return;
+            };
+
+            player.animation.apply(&clips, 1.0, &mut target_entity);
+
+            for transition in &player.transitions {
+                transition
+                    .animation
+                    .apply(&clips, transition.current_weight, &mut target_entity);
+            }
+        });
+    });
 }
 
 /// Update `weights` based on weights in `keyframe` with a linear interpolation
@@ -691,217 +709,308 @@ where
         + tangent_in_end * step_duration * (lerp.powi(3) - lerp.powi(2))
 }
 
-#[allow(clippy::too_many_arguments)]
-fn apply_animation(
-    weight: f32,
+fn update_transitions(player: &mut AnimationPlayer, time: &Time) {
+    player.transitions.retain_mut(|animation| {
+        animation.current_weight -= animation.weight_decline_per_sec * time.delta_seconds();
+        animation.current_weight > 0.0
+    });
+}
+
+/// Adds animation support to an app
+#[derive(Default)]
+pub struct AnimationPlugin;
+
+impl Plugin for AnimationPlugin {
+    fn build(&self, app: &mut App) {
+        app.init_asset::<AnimationClip>()
+            .register_asset_reflect::<AnimationClip>()
+            .register_type::<AnimationPlayer>()
+            .add_systems(Update, update_animation_targets)
+            .add_systems(
+                PostUpdate,
+                (animation_player, animate_targets)
+                    .chain()
+                    .before(TransformSystem::TransformPropagate),
+            );
+    }
+}
+
+impl AnimationTarget {
+    // For private use in `update_animation_targets` only.
+    fn new() -> AnimationTarget {
+        AnimationTarget {
+            root: Entity::PLACEHOLDER,
+            path: EntityPath::new(),
+        }
+    }
+}
+
+fn update_animation(
     animation: &mut PlayingAnimation,
     paused: bool,
-    root: Entity,
     time: &Time,
-    animations: &Assets<AnimationClip>,
-    names: &Query<&Name>,
-    transforms: &Query<&mut Transform>,
-    morphs: &Query<&mut MorphWeights>,
-    maybe_parent: Option<&Parent>,
-    parents: &Query<(Has<AnimationPlayer>, Option<&Parent>)>,
-    children: &Query<&Children>,
+    animation_clips: &Assets<AnimationClip>,
 ) {
-    let Some(animation_clip) = animations.get(&animation.animation_clip) else {
+    let Some(animation_clip) = animation_clips.get(&animation.animation_clip) else {
         return;
     };
 
-    // We don't return early because seek_to() may have been called on the animation player.
-    animation.update(
-        if paused { 0.0 } else { time.delta_seconds() },
-        animation_clip.duration,
-    );
+    let delta = if paused { 0.0 } else { time.delta_seconds() };
+
+    animation.update(delta, animation_clip.duration);
+}
 
-    if animation.path_cache.len() != animation_clip.paths.len() {
-        let new_len = animation_clip.paths.len();
-        animation.path_cache.iter_mut().for_each(|v| v.clear());
-        animation.path_cache.resize_with(new_len, Vec::new);
+impl From<Name> for EntityPath {
+    fn from(value: Name) -> Self {
+        EntityPath { parts: vec![value] }
     }
-    if !verify_no_ancestor_player(maybe_parent, parents) {
-        warn!("Animation player on {:?} has a conflicting animation player on an ancestor. Cannot safely animate.", root);
-        return;
+}
+
+impl EntityPath {
+    fn new() -> EntityPath {
+        EntityPath { parts: vec![] }
     }
 
-    let mut any_path_found = false;
-    for (path, bone_id) in &animation_clip.paths {
-        let cached_path = &mut animation.path_cache[*bone_id];
-        let curves = animation_clip.get_curves(*bone_id).unwrap();
-        let Some(target) = entity_from_path(root, path, children, names, cached_path) else {
-            continue;
+    fn is_empty(&self) -> bool {
+        self.parts.is_empty()
+    }
+}
+
+impl PlayingAnimation {
+    fn apply(&self, clips: &Assets<AnimationClip>, weight: f32, target_entity: &mut EntityMut) {
+        let target = target_entity.get::<AnimationTarget>().unwrap();
+
+        let Some(clip) = clips.get(&self.animation_clip) else {
+            error!(
+                "Couldn't find the animation clip for {:?}",
+                target_entity.id()
+            );
+            return;
         };
-        any_path_found = true;
-        // SAFETY: The verify_no_ancestor_player check above ensures that two animation players cannot alias
-        // any of their descendant Transforms.
-        //
-        // The system scheduler prevents any other system from mutating Transforms at the same time,
-        // so the only way this fetch can alias is if two AnimationPlayers are targeting the same bone.
-        // This can only happen if there are two or more AnimationPlayers are ancestors to the same
-        // entities. By verifying that there is no other AnimationPlayer in the ancestors of a
-        // running AnimationPlayer before animating any entity, this fetch cannot alias.
-        //
-        // This means only the AnimationPlayers closest to the root of the hierarchy will be able
-        // to run their animation. Any players in the children or descendants will log a warning
-        // and do nothing.
-        let Ok(mut transform) = (unsafe { transforms.get_unchecked(target) }) else {
-            continue;
+
+        // Extract the bone ID. If this bone isn't being animated, bail.
+        let Some(bone_id) = clip.paths.get(&target.path) else {
+            return;
+        };
+
+        let Some(curves) = clip.get_curves(*bone_id) else {
+            error!("Couldn't find the curves for bone {:?}", target.path);
+            return;
         };
-        // SAFETY: As above, there can't be other AnimationPlayers with this target so this fetch can't alias
-        let mut morphs = unsafe { morphs.get_unchecked(target) }.ok();
+
+        self.apply_curves(curves, weight, target_entity)
+    }
+
+    fn apply_curves(&self, curves: &[VariableCurve], weight: f32, target: &mut EntityMut) {
         for curve in curves {
             // Some curves have only one keyframe used to set a transform
             if curve.keyframe_timestamps.len() == 1 {
-                match &curve.keyframes {
-                    Keyframes::Rotation(keyframes) => {
-                        transform.rotation = transform.rotation.slerp(keyframes[0], weight);
-                    }
-                    Keyframes::Translation(keyframes) => {
-                        transform.translation = transform.translation.lerp(keyframes[0], weight);
-                    }
-                    Keyframes::Scale(keyframes) => {
-                        transform.scale = transform.scale.lerp(keyframes[0], weight);
-                    }
-                    Keyframes::Weights(keyframes) => {
-                        if let Some(morphs) = &mut morphs {
-                            let target_count = morphs.weights().len();
-                            lerp_morph_weights(
-                                morphs.weights_mut(),
-                                get_keyframe(target_count, keyframes, 0).iter().copied(),
-                                weight,
-                            );
-                        }
-                    }
-                }
-                continue;
+                self.apply_single_keyframe(curve, weight, target);
+                return;
             }
 
             // Find the current keyframe
-            let Some(step_start) = curve.find_current_keyframe(animation.seek_time) else {
-                continue;
+            let Some(step_start) = curve.find_current_keyframe(self.seek_time) else {
+                return;
             };
 
             let timestamp_start = curve.keyframe_timestamps[step_start];
             let timestamp_end = curve.keyframe_timestamps[step_start + 1];
             // Compute how far we are through the keyframe, normalized to [0, 1]
-            let lerp = f32::inverse_lerp(timestamp_start, timestamp_end, animation.seek_time);
+            let lerp = f32::inverse_lerp(timestamp_start, timestamp_end, self.seek_time);
 
-            apply_keyframe(
+            self.apply_tweened_keyframe(
                 curve,
                 step_start,
                 weight,
                 lerp,
                 timestamp_end - timestamp_start,
-                &mut transform,
-                &mut morphs,
+                target,
             );
         }
     }
 
-    if !any_path_found {
-        warn!("Animation player on {root:?} did not match any entity paths.");
-    }
-}
+    fn apply_single_keyframe(&self, curve: &VariableCurve, weight: f32, target: &mut EntityMut) {
+        match &curve.keyframes {
+            Keyframes::Rotation(keyframes) => {
+                if let Some(mut transform) = target.get_mut::<Transform>() {
+                    transform.rotation = transform.rotation.slerp(keyframes[0], weight);
+                }
+            }
 
-#[inline(always)]
-fn apply_keyframe(
-    curve: &VariableCurve,
-    step_start: usize,
-    weight: f32,
-    lerp: f32,
-    duration: f32,
-    transform: &mut Mut<Transform>,
-    morphs: &mut Option<Mut<MorphWeights>>,
-) {
-    match (&curve.interpolation, &curve.keyframes) {
-        (Interpolation::Step, Keyframes::Rotation(keyframes)) => {
-            transform.rotation = transform.rotation.slerp(keyframes[step_start], weight);
-        }
-        (Interpolation::Linear, Keyframes::Rotation(keyframes)) => {
-            let rot_start = keyframes[step_start];
-            let mut rot_end = keyframes[step_start + 1];
-            // Choose the smallest angle for the rotation
-            if rot_end.dot(rot_start) < 0.0 {
-                rot_end = -rot_end;
+            Keyframes::Translation(keyframes) => {
+                if let Some(mut transform) = target.get_mut::<Transform>() {
+                    transform.translation = transform.translation.lerp(keyframes[0], weight);
+                }
+            }
+
+            Keyframes::Scale(keyframes) => {
+                if let Some(mut transform) = target.get_mut::<Transform>() {
+                    transform.scale = transform.scale.lerp(keyframes[0], weight);
+                }
+            }
+
+            Keyframes::Weights(keyframes) => {
+                let Some(mut morphs) = target.get_mut::<MorphWeights>() else {
+                    error!(
+                        "Tried to animate morphs on {:?}, but no `MorphWeights` was found",
+                        target.id()
+                    );
+                    return;
+                };
+
+                let target_count = morphs.weights().len();
+                lerp_morph_weights(
+                    morphs.weights_mut(),
+                    get_keyframe(target_count, keyframes, 0).iter().copied(),
+                    weight,
+                );
             }
-            // Rotations are using a spherical linear interpolation
-            let rot = rot_start.normalize().slerp(rot_end.normalize(), lerp);
-            transform.rotation = transform.rotation.slerp(rot, weight);
-        }
-        (Interpolation::CubicSpline, Keyframes::Rotation(keyframes)) => {
-            let value_start = keyframes[step_start * 3 + 1];
-            let tangent_out_start = keyframes[step_start * 3 + 2];
-            let tangent_in_end = keyframes[(step_start + 1) * 3];
-            let value_end = keyframes[(step_start + 1) * 3 + 1];
-            let result = cubic_spline_interpolation(
-                value_start,
-                tangent_out_start,
-                tangent_in_end,
-                value_end,
-                lerp,
-                duration,
-            );
-            transform.rotation = transform.rotation.slerp(result.normalize(), weight);
-        }
-        (Interpolation::Step, Keyframes::Translation(keyframes)) => {
-            transform.translation = transform.translation.lerp(keyframes[step_start], weight);
-        }
-        (Interpolation::Linear, Keyframes::Translation(keyframes)) => {
-            let translation_start = keyframes[step_start];
-            let translation_end = keyframes[step_start + 1];
-            let result = translation_start.lerp(translation_end, lerp);
-            transform.translation = transform.translation.lerp(result, weight);
-        }
-        (Interpolation::CubicSpline, Keyframes::Translation(keyframes)) => {
-            let value_start = keyframes[step_start * 3 + 1];
-            let tangent_out_start = keyframes[step_start * 3 + 2];
-            let tangent_in_end = keyframes[(step_start + 1) * 3];
-            let value_end = keyframes[(step_start + 1) * 3 + 1];
-            let result = cubic_spline_interpolation(
-                value_start,
-                tangent_out_start,
-                tangent_in_end,
-                value_end,
-                lerp,
-                duration,
-            );
-            transform.translation = transform.translation.lerp(result, weight);
-        }
-        (Interpolation::Step, Keyframes::Scale(keyframes)) => {
-            transform.scale = transform.scale.lerp(keyframes[step_start], weight);
-        }
-        (Interpolation::Linear, Keyframes::Scale(keyframes)) => {
-            let scale_start = keyframes[step_start];
-            let scale_end = keyframes[step_start + 1];
-            let result = scale_start.lerp(scale_end, lerp);
-            transform.scale = transform.scale.lerp(result, weight);
-        }
-        (Interpolation::CubicSpline, Keyframes::Scale(keyframes)) => {
-            let value_start = keyframes[step_start * 3 + 1];
-            let tangent_out_start = keyframes[step_start * 3 + 2];
-            let tangent_in_end = keyframes[(step_start + 1) * 3];
-            let value_end = keyframes[(step_start + 1) * 3 + 1];
-            let result = cubic_spline_interpolation(
-                value_start,
-                tangent_out_start,
-                tangent_in_end,
-                value_end,
-                lerp,
-                duration,
-            );
-            transform.scale = transform.scale.lerp(result, weight);
         }
-        (Interpolation::Step, Keyframes::Weights(keyframes)) => {
-            if let Some(morphs) = morphs {
+    }
+
+    fn apply_tweened_keyframe(
+        &self,
+        curve: &VariableCurve,
+        step_start: usize,
+        weight: f32,
+        lerp: f32,
+        duration: f32,
+        target: &mut EntityMut,
+    ) {
+        match (&curve.interpolation, &curve.keyframes) {
+            (Interpolation::Step, Keyframes::Rotation(keyframes)) => {
+                if let Some(mut transform) = target.get_mut::<Transform>() {
+                    transform.rotation = transform.rotation.slerp(keyframes[step_start], weight);
+                }
+            }
+
+            (Interpolation::Linear, Keyframes::Rotation(keyframes)) => {
+                let Some(mut transform) = target.get_mut::<Transform>() else {
+                    return;
+                };
+
+                let rot_start = keyframes[step_start];
+                let mut rot_end = keyframes[step_start + 1];
+                // Choose the smallest angle for the rotation
+                if rot_end.dot(rot_start) < 0.0 {
+                    rot_end = -rot_end;
+                }
+                // Rotations are using a spherical linear interpolation
+                let rot = rot_start.normalize().slerp(rot_end.normalize(), lerp);
+                transform.rotation = transform.rotation.slerp(rot, weight);
+            }
+
+            (Interpolation::CubicSpline, Keyframes::Rotation(keyframes)) => {
+                let Some(mut transform) = target.get_mut::<Transform>() else {
+                    return;
+                };
+
+                let value_start = keyframes[step_start * 3 + 1];
+                let tangent_out_start = keyframes[step_start * 3 + 2];
+                let tangent_in_end = keyframes[(step_start + 1) * 3];
+                let value_end = keyframes[(step_start + 1) * 3 + 1];
+                let result = cubic_spline_interpolation(
+                    value_start,
+                    tangent_out_start,
+                    tangent_in_end,
+                    value_end,
+                    lerp,
+                    duration,
+                );
+                transform.rotation = transform.rotation.slerp(result.normalize(), weight);
+            }
+
+            (Interpolation::Step, Keyframes::Translation(keyframes)) => {
+                if let Some(mut transform) = target.get_mut::<Transform>() {
+                    transform.translation =
+                        transform.translation.lerp(keyframes[step_start], weight);
+                }
+            }
+
+            (Interpolation::Linear, Keyframes::Translation(keyframes)) => {
+                let Some(mut transform) = target.get_mut::<Transform>() else {
+                    return;
+                };
+
+                let translation_start = keyframes[step_start];
+                let translation_end = keyframes[step_start + 1];
+                let result = translation_start.lerp(translation_end, lerp);
+                transform.translation = transform.translation.lerp(result, weight);
+            }
+
+            (Interpolation::CubicSpline, Keyframes::Translation(keyframes)) => {
+                let Some(mut transform) = target.get_mut::<Transform>() else {
+                    return;
+                };
+
+                let value_start = keyframes[step_start * 3 + 1];
+                let tangent_out_start = keyframes[step_start * 3 + 2];
+                let tangent_in_end = keyframes[(step_start + 1) * 3];
+                let value_end = keyframes[(step_start + 1) * 3 + 1];
+                let result = cubic_spline_interpolation(
+                    value_start,
+                    tangent_out_start,
+                    tangent_in_end,
+                    value_end,
+                    lerp,
+                    duration,
+                );
+                transform.translation = transform.translation.lerp(result, weight);
+            }
+
+            (Interpolation::Step, Keyframes::Scale(keyframes)) => {
+                if let Some(mut transform) = target.get_mut::<Transform>() {
+                    transform.scale = transform.scale.lerp(keyframes[step_start], weight);
+                }
+            }
+
+            (Interpolation::Linear, Keyframes::Scale(keyframes)) => {
+                let Some(mut transform) = target.get_mut::<Transform>() else {
+                    return;
+                };
+
+                let scale_start = keyframes[step_start];
+                let scale_end = keyframes[step_start + 1];
+                let result = scale_start.lerp(scale_end, lerp);
+                transform.scale = transform.scale.lerp(result, weight);
+            }
+
+            (Interpolation::CubicSpline, Keyframes::Scale(keyframes)) => {
+                let Some(mut transform) = target.get_mut::<Transform>() else {
+                    return;
+                };
+
+                let value_start = keyframes[step_start * 3 + 1];
+                let tangent_out_start = keyframes[step_start * 3 + 2];
+                let tangent_in_end = keyframes[(step_start + 1) * 3];
+                let value_end = keyframes[(step_start + 1) * 3 + 1];
+                let result = cubic_spline_interpolation(
+                    value_start,
+                    tangent_out_start,
+                    tangent_in_end,
+                    value_end,
+                    lerp,
+                    duration,
+                );
+                transform.scale = transform.scale.lerp(result, weight);
+            }
+
+            (Interpolation::Step, Keyframes::Weights(keyframes)) => {
+                let Some(mut morphs) = target.get_mut::<MorphWeights>() else {
+                    return;
+                };
+
                 let target_count = morphs.weights().len();
                 let morph_start = get_keyframe(target_count, keyframes, step_start);
                 lerp_morph_weights(morphs.weights_mut(), morph_start.iter().copied(), weight);
             }
-        }
-        (Interpolation::Linear, Keyframes::Weights(keyframes)) => {
-            if let Some(morphs) = morphs {
+
+            (Interpolation::Linear, Keyframes::Weights(keyframes)) => {
+                let Some(mut morphs) = target.get_mut::<MorphWeights>() else {
+                    return;
+                };
+
                 let target_count = morphs.weights().len();
                 let morph_start = get_keyframe(target_count, keyframes, step_start);
                 let morph_end = get_keyframe(target_count, keyframes, step_start + 1);
@@ -911,9 +1020,12 @@ fn apply_keyframe(
                     .map(|(a, b)| a.lerp(*b, lerp));
                 lerp_morph_weights(morphs.weights_mut(), result, weight);
             }
-        }
-        (Interpolation::CubicSpline, Keyframes::Weights(keyframes)) => {
-            if let Some(morphs) = morphs {
+
+            (Interpolation::CubicSpline, Keyframes::Weights(keyframes)) => {
+                let Some(mut morphs) = target.get_mut::<MorphWeights>() else {
+                    return;
+                };
+
                 let target_count = morphs.weights().len();
                 let morph_start = get_keyframe(target_count, keyframes, step_start * 3 + 1);
                 let tangents_out_start = get_keyframe(target_count, keyframes, step_start * 3 + 2);
@@ -942,29 +1054,6 @@ fn apply_keyframe(
     }
 }
 
-fn update_transitions(player: &mut AnimationPlayer, time: &Time) {
-    player.transitions.retain_mut(|animation| {
-        animation.current_weight -= animation.weight_decline_per_sec * time.delta_seconds();
-        animation.current_weight > 0.0
-    });
-}
-
-/// Adds animation support to an app
-#[derive(Default)]
-pub struct AnimationPlugin;
-
-impl Plugin for AnimationPlugin {
-    fn build(&self, app: &mut App) {
-        app.init_asset::<AnimationClip>()
-            .register_asset_reflect::<AnimationClip>()
-            .register_type::<AnimationPlayer>()
-            .add_systems(
-                PostUpdate,
-                animation_player.before(TransformSystem::TransformPropagate),
-            );
-    }
-}
-
 #[cfg(test)]
 mod tests {
     use crate::VariableCurve;

From 7fc17b15e686a64d925eadb59e1ad693a42935c7 Mon Sep 17 00:00:00 2001
From: Patrick Walton <pcwalton@mimiga.net>
Date: Sun, 4 Feb 2024 18:16:13 -0800
Subject: [PATCH 02/27] Remove `AnimationTarget` from disconnected subtrees

---
 crates/bevy_animation/src/lib.rs | 64 +++++++++++++++++---------------
 1 file changed, 35 insertions(+), 29 deletions(-)

diff --git a/crates/bevy_animation/src/lib.rs b/crates/bevy_animation/src/lib.rs
index 1b6875d58855c..90c3bd28366ac 100644
--- a/crates/bevy_animation/src/lib.rs
+++ b/crates/bevy_animation/src/lib.rs
@@ -495,11 +495,7 @@ pub fn update_animation_targets(
             &Name,
             Option<&AnimationTarget>,
         ),
-        (
-            Or<(With<Parent>, With<Children>)>,
-            Or<(Changed<Parent>, Changed<Children>, Changed<Name>)>,
-            Without<AnimationPlayer>,
-        ),
+        (Or<(With<Parent>, With<Children>)>, Without<AnimationPlayer>),
     >,
     players: Query<(
         Entity,
@@ -507,16 +503,23 @@ pub fn update_animation_targets(
         Option<Ref<Children>>,
         Ref<Name>,
     )>,
+    changed_targets: Query<Entity, (With<AnimationTarget>, Or<(Changed<Parent>, Changed<Name>)>)>,
+    mut removed_entities: RemovedComponents<Parent>,
 ) {
-    // Get ready to mark all dirty entities. Start with all targets that were
-    // modified, as well as any children of animation players that were
-    // modified.
+    // Get ready to mark all dirty entities. Start with all targets that moved
+    // in the tree (changed `Parent`). Also, if the player itself was renamed,
+    // invalidate all its children.
     //
     // TODO: Make this a Local.
-    let mut worklist: Vec<Entity> = candidate_targets.iter().map(|row| row.0).collect();
+    let mut worklist: Vec<Entity> = changed_targets.iter().collect();
+    worklist.extend(
+        removed_entities
+            .read()
+            .filter(|&entity| candidate_targets.contains(entity)),
+    );
     for (_, _, kids, name) in players.iter() {
         if let Some(kids) = kids {
-            if name.is_changed() || kids.is_changed() {
+            if name.is_changed() {
                 worklist.extend(kids.iter().map(|kid| *kid));
             }
         }
@@ -539,6 +542,7 @@ pub fn update_animation_targets(
     worklist = dirty_entities
         .iter()
         .filter(|&&entity| {
+            // Do we have a clean parent? If so, we're a root.
             !candidate_targets
                 .get(entity)
                 .is_ok_and(|(_, parent, _, _, _)| dirty_entities.contains(&parent.get()))
@@ -548,6 +552,7 @@ pub fn update_animation_targets(
 
     // Compute new `AnimationTarget` components.
     let mut new_targets: EntityHashMap<Entity, AnimationTarget> = EntityHashMap::default();
+    let mut removed_targets: EntityHashSet<Entity> = EntityHashSet::default();
     while let Some(entity) = worklist.pop() {
         let (_, parent, kids, name, _) = candidate_targets
             .get(entity)
@@ -568,29 +573,27 @@ pub fn update_animation_targets(
             }
         }
 
-        // If we still didn't find an `AnimationTarget` for the parent, we must be an
-        // immediate child of an animation player. Handle that case.
+        // If we still didn't find an `AnimationTarget` for the parent, we could
+        // be an immediate child of an animation player. Handle that case.
         if new_target.path.is_empty() {
-            match players.get(parent.get()) {
-                Ok((_, _, _, name)) => {
-                    new_target = AnimationTarget {
-                        root: parent.get(),
-                        path: EntityPath::from(name.clone()),
-                    }
-                }
-                Err(_) => {
-                    error!(
-                        "Couldn't determine an appropriate path for {:?}. Is the tree malformed?",
-                        entity
-                    );
-                    break;
-                }
+            if let Ok((_, _, _, name)) = players.get(parent.get()) {
+                new_target = AnimationTarget {
+                    root: parent.get(),
+                    path: EntityPath::from(name.clone()),
+                };
             }
         }
 
-        // Push our own name onto the path and record it.
-        new_target.path.parts.push(name.clone());
-        new_targets.insert(entity, new_target);
+        // If we still don't have an animation target after all that, we're part
+        // of a subtree that was once an animation target but got disconnected.
+        // Remove the `AnimationTarget` component entirely.
+        if new_target.path.is_empty() {
+            removed_targets.insert(entity);
+        } else {
+            // Push our own name onto the path and record it.
+            new_target.path.parts.push(name.clone());
+            new_targets.insert(entity, new_target);
+        }
 
         // Enqueue children.
         if let Some(kids) = kids {
@@ -602,6 +605,9 @@ pub fn update_animation_targets(
     for (entity, animation_target) in new_targets.drain() {
         commands.entity(entity).insert(animation_target);
     }
+    for entity in removed_targets.drain() {
+        commands.entity(entity).remove::<AnimationTarget>();
+    }
 }
 
 pub fn animation_player(

From c2548b59b72e45f35fdf926396888687e9fd5d8a Mon Sep 17 00:00:00 2001
From: Patrick Walton <pcwalton@mimiga.net>
Date: Sun, 4 Feb 2024 18:49:05 -0800
Subject: [PATCH 03/27] Fix some missing cases in change detection

---
 crates/bevy_animation/src/lib.rs | 103 +++++++++++++++++++------------
 1 file changed, 64 insertions(+), 39 deletions(-)

diff --git a/crates/bevy_animation/src/lib.rs b/crates/bevy_animation/src/lib.rs
index 90c3bd28366ac..3f7cbf73181dc 100644
--- a/crates/bevy_animation/src/lib.rs
+++ b/crates/bevy_animation/src/lib.rs
@@ -490,37 +490,57 @@ pub fn update_animation_targets(
     candidate_targets: Query<
         (
             Entity,
-            &Parent,
+            Option<&Parent>,
             Option<&Children>,
             &Name,
             Option<&AnimationTarget>,
         ),
-        (Or<(With<Parent>, With<Children>)>, Without<AnimationPlayer>),
+        Without<AnimationPlayer>,
     >,
     players: Query<(
         Entity,
-        &mut AnimationPlayer,
+        Ref<AnimationPlayer>,
         Option<Ref<Children>>,
         Ref<Name>,
     )>,
-    changed_targets: Query<Entity, (With<AnimationTarget>, Or<(Changed<Parent>, Changed<Name>)>)>,
-    mut removed_entities: RemovedComponents<Parent>,
+    changed_targets: Query<Entity, Or<(Changed<Parent>, Changed<Name>)>>,
+    mut entities_with_removed_parents: RemovedComponents<Parent>,
+    mut entities_with_removed_names: RemovedComponents<Name>,
 ) {
-    // Get ready to mark all dirty entities. Start with all targets that moved
-    // in the tree (changed `Parent`). Also, if the player itself was renamed,
-    // invalidate all its children.
+    // Mark the roots of all dirty subtrees.
     //
-    // TODO: Make this a Local.
-    let mut worklist: Vec<Entity> = changed_targets.iter().collect();
-    worklist.extend(
-        removed_entities
-            .read()
-            .filter(|&entity| candidate_targets.contains(entity)),
-    );
-    for (_, _, kids, name) in players.iter() {
-        if let Some(kids) = kids {
-            if name.is_changed() {
-                worklist.extend(kids.iter().map(|kid| *kid));
+    // TODO: Make this a `Local`.
+    let mut worklist: Vec<Entity> = vec![];
+
+    // If an entity was renamed or moved in the tree (changed `Parent`), it's
+    // dirty if its new parent is either an animation target or an animation
+    // player.
+    for entity in changed_targets.iter() {
+        if candidate_targets.get(entity).is_ok_and(|(_, maybe_parent, _, _, _)| {
+            maybe_parent.is_some_and(|parent| {
+                candidate_targets.get(parent.get()).is_ok_and(|(_, _, _, _, target)| {
+                    target.is_some() || players.contains(entity)
+                })
+            })
+        }) {
+            worklist.push(entity);
+        }
+    }
+
+    // If an entity was removed from the tree (removed `Parent`), or had its
+    // name removed and was previously an animation target, it's dirty.
+    for entity in entities_with_removed_parents.read().chain(entities_with_removed_names.read()) {
+        if candidate_targets.get(entity).is_ok_and(|(_, _, _, _, target)| target.is_some()) {
+            worklist.push(entity);
+        }
+    }
+
+    // If an animation player was just added to the tree or changed names, all
+    // its children are dirty.
+    for (_, player, maybe_kids, name) in players.iter() {
+        if player.is_added() || name.is_changed() {
+            if let Some(kids) = maybe_kids {
+                worklist.extend(kids.iter());
             }
         }
     }
@@ -545,7 +565,9 @@ pub fn update_animation_targets(
             // Do we have a clean parent? If so, we're a root.
             !candidate_targets
                 .get(entity)
-                .is_ok_and(|(_, parent, _, _, _)| dirty_entities.contains(&parent.get()))
+                .is_ok_and(|(_, parent, _, _, _)| {
+                    parent.is_some_and(|parent| dirty_entities.contains(&parent.get()))
+                })
         })
         .cloned()
         .collect();
@@ -558,29 +580,32 @@ pub fn update_animation_targets(
             .get(entity)
             .expect("Shouldn't have dirtied an entity that wasn't a target");
 
-        // Figure out the parent path. First, if we've already computed the path
-        // of the parent, just use that.
+        // Determine the path based on our parent.
         let mut new_target = AnimationTarget::new();
-        if let Some(parent_target) = new_targets.get(&parent.get()) {
-            new_target = parent_target.clone();
-        }
-
-        // Otherwise, if this is a root of a tree in the forest, we know the
-        // parent's `AnimationTarget` is up to date, so fetch that.
-        if new_target.path.is_empty() {
-            if let Ok((_, _, _, _, Some(parent_target))) = candidate_targets.get(parent.get()) {
+        if let Some(parent) = parent {
+            // Figure out the parent path. First, if we've already computed the path
+            // of the parent, just use that.
+            if let Some(parent_target) = new_targets.get(&parent.get()) {
                 new_target = parent_target.clone();
             }
-        }
 
-        // If we still didn't find an `AnimationTarget` for the parent, we could
-        // be an immediate child of an animation player. Handle that case.
-        if new_target.path.is_empty() {
-            if let Ok((_, _, _, name)) = players.get(parent.get()) {
-                new_target = AnimationTarget {
-                    root: parent.get(),
-                    path: EntityPath::from(name.clone()),
-                };
+            // Otherwise, if this is a root of a tree in the forest, we know the
+            // parent's `AnimationTarget` is up to date, so fetch that.
+            if new_target.path.is_empty() {
+                if let Ok((_, _, _, _, Some(parent_target))) = candidate_targets.get(parent.get()) {
+                    new_target = parent_target.clone();
+                }
+            }
+
+            // If we still didn't find an `AnimationTarget` for the parent, we could
+            // be an immediate child of an animation player. Handle that case.
+            if new_target.path.is_empty() {
+                if let Ok((_, _, _, name)) = players.get(parent.get()) {
+                    new_target = AnimationTarget {
+                        root: parent.get(),
+                        path: EntityPath::from(name.clone()),
+                    };
+                }
             }
         }
 

From 56bed0dd787ad77a6ad8f3c471b10f7bd9eb83b1 Mon Sep 17 00:00:00 2001
From: Patrick Walton <pcwalton@mimiga.net>
Date: Mon, 5 Feb 2024 14:36:51 -0800
Subject: [PATCH 04/27] Switch to using UUIDs for bones

---
 crates/bevy_animation/Cargo.toml         |   4 +
 crates/bevy_animation/src/lib.rs         | 273 ++++-------------------
 crates/bevy_gltf/src/loader.rs           |  39 +++-
 examples/animation/animated_transform.rs |  73 +++---
 4 files changed, 129 insertions(+), 260 deletions(-)

diff --git a/crates/bevy_animation/Cargo.toml b/crates/bevy_animation/Cargo.toml
index 50f97e30d3116..08ee8bdfe1107 100644
--- a/crates/bevy_animation/Cargo.toml
+++ b/crates/bevy_animation/Cargo.toml
@@ -25,5 +25,9 @@ bevy_ecs = { path = "../bevy_ecs", version = "0.12.0" }
 bevy_transform = { path = "../bevy_transform", version = "0.12.0" }
 bevy_hierarchy = { path = "../bevy_hierarchy", version = "0.12.0" }
 
+# other
+sha1_smol = { version = "1.0" }
+uuid = { version = "1.7", features = ["v5"] }
+
 [lints]
 workspace = true
diff --git a/crates/bevy_animation/src/lib.rs b/crates/bevy_animation/src/lib.rs
index 3f7cbf73181dc..24887133d85d7 100644
--- a/crates/bevy_animation/src/lib.rs
+++ b/crates/bevy_animation/src/lib.rs
@@ -9,7 +9,9 @@ use std::time::Duration;
 use bevy_app::{App, Plugin, PostUpdate, Update};
 use bevy_asset::{Asset, AssetApp, Assets, Handle};
 use bevy_core::Name;
+use bevy_ecs::entity::MapEntities;
 use bevy_ecs::prelude::*;
+use bevy_ecs::reflect::ReflectMapEntities;
 use bevy_ecs::system::SystemState;
 use bevy_hierarchy::{Children, Parent};
 use bevy_log::error;
@@ -18,17 +20,20 @@ use bevy_reflect::Reflect;
 use bevy_render::mesh::morph::MorphWeights;
 use bevy_time::Time;
 use bevy_transform::{prelude::Transform, TransformSystem};
-use bevy_utils::{EntityHashMap, EntityHashSet, HashMap};
+use bevy_utils::{EntityHashMap, EntityHashSet, HashMap, Uuid};
+use sha1_smol::Sha1;
 
 #[allow(missing_docs)]
 pub mod prelude {
     #[doc(hidden)]
     pub use crate::{
-        animatable::*, AnimationClip, AnimationPlayer, AnimationPlugin, EntityPath, Interpolation,
-        Keyframes, VariableCurve,
+        animatable::*, AnimationClip, AnimationPlayer, AnimationPlugin, Interpolation, Keyframes,
+        VariableCurve,
     };
 }
 
+pub static ANIMATION_TARGET_NAMESPACE: Uuid = Uuid::from_u128(0x3179f519d9274ff2b5966fd077023911);
+
 /// List of keyframes for one of the attribute of a [`Transform`].
 #[derive(Reflect, Clone, Debug)]
 pub enum Keyframes {
@@ -140,31 +145,33 @@ pub enum Interpolation {
     CubicSpline,
 }
 
-/// Path to an entity, with [`Name`]s. Each entity in a path must have a name.
-#[derive(Reflect, Clone, Debug, Hash, PartialEq, Eq, Default)]
-pub struct EntityPath {
-    /// Parts of the path
-    pub parts: Vec<Name>,
-}
-
-/// A list of [`VariableCurve`], and the [`EntityPath`] to which they apply.
+/// A list of [`VariableCurve`], and the [`AnimationTargetId`] to which they apply.
 #[derive(Asset, Reflect, Clone, Debug, Default)]
 pub struct AnimationClip {
-    curves: Vec<Vec<VariableCurve>>,
-    paths: HashMap<EntityPath, usize>,
+    curves: HashMap<AnimationTargetId, Vec<VariableCurve>>,
     duration: f32,
 }
 
-#[derive(Clone, Component)]
+// TODO: Use a custom Hash implementation that doesn't actually hash the results.
+#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Reflect, Debug)]
+pub struct AnimationTargetId(pub Uuid);
+
+#[derive(Clone, Component, Reflect)]
+#[reflect(Component, MapEntities)]
 pub struct AnimationTarget {
-    root: Entity,
-    path: EntityPath,
+    /// The ID of this bone.
+    ///
+    /// Typically, this is derived from the path.
+    pub id: AnimationTargetId,
+
+    /// The entity containing the [`AnimationPlayer`].
+    pub root: Entity,
 }
 
 impl AnimationClip {
     #[inline]
     /// [`VariableCurve`]s for each bone. Indexed by the bone ID.
-    pub fn curves(&self) -> &Vec<Vec<VariableCurve>> {
+    pub fn curves(&self) -> &HashMap<AnimationTargetId, Vec<VariableCurve>> {
         &self.curves
     }
 
@@ -172,16 +179,8 @@ impl AnimationClip {
     ///
     /// Returns `None` if the bone is invalid.
     #[inline]
-    pub fn get_curves(&self, bone_id: usize) -> Option<&'_ Vec<VariableCurve>> {
-        self.curves.get(bone_id)
-    }
-
-    /// Gets the curves by it's [`EntityPath`].
-    ///
-    /// Returns `None` if the bone is invalid.
-    #[inline]
-    pub fn get_curves_by_path(&self, path: &EntityPath) -> Option<&'_ Vec<VariableCurve>> {
-        self.paths.get(path).and_then(|id| self.curves.get(*id))
+    pub fn get_curves(&self, bone_id: AnimationTargetId) -> Option<&'_ Vec<VariableCurve>> {
+        self.curves.get(&bone_id)
     }
 
     /// Duration of the clip, represented in seconds
@@ -191,23 +190,12 @@ impl AnimationClip {
     }
 
     /// Add a [`VariableCurve`] to an [`EntityPath`].
-    pub fn add_curve_to_path(&mut self, path: EntityPath, curve: VariableCurve) {
+    pub fn add_curve_to_path(&mut self, bone_id: AnimationTargetId, curve: VariableCurve) {
         // Update the duration of the animation by this curve duration if it's longer
         self.duration = self
             .duration
             .max(*curve.keyframe_timestamps.last().unwrap_or(&0.0));
-        if let Some(bone_id) = self.paths.get(&path) {
-            self.curves[*bone_id].push(curve);
-        } else {
-            let idx = self.curves.len();
-            self.curves.push(vec![curve]);
-            self.paths.insert(path, idx);
-        }
-    }
-
-    /// Whether this animation clip can run on entity with given [`Name`].
-    pub fn compatible_with(&self, name: &Name) -> bool {
-        self.paths.keys().any(|path| &path.parts[0] == name)
+        self.curves.entry(bone_id).or_default().push(curve);
     }
 }
 
@@ -485,156 +473,6 @@ impl AnimationPlayer {
     }
 }
 
-pub fn update_animation_targets(
-    mut commands: Commands,
-    candidate_targets: Query<
-        (
-            Entity,
-            Option<&Parent>,
-            Option<&Children>,
-            &Name,
-            Option<&AnimationTarget>,
-        ),
-        Without<AnimationPlayer>,
-    >,
-    players: Query<(
-        Entity,
-        Ref<AnimationPlayer>,
-        Option<Ref<Children>>,
-        Ref<Name>,
-    )>,
-    changed_targets: Query<Entity, Or<(Changed<Parent>, Changed<Name>)>>,
-    mut entities_with_removed_parents: RemovedComponents<Parent>,
-    mut entities_with_removed_names: RemovedComponents<Name>,
-) {
-    // Mark the roots of all dirty subtrees.
-    //
-    // TODO: Make this a `Local`.
-    let mut worklist: Vec<Entity> = vec![];
-
-    // If an entity was renamed or moved in the tree (changed `Parent`), it's
-    // dirty if its new parent is either an animation target or an animation
-    // player.
-    for entity in changed_targets.iter() {
-        if candidate_targets.get(entity).is_ok_and(|(_, maybe_parent, _, _, _)| {
-            maybe_parent.is_some_and(|parent| {
-                candidate_targets.get(parent.get()).is_ok_and(|(_, _, _, _, target)| {
-                    target.is_some() || players.contains(entity)
-                })
-            })
-        }) {
-            worklist.push(entity);
-        }
-    }
-
-    // If an entity was removed from the tree (removed `Parent`), or had its
-    // name removed and was previously an animation target, it's dirty.
-    for entity in entities_with_removed_parents.read().chain(entities_with_removed_names.read()) {
-        if candidate_targets.get(entity).is_ok_and(|(_, _, _, _, target)| target.is_some()) {
-            worklist.push(entity);
-        }
-    }
-
-    // If an animation player was just added to the tree or changed names, all
-    // its children are dirty.
-    for (_, player, maybe_kids, name) in players.iter() {
-        if player.is_added() || name.is_changed() {
-            if let Some(kids) = maybe_kids {
-                worklist.extend(kids.iter());
-            }
-        }
-    }
-
-    // Iteratively mark all descendants of dirty entities as dirty themselves.
-    // At the end of this process, `dirty_entities` will consist of a forest.
-    let mut dirty_entities: EntityHashSet<Entity> = EntityHashSet::default();
-    while let Some(entity) = worklist.pop() {
-        if dirty_entities.contains(&entity) {
-            continue;
-        }
-        if let Ok((_, _, Some(kids), _, _)) = candidate_targets.get(entity) {
-            worklist.extend(kids.iter().map(|&kid| kid));
-        }
-        dirty_entities.insert(entity);
-    }
-
-    // Find the roots of the trees in the forest.
-    worklist = dirty_entities
-        .iter()
-        .filter(|&&entity| {
-            // Do we have a clean parent? If so, we're a root.
-            !candidate_targets
-                .get(entity)
-                .is_ok_and(|(_, parent, _, _, _)| {
-                    parent.is_some_and(|parent| dirty_entities.contains(&parent.get()))
-                })
-        })
-        .cloned()
-        .collect();
-
-    // Compute new `AnimationTarget` components.
-    let mut new_targets: EntityHashMap<Entity, AnimationTarget> = EntityHashMap::default();
-    let mut removed_targets: EntityHashSet<Entity> = EntityHashSet::default();
-    while let Some(entity) = worklist.pop() {
-        let (_, parent, kids, name, _) = candidate_targets
-            .get(entity)
-            .expect("Shouldn't have dirtied an entity that wasn't a target");
-
-        // Determine the path based on our parent.
-        let mut new_target = AnimationTarget::new();
-        if let Some(parent) = parent {
-            // Figure out the parent path. First, if we've already computed the path
-            // of the parent, just use that.
-            if let Some(parent_target) = new_targets.get(&parent.get()) {
-                new_target = parent_target.clone();
-            }
-
-            // Otherwise, if this is a root of a tree in the forest, we know the
-            // parent's `AnimationTarget` is up to date, so fetch that.
-            if new_target.path.is_empty() {
-                if let Ok((_, _, _, _, Some(parent_target))) = candidate_targets.get(parent.get()) {
-                    new_target = parent_target.clone();
-                }
-            }
-
-            // If we still didn't find an `AnimationTarget` for the parent, we could
-            // be an immediate child of an animation player. Handle that case.
-            if new_target.path.is_empty() {
-                if let Ok((_, _, _, name)) = players.get(parent.get()) {
-                    new_target = AnimationTarget {
-                        root: parent.get(),
-                        path: EntityPath::from(name.clone()),
-                    };
-                }
-            }
-        }
-
-        // If we still don't have an animation target after all that, we're part
-        // of a subtree that was once an animation target but got disconnected.
-        // Remove the `AnimationTarget` component entirely.
-        if new_target.path.is_empty() {
-            removed_targets.insert(entity);
-        } else {
-            // Push our own name onto the path and record it.
-            new_target.path.parts.push(name.clone());
-            new_targets.insert(entity, new_target);
-        }
-
-        // Enqueue children.
-        if let Some(kids) = kids {
-            worklist.extend(kids);
-        }
-    }
-
-    // Finally, update the `AnimationTarget` components.
-    for (entity, animation_target) in new_targets.drain() {
-        commands.entity(entity).insert(animation_target);
-    }
-    for entity in removed_targets.drain() {
-        commands.entity(entity).remove::<AnimationTarget>();
-    }
-}
-
 pub fn animation_player(
     time: Res<Time>,
     animation_clips: Res<Assets<AnimationClip>>,
@@ -681,7 +519,8 @@ pub fn animate_targets(
             let target = target_entity.get::<AnimationTarget>().unwrap();
             let Ok(player) = players.get(target.root) else {
                 error!(
-                    "Couldn't find the animation player for {:?}",
+                    "Couldn't find the animation player {:?} for {:?}",
+                    target.root,
                     target_entity.id()
                 );
                 return;
@@ -756,7 +595,7 @@ impl Plugin for AnimationPlugin {
         app.init_asset::<AnimationClip>()
             .register_asset_reflect::<AnimationClip>()
             .register_type::<AnimationPlayer>()
-            .add_systems(Update, update_animation_targets)
+            .register_type::<AnimationTarget>()
             .add_systems(
                 PostUpdate,
                 (animation_player, animate_targets)
@@ -766,16 +605,6 @@ impl Plugin for AnimationPlugin {
     }
 }
 
-impl AnimationTarget {
-    // For private use in `update_animation_targets` only.
-    fn new() -> AnimationTarget {
-        AnimationTarget {
-            root: Entity::PLACEHOLDER,
-            path: EntityPath::new(),
-        }
-    }
-}
-
 fn update_animation(
     animation: &mut PlayingAnimation,
     paused: bool,
@@ -791,22 +620,6 @@ fn update_animation(
     animation.update(delta, animation_clip.duration);
 }
 
-impl From<Name> for EntityPath {
-    fn from(value: Name) -> Self {
-        EntityPath { parts: vec![value] }
-    }
-}
-
-impl EntityPath {
-    fn new() -> EntityPath {
-        EntityPath { parts: vec![] }
-    }
-
-    fn is_empty(&self) -> bool {
-        self.parts.is_empty()
-    }
-}
-
 impl PlayingAnimation {
     fn apply(&self, clips: &Assets<AnimationClip>, weight: f32, target_entity: &mut EntityMut) {
         let target = target_entity.get::<AnimationTarget>().unwrap();
@@ -819,13 +632,7 @@ impl PlayingAnimation {
             return;
         };
 
-        // Extract the bone ID. If this bone isn't being animated, bail.
-        let Some(bone_id) = clip.paths.get(&target.path) else {
-            return;
-        };
-
-        let Some(curves) = clip.get_curves(*bone_id) else {
-            error!("Couldn't find the curves for bone {:?}", target.path);
+        let Some(curves) = clip.get_curves(target.id) else {
             return;
         };
 
@@ -1085,6 +892,22 @@ impl PlayingAnimation {
     }
 }
 
+impl AnimationTargetId {
+    pub fn from_names(names: &[Name]) -> AnimationTargetId {
+        let mut sha1 = Sha1::new();
+        sha1.update(ANIMATION_TARGET_NAMESPACE.as_bytes());
+        names.iter().for_each(|name| sha1.update(name.as_bytes()));
+        let hash = sha1.digest().bytes()[0..16].try_into().unwrap();
+        AnimationTargetId(*uuid::Builder::from_sha1_bytes(hash).as_uuid())
+    }
+}
+
+impl MapEntities for AnimationTarget {
+    fn map_entities<M: EntityMapper>(&mut self, entity_mapper: &mut M) {
+        self.root = entity_mapper.map_entity(self.root);
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use crate::VariableCurve;
diff --git a/crates/bevy_gltf/src/loader.rs b/crates/bevy_gltf/src/loader.rs
index c3615ff5da259..bef55c95897da 100644
--- a/crates/bevy_gltf/src/loader.rs
+++ b/crates/bevy_gltf/src/loader.rs
@@ -1,4 +1,5 @@
 use crate::{vertex_attributes::convert_attribute, Gltf, GltfExtras, GltfNode};
+use bevy_animation::{AnimationTarget, AnimationTargetId};
 use bevy_asset::{
     io::Reader, AssetLoadError, AssetLoader, AsyncReadExt, Handle, LoadContext, ReadAssetBytesError,
 };
@@ -258,11 +259,9 @@ async fn load_gltf<'a, 'b, 'c>(
                 };
 
                 if let Some((root_index, path)) = paths.get(&node.index()) {
-                    animation_roots.insert(root_index);
+                    animation_roots.insert(*root_index);
                     animation_clip.add_curve_to_path(
-                        bevy_animation::EntityPath {
-                            parts: path.clone(),
-                        },
+                        AnimationTargetId::from_names(path),
                         bevy_animation::VariableCurve {
                             keyframe_timestamps,
                             keyframes,
@@ -588,7 +587,9 @@ async fn load_gltf<'a, 'b, 'c>(
                         &mut node_index_to_entity_map,
                         &mut entity_to_skin_index_map,
                         &mut active_camera_found,
+                        &animation_roots,
                         &Transform::default(),
+                        None,
                     );
                     if result.is_err() {
                         err = Some(result);
@@ -924,7 +925,9 @@ fn load_node(
     node_index_to_entity_map: &mut HashMap<usize, Entity>,
     entity_to_skin_index_map: &mut EntityHashMap<Entity, usize>,
     active_camera_found: &mut bool,
+    animation_roots: &HashSet<usize>,
     parent_transform: &Transform,
+    animation_root: Option<(Entity, &[Name])>,
 ) -> Result<(), GltfError> {
     let mut gltf_error = None;
     let transform = node_transform(gltf_node);
@@ -938,7 +941,31 @@ fn load_node(
     let is_scale_inverted = world_transform.scale.is_negative_bitmask().count_ones() & 1 == 1;
     let mut node = world_builder.spawn(SpatialBundle::from(transform));
 
-    node.insert(node_name(gltf_node));
+    let name = node_name(gltf_node);
+    node.insert(name.clone());
+
+    let mut path_buf = vec![];
+    let animation_root = match animation_root {
+        None => {
+            if animation_roots.contains(&gltf_node.index()) {
+                path_buf.push(name);
+                Some((node.id(), &path_buf[..]))
+            } else {
+                None
+            }
+        }
+        Some((root, path)) => {
+            path_buf.extend(path.iter().cloned());
+            path_buf.push(name);
+
+            node.insert(AnimationTarget {
+                id: AnimationTargetId::from_names(&path_buf),
+                root,
+            });
+
+            Some((root, &path_buf[..]))
+        }
+    };
 
     if let Some(extras) = gltf_node.extras() {
         node.insert(GltfExtras {
@@ -1151,7 +1178,9 @@ fn load_node(
                 node_index_to_entity_map,
                 entity_to_skin_index_map,
                 active_camera_found,
+                animation_roots,
                 &world_transform,
+                animation_root,
             ) {
                 gltf_error = Some(err);
                 return;
diff --git a/examples/animation/animated_transform.rs b/examples/animation/animated_transform.rs
index b9ee301a02ae6..eea4129d456d6 100644
--- a/examples/animation/animated_transform.rs
+++ b/examples/animation/animated_transform.rs
@@ -3,6 +3,7 @@
 use std::f32::consts::PI;
 
 use bevy::prelude::*;
+use bevy_internal::animation::{AnimationTarget, AnimationTargetId};
 
 fn main() {
     App::new()
@@ -36,9 +37,7 @@ fn setup(
     let mut animation = AnimationClip::default();
     // A curve can modify a single part of a transform, here the translation
     animation.add_curve_to_path(
-        EntityPath {
-            parts: vec![planet.clone()],
-        },
+        AnimationTargetId::from_names(&[planet.clone()]),
         VariableCurve {
             keyframe_timestamps: vec![0.0, 1.0, 2.0, 3.0, 4.0],
             keyframes: Keyframes::Translation(vec![
@@ -56,10 +55,10 @@ fn setup(
     // Or it can modify the rotation of the transform.
     // To find the entity to modify, the hierarchy will be traversed looking for
     // an entity with the right name at each level
+    let orbit_controller_animation_target_id =
+        AnimationTargetId::from_names(&[planet.clone(), orbit_controller.clone()]);
     animation.add_curve_to_path(
-        EntityPath {
-            parts: vec![planet.clone(), orbit_controller.clone()],
-        },
+        orbit_controller_animation_target_id,
         VariableCurve {
             keyframe_timestamps: vec![0.0, 1.0, 2.0, 3.0, 4.0],
             keyframes: Keyframes::Rotation(vec![
@@ -75,10 +74,13 @@ fn setup(
     // If a curve in an animation is shorter than the other, it will not repeat
     // until all other curves are finished. In that case, another animation should
     // be created for each part that would have a different duration / period
+    let satellite_animation_target_id = AnimationTargetId::from_names(&[
+        planet.clone(),
+        orbit_controller.clone(),
+        satellite.clone(),
+    ]);
     animation.add_curve_to_path(
-        EntityPath {
-            parts: vec![planet.clone(), orbit_controller.clone(), satellite.clone()],
-        },
+        satellite_animation_target_id,
         VariableCurve {
             keyframe_timestamps: vec![0.0, 0.5, 1.0, 1.5, 2.0, 2.5, 3.0, 3.5, 4.0],
             keyframes: Keyframes::Scale(vec![
@@ -97,9 +99,11 @@ fn setup(
     );
     // There can be more than one curve targeting the same entity path
     animation.add_curve_to_path(
-        EntityPath {
-            parts: vec![planet.clone(), orbit_controller.clone(), satellite.clone()],
-        },
+        AnimationTargetId::from_names(&[
+            planet.clone(),
+            orbit_controller.clone(),
+            satellite.clone(),
+        ]),
         VariableCurve {
             keyframe_timestamps: vec![0.0, 1.0, 2.0, 3.0, 4.0],
             keyframes: Keyframes::Rotation(vec![
@@ -119,7 +123,7 @@ fn setup(
 
     // Create the scene that will be animated
     // First entity is the planet
-    commands
+    let planet_entity = commands
         .spawn((
             PbrBundle {
                 mesh: meshes.add(Mesh::try_from(shape::Icosphere::default()).unwrap()),
@@ -130,25 +134,34 @@ fn setup(
             planet,
             player,
         ))
+        .id();
+    commands.entity(planet_entity).with_children(|p| {
+        // This entity is just used for animation, but doesn't display anything
+        p.spawn((
+            SpatialBundle::INHERITED_IDENTITY,
+            // Add the Name component
+            orbit_controller,
+            AnimationTarget {
+                id: orbit_controller_animation_target_id,
+                root: planet_entity,
+            },
+        ))
         .with_children(|p| {
-            // This entity is just used for animation, but doesn't display anything
+            // The satellite, placed at a distance of the planet
             p.spawn((
-                SpatialBundle::INHERITED_IDENTITY,
+                PbrBundle {
+                    transform: Transform::from_xyz(1.5, 0.0, 0.0),
+                    mesh: meshes.add(shape::Cube { size: 0.5 }),
+                    material: materials.add(Color::rgb(0.3, 0.9, 0.3)),
+                    ..default()
+                },
                 // Add the Name component
-                orbit_controller,
-            ))
-            .with_children(|p| {
-                // The satellite, placed at a distance of the planet
-                p.spawn((
-                    PbrBundle {
-                        transform: Transform::from_xyz(1.5, 0.0, 0.0),
-                        mesh: meshes.add(shape::Cube { size: 0.5 }),
-                        material: materials.add(Color::rgb(0.3, 0.9, 0.3)),
-                        ..default()
-                    },
-                    // Add the Name component
-                    satellite,
-                ));
-            });
+                satellite,
+                AnimationTarget {
+                    id: satellite_animation_target_id,
+                    root: planet_entity,
+                },
+            ));
         });
+    });
 }

From 39aff530648161436db8ece5948639af3987f3c1 Mon Sep 17 00:00:00 2001
From: Patrick Walton <pcwalton@mimiga.net>
Date: Mon, 5 Feb 2024 16:29:42 -0800
Subject: [PATCH 05/27] Write documentation and use no-op hashing

---
 crates/bevy_animation/src/lib.rs              | 99 ++++++++++++++-----
 crates/bevy_gltf/src/loader.rs                | 59 +++++++----
 crates/bevy_internal/src/default_plugins.rs   |  2 +-
 crates/bevy_reflect/src/impls/std.rs          |  1 +
 crates/bevy_reflect/src/utility.rs            |  4 +-
 crates/bevy_utils/src/lib.rs                  | 25 ++---
 examples/animation/animated_transform.rs      | 28 +++---
 .../tools/scene_viewer/animation_plugin.rs    |  6 +-
 8 files changed, 145 insertions(+), 79 deletions(-)

diff --git a/crates/bevy_animation/src/lib.rs b/crates/bevy_animation/src/lib.rs
index 24887133d85d7..f9df665d06c9c 100644
--- a/crates/bevy_animation/src/lib.rs
+++ b/crates/bevy_animation/src/lib.rs
@@ -3,24 +3,26 @@
 mod animatable;
 mod util;
 
+use std::hash::{Hash, Hasher};
+use std::iter;
 use std::ops::{Add, Mul};
 use std::time::Duration;
 
-use bevy_app::{App, Plugin, PostUpdate, Update};
+use bevy_app::{App, Plugin, PostUpdate};
 use bevy_asset::{Asset, AssetApp, Assets, Handle};
 use bevy_core::Name;
 use bevy_ecs::entity::MapEntities;
 use bevy_ecs::prelude::*;
 use bevy_ecs::reflect::ReflectMapEntities;
 use bevy_ecs::system::SystemState;
-use bevy_hierarchy::{Children, Parent};
-use bevy_log::error;
+use bevy_log::{error, warn};
 use bevy_math::{FloatExt, Quat, Vec3};
 use bevy_reflect::Reflect;
 use bevy_render::mesh::morph::MorphWeights;
 use bevy_time::Time;
 use bevy_transform::{prelude::Transform, TransformSystem};
-use bevy_utils::{EntityHashMap, EntityHashSet, HashMap, Uuid};
+use bevy_utils::hashbrown::HashMap;
+use bevy_utils::{NoOpHash, Uuid};
 use sha1_smol::Sha1;
 
 #[allow(missing_docs)]
@@ -32,6 +34,9 @@ pub mod prelude {
     };
 }
 
+/// The [UUID namespace] of animation targets (bones).
+///
+/// [UUID namespace]: https://en.wikipedia.org/wiki/Universally_unique_identifier#Versions_3_and_5_(namespace_name-based)
 pub static ANIMATION_TARGET_NAMESPACE: Uuid = Uuid::from_u128(0x3179f519d9274ff2b5966fd077023911);
 
 /// List of keyframes for one of the attribute of a [`Transform`].
@@ -145,21 +150,48 @@ pub enum Interpolation {
     CubicSpline,
 }
 
-/// A list of [`VariableCurve`], and the [`AnimationTargetId`] to which they apply.
+/// A list of [`VariableCurve`]s and the [`AnimationTargetId`]s to which they
+/// apply.
+///
+/// Because animation clips refer to targets by UUID, they can target any
+/// [`AnimationTarget`] with that ID.
 #[derive(Asset, Reflect, Clone, Debug, Default)]
 pub struct AnimationClip {
-    curves: HashMap<AnimationTargetId, Vec<VariableCurve>>,
+    curves: HashMap<AnimationTargetId, Vec<VariableCurve>, NoOpHash>,
     duration: f32,
 }
 
-// TODO: Use a custom Hash implementation that doesn't actually hash the results.
-#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Reflect, Debug)]
+/// A unique [UUID] for an animation target (bone).
+///
+/// The [`AnimationClip`] asset and the [`AnimationTarget`] component both use
+/// this to refer to targets (bones) to be animated.
+///
+/// When importing an armature or an animation clip, asset loaders typically use
+/// the full path name from the armature to the bone to generate these IDs. So,
+/// for example, any imported armature with a bone at the root named `Hips` will
+/// assign the same [`AnimationTargetId`] to its root bone. Likewise, any
+/// imported animation clip that animates a root bone named `Hips` will
+/// reference the same [`AnimationTargetId`]. What this means is that any
+/// animation is playable on any armature as long as the bone names match,
+/// which allows for easy animation retargeting.
+///
+/// Note that asset loaders generally use the *full* path name to generate the
+/// [`AnimationTargetId`]. Thus a bone named `Chest` directly connected to a
+/// bone named `Hips` will have a different ID from a bone named `Chest` that's
+/// connected to a bone named `Stomach`.
+///
+/// [UUID]: https://en.wikipedia.org/wiki/Universally_unique_identifier
+#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Reflect, Debug)]
 pub struct AnimationTargetId(pub Uuid);
 
+/// An entity that can be animated by an [`AnimationPlayer`].
+///
+/// These are frequently referred to as *bones* or *joints*, because they often
+/// refer to individually-animatable parts of an armature.
 #[derive(Clone, Component, Reflect)]
 #[reflect(Component, MapEntities)]
 pub struct AnimationTarget {
-    /// The ID of this bone.
+    /// The ID of this animation target.
     ///
     /// Typically, this is derived from the path.
     pub id: AnimationTargetId,
@@ -170,32 +202,32 @@ pub struct AnimationTarget {
 
 impl AnimationClip {
     #[inline]
-    /// [`VariableCurve`]s for each bone. Indexed by the bone ID.
-    pub fn curves(&self) -> &HashMap<AnimationTargetId, Vec<VariableCurve>> {
+    /// [`VariableCurve`]s for each animation target. Indexed by the [`AnimationTargetId`].
+    pub fn curves(&self) -> &HashMap<AnimationTargetId, Vec<VariableCurve>, NoOpHash> {
         &self.curves
     }
 
     /// Gets the curves for a bone.
     ///
-    /// Returns `None` if the bone is invalid.
+    /// Returns `None` if this clip doesn't animate this bone.
     #[inline]
     pub fn get_curves(&self, bone_id: AnimationTargetId) -> Option<&'_ Vec<VariableCurve>> {
         self.curves.get(&bone_id)
     }
 
-    /// Duration of the clip, represented in seconds
+    /// Duration of the clip, represented in seconds.
     #[inline]
     pub fn duration(&self) -> f32 {
         self.duration
     }
 
-    /// Add a [`VariableCurve`] to an [`EntityPath`].
-    pub fn add_curve_to_path(&mut self, bone_id: AnimationTargetId, curve: VariableCurve) {
+    /// Adds a [`VariableCurve`] to an [`AnimationTarget`] named by an ID.
+    pub fn add_curve_to_target(&mut self, target_id: AnimationTargetId, curve: VariableCurve) {
         // Update the duration of the animation by this curve duration if it's longer
         self.duration = self
             .duration
             .max(*curve.keyframe_timestamps.last().unwrap_or(&0.0));
-        self.curves.entry(bone_id).or_default().push(curve);
+        self.curves.entry(target_id).or_default().push(curve);
     }
 }
 
@@ -473,7 +505,8 @@ impl AnimationPlayer {
     }
 }
 
-pub fn animation_player(
+/// A system that advances the time for all playing animations.
+pub fn update_animations(
     time: Res<Time>,
     animation_clips: Res<Assets<AnimationClip>>,
     mut players: Query<&mut AnimationPlayer>,
@@ -491,11 +524,13 @@ pub fn animation_player(
 
         // Update transition animations.
         for transition in &mut player.transitions {
-            update_animation(&mut transition.animation, paused, &time, &animation_clips)
+            update_animation(&mut transition.animation, paused, &time, &animation_clips);
         }
     }
 }
 
+/// A system that modifies animation targets (bones) according to the
+/// currently-playing animation.
 pub fn animate_targets(
     world: &mut World,
     mut system_state: Local<
@@ -598,7 +633,7 @@ impl Plugin for AnimationPlugin {
             .register_type::<AnimationTarget>()
             .add_systems(
                 PostUpdate,
-                (animation_player, animate_targets)
+                (update_animations, animate_targets)
                     .chain()
                     .before(TransformSystem::TransformPropagate),
             );
@@ -625,7 +660,7 @@ impl PlayingAnimation {
         let target = target_entity.get::<AnimationTarget>().unwrap();
 
         let Some(clip) = clips.get(&self.animation_clip) else {
-            error!(
+            warn!(
                 "Couldn't find the animation clip for {:?}",
                 target_entity.id()
             );
@@ -636,7 +671,7 @@ impl PlayingAnimation {
             return;
         };
 
-        self.apply_curves(curves, weight, target_entity)
+        self.apply_curves(curves, weight, target_entity);
     }
 
     fn apply_curves(&self, curves: &[VariableCurve], weight: f32, target: &mut EntityMut) {
@@ -893,12 +928,21 @@ impl PlayingAnimation {
 }
 
 impl AnimationTargetId {
-    pub fn from_names(names: &[Name]) -> AnimationTargetId {
+    /// Creates a new [`AnimationTargetId`] by hashing a list of names.
+    ///
+    /// Typically, this will be the path from the animation root to the
+    /// animation target (bone) that is to be animated.
+    pub fn from_names<'a>(names: impl Iterator<Item = &'a Name>) -> Self {
         let mut sha1 = Sha1::new();
         sha1.update(ANIMATION_TARGET_NAMESPACE.as_bytes());
-        names.iter().for_each(|name| sha1.update(name.as_bytes()));
+        names.for_each(|name| sha1.update(name.as_bytes()));
         let hash = sha1.digest().bytes()[0..16].try_into().unwrap();
-        AnimationTargetId(*uuid::Builder::from_sha1_bytes(hash).as_uuid())
+        Self(*uuid::Builder::from_sha1_bytes(hash).as_uuid())
+    }
+
+    /// Creates a new [`AnimationTargetId`] by hashing a single name.
+    pub fn from_name(name: &Name) -> Self {
+        Self::from_names(iter::once(name))
     }
 }
 
@@ -908,6 +952,13 @@ impl MapEntities for AnimationTarget {
     }
 }
 
+impl Hash for AnimationTargetId {
+    fn hash<H: Hasher>(&self, state: &mut H) {
+        let (hi, lo) = self.0.as_u64_pair();
+        state.write_u64(hi ^ lo);
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use crate::VariableCurve;
diff --git a/crates/bevy_gltf/src/loader.rs b/crates/bevy_gltf/src/loader.rs
index bef55c95897da..7d6edcb912027 100644
--- a/crates/bevy_gltf/src/loader.rs
+++ b/crates/bevy_gltf/src/loader.rs
@@ -34,7 +34,10 @@ use bevy_scene::Scene;
 #[cfg(not(target_arch = "wasm32"))]
 use bevy_tasks::IoTaskPool;
 use bevy_transform::components::Transform;
-use bevy_utils::{EntityHashMap, HashMap, HashSet};
+use bevy_utils::{
+    smallvec::{smallvec, SmallVec},
+    EntityHashMap, HashMap, HashSet,
+};
 use gltf::{
     accessor::Iter,
     mesh::{util::ReadIndices, Mode},
@@ -260,8 +263,8 @@ async fn load_gltf<'a, 'b, 'c>(
 
                 if let Some((root_index, path)) = paths.get(&node.index()) {
                     animation_roots.insert(*root_index);
-                    animation_clip.add_curve_to_path(
-                        AnimationTargetId::from_names(path),
+                    animation_clip.add_curve_to_target(
+                        AnimationTargetId::from_names(path.iter()),
                         bevy_animation::VariableCurve {
                             keyframe_timestamps,
                             keyframes,
@@ -587,8 +590,8 @@ async fn load_gltf<'a, 'b, 'c>(
                         &mut node_index_to_entity_map,
                         &mut entity_to_skin_index_map,
                         &mut active_camera_found,
-                        &animation_roots,
                         &Transform::default(),
+                        &animation_roots,
                         None,
                     );
                     if result.is_err() {
@@ -925,9 +928,9 @@ fn load_node(
     node_index_to_entity_map: &mut HashMap<usize, Entity>,
     entity_to_skin_index_map: &mut EntityHashMap<Entity, usize>,
     active_camera_found: &mut bool,
-    animation_roots: &HashSet<usize>,
     parent_transform: &Transform,
-    animation_root: Option<(Entity, &[Name])>,
+    animation_roots: &HashSet<usize>,
+    mut animation_context: Option<AnimationContext>,
 ) -> Result<(), GltfError> {
     let mut gltf_error = None;
     let transform = node_transform(gltf_node);
@@ -944,26 +947,24 @@ fn load_node(
     let name = node_name(gltf_node);
     node.insert(name.clone());
 
-    let mut path_buf = vec![];
-    let animation_root = match animation_root {
+    #[cfg(feature = "bevy_animation")]
+    match animation_context {
         None => {
             if animation_roots.contains(&gltf_node.index()) {
-                path_buf.push(name);
-                Some((node.id(), &path_buf[..]))
-            } else {
-                None
+                // This is an animation root. Make a new animation context.
+                animation_context = Some(AnimationContext {
+                    root: node.id(),
+                    path: smallvec![name],
+                });
             }
         }
-        Some((root, path)) => {
-            path_buf.extend(path.iter().cloned());
-            path_buf.push(name);
+        Some(ref mut animation_context) => {
+            animation_context.path.push(name);
 
             node.insert(AnimationTarget {
-                id: AnimationTargetId::from_names(&path_buf),
-                root,
+                id: AnimationTargetId::from_names(animation_context.path.iter()),
+                root: animation_context.root,
             });
-
-            Some((root, &path_buf[..]))
         }
     };
 
@@ -1178,9 +1179,9 @@ fn load_node(
                 node_index_to_entity_map,
                 entity_to_skin_index_map,
                 active_camera_found,
-                animation_roots,
                 &world_transform,
-                animation_root,
+                animation_roots,
+                animation_context.clone(),
             ) {
                 gltf_error = Some(err);
                 return;
@@ -1537,6 +1538,22 @@ struct MorphTargetNames {
     pub target_names: Vec<String>,
 }
 
+// A helper structure for `load_node` that contains information about the
+// nearest ancestor animation root.
+#[cfg(feature = "bevy_animation")]
+#[derive(Clone)]
+struct AnimationContext {
+    // The nearest ancestor animation root.
+    root: Entity,
+    // The path to the animation root. This is used for constructing the
+    // animation target UUIDs.
+    path: SmallVec<[Name; 8]>,
+}
+
+#[cfg(not(feature = "bevy_animation"))]
+#[derive(Clone)]
+struct AnimationContext;
+
 #[cfg(test)]
 mod test {
     use std::path::PathBuf;
diff --git a/crates/bevy_internal/src/default_plugins.rs b/crates/bevy_internal/src/default_plugins.rs
index ccf992cd02e5b..68a0583cda51d 100644
--- a/crates/bevy_internal/src/default_plugins.rs
+++ b/crates/bevy_internal/src/default_plugins.rs
@@ -149,7 +149,7 @@ impl Plugin for IgnoreAmbiguitiesPlugin {
         #[cfg(all(feature = "bevy_animation", feature = "bevy_ui"))]
         app.ignore_ambiguity(
             bevy_app::PostUpdate,
-            bevy_animation::animation_player,
+            bevy_animation::update_animations,
             bevy_ui::ui_layout_system,
         );
 
diff --git a/crates/bevy_reflect/src/impls/std.rs b/crates/bevy_reflect/src/impls/std.rs
index 61ae6db26bc77..d51332dcc9751 100644
--- a/crates/bevy_reflect/src/impls/std.rs
+++ b/crates/bevy_reflect/src/impls/std.rs
@@ -610,6 +610,7 @@ impl_type_path!(::std::collections::HashMap<K, V, S>);
 
 impl_reflect_for_hashmap!(bevy_utils::hashbrown::HashMap<K, V, S>);
 impl_type_path!(::bevy_utils::hashbrown::hash_map::DefaultHashBuilder);
+impl_type_path!(::bevy_utils::NoOpHash);
 impl_type_path!(::bevy_utils::hashbrown::HashMap<K, V, S>);
 
 impl<T: Reflect + TypePath, const N: usize> Array for [T; N] {
diff --git a/crates/bevy_reflect/src/utility.rs b/crates/bevy_reflect/src/utility.rs
index c658fa64c8c50..2c026441ed5c1 100644
--- a/crates/bevy_reflect/src/utility.rs
+++ b/crates/bevy_reflect/src/utility.rs
@@ -1,7 +1,7 @@
 //! Helpers for working with Bevy reflection.
 
 use crate::TypeInfo;
-use bevy_utils::{FixedState, NoOpTypeIdHash, TypeIdMap};
+use bevy_utils::{FixedState, NoOpHash, TypeIdMap};
 use std::{
     any::{Any, TypeId},
     hash::BuildHasher,
@@ -205,7 +205,7 @@ pub type GenericTypePathCell = GenericTypeCell<TypePathComponent>;
 impl<T: TypedProperty> GenericTypeCell<T> {
     /// Initialize a [`GenericTypeCell`] for generic types.
     pub const fn new() -> Self {
-        Self(RwLock::new(TypeIdMap::with_hasher(NoOpTypeIdHash)))
+        Self(RwLock::new(TypeIdMap::with_hasher(NoOpHash)))
     }
 
     /// Returns a reference to the [`TypedProperty`] stored in the cell.
diff --git a/crates/bevy_utils/src/lib.rs b/crates/bevy_utils/src/lib.rs
index 840b7ae80c316..8aa88df10257b 100644
--- a/crates/bevy_utils/src/lib.rs
+++ b/crates/bevy_utils/src/lib.rs
@@ -340,38 +340,39 @@ pub type EntityHashSet<T> = hashbrown::HashSet<T, EntityHash>;
 
 /// A specialized hashmap type with Key of [`TypeId`]
 /// Iteration order only depends on the order of insertions and deletions.
-pub type TypeIdMap<V> = hashbrown::HashMap<TypeId, V, NoOpTypeIdHash>;
+pub type TypeIdMap<V> = hashbrown::HashMap<TypeId, V, NoOpHash>;
 
-/// [`BuildHasher`] for [`TypeId`]s.
-#[derive(Default)]
-pub struct NoOpTypeIdHash;
+/// [`BuildHasher`] for types that already contain a high-quality hash.
+#[derive(Clone, Default)]
+pub struct NoOpHash;
 
-impl BuildHasher for NoOpTypeIdHash {
-    type Hasher = NoOpTypeIdHasher;
+impl BuildHasher for NoOpHash {
+    type Hasher = NoOpHasher;
 
     fn build_hasher(&self) -> Self::Hasher {
-        NoOpTypeIdHasher(0)
+        NoOpHasher(0)
     }
 }
 
 #[doc(hidden)]
-pub struct NoOpTypeIdHasher(u64);
+pub struct NoOpHasher(u64);
 
-// TypeId already contains a high-quality hash, so skip re-hashing that hash.
-impl std::hash::Hasher for NoOpTypeIdHasher {
+// This is for types that already contain a high-quality hash and want to skip
+// re-hashing that hash.
+impl std::hash::Hasher for NoOpHasher {
     fn finish(&self) -> u64 {
         self.0
     }
 
     fn write(&mut self, bytes: &[u8]) {
-        // This will never be called: TypeId always just calls write_u64 once!
-        // This is a known trick and unlikely to change, but isn't officially guaranteed.
+        // This should never be called by consumers. Prefer to call `write_u64` instead.
         // Don't break applications (slower fallback, just check in test):
         self.0 = bytes.iter().fold(self.0, |hash, b| {
             hash.rotate_left(8).wrapping_add(*b as u64)
         });
     }
 
+    #[inline]
     fn write_u64(&mut self, i: u64) {
         self.0 = i;
     }
diff --git a/examples/animation/animated_transform.rs b/examples/animation/animated_transform.rs
index eea4129d456d6..90ee615d2ef28 100644
--- a/examples/animation/animated_transform.rs
+++ b/examples/animation/animated_transform.rs
@@ -36,8 +36,8 @@ fn setup(
     // Creating the animation
     let mut animation = AnimationClip::default();
     // A curve can modify a single part of a transform, here the translation
-    animation.add_curve_to_path(
-        AnimationTargetId::from_names(&[planet.clone()]),
+    animation.add_curve_to_target(
+        AnimationTargetId::from_name(&planet),
         VariableCurve {
             keyframe_timestamps: vec![0.0, 1.0, 2.0, 3.0, 4.0],
             keyframes: Keyframes::Translation(vec![
@@ -56,8 +56,8 @@ fn setup(
     // To find the entity to modify, the hierarchy will be traversed looking for
     // an entity with the right name at each level
     let orbit_controller_animation_target_id =
-        AnimationTargetId::from_names(&[planet.clone(), orbit_controller.clone()]);
-    animation.add_curve_to_path(
+        AnimationTargetId::from_names([planet.clone(), orbit_controller.clone()].iter());
+    animation.add_curve_to_target(
         orbit_controller_animation_target_id,
         VariableCurve {
             keyframe_timestamps: vec![0.0, 1.0, 2.0, 3.0, 4.0],
@@ -74,12 +74,10 @@ fn setup(
     // If a curve in an animation is shorter than the other, it will not repeat
     // until all other curves are finished. In that case, another animation should
     // be created for each part that would have a different duration / period
-    let satellite_animation_target_id = AnimationTargetId::from_names(&[
-        planet.clone(),
-        orbit_controller.clone(),
-        satellite.clone(),
-    ]);
-    animation.add_curve_to_path(
+    let satellite_animation_target_id = AnimationTargetId::from_names(
+        [planet.clone(), orbit_controller.clone(), satellite.clone()].iter(),
+    );
+    animation.add_curve_to_target(
         satellite_animation_target_id,
         VariableCurve {
             keyframe_timestamps: vec![0.0, 0.5, 1.0, 1.5, 2.0, 2.5, 3.0, 3.5, 4.0],
@@ -98,12 +96,10 @@ fn setup(
         },
     );
     // There can be more than one curve targeting the same entity path
-    animation.add_curve_to_path(
-        AnimationTargetId::from_names(&[
-            planet.clone(),
-            orbit_controller.clone(),
-            satellite.clone(),
-        ]),
+    animation.add_curve_to_target(
+        AnimationTargetId::from_names(
+            [planet.clone(), orbit_controller.clone(), satellite.clone()].iter(),
+        ),
         VariableCurve {
             keyframe_timestamps: vec![0.0, 1.0, 2.0, 3.0, 4.0],
             keyframes: Keyframes::Rotation(vec![
diff --git a/examples/tools/scene_viewer/animation_plugin.rs b/examples/tools/scene_viewer/animation_plugin.rs
index ac5c2fdbaee92..a0f65dcc37830 100644
--- a/examples/tools/scene_viewer/animation_plugin.rs
+++ b/examples/tools/scene_viewer/animation_plugin.rs
@@ -31,7 +31,7 @@ impl Clips {
 /// animation clips for given entities in the [`Clips`] component we defined
 /// earlier in this file.
 fn assign_clips(
-    mut players: Query<(Entity, &mut AnimationPlayer, &Name)>,
+    mut players: Query<(Entity, &mut AnimationPlayer)>,
     scene_handle: Res<SceneHandle>,
     clips: Res<Assets<AnimationClip>>,
     gltf_assets: Res<Assets<Gltf>>,
@@ -53,10 +53,10 @@ fn assign_clips(
         let names: Vec<_> = gltf.named_animations.keys().collect();
         info!("Animation names: {names:?}");
     }
-    for (entity, mut player, name) in &mut players {
+    for (entity, mut player) in &mut players {
         let clips = clips
             .iter()
-            .filter_map(|(k, v)| v.compatible_with(name).then_some(k))
+            .map(|(k, _)| k)
             .map(|id| assets.get_id_handle(id).unwrap())
             .collect();
         let animations = Clips::new(clips);

From 4392fa73fe7068ae2ea9037587aaf32c56df480d Mon Sep 17 00:00:00 2001
From: Patrick Walton <pcwalton@mimiga.net>
Date: Mon, 5 Feb 2024 16:31:08 -0800
Subject: [PATCH 06/27] No `bevy_internal` usage

---
 examples/animation/animated_transform.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/examples/animation/animated_transform.rs b/examples/animation/animated_transform.rs
index 90ee615d2ef28..15e573c1b18b8 100644
--- a/examples/animation/animated_transform.rs
+++ b/examples/animation/animated_transform.rs
@@ -2,8 +2,8 @@
 
 use std::f32::consts::PI;
 
+use bevy::animation::{AnimationTarget, AnimationTargetId};
 use bevy::prelude::*;
-use bevy_internal::animation::{AnimationTarget, AnimationTargetId};
 
 fn main() {
     App::new()

From 65c3f2ed3f4e3fd859a5ed695509a7a498fb188d Mon Sep 17 00:00:00 2001
From: Patrick Walton <pcwalton@mimiga.net>
Date: Mon, 5 Feb 2024 16:32:57 -0800
Subject: [PATCH 07/27] Address review comment

---
 crates/bevy_animation/src/lib.rs | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/crates/bevy_animation/src/lib.rs b/crates/bevy_animation/src/lib.rs
index f9df665d06c9c..ed23b97532da6 100644
--- a/crates/bevy_animation/src/lib.rs
+++ b/crates/bevy_animation/src/lib.rs
@@ -512,7 +512,10 @@ pub fn update_animations(
     mut players: Query<&mut AnimationPlayer>,
 ) {
     for mut player in players.iter_mut() {
-        update_transitions(&mut player, &time);
+        player.transitions.retain_mut(|animation| {
+            animation.current_weight -= animation.weight_decline_per_sec * time.delta_seconds();
+            animation.current_weight > 0.0
+        });
 
         let paused = player.paused;
         if paused {
@@ -614,13 +617,6 @@ where
         + tangent_in_end * step_duration * (lerp.powi(3) - lerp.powi(2))
 }
 
-fn update_transitions(player: &mut AnimationPlayer, time: &Time) {
-    player.transitions.retain_mut(|animation| {
-        animation.current_weight -= animation.weight_decline_per_sec * time.delta_seconds();
-        animation.current_weight > 0.0
-    });
-}
-
 /// Adds animation support to an app
 #[derive(Default)]
 pub struct AnimationPlugin;

From 6d28e754358e2def42532aa5805660c1de5a65d6 Mon Sep 17 00:00:00 2001
From: Patrick Walton <pcwalton@mimiga.net>
Date: Mon, 5 Feb 2024 16:35:19 -0800
Subject: [PATCH 08/27] Rename `update_animations` to `advance_animations`

---
 crates/bevy_animation/src/lib.rs            | 4 ++--
 crates/bevy_internal/src/default_plugins.rs | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/crates/bevy_animation/src/lib.rs b/crates/bevy_animation/src/lib.rs
index ed23b97532da6..ac652fff02585 100644
--- a/crates/bevy_animation/src/lib.rs
+++ b/crates/bevy_animation/src/lib.rs
@@ -506,7 +506,7 @@ impl AnimationPlayer {
 }
 
 /// A system that advances the time for all playing animations.
-pub fn update_animations(
+pub fn advance_animations(
     time: Res<Time>,
     animation_clips: Res<Assets<AnimationClip>>,
     mut players: Query<&mut AnimationPlayer>,
@@ -629,7 +629,7 @@ impl Plugin for AnimationPlugin {
             .register_type::<AnimationTarget>()
             .add_systems(
                 PostUpdate,
-                (update_animations, animate_targets)
+                (advance_animations, animate_targets)
                     .chain()
                     .before(TransformSystem::TransformPropagate),
             );
diff --git a/crates/bevy_internal/src/default_plugins.rs b/crates/bevy_internal/src/default_plugins.rs
index 68a0583cda51d..c0869a24909f2 100644
--- a/crates/bevy_internal/src/default_plugins.rs
+++ b/crates/bevy_internal/src/default_plugins.rs
@@ -149,7 +149,7 @@ impl Plugin for IgnoreAmbiguitiesPlugin {
         #[cfg(all(feature = "bevy_animation", feature = "bevy_ui"))]
         app.ignore_ambiguity(
             bevy_app::PostUpdate,
-            bevy_animation::update_animations,
+            bevy_animation::advance_animations,
             bevy_ui::ui_layout_system,
         );
 

From ee92b8f69ccd8ba7fd282afe57ea87df5ddb2d33 Mon Sep 17 00:00:00 2001
From: Patrick Walton <pcwalton@mimiga.net>
Date: Mon, 5 Feb 2024 17:05:45 -0800
Subject: [PATCH 09/27] Address review comments

---
 crates/bevy_animation/src/lib.rs | 84 ++++++++++++++++++++------------
 1 file changed, 53 insertions(+), 31 deletions(-)

diff --git a/crates/bevy_animation/src/lib.rs b/crates/bevy_animation/src/lib.rs
index ac652fff02585..af78ef102d497 100644
--- a/crates/bevy_animation/src/lib.rs
+++ b/crates/bevy_animation/src/lib.rs
@@ -512,28 +512,39 @@ pub fn advance_animations(
     mut players: Query<&mut AnimationPlayer>,
 ) {
     for mut player in players.iter_mut() {
-        player.transitions.retain_mut(|animation| {
-            animation.current_weight -= animation.weight_decline_per_sec * time.delta_seconds();
-            animation.current_weight > 0.0
-        });
-
         let paused = player.paused;
         if paused {
             continue;
         }
 
-        // Update the main animation.
-        update_animation(&mut player.animation, paused, &time, &animation_clips);
+        // Advance the main animation.
+        if let Some(animation_clip) = animation_clips.get(&player.animation.animation_clip) {
+            player
+                .animation
+                .update(time.delta_seconds(), animation_clip.duration);
+        };
 
-        // Update transition animations.
+        // Advance transition animations.
         for transition in &mut player.transitions {
-            update_animation(&mut transition.animation, paused, &time, &animation_clips);
+            // Decrease weight. But don't expire the transition yet if the time
+            // ran out; we still need to update the bones.
+            transition.current_weight = (transition.current_weight
+                - transition.weight_decline_per_sec * time.delta_seconds())
+            .max(0.0);
+
+            if let Some(animation_clip) = animation_clips.get(&transition.animation.animation_clip)
+            {
+                transition
+                    .animation
+                    .update(time.delta_seconds(), animation_clip.duration);
+            };
         }
     }
 }
 
 /// A system that modifies animation targets (bones) according to the
 /// currently-playing animation.
+///
 pub fn animate_targets(
     world: &mut World,
     mut system_state: Local<
@@ -549,17 +560,25 @@ pub fn animate_targets(
         *system_state = Some(SystemState::new(world));
     }
 
+    // We use two queries here: one read-only query for animation players and
+    // one read-write query for animation targets (bones). The `AnimationPlayer`
+    // query is read-only shared memory accessible from all animation targets,
+    // which are evaluated in parallel.
+
     // This elaborate setup using `resource_scope` is a workaround for the fact
     // that `EntityMut` queries lock out resources. This is a known bug in Bevy.
     world.resource_scope(|world, clips: Mut<Assets<AnimationClip>>| {
         let (players, mut targets) = system_state.as_mut().unwrap().get_mut(world);
+
+        // Iterate over all animation targets (bones) in parallel.
         targets.par_iter_mut().for_each(|mut target_entity| {
             let target = target_entity.get::<AnimationTarget>().unwrap();
             let Ok(player) = players.get(target.root) else {
                 error!(
-                    "Couldn't find the animation player {:?} for {:?}",
+                    "Couldn't find the animation player {:?} for {:?} ({:?})",
                     target.root,
-                    target_entity.id()
+                    target_entity.id(),
+                    target_entity.get::<Name>()
                 );
                 return;
             };
@@ -575,6 +594,18 @@ pub fn animate_targets(
     });
 }
 
+/// Removes animation transitions that have expired.
+///
+/// This needs to be done after applying the animations so that transitions
+/// don't miss applying their last frame on the frame they expire.
+pub fn expire_animation_transitions(mut players: Query<&mut AnimationPlayer>) {
+    for mut player in players.iter_mut() {
+        player
+            .transitions
+            .retain_mut(|transition| transition.current_weight > 0.0);
+    }
+}
+
 /// Update `weights` based on weights in `keyframe` with a linear interpolation
 /// on `key_lerp`.
 fn lerp_morph_weights(weights: &mut [f32], keyframe: impl Iterator<Item = f32>, key_lerp: f32) {
@@ -629,36 +660,26 @@ impl Plugin for AnimationPlugin {
             .register_type::<AnimationTarget>()
             .add_systems(
                 PostUpdate,
-                (advance_animations, animate_targets)
+                (
+                    advance_animations,
+                    animate_targets,
+                    expire_animation_transitions,
+                )
                     .chain()
                     .before(TransformSystem::TransformPropagate),
             );
     }
 }
 
-fn update_animation(
-    animation: &mut PlayingAnimation,
-    paused: bool,
-    time: &Time,
-    animation_clips: &Assets<AnimationClip>,
-) {
-    let Some(animation_clip) = animation_clips.get(&animation.animation_clip) else {
-        return;
-    };
-
-    let delta = if paused { 0.0 } else { time.delta_seconds() };
-
-    animation.update(delta, animation_clip.duration);
-}
-
 impl PlayingAnimation {
     fn apply(&self, clips: &Assets<AnimationClip>, weight: f32, target_entity: &mut EntityMut) {
         let target = target_entity.get::<AnimationTarget>().unwrap();
 
         let Some(clip) = clips.get(&self.animation_clip) else {
             warn!(
-                "Couldn't find the animation clip for {:?}",
-                target_entity.id()
+                "Couldn't find the animation clip for {:?} ({:?})",
+                target_entity.id(),
+                target_entity.get::<Name>()
             );
             return;
         };
@@ -722,8 +743,9 @@ impl PlayingAnimation {
             Keyframes::Weights(keyframes) => {
                 let Some(mut morphs) = target.get_mut::<MorphWeights>() else {
                     error!(
-                        "Tried to animate morphs on {:?}, but no `MorphWeights` was found",
-                        target.id()
+                        "Tried to animate morphs on {:?} ({:?}), but no `MorphWeights` was found",
+                        target.id(),
+                        target.get::<Name>()
                     );
                     return;
                 };

From b08435ca2f73187672c6cb8f8122629b6e41414c Mon Sep 17 00:00:00 2001
From: Patrick Walton <pcwalton@mimiga.net>
Date: Mon, 5 Feb 2024 17:09:24 -0800
Subject: [PATCH 10/27] Address review comment

---
 crates/bevy_animation/src/lib.rs | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/crates/bevy_animation/src/lib.rs b/crates/bevy_animation/src/lib.rs
index af78ef102d497..905426b8f6585 100644
--- a/crates/bevy_animation/src/lib.rs
+++ b/crates/bevy_animation/src/lib.rs
@@ -157,10 +157,13 @@ pub enum Interpolation {
 /// [`AnimationTarget`] with that ID.
 #[derive(Asset, Reflect, Clone, Debug, Default)]
 pub struct AnimationClip {
-    curves: HashMap<AnimationTargetId, Vec<VariableCurve>, NoOpHash>,
+    curves: AnimationCurves,
     duration: f32,
 }
 
+/// A mapping from [`AnimationTargetId`] (bone) to the animation curves.
+pub type AnimationCurves = HashMap<AnimationTargetId, Vec<VariableCurve>, NoOpHash>;
+
 /// A unique [UUID] for an animation target (bone).
 ///
 /// The [`AnimationClip`] asset and the [`AnimationTarget`] component both use
@@ -203,7 +206,7 @@ pub struct AnimationTarget {
 impl AnimationClip {
     #[inline]
     /// [`VariableCurve`]s for each animation target. Indexed by the [`AnimationTargetId`].
-    pub fn curves(&self) -> &HashMap<AnimationTargetId, Vec<VariableCurve>, NoOpHash> {
+    pub fn curves(&self) -> &AnimationCurves {
         &self.curves
     }
 

From 30ccfac62418c677e95210c54a5265ec1a39c6f4 Mon Sep 17 00:00:00 2001
From: Patrick Walton <pcwalton@mimiga.net>
Date: Mon, 5 Feb 2024 17:16:31 -0800
Subject: [PATCH 11/27] Address review comment

---
 crates/bevy_animation/src/lib.rs | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/crates/bevy_animation/src/lib.rs b/crates/bevy_animation/src/lib.rs
index 905426b8f6585..608f0222d1c1e 100644
--- a/crates/bevy_animation/src/lib.rs
+++ b/crates/bevy_animation/src/lib.rs
@@ -224,7 +224,12 @@ impl AnimationClip {
         self.duration
     }
 
-    /// Adds a [`VariableCurve`] to an [`AnimationTarget`] named by an ID.
+    /// Adds a [`VariableCurve`] to an [`AnimationTarget`] named by an
+    /// [`AnimationTargetId`].
+    ///
+    /// If the curve extends beyond the current duration of this clip, this
+    /// method lengthens this clip to include the entire time span that the
+    /// curve covers.
     pub fn add_curve_to_target(&mut self, target_id: AnimationTargetId, curve: VariableCurve) {
         // Update the duration of the animation by this curve duration if it's longer
         self.duration = self

From 59d25a122e28bd7cd25162d6e3724aaffe75ee21 Mon Sep 17 00:00:00 2001
From: Patrick Walton <pcwalton@mimiga.net>
Date: Tue, 6 Feb 2024 10:55:50 -0800
Subject: [PATCH 12/27] Address review comment

---
 crates/bevy_animation/src/lib.rs | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/crates/bevy_animation/src/lib.rs b/crates/bevy_animation/src/lib.rs
index 608f0222d1c1e..19f908ed2b34f 100644
--- a/crates/bevy_animation/src/lib.rs
+++ b/crates/bevy_animation/src/lib.rs
@@ -555,19 +555,11 @@ pub fn advance_animations(
 ///
 pub fn animate_targets(
     world: &mut World,
-    mut system_state: Local<
-        Option<
-            SystemState<(
-                Query<&AnimationPlayer, Without<AnimationTarget>>,
-                Query<EntityMut, With<AnimationTarget>>,
-            )>,
-        >,
-    >,
+    system_state: &mut SystemState<(
+        Query<&AnimationPlayer, Without<AnimationTarget>>,
+        Query<EntityMut, With<AnimationTarget>>,
+    )>,
 ) {
-    if system_state.is_none() {
-        *system_state = Some(SystemState::new(world));
-    }
-
     // We use two queries here: one read-only query for animation players and
     // one read-write query for animation targets (bones). The `AnimationPlayer`
     // query is read-only shared memory accessible from all animation targets,
@@ -576,7 +568,7 @@ pub fn animate_targets(
     // This elaborate setup using `resource_scope` is a workaround for the fact
     // that `EntityMut` queries lock out resources. This is a known bug in Bevy.
     world.resource_scope(|world, clips: Mut<Assets<AnimationClip>>| {
-        let (players, mut targets) = system_state.as_mut().unwrap().get_mut(world);
+        let (players, mut targets) = system_state.get_mut(world);
 
         // Iterate over all animation targets (bones) in parallel.
         targets.par_iter_mut().for_each(|mut target_entity| {

From 38c0fa76bdf9b74bd62c03a54f15b91fabffa5a8 Mon Sep 17 00:00:00 2001
From: Patrick Walton <pcwalton@mimiga.net>
Date: Tue, 6 Feb 2024 20:23:37 -0800
Subject: [PATCH 13/27] Address review comments

---
 crates/bevy_animation/src/lib.rs | 121 ++++++++++++++++++-------------
 1 file changed, 72 insertions(+), 49 deletions(-)

diff --git a/crates/bevy_animation/src/lib.rs b/crates/bevy_animation/src/lib.rs
index 19f908ed2b34f..4c9ba00bd8386 100644
--- a/crates/bevy_animation/src/lib.rs
+++ b/crates/bevy_animation/src/lib.rs
@@ -14,7 +14,6 @@ use bevy_core::Name;
 use bevy_ecs::entity::MapEntities;
 use bevy_ecs::prelude::*;
 use bevy_ecs::reflect::ReflectMapEntities;
-use bevy_ecs::system::SystemState;
 use bevy_log::{error, warn};
 use bevy_math::{FloatExt, Quat, Vec3};
 use bevy_reflect::Reflect;
@@ -359,6 +358,16 @@ pub struct AnimationPlayer {
     transitions: Vec<AnimationTransition>,
 }
 
+// The components that we might need to read or write during animation of each
+// animation target.
+struct AnimationTargetContext<'a> {
+    id: Entity,
+    target: &'a AnimationTarget,
+    name: Option<&'a Name>,
+    transform: Option<Mut<'a, Transform>>,
+    morph_weights: Option<Mut<'a, MorphWeights>>,
+}
+
 impl AnimationPlayer {
     /// Start playing an animation, resetting state of the player.
     /// This will use a linear blending between the previous and the new animation to make a smooth transition.
@@ -552,12 +561,14 @@ pub fn advance_animations(
 
 /// A system that modifies animation targets (bones) according to the
 /// currently-playing animation.
-///
 pub fn animate_targets(
-    world: &mut World,
-    system_state: &mut SystemState<(
-        Query<&AnimationPlayer, Without<AnimationTarget>>,
-        Query<EntityMut, With<AnimationTarget>>,
+    clips: Res<Assets<AnimationClip>>,
+    players: Query<&AnimationPlayer, Without<AnimationTarget>>,
+    mut targets: Query<(
+        Entity,
+        &AnimationTarget,
+        Option<&Name>,
+        AnyOf<(&mut Transform, &mut MorphWeights)>,
     )>,
 ) {
     // We use two queries here: one read-only query for animation players and
@@ -565,33 +576,34 @@ pub fn animate_targets(
     // query is read-only shared memory accessible from all animation targets,
     // which are evaluated in parallel.
 
-    // This elaborate setup using `resource_scope` is a workaround for the fact
-    // that `EntityMut` queries lock out resources. This is a known bug in Bevy.
-    world.resource_scope(|world, clips: Mut<Assets<AnimationClip>>| {
-        let (players, mut targets) = system_state.get_mut(world);
+    // Iterate over all animation targets (bones) in parallel.
+    targets
+        .par_iter_mut()
+        .for_each(|(id, target, name, (transform, morph_weights))| {
+            let mut target_context = AnimationTargetContext {
+                id,
+                target,
+                name,
+                transform,
+                morph_weights,
+            };
 
-        // Iterate over all animation targets (bones) in parallel.
-        targets.par_iter_mut().for_each(|mut target_entity| {
-            let target = target_entity.get::<AnimationTarget>().unwrap();
             let Ok(player) = players.get(target.root) else {
                 error!(
                     "Couldn't find the animation player {:?} for {:?} ({:?})",
-                    target.root,
-                    target_entity.id(),
-                    target_entity.get::<Name>()
+                    target.root, target_context.id, target_context.name,
                 );
                 return;
             };
 
-            player.animation.apply(&clips, 1.0, &mut target_entity);
+            player.animation.apply(&clips, 1.0, &mut target_context);
 
             for transition in &player.transitions {
                 transition
                     .animation
-                    .apply(&clips, transition.current_weight, &mut target_entity);
+                    .apply(&clips, transition.current_weight, &mut target_context);
             }
         });
-    });
 }
 
 /// Removes animation transitions that have expired.
@@ -672,30 +684,37 @@ impl Plugin for AnimationPlugin {
 }
 
 impl PlayingAnimation {
-    fn apply(&self, clips: &Assets<AnimationClip>, weight: f32, target_entity: &mut EntityMut) {
-        let target = target_entity.get::<AnimationTarget>().unwrap();
-
+    fn apply(
+        &self,
+        clips: &Assets<AnimationClip>,
+        weight: f32,
+        target_context: &mut AnimationTargetContext,
+    ) {
         let Some(clip) = clips.get(&self.animation_clip) else {
             warn!(
                 "Couldn't find the animation clip for {:?} ({:?})",
-                target_entity.id(),
-                target_entity.get::<Name>()
+                target_context.id, target_context.name,
             );
             return;
         };
 
-        let Some(curves) = clip.get_curves(target.id) else {
+        let Some(curves) = clip.get_curves(target_context.target.id) else {
             return;
         };
 
-        self.apply_curves(curves, weight, target_entity);
+        self.apply_curves(curves, weight, target_context);
     }
 
-    fn apply_curves(&self, curves: &[VariableCurve], weight: f32, target: &mut EntityMut) {
+    fn apply_curves(
+        &self,
+        curves: &[VariableCurve],
+        weight: f32,
+        target_context: &mut AnimationTargetContext,
+    ) {
         for curve in curves {
             // Some curves have only one keyframe used to set a transform
             if curve.keyframe_timestamps.len() == 1 {
-                self.apply_single_keyframe(curve, weight, target);
+                self.apply_single_keyframe(curve, weight, target_context);
                 return;
             }
 
@@ -715,37 +734,41 @@ impl PlayingAnimation {
                 weight,
                 lerp,
                 timestamp_end - timestamp_start,
-                target,
+                target_context,
             );
         }
     }
 
-    fn apply_single_keyframe(&self, curve: &VariableCurve, weight: f32, target: &mut EntityMut) {
+    fn apply_single_keyframe(
+        &self,
+        curve: &VariableCurve,
+        weight: f32,
+        target_context: &mut AnimationTargetContext,
+    ) {
         match &curve.keyframes {
             Keyframes::Rotation(keyframes) => {
-                if let Some(mut transform) = target.get_mut::<Transform>() {
+                if let Some(ref mut transform) = target_context.transform {
                     transform.rotation = transform.rotation.slerp(keyframes[0], weight);
                 }
             }
 
             Keyframes::Translation(keyframes) => {
-                if let Some(mut transform) = target.get_mut::<Transform>() {
+                if let Some(ref mut transform) = target_context.transform {
                     transform.translation = transform.translation.lerp(keyframes[0], weight);
                 }
             }
 
             Keyframes::Scale(keyframes) => {
-                if let Some(mut transform) = target.get_mut::<Transform>() {
+                if let Some(ref mut transform) = target_context.transform {
                     transform.scale = transform.scale.lerp(keyframes[0], weight);
                 }
             }
 
             Keyframes::Weights(keyframes) => {
-                let Some(mut morphs) = target.get_mut::<MorphWeights>() else {
+                let Some(ref mut morphs) = target_context.morph_weights else {
                     error!(
                         "Tried to animate morphs on {:?} ({:?}), but no `MorphWeights` was found",
-                        target.id(),
-                        target.get::<Name>()
+                        target_context.id, target_context.name,
                     );
                     return;
                 };
@@ -767,17 +790,17 @@ impl PlayingAnimation {
         weight: f32,
         lerp: f32,
         duration: f32,
-        target: &mut EntityMut,
+        target_context: &mut AnimationTargetContext,
     ) {
         match (&curve.interpolation, &curve.keyframes) {
             (Interpolation::Step, Keyframes::Rotation(keyframes)) => {
-                if let Some(mut transform) = target.get_mut::<Transform>() {
+                if let Some(ref mut transform) = target_context.transform {
                     transform.rotation = transform.rotation.slerp(keyframes[step_start], weight);
                 }
             }
 
             (Interpolation::Linear, Keyframes::Rotation(keyframes)) => {
-                let Some(mut transform) = target.get_mut::<Transform>() else {
+                let Some(ref mut transform) = target_context.transform else {
                     return;
                 };
 
@@ -793,7 +816,7 @@ impl PlayingAnimation {
             }
 
             (Interpolation::CubicSpline, Keyframes::Rotation(keyframes)) => {
-                let Some(mut transform) = target.get_mut::<Transform>() else {
+                let Some(ref mut transform) = target_context.transform else {
                     return;
                 };
 
@@ -813,14 +836,14 @@ impl PlayingAnimation {
             }
 
             (Interpolation::Step, Keyframes::Translation(keyframes)) => {
-                if let Some(mut transform) = target.get_mut::<Transform>() {
+                if let Some(ref mut transform) = target_context.transform {
                     transform.translation =
                         transform.translation.lerp(keyframes[step_start], weight);
                 }
             }
 
             (Interpolation::Linear, Keyframes::Translation(keyframes)) => {
-                let Some(mut transform) = target.get_mut::<Transform>() else {
+                let Some(ref mut transform) = target_context.transform else {
                     return;
                 };
 
@@ -831,7 +854,7 @@ impl PlayingAnimation {
             }
 
             (Interpolation::CubicSpline, Keyframes::Translation(keyframes)) => {
-                let Some(mut transform) = target.get_mut::<Transform>() else {
+                let Some(ref mut transform) = target_context.transform else {
                     return;
                 };
 
@@ -851,13 +874,13 @@ impl PlayingAnimation {
             }
 
             (Interpolation::Step, Keyframes::Scale(keyframes)) => {
-                if let Some(mut transform) = target.get_mut::<Transform>() {
+                if let Some(ref mut transform) = target_context.transform {
                     transform.scale = transform.scale.lerp(keyframes[step_start], weight);
                 }
             }
 
             (Interpolation::Linear, Keyframes::Scale(keyframes)) => {
-                let Some(mut transform) = target.get_mut::<Transform>() else {
+                let Some(ref mut transform) = target_context.transform else {
                     return;
                 };
 
@@ -868,7 +891,7 @@ impl PlayingAnimation {
             }
 
             (Interpolation::CubicSpline, Keyframes::Scale(keyframes)) => {
-                let Some(mut transform) = target.get_mut::<Transform>() else {
+                let Some(ref mut transform) = target_context.transform else {
                     return;
                 };
 
@@ -888,7 +911,7 @@ impl PlayingAnimation {
             }
 
             (Interpolation::Step, Keyframes::Weights(keyframes)) => {
-                let Some(mut morphs) = target.get_mut::<MorphWeights>() else {
+                let Some(ref mut morphs) = target_context.morph_weights else {
                     return;
                 };
 
@@ -898,7 +921,7 @@ impl PlayingAnimation {
             }
 
             (Interpolation::Linear, Keyframes::Weights(keyframes)) => {
-                let Some(mut morphs) = target.get_mut::<MorphWeights>() else {
+                let Some(ref mut morphs) = target_context.morph_weights else {
                     return;
                 };
 
@@ -913,7 +936,7 @@ impl PlayingAnimation {
             }
 
             (Interpolation::CubicSpline, Keyframes::Weights(keyframes)) => {
-                let Some(mut morphs) = target.get_mut::<MorphWeights>() else {
+                let Some(ref mut morphs) = target_context.morph_weights else {
                     return;
                 };
 

From dfe424b7fbf18f05d0e63e5d891fb7c63f398ce8 Mon Sep 17 00:00:00 2001
From: Patrick Walton <pcwalton@mimiga.net>
Date: Tue, 6 Feb 2024 20:38:44 -0800
Subject: [PATCH 14/27] Update documentation

---
 crates/bevy_animation/src/lib.rs         | 25 ++++++++++++++++++++----
 crates/bevy_gltf/src/loader.rs           |  2 +-
 examples/animation/animated_transform.rs |  4 ++--
 3 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/crates/bevy_animation/src/lib.rs b/crates/bevy_animation/src/lib.rs
index 4c9ba00bd8386..f37d51a661827 100644
--- a/crates/bevy_animation/src/lib.rs
+++ b/crates/bevy_animation/src/lib.rs
@@ -190,6 +190,23 @@ pub struct AnimationTargetId(pub Uuid);
 ///
 /// These are frequently referred to as *bones* or *joints*, because they often
 /// refer to individually-animatable parts of an armature.
+///
+/// Asset loaders for armatures are responsible for adding these as necessary.
+/// Typically, they're generated from hashed versions of the entire name path
+/// from the root of the armature to the bone. See the [`AnimationTargetId`]
+/// documentation for more details.
+///
+/// By convention, asset loaders add [`AnimationTarget`] components to the
+/// descendants of an [`AnimationPlayer`], but Bevy doesn't require this in any
+/// way. So, for example, it's entirely possible for an [`AnimationPlayer`] to
+/// animate a target that it isn't an ancestor of. If you add a new bone to or
+/// delete a bone from an armature at runtime, you may want to update the
+/// [`AnimationTarget`] component as appropriate, as Bevy won't do this
+/// automatically.
+///
+/// Note that each entity can only be animated by one animation player at a
+/// time. However, you can change [`AnimationTarget`]'s `player` property at
+/// runtime to change which player is responsible for animating the entity.
 #[derive(Clone, Component, Reflect)]
 #[reflect(Component, MapEntities)]
 pub struct AnimationTarget {
@@ -199,7 +216,7 @@ pub struct AnimationTarget {
     pub id: AnimationTargetId,
 
     /// The entity containing the [`AnimationPlayer`].
-    pub root: Entity,
+    pub player: Entity,
 }
 
 impl AnimationClip {
@@ -588,10 +605,10 @@ pub fn animate_targets(
                 morph_weights,
             };
 
-            let Ok(player) = players.get(target.root) else {
+            let Ok(player) = players.get(target.player) else {
                 error!(
                     "Couldn't find the animation player {:?} for {:?} ({:?})",
-                    target.root, target_context.id, target_context.name,
+                    target.player, target_context.id, target_context.name,
                 );
                 return;
             };
@@ -989,7 +1006,7 @@ impl AnimationTargetId {
 
 impl MapEntities for AnimationTarget {
     fn map_entities<M: EntityMapper>(&mut self, entity_mapper: &mut M) {
-        self.root = entity_mapper.map_entity(self.root);
+        self.player = entity_mapper.map_entity(self.player);
     }
 }
 
diff --git a/crates/bevy_gltf/src/loader.rs b/crates/bevy_gltf/src/loader.rs
index 9cfe3f050ef36..8007b0644af2c 100644
--- a/crates/bevy_gltf/src/loader.rs
+++ b/crates/bevy_gltf/src/loader.rs
@@ -963,7 +963,7 @@ fn load_node(
 
             node.insert(AnimationTarget {
                 id: AnimationTargetId::from_names(animation_context.path.iter()),
-                root: animation_context.root,
+                player: animation_context.root,
             });
         }
     };
diff --git a/examples/animation/animated_transform.rs b/examples/animation/animated_transform.rs
index 15e573c1b18b8..e6418ecdf66d0 100644
--- a/examples/animation/animated_transform.rs
+++ b/examples/animation/animated_transform.rs
@@ -139,7 +139,7 @@ fn setup(
             orbit_controller,
             AnimationTarget {
                 id: orbit_controller_animation_target_id,
-                root: planet_entity,
+                player: planet_entity,
             },
         ))
         .with_children(|p| {
@@ -155,7 +155,7 @@ fn setup(
                 satellite,
                 AnimationTarget {
                     id: satellite_animation_target_id,
-                    root: planet_entity,
+                    player: planet_entity,
                 },
             ));
         });

From c88d3159b49fdef87d19940ec17be33eba8adda7 Mon Sep 17 00:00:00 2001
From: Patrick Walton <pcwalton@mimiga.net>
Date: Wed, 7 Feb 2024 11:05:04 -0800
Subject: [PATCH 15/27] Remove the `expire_transitions` system

---
 crates/bevy_animation/src/lib.rs | 35 ++++++++++----------------------
 1 file changed, 11 insertions(+), 24 deletions(-)

diff --git a/crates/bevy_animation/src/lib.rs b/crates/bevy_animation/src/lib.rs
index f37d51a661827..8bdae3029fcc9 100644
--- a/crates/bevy_animation/src/lib.rs
+++ b/crates/bevy_animation/src/lib.rs
@@ -559,12 +559,13 @@ pub fn advance_animations(
         };
 
         // Advance transition animations.
-        for transition in &mut player.transitions {
-            // Decrease weight. But don't expire the transition yet if the time
-            // ran out; we still need to update the bones.
-            transition.current_weight = (transition.current_weight
-                - transition.weight_decline_per_sec * time.delta_seconds())
-            .max(0.0);
+        player.transitions.retain_mut(|transition| {
+            // Decrease weight. Expire the transition if necessary.
+            transition.current_weight = transition.current_weight
+                - transition.weight_decline_per_sec * time.delta_seconds();
+            if transition.current_weight <= 0.0 {
+                return false;
+            }
 
             if let Some(animation_clip) = animation_clips.get(&transition.animation.animation_clip)
             {
@@ -572,7 +573,9 @@ pub fn advance_animations(
                     .animation
                     .update(time.delta_seconds(), animation_clip.duration);
             };
-        }
+
+            true
+        });
     }
 }
 
@@ -623,18 +626,6 @@ pub fn animate_targets(
         });
 }
 
-/// Removes animation transitions that have expired.
-///
-/// This needs to be done after applying the animations so that transitions
-/// don't miss applying their last frame on the frame they expire.
-pub fn expire_animation_transitions(mut players: Query<&mut AnimationPlayer>) {
-    for mut player in players.iter_mut() {
-        player
-            .transitions
-            .retain_mut(|transition| transition.current_weight > 0.0);
-    }
-}
-
 /// Update `weights` based on weights in `keyframe` with a linear interpolation
 /// on `key_lerp`.
 fn lerp_morph_weights(weights: &mut [f32], keyframe: impl Iterator<Item = f32>, key_lerp: f32) {
@@ -689,11 +680,7 @@ impl Plugin for AnimationPlugin {
             .register_type::<AnimationTarget>()
             .add_systems(
                 PostUpdate,
-                (
-                    advance_animations,
-                    animate_targets,
-                    expire_animation_transitions,
-                )
+                (advance_animations, animate_targets)
                     .chain()
                     .before(TransformSystem::TransformPropagate),
             );

From b93b26773404ef69d178e3120044e4cd63cef4ca Mon Sep 17 00:00:00 2001
From: Patrick Walton <pcwalton@mimiga.net>
Date: Wed, 7 Feb 2024 11:07:16 -0800
Subject: [PATCH 16/27] Address a couple of review comments

---
 crates/bevy_animation/src/lib.rs | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/crates/bevy_animation/src/lib.rs b/crates/bevy_animation/src/lib.rs
index 8bdae3029fcc9..67b766f5e6a5f 100644
--- a/crates/bevy_animation/src/lib.rs
+++ b/crates/bevy_animation/src/lib.rs
@@ -186,6 +186,13 @@ pub type AnimationCurves = HashMap<AnimationTargetId, Vec<VariableCurve>, NoOpHa
 #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Reflect, Debug)]
 pub struct AnimationTargetId(pub Uuid);
 
+impl Hash for AnimationTargetId {
+    fn hash<H: Hasher>(&self, state: &mut H) {
+        let (hi, lo) = self.0.as_u64_pair();
+        state.write_u64(hi ^ lo);
+    }
+}
+
 /// An entity that can be animated by an [`AnimationPlayer`].
 ///
 /// These are frequently referred to as *bones* or *joints*, because they often
@@ -991,16 +998,15 @@ impl AnimationTargetId {
     }
 }
 
-impl MapEntities for AnimationTarget {
-    fn map_entities<M: EntityMapper>(&mut self, entity_mapper: &mut M) {
-        self.player = entity_mapper.map_entity(self.player);
+impl From<&Name> for AnimationTargetId {
+    fn from(name: &Name) -> Self {
+        AnimationTargetId::from_name(name)
     }
 }
 
-impl Hash for AnimationTargetId {
-    fn hash<H: Hasher>(&self, state: &mut H) {
-        let (hi, lo) = self.0.as_u64_pair();
-        state.write_u64(hi ^ lo);
+impl MapEntities for AnimationTarget {
+    fn map_entities<M: EntityMapper>(&mut self, entity_mapper: &mut M) {
+        self.player = entity_mapper.map_entity(self.player);
     }
 }
 

From 5068a7efdae55ee86ee83d684e85a62ad0e47569 Mon Sep 17 00:00:00 2001
From: Patrick Walton <pcwalton@mimiga.net>
Date: Wed, 7 Feb 2024 11:12:10 -0800
Subject: [PATCH 17/27] Address more review comments

---
 crates/bevy_animation/src/lib.rs | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/crates/bevy_animation/src/lib.rs b/crates/bevy_animation/src/lib.rs
index 67b766f5e6a5f..57f61d8f717e7 100644
--- a/crates/bevy_animation/src/lib.rs
+++ b/crates/bevy_animation/src/lib.rs
@@ -33,7 +33,7 @@ pub mod prelude {
     };
 }
 
-/// The [UUID namespace] of animation targets (bones).
+/// The [UUID namespace] of animation targets (e.g. bones).
 ///
 /// [UUID namespace]: https://en.wikipedia.org/wiki/Universally_unique_identifier#Versions_3_and_5_(namespace_name-based)
 pub static ANIMATION_TARGET_NAMESPACE: Uuid = Uuid::from_u128(0x3179f519d9274ff2b5966fd077023911);
@@ -166,7 +166,7 @@ pub type AnimationCurves = HashMap<AnimationTargetId, Vec<VariableCurve>, NoOpHa
 /// A unique [UUID] for an animation target (bone).
 ///
 /// The [`AnimationClip`] asset and the [`AnimationTarget`] component both use
-/// this to refer to targets (bones) to be animated.
+/// this to refer to targets (e.g. bones in a skinned mesh) to be animated.
 ///
 /// When importing an armature or an animation clip, asset loaders typically use
 /// the full path name from the armature to the bone to generate these IDs. So,
@@ -233,12 +233,15 @@ impl AnimationClip {
         &self.curves
     }
 
-    /// Gets the curves for a bone.
+    /// Gets the curves for a single animation target.
     ///
-    /// Returns `None` if this clip doesn't animate this bone.
+    /// Returns `None` if this clip doesn't animate the target.
     #[inline]
-    pub fn get_curves(&self, bone_id: AnimationTargetId) -> Option<&'_ Vec<VariableCurve>> {
-        self.curves.get(&bone_id)
+    pub fn curves_for_target(
+        &self,
+        target_id: AnimationTargetId,
+    ) -> Option<&'_ Vec<VariableCurve>> {
+        self.curves.get(&target_id)
     }
 
     /// Duration of the clip, represented in seconds.
@@ -586,8 +589,8 @@ pub fn advance_animations(
     }
 }
 
-/// A system that modifies animation targets (bones) according to the
-/// currently-playing animation.
+/// A system that modifies animation targets (e.g. bones in a skinned mesh)
+/// according to the currently-playing animation.
 pub fn animate_targets(
     clips: Res<Assets<AnimationClip>>,
     players: Query<&AnimationPlayer, Without<AnimationTarget>>,
@@ -599,11 +602,11 @@ pub fn animate_targets(
     )>,
 ) {
     // We use two queries here: one read-only query for animation players and
-    // one read-write query for animation targets (bones). The `AnimationPlayer`
-    // query is read-only shared memory accessible from all animation targets,
-    // which are evaluated in parallel.
+    // one read-write query for animation targets (e.g. bones). The
+    // `AnimationPlayer` query is read-only shared memory accessible from all
+    // animation targets, which are evaluated in parallel.
 
-    // Iterate over all animation targets (bones) in parallel.
+    // Iterate over all animation targets in parallel.
     targets
         .par_iter_mut()
         .for_each(|(id, target, name, (transform, morph_weights))| {
@@ -709,7 +712,7 @@ impl PlayingAnimation {
             return;
         };
 
-        let Some(curves) = clip.get_curves(target_context.target.id) else {
+        let Some(curves) = clip.curves_for_target(target_context.target.id) else {
             return;
         };
 

From 46538109e94b6270f1c60f90623efb4668d58d9b Mon Sep 17 00:00:00 2001
From: Patrick Walton <pcwalton@mimiga.net>
Date: Wed, 7 Feb 2024 11:16:32 -0800
Subject: [PATCH 18/27] Address more review comments

---
 crates/bevy_animation/src/lib.rs | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/crates/bevy_animation/src/lib.rs b/crates/bevy_animation/src/lib.rs
index 57f61d8f717e7..ec79a43c6778d 100644
--- a/crates/bevy_animation/src/lib.rs
+++ b/crates/bevy_animation/src/lib.rs
@@ -388,7 +388,7 @@ pub struct AnimationPlayer {
 // The components that we might need to read or write during animation of each
 // animation target.
 struct AnimationTargetContext<'a> {
-    id: Entity,
+    entity: Entity,
     target: &'a AnimationTarget,
     name: Option<&'a Name>,
     transform: Option<Mut<'a, Transform>>,
@@ -611,7 +611,7 @@ pub fn animate_targets(
         .par_iter_mut()
         .for_each(|(id, target, name, (transform, morph_weights))| {
             let mut target_context = AnimationTargetContext {
-                id,
+                entity: id,
                 target,
                 name,
                 transform,
@@ -620,8 +620,8 @@ pub fn animate_targets(
 
             let Ok(player) = players.get(target.player) else {
                 error!(
-                    "Couldn't find the animation player {:?} for {:?} ({:?})",
-                    target.player, target_context.id, target_context.name,
+                    "Couldn't find the animation player {:?} for the target entity {:?} ({:?})",
+                    target.player, target_context.entity, target_context.name,
                 );
                 return;
             };
@@ -706,8 +706,8 @@ impl PlayingAnimation {
     ) {
         let Some(clip) = clips.get(&self.animation_clip) else {
             warn!(
-                "Couldn't find the animation clip for {:?} ({:?})",
-                target_context.id, target_context.name,
+                "Couldn't find the animation clip for the target entity {:?} ({:?})",
+                target_context.entity, target_context.name,
             );
             return;
         };
@@ -716,15 +716,6 @@ impl PlayingAnimation {
             return;
         };
 
-        self.apply_curves(curves, weight, target_context);
-    }
-
-    fn apply_curves(
-        &self,
-        curves: &[VariableCurve],
-        weight: f32,
-        target_context: &mut AnimationTargetContext,
-    ) {
         for curve in curves {
             // Some curves have only one keyframe used to set a transform
             if curve.keyframe_timestamps.len() == 1 {
@@ -782,7 +773,7 @@ impl PlayingAnimation {
                 let Some(ref mut morphs) = target_context.morph_weights else {
                     error!(
                         "Tried to animate morphs on {:?} ({:?}), but no `MorphWeights` was found",
-                        target_context.id, target_context.name,
+                        target_context.entity, target_context.name,
                     );
                     return;
                 };

From f16b72bebd9d8648df6d38bd000151bc7f142161 Mon Sep 17 00:00:00 2001
From: Patrick Walton <pcwalton@mimiga.net>
Date: Wed, 7 Feb 2024 11:28:42 -0800
Subject: [PATCH 19/27] Address review comments

---
 crates/bevy_animation/src/lib.rs | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/crates/bevy_animation/src/lib.rs b/crates/bevy_animation/src/lib.rs
index ec79a43c6778d..561fe2592a0f7 100644
--- a/crates/bevy_animation/src/lib.rs
+++ b/crates/bevy_animation/src/lib.rs
@@ -160,22 +160,24 @@ pub struct AnimationClip {
     duration: f32,
 }
 
-/// A mapping from [`AnimationTargetId`] (bone) to the animation curves.
+/// A mapping from [`AnimationTargetId`] (e.g. bone in a skinned mesh) to the
+/// animation curves.
 pub type AnimationCurves = HashMap<AnimationTargetId, Vec<VariableCurve>, NoOpHash>;
 
-/// A unique [UUID] for an animation target (bone).
+/// A unique [UUID] for an animation target (e.g. bone in a skinned mesh).
 ///
 /// The [`AnimationClip`] asset and the [`AnimationTarget`] component both use
 /// this to refer to targets (e.g. bones in a skinned mesh) to be animated.
 ///
 /// When importing an armature or an animation clip, asset loaders typically use
-/// the full path name from the armature to the bone to generate these IDs. So,
-/// for example, any imported armature with a bone at the root named `Hips` will
+/// the full path name from the armature to the bone to generate these UUIDs.
+/// The ID is unique to the full path name and based only on the names. So, for
+/// example, any imported armature with a bone at the root named `Hips` will
 /// assign the same [`AnimationTargetId`] to its root bone. Likewise, any
 /// imported animation clip that animates a root bone named `Hips` will
-/// reference the same [`AnimationTargetId`]. What this means is that any
-/// animation is playable on any armature as long as the bone names match,
-/// which allows for easy animation retargeting.
+/// reference the same [`AnimationTargetId`]. Any animation is playable on any
+/// armature as long as the bone names match, which allows for easy animation
+/// retargeting.
 ///
 /// Note that asset loaders generally use the *full* path name to generate the
 /// [`AnimationTargetId`]. Thus a bone named `Chest` directly connected to a
@@ -214,6 +216,10 @@ impl Hash for AnimationTargetId {
 /// Note that each entity can only be animated by one animation player at a
 /// time. However, you can change [`AnimationTarget`]'s `player` property at
 /// runtime to change which player is responsible for animating the entity.
+///
+/// An [`AnimationPlayer`] and [`AnimationTarget`] can't simultaneously be on
+/// the same entity. Animation players always target entities other than
+/// themselves.
 #[derive(Clone, Component, Reflect)]
 #[reflect(Component, MapEntities)]
 pub struct AnimationTarget {
@@ -369,6 +375,10 @@ struct AnimationTransition {
 }
 
 /// Animation controls
+///
+/// Note that an [`AnimationPlayer`] and [`AnimationTarget`] can't
+/// simultaneously be on the same entity. Animation players always target
+/// entities other than themselves.
 #[derive(Component, Default, Reflect)]
 #[reflect(Component)]
 pub struct AnimationPlayer {
@@ -977,7 +987,7 @@ impl AnimationTargetId {
     /// Creates a new [`AnimationTargetId`] by hashing a list of names.
     ///
     /// Typically, this will be the path from the animation root to the
-    /// animation target (bone) that is to be animated.
+    /// animation target (e.g. bone) that is to be animated.
     pub fn from_names<'a>(names: impl Iterator<Item = &'a Name>) -> Self {
         let mut sha1 = Sha1::new();
         sha1.update(ANIMATION_TARGET_NAMESPACE.as_bytes());

From c64e6f7b578d1a7185e665d7161dcf23f51a5377 Mon Sep 17 00:00:00 2001
From: Patrick Walton <pcwalton@mimiga.net>
Date: Wed, 7 Feb 2024 12:43:03 -0800
Subject: [PATCH 20/27] Address review comment

---
 crates/bevy_animation/src/lib.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/crates/bevy_animation/src/lib.rs b/crates/bevy_animation/src/lib.rs
index 561fe2592a0f7..760462cd87ad3 100644
--- a/crates/bevy_animation/src/lib.rs
+++ b/crates/bevy_animation/src/lib.rs
@@ -395,8 +395,8 @@ pub struct AnimationPlayer {
     transitions: Vec<AnimationTransition>,
 }
 
-// The components that we might need to read or write during animation of each
-// animation target.
+/// The components that we might need to read or write during animation of each
+/// animation target.
 struct AnimationTargetContext<'a> {
     entity: Entity,
     target: &'a AnimationTarget,

From e2f6770eaafe636a832fd445361ebb9f58367c9a Mon Sep 17 00:00:00 2001
From: Patrick Walton <pcwalton@mimiga.net>
Date: Wed, 7 Feb 2024 15:46:23 -0800
Subject: [PATCH 21/27] Clippy police

---
 crates/bevy_animation/src/lib.rs | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/crates/bevy_animation/src/lib.rs b/crates/bevy_animation/src/lib.rs
index 760462cd87ad3..8ccde7eeda19e 100644
--- a/crates/bevy_animation/src/lib.rs
+++ b/crates/bevy_animation/src/lib.rs
@@ -581,8 +581,7 @@ pub fn advance_animations(
         // Advance transition animations.
         player.transitions.retain_mut(|transition| {
             // Decrease weight. Expire the transition if necessary.
-            transition.current_weight = transition.current_weight
-                - transition.weight_decline_per_sec * time.delta_seconds();
+            transition.current_weight -= transition.weight_decline_per_sec * time.delta_seconds();
             if transition.current_weight <= 0.0 {
                 return false;
             }

From 9092e54dd8f5707da01a94928e6304b897f25a4e Mon Sep 17 00:00:00 2001
From: Patrick Walton <pcwalton@mimiga.net>
Date: Wed, 7 Feb 2024 15:48:03 -0800
Subject: [PATCH 22/27] Get rid of the warning spam while clips are loading

---
 crates/bevy_animation/src/lib.rs | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/crates/bevy_animation/src/lib.rs b/crates/bevy_animation/src/lib.rs
index 8ccde7eeda19e..144ff517d2028 100644
--- a/crates/bevy_animation/src/lib.rs
+++ b/crates/bevy_animation/src/lib.rs
@@ -714,10 +714,7 @@ impl PlayingAnimation {
         target_context: &mut AnimationTargetContext,
     ) {
         let Some(clip) = clips.get(&self.animation_clip) else {
-            warn!(
-                "Couldn't find the animation clip for the target entity {:?} ({:?})",
-                target_context.entity, target_context.name,
-            );
+            // The clip probably hasn't loaded yet. Bail.
             return;
         };
 

From 13e46d77ce0dacaad592dbc76f28fdbf8a68583f Mon Sep 17 00:00:00 2001
From: Patrick Walton <pcwalton@mimiga.net>
Date: Wed, 7 Feb 2024 18:07:38 -0800
Subject: [PATCH 23/27] Warning police

---
 crates/bevy_animation/src/lib.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crates/bevy_animation/src/lib.rs b/crates/bevy_animation/src/lib.rs
index 144ff517d2028..6494f7b2ba332 100644
--- a/crates/bevy_animation/src/lib.rs
+++ b/crates/bevy_animation/src/lib.rs
@@ -14,7 +14,7 @@ use bevy_core::Name;
 use bevy_ecs::entity::MapEntities;
 use bevy_ecs::prelude::*;
 use bevy_ecs::reflect::ReflectMapEntities;
-use bevy_log::{error, warn};
+use bevy_log::error;
 use bevy_math::{FloatExt, Quat, Vec3};
 use bevy_reflect::Reflect;
 use bevy_render::mesh::morph::MorphWeights;

From ee7154eba79ee3f8fe1f677b2c082793ed592712 Mon Sep 17 00:00:00 2001
From: Patrick Walton <pcwalton@mimiga.net>
Date: Fri, 9 Feb 2024 18:52:01 -0800
Subject: [PATCH 24/27] Allow `AnimationPlayer` and `AnimationTarget` to
 coexist on the same entity

---
 crates/bevy_animation/src/lib.rs         | 22 ++++------
 crates/bevy_gltf/src/loader.rs           | 34 ++++++++--------
 examples/animation/animated_transform.rs | 51 ++++++++++++++----------
 3 files changed, 52 insertions(+), 55 deletions(-)

diff --git a/crates/bevy_animation/src/lib.rs b/crates/bevy_animation/src/lib.rs
index 6494f7b2ba332..45499a3700794 100644
--- a/crates/bevy_animation/src/lib.rs
+++ b/crates/bevy_animation/src/lib.rs
@@ -206,20 +206,16 @@ impl Hash for AnimationTargetId {
 /// documentation for more details.
 ///
 /// By convention, asset loaders add [`AnimationTarget`] components to the
-/// descendants of an [`AnimationPlayer`], but Bevy doesn't require this in any
-/// way. So, for example, it's entirely possible for an [`AnimationPlayer`] to
-/// animate a target that it isn't an ancestor of. If you add a new bone to or
-/// delete a bone from an armature at runtime, you may want to update the
-/// [`AnimationTarget`] component as appropriate, as Bevy won't do this
-/// automatically.
+/// descendants of an [`AnimationPlayer`], as well as to the [`AnimationPlayer`]
+/// entity itself, but Bevy doesn't require this in any way. So, for example,
+/// it's entirely possible for an [`AnimationPlayer`] to animate a target that
+/// it isn't an ancestor of. If you add a new bone to or delete a bone from an
+/// armature at runtime, you may want to update the [`AnimationTarget`]
+/// component as appropriate, as Bevy won't do this automatically.
 ///
 /// Note that each entity can only be animated by one animation player at a
 /// time. However, you can change [`AnimationTarget`]'s `player` property at
 /// runtime to change which player is responsible for animating the entity.
-///
-/// An [`AnimationPlayer`] and [`AnimationTarget`] can't simultaneously be on
-/// the same entity. Animation players always target entities other than
-/// themselves.
 #[derive(Clone, Component, Reflect)]
 #[reflect(Component, MapEntities)]
 pub struct AnimationTarget {
@@ -375,10 +371,6 @@ struct AnimationTransition {
 }
 
 /// Animation controls
-///
-/// Note that an [`AnimationPlayer`] and [`AnimationTarget`] can't
-/// simultaneously be on the same entity. Animation players always target
-/// entities other than themselves.
 #[derive(Component, Default, Reflect)]
 #[reflect(Component)]
 pub struct AnimationPlayer {
@@ -602,7 +594,7 @@ pub fn advance_animations(
 /// according to the currently-playing animation.
 pub fn animate_targets(
     clips: Res<Assets<AnimationClip>>,
-    players: Query<&AnimationPlayer, Without<AnimationTarget>>,
+    players: Query<&AnimationPlayer>,
     mut targets: Query<(
         Entity,
         &AnimationTarget,
diff --git a/crates/bevy_gltf/src/loader.rs b/crates/bevy_gltf/src/loader.rs
index 8007b0644af2c..8d1de9f3d953f 100644
--- a/crates/bevy_gltf/src/loader.rs
+++ b/crates/bevy_gltf/src/loader.rs
@@ -948,25 +948,23 @@ fn load_node(
     node.insert(name.clone());
 
     #[cfg(feature = "bevy_animation")]
-    match animation_context {
-        None => {
-            if animation_roots.contains(&gltf_node.index()) {
-                // This is an animation root. Make a new animation context.
-                animation_context = Some(AnimationContext {
-                    root: node.id(),
-                    path: smallvec![name],
-                });
-            }
-        }
-        Some(ref mut animation_context) => {
-            animation_context.path.push(name);
+    if animation_context.is_none() && animation_roots.contains(&gltf_node.index()) {
+        // This is an animation root. Make a new animation context.
+        animation_context = Some(AnimationContext {
+            root: node.id(),
+            path: smallvec![],
+        });
+    }
 
-            node.insert(AnimationTarget {
-                id: AnimationTargetId::from_names(animation_context.path.iter()),
-                player: animation_context.root,
-            });
-        }
-    };
+    #[cfg(feature = "bevy_animation")]
+    if let Some(ref mut animation_context) = animation_context {
+        animation_context.path.push(name);
+
+        node.insert(AnimationTarget {
+            id: AnimationTargetId::from_names(animation_context.path.iter()),
+            player: animation_context.root,
+        });
+    }
 
     if let Some(extras) = gltf_node.extras() {
         node.insert(GltfExtras {
diff --git a/examples/animation/animated_transform.rs b/examples/animation/animated_transform.rs
index 510ac98140135..c54a2695617b3 100644
--- a/examples/animation/animated_transform.rs
+++ b/examples/animation/animated_transform.rs
@@ -36,8 +36,9 @@ fn setup(
     // Creating the animation
     let mut animation = AnimationClip::default();
     // A curve can modify a single part of a transform, here the translation
+    let planet_animation_target_id = AnimationTargetId::from_name(&planet);
     animation.add_curve_to_target(
-        AnimationTargetId::from_name(&planet),
+        planet_animation_target_id,
         VariableCurve {
             keyframe_timestamps: vec![0.0, 1.0, 2.0, 3.0, 4.0],
             keyframes: Keyframes::Translation(vec![
@@ -131,29 +132,35 @@ fn setup(
             player,
         ))
         .id();
-    commands.entity(planet_entity).with_children(|p| {
-        // This entity is just used for animation, but doesn't display anything
-        p.spawn((
-            SpatialBundle::INHERITED_IDENTITY,
-            // Add the Name component
-            orbit_controller,
-            AnimationTarget {
-                id: orbit_controller_animation_target_id,
-                player: planet_entity,
-            },
-        ))
+    commands
+        .entity(planet_entity)
+        .insert(AnimationTarget {
+            id: planet_animation_target_id,
+            player: planet_entity,
+        })
         .with_children(|p| {
-            // The satellite, placed at a distance of the planet
+            // This entity is just used for animation, but doesn't display anything
             p.spawn((
-                PbrBundle {
-                    transform: Transform::from_xyz(1.5, 0.0, 0.0),
-                    mesh: meshes.add(Cuboid::new(0.5, 0.5, 0.5)),
-                    material: materials.add(Color::rgb(0.3, 0.9, 0.3)),
-                    ..default()
-                },
+                SpatialBundle::INHERITED_IDENTITY,
                 // Add the Name component
-                satellite,
-            ));
+                orbit_controller,
+                AnimationTarget {
+                    id: orbit_controller_animation_target_id,
+                    player: planet_entity,
+                },
+            ))
+            .with_children(|p| {
+                // The satellite, placed at a distance of the planet
+                p.spawn((
+                    PbrBundle {
+                        transform: Transform::from_xyz(1.5, 0.0, 0.0),
+                        mesh: meshes.add(Cuboid::new(0.5, 0.5, 0.5)),
+                        material: materials.add(Color::rgb(0.3, 0.9, 0.3)),
+                        ..default()
+                    },
+                    // Add the Name component
+                    satellite,
+                ));
+            });
         });
-    });
 }

From 3928dcbe7f8b49929dab769619776f93f747723c Mon Sep 17 00:00:00 2001
From: Patrick Walton <pcwalton@mimiga.net>
Date: Sun, 18 Feb 2024 16:50:16 -0800
Subject: [PATCH 25/27] Make `animated_transform` behave as before

---
 examples/animation/animated_transform.rs | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/examples/animation/animated_transform.rs b/examples/animation/animated_transform.rs
index c54a2695617b3..3e177f0f701b8 100644
--- a/examples/animation/animated_transform.rs
+++ b/examples/animation/animated_transform.rs
@@ -28,7 +28,8 @@ fn setup(
         ..default()
     });
 
-    // The animation API uses the `Name` component to target entities
+    // Let's use the `Name` component to target entities. We can use anything we
+    // like, but names are convenient.
     let planet = Name::new("planet");
     let orbit_controller = Name::new("orbit_controller");
     let satellite = Name::new("satellite");
@@ -127,7 +128,7 @@ fn setup(
                 material: materials.add(Color::rgb(0.8, 0.7, 0.6)),
                 ..default()
             },
-            // Add the Name component, and the animation player
+            // Add the animation player
             planet,
             player,
         ))
@@ -142,7 +143,6 @@ fn setup(
             // This entity is just used for animation, but doesn't display anything
             p.spawn((
                 SpatialBundle::INHERITED_IDENTITY,
-                // Add the Name component
                 orbit_controller,
                 AnimationTarget {
                     id: orbit_controller_animation_target_id,
@@ -158,7 +158,10 @@ fn setup(
                         material: materials.add(Color::rgb(0.3, 0.9, 0.3)),
                         ..default()
                     },
-                    // Add the Name component
+                    AnimationTarget {
+                        id: satellite_animation_target_id,
+                        player: planet_entity,
+                    },
                     satellite,
                 ));
             });

From 26f1fb15c1e7347508d58888f057848240a24733 Mon Sep 17 00:00:00 2001
From: Patrick Walton <pcwalton@mimiga.net>
Date: Sun, 18 Feb 2024 19:16:14 -0800
Subject: [PATCH 26/27] Update the scene viewer. This fixes the
 InterpolationTest.

---
 .../tools/scene_viewer/animation_plugin.rs    | 116 ++++++++++++++----
 1 file changed, 95 insertions(+), 21 deletions(-)

diff --git a/examples/tools/scene_viewer/animation_plugin.rs b/examples/tools/scene_viewer/animation_plugin.rs
index a0f65dcc37830..6b23573632271 100644
--- a/examples/tools/scene_viewer/animation_plugin.rs
+++ b/examples/tools/scene_viewer/animation_plugin.rs
@@ -1,5 +1,7 @@
 //! Control animations of entities in the loaded scene.
-use bevy::{gltf::Gltf, prelude::*};
+use std::collections::HashMap;
+
+use bevy::{animation::AnimationTarget, ecs::entity::EntityHashMap, gltf::Gltf, prelude::*};
 
 use crate::scene_viewer_plugin::SceneHandle;
 
@@ -24,14 +26,13 @@ impl Clips {
     }
 }
 
-/// Read [`AnimationClip`]s from the loaded [`Gltf`] and assign them to the
-/// entities they control. [`AnimationClip`]s control specific entities, and
-/// trying to play them on an [`AnimationPlayer`] controlling a different
-/// entities will result in odd animations, we take extra care to store
-/// animation clips for given entities in the [`Clips`] component we defined
-/// earlier in this file.
+/// Automatically assign [`AnimationClip`]s to [`AnimationPlayer`] and play
+/// them, if the clips refer to descendants of the animation player (which is
+/// the common case).
 fn assign_clips(
-    mut players: Query<(Entity, &mut AnimationPlayer)>,
+    mut players: Query<&mut AnimationPlayer>,
+    targets: Query<(Entity, &AnimationTarget)>,
+    parents: Query<&Parent>,
     scene_handle: Res<SceneHandle>,
     clips: Res<Assets<AnimationClip>>,
     gltf_assets: Res<Assets<Gltf>>,
@@ -44,24 +45,97 @@ fn assign_clips(
     } else {
         return;
     }
+
     let gltf = gltf_assets.get(&scene_handle.gltf_handle).unwrap();
     let animations = &gltf.animations;
-    if !animations.is_empty() {
-        let count = animations.len();
-        let plural = if count == 1 { "" } else { "s" };
-        info!("Found {} animation{plural}", animations.len());
-        let names: Vec<_> = gltf.named_animations.keys().collect();
-        info!("Animation names: {names:?}");
+    if animations.is_empty() {
+        return;
     }
-    for (entity, mut player) in &mut players {
-        let clips = clips
-            .iter()
-            .map(|(k, _)| k)
-            .map(|id| assets.get_id_handle(id).unwrap())
-            .collect();
+
+    let count = animations.len();
+    let plural = if count == 1 { "" } else { "s" };
+    info!("Found {} animation{plural}", animations.len());
+    let names: Vec<_> = gltf.named_animations.keys().collect();
+    info!("Animation names: {names:?}");
+
+    // Map animation target IDs to entities.
+    let animation_target_id_to_entity: HashMap<_, _> = targets
+        .iter()
+        .map(|(entity, target)| (target.id, entity))
+        .collect();
+
+    // Build up a list of all animation clips that belong to each player. A clip
+    // is considered to belong to an animation player if all targets of the clip
+    // refer to entities whose nearest ancestor player is that animation player.
+
+    let mut player_to_clips: EntityHashMap<Vec<_>> = EntityHashMap::default();
+
+    for (clip_id, clip) in clips.iter() {
+        let mut ancestor_player = None;
+        for target_id in clip.curves().keys() {
+            // If the animation clip refers to entities that aren't present in
+            // the scene, bail.
+            let Some(&target) = animation_target_id_to_entity.get(target_id) else {
+                continue;
+            };
+
+            // Find the nearest ancestor animation player.
+            let mut current = Some(target);
+            while let Some(entity) = current {
+                if players.contains(entity) {
+                    match ancestor_player {
+                        None => {
+                            // If we haven't found a player yet, record the one
+                            // we found.
+                            ancestor_player = Some(entity)
+                        }
+                        Some(ancestor) => {
+                            // If we have found a player, then make sure it's
+                            // the same player we located before.
+                            if ancestor != entity {
+                                // It's a different player. Bail.
+                                ancestor_player = None;
+                                break;
+                            }
+                        }
+                    }
+                }
+
+                // Go to the next parent.
+                current = parents.get(entity).ok().map(|parent| parent.get());
+            }
+        }
+
+        let Some(ancestor_player) = ancestor_player else {
+            warn!(
+                "Unexpected animation hierarchy for animation clip {:?}; ignoring.",
+                clip_id
+            );
+            continue;
+        };
+
+        let Some(clip_handle) = assets.get_id_handle(clip_id) else {
+            warn!("Clip {:?} wasn't loaded.", clip_id);
+            continue;
+        };
+
+        player_to_clips
+            .entry(ancestor_player)
+            .or_default()
+            .push(clip_handle);
+    }
+
+    // Now that we've built up a list of all clips that belong to each player,
+    // package them up into a `Clips` component, play the first such animation,
+    // and add that component to the player.
+    for (player_entity, clips) in player_to_clips {
+        let Ok(mut player) = players.get_mut(player_entity) else {
+            warn!("Animation targets referenced a nonexistent player. This shouldn't happen.");
+            continue;
+        };
         let animations = Clips::new(clips);
         player.play(animations.current()).repeat();
-        commands.entity(entity).insert(animations);
+        commands.entity(player_entity).insert(animations);
     }
 }
 

From 7fc0421acfbe6735dfcbe98010506293fa3e525c Mon Sep 17 00:00:00 2001
From: Patrick Walton <pcwalton@mimiga.net>
Date: Mon, 19 Feb 2024 01:30:25 -0800
Subject: [PATCH 27/27] Clippy police

---
 examples/tools/scene_viewer/animation_plugin.rs | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/examples/tools/scene_viewer/animation_plugin.rs b/examples/tools/scene_viewer/animation_plugin.rs
index 6b23573632271..43f85b023bf64 100644
--- a/examples/tools/scene_viewer/animation_plugin.rs
+++ b/examples/tools/scene_viewer/animation_plugin.rs
@@ -29,6 +29,7 @@ impl Clips {
 /// Automatically assign [`AnimationClip`]s to [`AnimationPlayer`] and play
 /// them, if the clips refer to descendants of the animation player (which is
 /// the common case).
+#[allow(clippy::too_many_arguments)]
 fn assign_clips(
     mut players: Query<&mut AnimationPlayer>,
     targets: Query<(Entity, &AnimationTarget)>,
@@ -87,7 +88,7 @@ fn assign_clips(
                         None => {
                             // If we haven't found a player yet, record the one
                             // we found.
-                            ancestor_player = Some(entity)
+                            ancestor_player = Some(entity);
                         }
                         Some(ancestor) => {
                             // If we have found a player, then make sure it's