From 2ea51fc60f6983822922cdd74ca4be26e633ae50 Mon Sep 17 00:00:00 2001 From: "Al M." Date: Sun, 15 Sep 2024 07:33:39 -0700 Subject: [PATCH] Use `FromReflect` when extracting entities in dynamic scenes (#15174) # Objective Fix #10284. ## Solution When `DynamicSceneBuilder` extracts entities, they are cloned via `PartialReflect::clone_value`, making them into dynamic versions of the original components. This loses any custom `ReflectSerialize` type data. Dynamic scenes are deserialized with the original types, not the dynamic versions, and so any component with a custom serialize may fail. In this case `Rect` and `Vec2`. The dynamic version includes the field names 'x' and 'y' but the `Serialize` impl doesn't, hence the "expect float" error. The solution here: Instead of using `clone_value` to clone the components, `FromReflect` clones and retains the original information needed to serialize with any custom `Serialize` impls. I think using something like `reflect_clone` from (https://github.com/bevyengine/bevy/pull/13432) might make this more efficient. I also did the same when deserializing dynamic scenes to appease some of the round-trip tests which use `ReflectPartialEq`, which requires the types be the same and not a unique/proxy pair. I'm not sure it's otherwise necessary. Maybe this would also be more efficient when spawning dynamic scenes with `reflect_clone` instead of `FromReflect` again? An alternative solution would be to fall back to the dynamic version when deserializing `DynamicScene`s if the custom version fails. I think that's possible. Or maybe simply always deserializing via the dynamic route for dynamic scenes? ## Testing This example is similar to the original test case in #10284: ``` rust #![allow(missing_docs)] use bevy::{prelude::*, scene::SceneInstanceReady}; fn main() { App::new() .add_plugins(DefaultPlugins) .add_systems(Startup, (save, load).chain()) .observe(check) .run(); } static SAVEGAME_SAVE_PATH: &str = "savegame.scn.ron"; fn save(world: &mut World) { let entity = world.spawn(OrthographicProjection::default()).id(); let scene = DynamicSceneBuilder::from_world(world) .extract_entity(entity) .build(); if let Some(registry) = world.get_resource::() { let registry = registry.read(); let serialized_scene = scene.serialize(®istry).unwrap(); // println!("{}", serialized_scene); std::fs::write(format!("assets/{SAVEGAME_SAVE_PATH}"), serialized_scene).unwrap(); } world.entity_mut(entity).despawn_recursive(); } fn load(mut commands: Commands, asset_server: Res) { commands.spawn(DynamicSceneBundle { scene: asset_server.load(SAVEGAME_SAVE_PATH), ..default() }); } fn check(_trigger: Trigger, query: Query<&OrthographicProjection>) { dbg!(query.single()); } ``` ## Migration Guide The `DynamicScene` format is changed to use custom serialize impls so old scene files will need updating: Old: ```ron ( resources: {}, entities: { 4294967299: ( components: { "bevy_render::camera::projection::OrthographicProjection": ( near: 0.0, far: 1000.0, viewport_origin: ( x: 0.5, y: 0.5, ), scaling_mode: WindowSize(1.0), scale: 1.0, area: ( min: ( x: -1.0, y: -1.0, ), max: ( x: 1.0, y: 1.0, ), ), ), }, ), }, ) ``` New: ```ron ( resources: {}, entities: { 4294967299: ( components: { "bevy_render::camera::projection::OrthographicProjection": ( near: 0.0, far: 1000.0, viewport_origin: (0.5, 0.5), scaling_mode: WindowSize(1.0), scale: 1.0, area: ( min: (-1.0, -1.0), max: (1.0, 1.0), ), ), }, ), }, ) ``` --------- Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com> --- crates/bevy_reflect/src/impls/glam.rs | 78 ++++++++-------- crates/bevy_reflect/src/lib.rs | 26 +----- .../bevy_scene/src/dynamic_scene_builder.rs | 19 +++- crates/bevy_scene/src/serde.rs | 91 +++++++++++++++---- 4 files changed, 130 insertions(+), 84 deletions(-) diff --git a/crates/bevy_reflect/src/impls/glam.rs b/crates/bevy_reflect/src/impls/glam.rs index 06823374b0c08..8d54a5d5c9940 100644 --- a/crates/bevy_reflect/src/impls/glam.rs +++ b/crates/bevy_reflect/src/impls/glam.rs @@ -1,10 +1,10 @@ use crate as bevy_reflect; -use crate::prelude::ReflectDefault; +use crate::{std_traits::ReflectDefault, ReflectDeserialize, ReflectSerialize}; use bevy_reflect_derive::{impl_reflect, impl_reflect_value}; use glam::*; impl_reflect!( - #[reflect(Debug, Hash, PartialEq, Default)] + #[reflect(Debug, Hash, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct IVec2 { x: i32, @@ -12,7 +12,7 @@ impl_reflect!( } ); impl_reflect!( - #[reflect(Debug, Hash, PartialEq, Default)] + #[reflect(Debug, Hash, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct IVec3 { x: i32, @@ -21,7 +21,7 @@ impl_reflect!( } ); impl_reflect!( - #[reflect(Debug, Hash, PartialEq, Default)] + #[reflect(Debug, Hash, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct IVec4 { x: i32, @@ -32,7 +32,7 @@ impl_reflect!( ); impl_reflect!( - #[reflect(Debug, Hash, PartialEq, Default)] + #[reflect(Debug, Hash, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct I64Vec2 { x: i64, @@ -41,7 +41,7 @@ impl_reflect!( ); impl_reflect!( - #[reflect(Debug, Hash, PartialEq, Default)] + #[reflect(Debug, Hash, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct I64Vec3 { x: i64, @@ -51,7 +51,7 @@ impl_reflect!( ); impl_reflect!( - #[reflect(Debug, Hash, PartialEq, Default)] + #[reflect(Debug, Hash, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct I64Vec4 { x: i64, @@ -62,7 +62,7 @@ impl_reflect!( ); impl_reflect!( - #[reflect(Debug, Hash, PartialEq, Default)] + #[reflect(Debug, Hash, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct UVec2 { x: u32, @@ -70,7 +70,7 @@ impl_reflect!( } ); impl_reflect!( - #[reflect(Debug, Hash, PartialEq, Default)] + #[reflect(Debug, Hash, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct UVec3 { x: u32, @@ -79,7 +79,7 @@ impl_reflect!( } ); impl_reflect!( - #[reflect(Debug, Hash, PartialEq, Default)] + #[reflect(Debug, Hash, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct UVec4 { x: u32, @@ -90,7 +90,7 @@ impl_reflect!( ); impl_reflect!( - #[reflect(Debug, Hash, PartialEq, Default)] + #[reflect(Debug, Hash, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct U64Vec2 { x: u64, @@ -98,7 +98,7 @@ impl_reflect!( } ); impl_reflect!( - #[reflect(Debug, Hash, PartialEq, Default)] + #[reflect(Debug, Hash, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct U64Vec3 { x: u64, @@ -107,7 +107,7 @@ impl_reflect!( } ); impl_reflect!( - #[reflect(Debug, Hash, PartialEq, Default)] + #[reflect(Debug, Hash, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct U64Vec4 { x: u64, @@ -118,7 +118,7 @@ impl_reflect!( ); impl_reflect!( - #[reflect(Debug, PartialEq, Default)] + #[reflect(Debug, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct Vec2 { x: f32, @@ -126,7 +126,7 @@ impl_reflect!( } ); impl_reflect!( - #[reflect(Debug, PartialEq, Default)] + #[reflect(Debug, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct Vec3 { x: f32, @@ -135,7 +135,7 @@ impl_reflect!( } ); impl_reflect!( - #[reflect(Debug, PartialEq, Default)] + #[reflect(Debug, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct Vec3A { x: f32, @@ -144,7 +144,7 @@ impl_reflect!( } ); impl_reflect!( - #[reflect(Debug, PartialEq, Default)] + #[reflect(Debug, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct Vec4 { x: f32, @@ -155,7 +155,7 @@ impl_reflect!( ); impl_reflect!( - #[reflect(Debug, PartialEq, Default)] + #[reflect(Debug, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct BVec2 { x: bool, @@ -163,7 +163,7 @@ impl_reflect!( } ); impl_reflect!( - #[reflect(Debug, PartialEq, Default)] + #[reflect(Debug, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct BVec3 { x: bool, @@ -172,7 +172,7 @@ impl_reflect!( } ); impl_reflect!( - #[reflect(Debug, PartialEq, Default)] + #[reflect(Debug, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct BVec4 { x: bool, @@ -183,7 +183,7 @@ impl_reflect!( ); impl_reflect!( - #[reflect(Debug, PartialEq, Default)] + #[reflect(Debug, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct DVec2 { x: f64, @@ -191,7 +191,7 @@ impl_reflect!( } ); impl_reflect!( - #[reflect(Debug, PartialEq, Default)] + #[reflect(Debug, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct DVec3 { x: f64, @@ -200,7 +200,7 @@ impl_reflect!( } ); impl_reflect!( - #[reflect(Debug, PartialEq, Default)] + #[reflect(Debug, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct DVec4 { x: f64, @@ -211,7 +211,7 @@ impl_reflect!( ); impl_reflect!( - #[reflect(Debug, PartialEq, Default)] + #[reflect(Debug, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct Mat2 { x_axis: Vec2, @@ -219,7 +219,7 @@ impl_reflect!( } ); impl_reflect!( - #[reflect(Debug, PartialEq, Default)] + #[reflect(Debug, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct Mat3 { x_axis: Vec3, @@ -228,7 +228,7 @@ impl_reflect!( } ); impl_reflect!( - #[reflect(Debug, PartialEq, Default)] + #[reflect(Debug, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct Mat3A { x_axis: Vec3A, @@ -237,7 +237,7 @@ impl_reflect!( } ); impl_reflect!( - #[reflect(Debug, PartialEq, Default)] + #[reflect(Debug, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct Mat4 { x_axis: Vec4, @@ -248,7 +248,7 @@ impl_reflect!( ); impl_reflect!( - #[reflect(Debug, PartialEq, Default)] + #[reflect(Debug, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct DMat2 { x_axis: DVec2, @@ -256,7 +256,7 @@ impl_reflect!( } ); impl_reflect!( - #[reflect(Debug, PartialEq, Default)] + #[reflect(Debug, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct DMat3 { x_axis: DVec3, @@ -265,7 +265,7 @@ impl_reflect!( } ); impl_reflect!( - #[reflect(Debug, PartialEq, Default)] + #[reflect(Debug, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct DMat4 { x_axis: DVec4, @@ -276,7 +276,7 @@ impl_reflect!( ); impl_reflect!( - #[reflect(Debug, PartialEq, Default)] + #[reflect(Debug, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct Affine2 { matrix2: Mat2, @@ -284,7 +284,7 @@ impl_reflect!( } ); impl_reflect!( - #[reflect(Debug, PartialEq, Default)] + #[reflect(Debug, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct Affine3A { matrix3: Mat3A, @@ -293,7 +293,7 @@ impl_reflect!( ); impl_reflect!( - #[reflect(Debug, PartialEq, Default)] + #[reflect(Debug, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct DAffine2 { matrix2: DMat2, @@ -301,7 +301,7 @@ impl_reflect!( } ); impl_reflect!( - #[reflect(Debug, PartialEq, Default)] + #[reflect(Debug, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct DAffine3 { matrix3: DMat3, @@ -310,7 +310,7 @@ impl_reflect!( ); impl_reflect!( - #[reflect(Debug, PartialEq, Default)] + #[reflect(Debug, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct Quat { x: f32, @@ -320,7 +320,7 @@ impl_reflect!( } ); impl_reflect!( - #[reflect(Debug, PartialEq, Default)] + #[reflect(Debug, PartialEq, Default, Deserialize, Serialize)] #[type_path = "glam"] struct DQuat { x: f64, @@ -330,6 +330,6 @@ impl_reflect!( } ); -impl_reflect_value!(::glam::EulerRot(Debug, Default)); -impl_reflect_value!(::glam::BVec3A(Debug, Default)); -impl_reflect_value!(::glam::BVec4A(Debug, Default)); +impl_reflect_value!(::glam::EulerRot(Debug, Default, Deserialize, Serialize)); +impl_reflect_value!(::glam::BVec3A(Debug, Default, Deserialize, Serialize)); +impl_reflect_value!(::glam::BVec4A(Debug, Default, Deserialize, Serialize)); diff --git a/crates/bevy_reflect/src/lib.rs b/crates/bevy_reflect/src/lib.rs index 40f602ac35038..61479625e3bc0 100644 --- a/crates/bevy_reflect/src/lib.rs +++ b/crates/bevy_reflect/src/lib.rs @@ -2961,12 +2961,7 @@ bevy_reflect::tests::Test { let output = to_string_pretty(&ser, config).unwrap(); let expected = r#" { - "glam::Quat": ( - x: 1.0, - y: 2.0, - z: 3.0, - w: 4.0, - ), + "glam::Quat": (1.0, 2.0, 3.0, 4.0), }"#; assert_eq!(expected, format!("\n{output}")); @@ -2976,12 +2971,7 @@ bevy_reflect::tests::Test { fn quat_deserialization() { let data = r#" { - "glam::Quat": ( - x: 1.0, - y: 2.0, - z: 3.0, - w: 4.0, - ), + "glam::Quat": (1.0, 2.0, 3.0, 4.0), }"#; let mut registry = TypeRegistry::default(); @@ -3020,11 +3010,7 @@ bevy_reflect::tests::Test { let output = to_string_pretty(&ser, config).unwrap(); let expected = r#" { - "glam::Vec3": ( - x: 12.0, - y: 3.0, - z: -6.9, - ), + "glam::Vec3": (12.0, 3.0, -6.9), }"#; assert_eq!(expected, format!("\n{output}")); @@ -3034,11 +3020,7 @@ bevy_reflect::tests::Test { fn vec3_deserialization() { let data = r#" { - "glam::Vec3": ( - x: 12.0, - y: 3.0, - z: -6.9, - ), + "glam::Vec3": (12.0, 3.0, -6.9), }"#; let mut registry = TypeRegistry::default(); diff --git a/crates/bevy_scene/src/dynamic_scene_builder.rs b/crates/bevy_scene/src/dynamic_scene_builder.rs index 26084c1a3e27f..01e90a8e43cef 100644 --- a/crates/bevy_scene/src/dynamic_scene_builder.rs +++ b/crates/bevy_scene/src/dynamic_scene_builder.rs @@ -6,7 +6,7 @@ use bevy_ecs::{ reflect::{AppTypeRegistry, ReflectComponent, ReflectResource}, world::World, }; -use bevy_reflect::PartialReflect; +use bevy_reflect::{PartialReflect, ReflectFromReflect}; use bevy_utils::default; use std::collections::BTreeMap; @@ -274,11 +274,22 @@ impl<'w> DynamicSceneBuilder<'w> { return None; } - let component = type_registry - .get(type_id)? + let type_registration = type_registry.get(type_id)?; + + let component = type_registration .data::()? .reflect(original_entity)?; - entry.components.push(component.clone_value()); + + // Clone via `FromReflect`. Unlike `PartialReflect::clone_value` this + // retains the original type and `ReflectSerialize` type data which is needed to + // deserialize. + let component = type_registration + .data::() + .and_then(|fr| fr.from_reflect(component.as_partial_reflect())) + .map(PartialReflect::into_partial_reflect) + .unwrap_or_else(|| component.clone_value()); + + entry.components.push(component); Some(()) }; extract_and_push(); diff --git a/crates/bevy_scene/src/serde.rs b/crates/bevy_scene/src/serde.rs index 1b0f54736e48e..bbf57350186a9 100644 --- a/crates/bevy_scene/src/serde.rs +++ b/crates/bevy_scene/src/serde.rs @@ -3,11 +3,11 @@ use crate::{DynamicEntity, DynamicScene}; use bevy_ecs::entity::Entity; use bevy_reflect::serde::{TypedReflectDeserializer, TypedReflectSerializer}; -use bevy_reflect::PartialReflect; use bevy_reflect::{ serde::{ReflectDeserializer, TypeRegistrationDeserializer}, TypeRegistry, }; +use bevy_reflect::{PartialReflect, ReflectFromReflect}; use bevy_utils::HashSet; use serde::ser::SerializeMap; use serde::{ @@ -488,9 +488,19 @@ impl<'a, 'de> Visitor<'de> for SceneMapVisitor<'a> { ))); } - entries.push( - map.next_value_seed(TypedReflectDeserializer::new(registration, self.registry))?, - ); + let value = + map.next_value_seed(TypedReflectDeserializer::new(registration, self.registry))?; + + // Attempt to convert using FromReflect. + let value = self + .registry + .get(registration.type_id()) + .and_then(|tr| tr.data::()) + .and_then(|fr| fr.from_reflect(value.as_partial_reflect())) + .map(PartialReflect::into_partial_reflect) + .unwrap_or(value); + + entries.push(value); } Ok(entries) @@ -508,10 +518,10 @@ mod tests { use bevy_ecs::query::{With, Without}; use bevy_ecs::reflect::{AppTypeRegistry, ReflectMapEntities}; use bevy_ecs::world::FromWorld; - use bevy_reflect::{Reflect, ReflectSerialize}; + use bevy_reflect::{Reflect, ReflectDeserialize, ReflectSerialize}; use bincode::Options; use serde::de::DeserializeSeed; - use serde::Serialize; + use serde::{Deserialize, Serialize}; use std::io::BufReader; #[derive(Component, Reflect, Default)] @@ -524,6 +534,30 @@ mod tests { #[reflect(Component)] struct Baz(i32); + // De/serialize as hex. + mod qux { + use serde::{de::Error, Deserialize, Deserializer, Serializer}; + + pub fn serialize(value: &u32, serializer: S) -> Result + where + S: Serializer, + { + serializer.serialize_str(&format!("{:X}", value)) + } + + pub fn deserialize<'de, D>(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + u32::from_str_radix(<&str as Deserialize>::deserialize(deserializer)?, 16) + .map_err(Error::custom) + } + } + + #[derive(Component, Copy, Clone, Reflect, Debug, PartialEq, Serialize, Deserialize)] + #[reflect(Component, Serialize, Deserialize)] + struct Qux(#[serde(with = "qux")] u32); + #[derive(Component, Reflect, Default)] #[reflect(Component)] struct MyComponent { @@ -572,6 +606,7 @@ mod tests { registry.register::(); registry.register::(); registry.register::(); + registry.register::(); registry.register::(); registry.register::(); registry.register::(); @@ -696,6 +731,18 @@ mod tests { assert_eq!(1, dst_world.query::<&Baz>().iter(&dst_world).count()); } + fn roundtrip_ron(world: &World) -> (DynamicScene, DynamicScene) { + let scene = DynamicScene::from_world(world); + let registry = world.resource::().read(); + let serialized = scene.serialize(®istry).unwrap(); + let mut deserializer = ron::de::Deserializer::from_str(&serialized).unwrap(); + let scene_deserializer = SceneDeserializer { + type_registry: ®istry, + }; + let deserialized_scene = scene_deserializer.deserialize(&mut deserializer).unwrap(); + (scene, deserialized_scene) + } + #[test] fn should_roundtrip_with_later_generations_and_obsolete_references() { let mut world = create_world(); @@ -707,19 +754,7 @@ mod tests { world.despawn(a); world.spawn(MyEntityRef(foo)).insert(Bar(123)); - let registry = world.resource::(); - - let scene = DynamicScene::from_world(&world); - - let serialized = scene - .serialize(&world.resource::().read()) - .unwrap(); - let mut deserializer = ron::de::Deserializer::from_str(&serialized).unwrap(); - let scene_deserializer = SceneDeserializer { - type_registry: ®istry.0.read(), - }; - - let deserialized_scene = scene_deserializer.deserialize(&mut deserializer).unwrap(); + let (scene, deserialized_scene) = roundtrip_ron(&world); let mut map = EntityHashMap::default(); let mut dst_world = create_world(); @@ -747,6 +782,24 @@ mod tests { .all(|r| world.get_entity(r.0).is_none())); } + #[test] + fn should_roundtrip_with_custom_serialization() { + let mut world = create_world(); + let qux = Qux(42); + world.spawn(qux); + + let (scene, deserialized_scene) = roundtrip_ron(&world); + + assert_eq!(1, deserialized_scene.entities.len()); + assert_scene_eq(&scene, &deserialized_scene); + + let mut world = create_world(); + deserialized_scene + .write_to_world(&mut world, &mut EntityHashMap::default()) + .unwrap(); + assert_eq!(&qux, world.query::<&Qux>().single(&world)); + } + #[test] fn should_roundtrip_postcard() { let mut world = create_world();