Fix AnimationTargetId collisions (#22876)

## Objective

Fixes #22842. TLDR: `AnimationTargetId::from_iter` wrongly returns the
same id for `["ab"]` and `["a", "b"]`.

## Solution

As suggested in the issue, add the length of each string to the hash.
The original suggestion used `to_be_bytes`, but I used `to_le_bytes` on
the assumption that most users are targeting LE and this will be more
efficient.

- There's probably a way to break it with maliciously crafted names, but
I can't imagine a case where that's a problem.
- I considered hashing `0xffu8` after each string instead of the length.
- That would be more efficient, and should be safe since it's not a
valid UTF8 character.
  - But I wasn't quite confident enough.
- I also refactored `AnimationTargetId::from_names` to reuse
`AnimationTargetId::from_iter`.

## Testing

```sh
cargo test -p bevy_animation
cargo run --example animated_mesh
cargo run --example animated_mesh_events
cargo run --example animation_masks
```

Author: greeble-dev

crates/bevy_animation/src/lib.rs

@@ -1263,13 +1263,7 @@ impl AnimationTargetId {
     /// Typically, this will be the path from the animation root to the
     /// animation target (e.g. bone) that is to be animated.
     pub fn from_names<'a>(names: impl Iterator<Item = &'a Name>) -> Self {
-        let mut blake3 = blake3::Hasher::new();
-        blake3.update(ANIMATION_TARGET_NAMESPACE.as_bytes());
-        for name in names {
-            blake3.update(name.as_bytes());
-        }
-        let hash = blake3.finalize().as_bytes()[0..16].try_into().unwrap();
-        Self(*uuid::Builder::from_sha1_bytes(hash).as_uuid())
+        AnimationTargetId::from_iter(names.map(Name::as_str))
     }
 
     /// Creates a new [`AnimationTargetId`] by hashing a single name.
@@ -1287,6 +1281,9 @@ impl<T: AsRef<str>> FromIterator<T> for AnimationTargetId {
         let mut blake3 = blake3::Hasher::new();
         blake3.update(ANIMATION_TARGET_NAMESPACE.as_bytes());
         for str in iter {
+            // Include the string's length in the hash. This avoids ["ab"] and
+            // ["a", "b"] returning the same id.
+            blake3.update(&str.as_ref().len().to_le_bytes());
             blake3.update(str.as_ref().as_bytes());
         }
         let hash = blake3.finalize().as_bytes()[0..16].try_into().unwrap();
@@ -1664,4 +1661,47 @@ mod tests {
             Box::new(ActiveAnimation::default()),
         );
     }
+
+    #[test]
+    fn test_animation_target_id() {
+        let paths = &[
+            vec![],
+            vec![""],
+            vec!["", ""],
+            vec!["a"],
+            vec!["a", "a"],
+            vec!["a", "b"],
+            vec!["b", "a"],
+            vec!["aa"],
+            vec!["ab"],
+            vec!["ba"],
+            vec!["abc"],
+            vec!["a", "b", "c"],
+            vec!["ab", "c"],
+            vec!["a", "bc"],
+        ];
+
+        // Test that different paths map to a different `AnimationTargetId`.
+        for (li, lp) in paths.iter().enumerate() {
+            for rp in paths.iter().skip(li + 1) {
+                let lt = AnimationTargetId::from_iter(lp.iter());
+                let rt = AnimationTargetId::from_iter(rp.iter());
+
+                assert!(lt != rt, "{:?} and {:?} collided. {:?} ", lp, rp, lt);
+            }
+        }
+
+        // Test that `from_iter` is equivalent to `from_names`.
+        for str_path in paths {
+            let name_path = str_path.iter().map(|&s| Name::from(s)).collect::<Vec<_>>();
+
+            assert_eq!(
+                AnimationTargetId::from_iter(str_path),
+                AnimationTargetId::from_names(name_path.iter()),
+                "{:?} {:?}",
+                str_path,
+                &name_path
+            );
+        }
+    }
 }

release-content/migration-guides/animation_target_id_changes.md

@@ -0,0 +1,11 @@
+---
+title: "`AnimationTargetId` algorithm changes"
+pull_requests: [22876]
+---
+
+The algorithm used to calculate `AnimationTargetId` has changed. This fixes a
+[bug](https://github.com/bevyengine/bevy/issues/22842) where different joint
+hierarchies could mistakenly be assigned the same id.
+
+If you have serialized data containing `AnimationTargetId` values then these
+will need to be recalculated.