Skip to content

Prevent AnimationGraph from serializing AssetIds. #19615

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

andriyDev
Copy link
Contributor

@andriyDev andriyDev commented Jun 13, 2025

Objective

Solution

  • Stop allowing AssetId to be serialized by AnimationGraph. Serializing a handle with no path is now an error.
  • Add MigrationSerializedAnimationClip. This is an untagged enum for serde, meaning that it will take the first variant that deserializes. So it will first try the "modern" version, then it will fallback to the legacy version.
  • Add some logging/error messages to explain what users should do.

Note: one limitation here is that this removes the ability to serialize and deserialize UUIDs. In theory, someone could be using this to have a "default" animation. If someone inserts an empty AnimationClip into the Handle::default(), this might produce a T-pose. It might also do nothing though. Unclear! I think this is worth the risk for simplicity as it seems unlikely that people are sticking UUIDs in here (or that you want a default animation in any AnimationGraph).

Testing

  • Ran cargo r --example animation_graph -- --save on main, then ran cargo r --example animation_graph on this PR. The PR was able to load the old data (after Bump ron version to 0.10. #19631).

@andriyDev andriyDev requested a review from pcwalton June 13, 2025 07:10
@andriyDev andriyDev added A-Assets Load files from disk to use for things like images, models, and sounds A-Animation Make things move and change over time D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Blocked This cannot move forward until something else changes and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 13, 2025
@andriyDev andriyDev force-pushed the no-asset-id-animation-clip branch from d36b927 to a36ba69 Compare June 13, 2025 20:21
@andriyDev andriyDev added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Blocked This cannot move forward until something else changes labels Jun 13, 2025
@andriyDev
Copy link
Contributor Author

Fixed a migration issue with #19631 so this is ready to review!

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 A-Assets Load files from disk to use for things like images, models, and sounds D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Needs-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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant