Skip to content

Conversation

@greeble-dev
Copy link
Contributor

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

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

@greeble-dev greeble-dev added C-Bug An unexpected or incorrect behavior A-Animation Make things move and change over time M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Feb 9, 2026
}
}

// Test that `from_iter` is equivalent to `from_names`.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test might not be justified. I originally added it because from_iter and from_names had different implementations. But later on I refactored them to share the same implementation.

@hukasu
Copy link
Contributor

hukasu commented Feb 9, 2026

instead of 0xffu8, maybe any other control character since they would cause problems in actual strings?

@greeble-dev
Copy link
Contributor Author

instead of 0xffu8, maybe any other control character since they would cause problems in actual strings?

I think that would be less safe, although only slightly. It's difficult to construct a str with an invalid character (i.e. str::from_utf8(&[0xffu8]) is an error), but it can contain control characters. Kinda unlikely anyone would make a joint name with a control character, but I've seen stranger things.

@greeble-dev greeble-dev added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Feb 9, 2026
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 9, 2026
Merged via the queue into bevyengine:main with commit 2dc1d07 Feb 10, 2026
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Animation Make things move and change over time C-Bug An unexpected or incorrect behavior M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AnimationTargetId hash collisions

5 participants