Prevent AnimationGraph from serializing AssetIds. (#19615)

# Objective

- A step towards #19024.
- `AnimationGraph` can serialize raw `AssetId`s. However for normal
handles, this is a runtime ID. This means it is unlikely that the
`AssetId` will correspond to the same asset after deserializing -
effectively breaking the graph.

## 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 #19631).

Author: andriyDev

assets/animation_graphs/Fox.animgraph.ron

@@ -9,20 +9,20 @@
             (
                 node_type: Blend,
                 mask: 0,
-                weight: 1.0,
+                weight: 0.5,
             ),
             (
-                node_type: Clip(AssetPath("models/animated/Fox.glb#Animation0")),
+                node_type: Clip("models/animated/Fox.glb#Animation0"),
                 mask: 0,
                 weight: 1.0,
             ),
             (
-                node_type: Clip(AssetPath("models/animated/Fox.glb#Animation1")),
+                node_type: Clip("models/animated/Fox.glb#Animation1"),
                 mask: 0,
                 weight: 1.0,
             ),
             (
-                node_type: Clip(AssetPath("models/animated/Fox.glb#Animation2")),
+                node_type: Clip("models/animated/Fox.glb#Animation2"),
                 mask: 0,
                 weight: 1.0,
             ),

crates/bevy_animation/src/graph.rs

@@ -19,7 +19,7 @@ use bevy_ecs::{
     system::{Res, ResMut},
 };
 use bevy_platform::collections::HashMap;
-use bevy_reflect::{prelude::ReflectDefault, Reflect, ReflectSerialize};
+use bevy_reflect::{prelude::ReflectDefault, Reflect};
 use derive_more::derive::From;
 use petgraph::{
     graph::{DiGraph, NodeIndex},
@@ -29,6 +29,7 @@ use ron::de::SpannedError;
 use serde::{Deserialize, Serialize};
 use smallvec::SmallVec;
 use thiserror::Error;
+use tracing::warn;
 
 use crate::{AnimationClip, AnimationTargetId};
 
@@ -108,9 +109,8 @@ use crate::{AnimationClip, AnimationTargetId};
 /// [RON]: https://github.com/ron-rs/ron
 ///
 /// [RFC 51]: https://github.com/bevyengine/rfcs/blob/main/rfcs/51-animation-composition.md
-#[derive(Asset, Reflect, Clone, Debug, Serialize)]
-#[reflect(Serialize, Debug, Clone)]
-#[serde(into = "SerializedAnimationGraph")]
+#[derive(Asset, Reflect, Clone, Debug)]
+#[reflect(Debug, Clone)]
 pub struct AnimationGraph {
     /// The `petgraph` data structure that defines the animation graph.
     pub graph: AnimationDiGraph,
@@ -242,20 +242,40 @@ pub enum AnimationNodeType {
 #[derive(Default)]
 pub struct AnimationGraphAssetLoader;
 
-/// Various errors that can occur when serializing or deserializing animation
-/// graphs to and from RON, respectively.
+/// Errors that can occur when serializing animation graphs to RON.
+#[derive(Error, Debug)]
+pub enum AnimationGraphSaveError {
+    /// An I/O error occurred.
+    #[error(transparent)]
+    Io(#[from] io::Error),
+    /// An error occurred in RON serialization.
+    #[error(transparent)]
+    Ron(#[from] ron::Error),
+    /// An error occurred converting the graph to its serialization form.
+    #[error(transparent)]
+    ConvertToSerialized(#[from] NonPathHandleError),
+}
+
+/// Errors that can occur when deserializing animation graphs from RON.
 #[derive(Error, Debug)]
 pub enum AnimationGraphLoadError {
     /// An I/O error occurred.
-    #[error("I/O")]
+    #[error(transparent)]
     Io(#[from] io::Error),
-    /// An error occurred in RON serialization or deserialization.
-    #[error("RON serialization")]
+    /// An error occurred in RON deserialization.
+    #[error(transparent)]
     Ron(#[from] ron::Error),
     /// An error occurred in RON deserialization, and the location of the error
     /// is supplied.
-    #[error("RON serialization")]
+    #[error(transparent)]
     SpannedRon(#[from] SpannedError),
+    /// The deserialized graph contained legacy data that we no longer support.
+    #[error(
+        "The deserialized AnimationGraph contained an AnimationClip referenced by an AssetId, \
+    which is no longer supported. Consider manually deserializing the SerializedAnimationGraph \
+    type and determine how to migrate any SerializedAnimationClip::AssetId animation clips"
+    )]
+    GraphContainsLegacyAssetId,
 }
 
 /// Acceleration structures for animation graphs that allows Bevy to evaluate
@@ -388,18 +408,32 @@ pub struct SerializedAnimationGraphNode {
 #[derive(Serialize, Deserialize)]
 pub enum SerializedAnimationNodeType {
     /// Corresponds to [`AnimationNodeType::Clip`].
-    Clip(SerializedAnimationClip),
+    Clip(MigrationSerializedAnimationClip),
     /// Corresponds to [`AnimationNodeType::Blend`].
     Blend,
     /// Corresponds to [`AnimationNodeType::Add`].
     Add,
 }
 
-/// A version of `Handle<AnimationClip>` suitable for serializing as an asset.
+/// A type to facilitate migration from the legacy format of [`SerializedAnimationGraph`] to the
+/// new format.
 ///
-/// This replaces any handle that has a path with an [`AssetPath`]. Failing
-/// that, the asset ID is serialized directly.
+/// By using untagged serde deserialization, we can try to deserialize the modern form, then
+/// fallback to the legacy form. Users must migrate to the modern form by Bevy 0.18.
+// TODO: Delete this after Bevy 0.17.
 #[derive(Serialize, Deserialize)]
+#[serde(untagged)]
+pub enum MigrationSerializedAnimationClip {
+    /// This is the new type of this field.
+    Modern(AssetPath<'static>),
+    /// This is the legacy type of this field. Users must migrate away from this.
+    #[serde(skip_serializing)]
+    Legacy(SerializedAnimationClip),
+}
+
+/// The legacy form of serialized animation clips. This allows raw asset IDs to be deserialized.
+// TODO: Delete this after Bevy 0.17.
+#[derive(Deserialize)]
 pub enum SerializedAnimationClip {
     /// Records an asset path.
     AssetPath(AssetPath<'static>),
@@ -648,12 +682,13 @@ impl AnimationGraph {
     ///
     /// If writing to a file, it can later be loaded with the
     /// [`AnimationGraphAssetLoader`] to reconstruct the graph.
-    pub fn save<W>(&self, writer: &mut W) -> Result<(), AnimationGraphLoadError>
+    pub fn save<W>(&self, writer: &mut W) -> Result<(), AnimationGraphSaveError>
     where
         W: Write,
     {
         let mut ron_serializer = ron::ser::Serializer::new(writer, None)?;
-        Ok(self.serialize(&mut ron_serializer)?)
+        let serialized_graph: SerializedAnimationGraph = self.clone().try_into()?;
+        Ok(serialized_graph.serialize(&mut ron_serializer)?)
     }
 
     /// Adds an animation target (bone) to the mask group with the given ID.
@@ -758,28 +793,55 @@ impl AssetLoader for AnimationGraphAssetLoader {
         let serialized_animation_graph = SerializedAnimationGraph::deserialize(&mut deserializer)
             .map_err(|err| deserializer.span_error(err))?;
 
-        // Load all `AssetPath`s to convert from a
-        // `SerializedAnimationGraph` to a real `AnimationGraph`.
-        Ok(AnimationGraph {
-            graph: serialized_animation_graph.graph.map(
-                |_, serialized_node| AnimationGraphNode {
-                    node_type: match serialized_node.node_type {
-                        SerializedAnimationNodeType::Clip(ref clip) => match clip {
-                            SerializedAnimationClip::AssetId(asset_id) => {
-                                AnimationNodeType::Clip(Handle::Weak(*asset_id))
-                            }
-                            SerializedAnimationClip::AssetPath(asset_path) => {
-                                AnimationNodeType::Clip(load_context.load(asset_path))
+        // Load all `AssetPath`s to convert from a `SerializedAnimationGraph` to a real
+        // `AnimationGraph`. This is effectively a `DiGraph::map`, but this allows us to return
+        // errors.
+        let mut animation_graph = DiGraph::with_capacity(
+            serialized_animation_graph.graph.node_count(),
+            serialized_animation_graph.graph.edge_count(),
+        );
+
+        let mut already_warned = false;
+        for serialized_node in serialized_animation_graph.graph.node_weights() {
+            animation_graph.add_node(AnimationGraphNode {
+                node_type: match serialized_node.node_type {
+                    SerializedAnimationNodeType::Clip(ref clip) => match clip {
+                        MigrationSerializedAnimationClip::Modern(path) => {
+                            AnimationNodeType::Clip(load_context.load(path.clone()))
+                        }
+                        MigrationSerializedAnimationClip::Legacy(
+                            SerializedAnimationClip::AssetPath(path),
+                        ) => {
+                            if !already_warned {
+                                let path = load_context.asset_path();
+                                warn!(
+                                    "Loaded an AnimationGraph asset at \"{path}\" which contains a \
+                                    legacy-style SerializedAnimationClip. Please re-save the asset \
+                                    using AnimationGraph::save to automatically migrate to the new \
+                                    format"
+                                );
+                                already_warned = true;
                             }
-                        },
-                        SerializedAnimationNodeType::Blend => AnimationNodeType::Blend,
-                        SerializedAnimationNodeType::Add => AnimationNodeType::Add,
+                            AnimationNodeType::Clip(load_context.load(path.clone()))
+                        }
+                        MigrationSerializedAnimationClip::Legacy(
+                            SerializedAnimationClip::AssetId(_),
+                        ) => {
+                            return Err(AnimationGraphLoadError::GraphContainsLegacyAssetId);
+                        }
                     },
-                    mask: serialized_node.mask,
-                    weight: serialized_node.weight,
+                    SerializedAnimationNodeType::Blend => AnimationNodeType::Blend,
+                    SerializedAnimationNodeType::Add => AnimationNodeType::Add,
                 },
-                |_, _| (),
-            ),
+                mask: serialized_node.mask,
+                weight: serialized_node.weight,
+            });
+        }
+        for edge in serialized_animation_graph.graph.raw_edges() {
+            animation_graph.add_edge(edge.source(), edge.target(), ());
+        }
+        Ok(AnimationGraph {
+            graph: animation_graph,
             root: serialized_animation_graph.root,
             mask_groups: serialized_animation_graph.mask_groups,
         })
@@ -790,37 +852,50 @@ impl AssetLoader for AnimationGraphAssetLoader {
     }
 }
 
-impl From<AnimationGraph> for SerializedAnimationGraph {
-    fn from(animation_graph: AnimationGraph) -> Self {
-        // If any of the animation clips have paths, then serialize them as
-        // `SerializedAnimationClip::AssetPath` so that the
-        // `AnimationGraphAssetLoader` can load them.
-        Self {
-            graph: animation_graph.graph.map(
-                |_, node| SerializedAnimationGraphNode {
-                    weight: node.weight,
-                    mask: node.mask,
-                    node_type: match node.node_type {
-                        AnimationNodeType::Clip(ref clip) => match clip.path() {
-                            Some(path) => SerializedAnimationNodeType::Clip(
-                                SerializedAnimationClip::AssetPath(path.clone()),
-                            ),
-                            None => SerializedAnimationNodeType::Clip(
-                                SerializedAnimationClip::AssetId(clip.id()),
-                            ),
-                        },
-                        AnimationNodeType::Blend => SerializedAnimationNodeType::Blend,
-                        AnimationNodeType::Add => SerializedAnimationNodeType::Add,
+impl TryFrom<AnimationGraph> for SerializedAnimationGraph {
+    type Error = NonPathHandleError;
+
+    fn try_from(animation_graph: AnimationGraph) -> Result<Self, NonPathHandleError> {
+        // Convert all the `Handle<AnimationClip>` to AssetPath, so that
+        // `AnimationGraphAssetLoader` can load them. This is effectively just doing a
+        // `DiGraph::map`, except we need to return an error if any handles aren't associated to a
+        // path.
+        let mut serialized_graph = DiGraph::with_capacity(
+            animation_graph.graph.node_count(),
+            animation_graph.graph.edge_count(),
+        );
+        for node in animation_graph.graph.node_weights() {
+            serialized_graph.add_node(SerializedAnimationGraphNode {
+                weight: node.weight,
+                mask: node.mask,
+                node_type: match node.node_type {
+                    AnimationNodeType::Clip(ref clip) => match clip.path() {
+                        Some(path) => SerializedAnimationNodeType::Clip(
+                            MigrationSerializedAnimationClip::Modern(path.clone()),
+                        ),
+                        None => return Err(NonPathHandleError),
                     },
+                    AnimationNodeType::Blend => SerializedAnimationNodeType::Blend,
+                    AnimationNodeType::Add => SerializedAnimationNodeType::Add,
                 },
-                |_, _| (),
-            ),
+            });
+        }
+        for edge in animation_graph.graph.raw_edges() {
+            serialized_graph.add_edge(edge.source(), edge.target(), ());
+        }
+        Ok(Self {
+            graph: serialized_graph,
             root: animation_graph.root,
             mask_groups: animation_graph.mask_groups,
-        }
+        })
     }
 }
 
+/// Error for when only path [`Handle`]s are supported.
+#[derive(Error, Debug)]
+#[error("AnimationGraph contains a handle to an AnimationClip that does not correspond to an asset path")]
+pub struct NonPathHandleError;
+
 /// A system that creates, updates, and removes [`ThreadedAnimationGraph`]
 /// structures for every changed [`AnimationGraph`].
 ///

examples/animation/animation_graph.rs

@@ -182,6 +182,10 @@ fn setup_assets_programmatically(
             .spawn(async move {
                 use std::io::Write;
 
+                let animation_graph: SerializedAnimationGraph = animation_graph
+                    .try_into()
+                    .expect("The animation graph failed to convert to its serialized form");
+
                 let serialized_graph =
                     ron::ser::to_string_pretty(&animation_graph, PrettyConfig::default())
                         .expect("Failed to serialize the animation graph");

release-content/migration-guides/animation_graph_no_more_asset_ids.md

@@ -0,0 +1,20 @@
+---
+title: `AnimationGraph` no longer supports raw AssetIds.
+pull_requests: []
+---
+
+In previous versions of Bevy, `AnimationGraph` would serialize `Handle<AnimationClip>` as an asset
+path, and if that wasn't available it would fallback to serializing `AssetId<AnimationClip>`. In
+practice, this was not very useful. `AssetId` is (usually) a runtime-generated ID. This means for an
+arbitrary `Handle<AnimationClip>`, it was incredibly unlikely that your handle before serialization
+would correspond to the same asset as after serialization.
+
+This confusing behavior has been removed. As a side-effect, any `AnimationGraph`s you previously
+saved (via `AnimationGraph::save`) will need to be re-saved. These legacy `AnimationGraph`s can
+still be loaded until the next Bevy version. Loading and then saving the `AnimationGraph` again will
+automatically migrate the `AnimationGraph`.
+
+If your `AnimationGraph` contained serialized `AssetId`s, you will need to manually load the bytes
+of the saved graph, deserialize it into `SerializedAnimationGraph`, and then manually decide how to
+migrate those `AssetId`s. Alternatively, you could simply rebuild the graph from scratch and save a
+new instance. We expect this to be a very rare situation.