From 956eb1e38532972a062207c775bd95d6a6f71f9e Mon Sep 17 00:00:00 2001 From: Peter Hayman Date: Thu, 14 Mar 2024 09:25:34 +1100 Subject: [PATCH 1/9] test: add failing `should_reserialize` test --- crates/bevy_scene/src/serde.rs | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/crates/bevy_scene/src/serde.rs b/crates/bevy_scene/src/serde.rs index 0904502f2d4e4..ff82d11b10d37 100644 --- a/crates/bevy_scene/src/serde.rs +++ b/crates/bevy_scene/src/serde.rs @@ -693,6 +693,31 @@ mod tests { assert_eq!(1, dst_world.query::<&Baz>().iter(&dst_world).count()); } + #[test] + fn should_reserialize() { + let mut world = create_world(); + + world.spawn(MyComponent { + foo: [1, 2, 3], + bar: (1.3, 3.7), + baz: MyEnum::Tuple("Hello World!".to_string()), + }); + + let registry = world.resource::(); + let scene = DynamicScene::from_world(&world); + let serialized_scene1 = scene.serialize_ron(®istry.0).unwrap(); + + let scene_deserializer = SceneDeserializer { + type_registry: ®istry.0.read(), + }; + let mut deserializer = ron::de::Deserializer::from_str(&serialized_scene1).unwrap(); + let deserialized_scene = scene_deserializer.deserialize(&mut deserializer).unwrap(); + + let serialized_scene2 = deserialized_scene.serialize_ron(®istry.0).unwrap(); + + assert_eq!(serialized_scene1, serialized_scene2); + } + #[test] fn should_roundtrip_with_later_generations_and_obsolete_references() { let mut world = create_world(); From a4a5980e847e929dc151e2e0220ffc02191380f6 Mon Sep 17 00:00:00 2001 From: Peter Hayman Date: Thu, 14 Mar 2024 09:39:27 +1100 Subject: [PATCH 2/9] Add regression test comment --- crates/bevy_scene/src/serde.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/bevy_scene/src/serde.rs b/crates/bevy_scene/src/serde.rs index ff82d11b10d37..a5cf2a895249f 100644 --- a/crates/bevy_scene/src/serde.rs +++ b/crates/bevy_scene/src/serde.rs @@ -693,6 +693,7 @@ mod tests { assert_eq!(1, dst_world.query::<&Baz>().iter(&dst_world).count()); } + // Regression test regarding [#12462](https://github.com/bevyengine/bevy/issues/12462) #[test] fn should_reserialize() { let mut world = create_world(); From ac6ee8af3a13b5ebd80104a2908811a42b674f69 Mon Sep 17 00:00:00 2001 From: Peter Hayman Date: Thu, 14 Mar 2024 09:52:09 +1100 Subject: [PATCH 3/9] fix: use DynamicEnum::set_variant_with_index --- crates/bevy_reflect/src/serde/de.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/crates/bevy_reflect/src/serde/de.rs b/crates/bevy_reflect/src/serde/de.rs index 0949085bc4976..c84e049f380be 100644 --- a/crates/bevy_reflect/src/serde/de.rs +++ b/crates/bevy_reflect/src/serde/de.rs @@ -753,8 +753,12 @@ impl<'a, 'de> Visitor<'de> for EnumVisitor<'a> { )? .into(), }; - - dynamic_enum.set_variant(variant_info.name(), value); + let variant_name = variant_info.name(); + let variant_index = self + .enum_info + .index_of(variant_name) + .expect("variant should exist"); + dynamic_enum.set_variant_with_index(variant_index, variant_name, value); Ok(dynamic_enum) } } From 88c8f314fb72fe5bec6d46b1cc01cd64fef238b8 Mon Sep 17 00:00:00 2001 From: Peter Hayman Date: Thu, 14 Mar 2024 10:11:21 +1100 Subject: [PATCH 4/9] test: add `DynamicEnum` reserialize test --- crates/bevy_reflect/src/serde/de.rs | 33 ++++++++++++++++++++++++++++- crates/bevy_scene/src/serde.rs | 2 +- 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/crates/bevy_reflect/src/serde/de.rs b/crates/bevy_reflect/src/serde/de.rs index c84e049f380be..5495c4ec8249c 100644 --- a/crates/bevy_reflect/src/serde/de.rs +++ b/crates/bevy_reflect/src/serde/de.rs @@ -1062,7 +1062,7 @@ mod tests { use bevy_utils::HashMap; use crate as bevy_reflect; - use crate::serde::{TypedReflectDeserializer, UntypedReflectDeserializer}; + use crate::serde::{ReflectSerializer, TypedReflectDeserializer, UntypedReflectDeserializer}; use crate::{DynamicEnum, FromReflect, Reflect, ReflectDeserialize, TypeRegistry}; #[derive(Reflect, Debug, PartialEq)] @@ -1429,6 +1429,37 @@ mod tests { assert!(expected.reflect_partial_eq(output.as_ref()).unwrap()); } + // Regression test for https://github.com/bevyengine/bevy/issues/12462 + #[test] + fn enum_should_be_correct_variant_on_reserialize() { + #[derive(Reflect, Default)] + enum MyEnum { + #[default] + Unit, + Tuple(String), + Struct { + value: u32, + }, + } + + let mut registry = TypeRegistry::default(); + registry.register::(); + + let value = MyEnum::Tuple("Hello world".to_string()); + + let serializer1 = ReflectSerializer::new(&value, ®istry); + let serialized1 = ron::ser::to_string(&serializer1).unwrap(); + + let mut deserializer = ron::de::Deserializer::from_str(&serialized1).unwrap(); + let reflect_deserializer = UntypedReflectDeserializer::new(®istry); + let value = reflect_deserializer.deserialize(&mut deserializer).unwrap(); + + let serializer2 = ReflectSerializer::new(&*value, ®istry); + let serialized2 = ron::ser::to_string(&serializer2).unwrap(); + + assert_eq!(serialized1, serialized2); + } + #[test] fn should_deserialize_non_self_describing_binary() { let mut map = HashMap::new(); diff --git a/crates/bevy_scene/src/serde.rs b/crates/bevy_scene/src/serde.rs index a5cf2a895249f..9bcd11c2df0af 100644 --- a/crates/bevy_scene/src/serde.rs +++ b/crates/bevy_scene/src/serde.rs @@ -693,7 +693,7 @@ mod tests { assert_eq!(1, dst_world.query::<&Baz>().iter(&dst_world).count()); } - // Regression test regarding [#12462](https://github.com/bevyengine/bevy/issues/12462) + // Regression test for https://github.com/bevyengine/bevy/issues/12462 #[test] fn should_reserialize() { let mut world = create_world(); From c15f4bc1701844b081fcb92f0d73ae5cf4673193 Mon Sep 17 00:00:00 2001 From: Peter Hayman Date: Thu, 14 Mar 2024 10:50:12 +1100 Subject: [PATCH 5/9] test: deduplicate code with setup function `get_my_struct()` --- crates/bevy_reflect/src/serde/de.rs | 78 +++++------------------------ 1 file changed, 12 insertions(+), 66 deletions(-) diff --git a/crates/bevy_reflect/src/serde/de.rs b/crates/bevy_reflect/src/serde/de.rs index 5495c4ec8249c..25e3055a7e630 100644 --- a/crates/bevy_reflect/src/serde/de.rs +++ b/crates/bevy_reflect/src/serde/de.rs @@ -1170,12 +1170,11 @@ mod tests { registry } - #[test] - fn should_deserialize() { + fn get_my_struct()->MyStruct{ let mut map = HashMap::new(); map.insert(64, 32); - let expected = MyStruct { + MyStruct { primitive_value: 123, option_value: Some(String::from("Hello world!")), option_value_complex: Some(SomeStruct { foo: 123 }), @@ -1202,7 +1201,14 @@ mod tests { value: 100, inner_struct: SomeDeserializableStruct { foo: 101 }, }, - }; + } + } + + #[test] + fn should_deserialize() { + + let expected = get_my_struct(); + let registry = get_registry(); let input = r#"{ "bevy_reflect::serde::de::tests::MyStruct": ( @@ -1462,38 +1468,8 @@ mod tests { #[test] fn should_deserialize_non_self_describing_binary() { - let mut map = HashMap::new(); - map.insert(64, 32); - - let expected = MyStruct { - primitive_value: 123, - option_value: Some(String::from("Hello world!")), - option_value_complex: Some(SomeStruct { foo: 123 }), - tuple_value: (PI, 1337), - list_value: vec![-2, -1, 0, 1, 2], - array_value: [-2, -1, 0, 1, 2], - map_value: map, - struct_value: SomeStruct { foo: 999999999 }, - tuple_struct_value: SomeTupleStruct(String::from("Tuple Struct")), - unit_struct: SomeUnitStruct, - unit_enum: SomeEnum::Unit, - newtype_enum: SomeEnum::NewType(123), - tuple_enum: SomeEnum::Tuple(1.23, 3.21), - struct_enum: SomeEnum::Struct { - foo: String::from("Struct variant value"), - }, - ignored_struct: SomeIgnoredStruct { ignored: 0 }, - ignored_tuple_struct: SomeIgnoredTupleStruct(0), - ignored_struct_variant: SomeIgnoredEnum::Struct { - foo: String::default(), - }, - ignored_tuple_variant: SomeIgnoredEnum::Tuple(0.0, 0.0), - custom_deserialize: CustomDeserialize { - value: 100, - inner_struct: SomeDeserializableStruct { foo: 101 }, - }, - }; + let expected = get_my_struct(); let registry = get_registry(); let input = vec![ @@ -1525,38 +1501,8 @@ mod tests { #[test] fn should_deserialize_self_describing_binary() { - let mut map = HashMap::new(); - map.insert(64, 32); - - let expected = MyStruct { - primitive_value: 123, - option_value: Some(String::from("Hello world!")), - option_value_complex: Some(SomeStruct { foo: 123 }), - tuple_value: (PI, 1337), - list_value: vec![-2, -1, 0, 1, 2], - array_value: [-2, -1, 0, 1, 2], - map_value: map, - struct_value: SomeStruct { foo: 999999999 }, - tuple_struct_value: SomeTupleStruct(String::from("Tuple Struct")), - unit_struct: SomeUnitStruct, - unit_enum: SomeEnum::Unit, - newtype_enum: SomeEnum::NewType(123), - tuple_enum: SomeEnum::Tuple(1.23, 3.21), - struct_enum: SomeEnum::Struct { - foo: String::from("Struct variant value"), - }, - ignored_struct: SomeIgnoredStruct { ignored: 0 }, - ignored_tuple_struct: SomeIgnoredTupleStruct(0), - ignored_struct_variant: SomeIgnoredEnum::Struct { - foo: String::default(), - }, - ignored_tuple_variant: SomeIgnoredEnum::Tuple(0.0, 0.0), - custom_deserialize: CustomDeserialize { - value: 100, - inner_struct: SomeDeserializableStruct { foo: 101 }, - }, - }; + let expected = get_my_struct(); let registry = get_registry(); let input = vec![ From b97b7de0342408e854241ef13e52691c0b08c1d9 Mon Sep 17 00:00:00 2001 From: Peter Hayman Date: Thu, 14 Mar 2024 10:51:23 +1100 Subject: [PATCH 6/9] test: use `MyStruct` for `should_reserialize` --- crates/bevy_reflect/src/serde/de.rs | 26 +++++++------------------- 1 file changed, 7 insertions(+), 19 deletions(-) diff --git a/crates/bevy_reflect/src/serde/de.rs b/crates/bevy_reflect/src/serde/de.rs index 25e3055a7e630..69f960844a0e4 100644 --- a/crates/bevy_reflect/src/serde/de.rs +++ b/crates/bevy_reflect/src/serde/de.rs @@ -1253,7 +1253,6 @@ mod tests { ), }"#; - let registry = get_registry(); let reflect_deserializer = UntypedReflectDeserializer::new(®istry); let mut ron_deserializer = ron::de::Deserializer::from_str(input).unwrap(); let dynamic_output = reflect_deserializer @@ -1437,30 +1436,19 @@ mod tests { // Regression test for https://github.com/bevyengine/bevy/issues/12462 #[test] - fn enum_should_be_correct_variant_on_reserialize() { - #[derive(Reflect, Default)] - enum MyEnum { - #[default] - Unit, - Tuple(String), - Struct { - value: u32, - }, - } - - let mut registry = TypeRegistry::default(); - registry.register::(); - - let value = MyEnum::Tuple("Hello world".to_string()); + fn should_reserialize() { + + let registry = get_registry(); + let input1 = get_my_struct(); - let serializer1 = ReflectSerializer::new(&value, ®istry); + let serializer1 = ReflectSerializer::new(&input1, ®istry); let serialized1 = ron::ser::to_string(&serializer1).unwrap(); let mut deserializer = ron::de::Deserializer::from_str(&serialized1).unwrap(); let reflect_deserializer = UntypedReflectDeserializer::new(®istry); - let value = reflect_deserializer.deserialize(&mut deserializer).unwrap(); + let input2 = reflect_deserializer.deserialize(&mut deserializer).unwrap(); - let serializer2 = ReflectSerializer::new(&*value, ®istry); + let serializer2 = ReflectSerializer::new(&*input2, ®istry); let serialized2 = ron::ser::to_string(&serializer2).unwrap(); assert_eq!(serialized1, serialized2); From b93a63a6bd55aed02522107a0f8c8f95227626c6 Mon Sep 17 00:00:00 2001 From: Peter Hayman Date: Thu, 14 Mar 2024 10:52:50 +1100 Subject: [PATCH 7/9] test: remove `bevy_scene` regression test In favor of `bevy_reflect::should_reserialize` --- crates/bevy_scene/src/serde.rs | 26 -------------------------- 1 file changed, 26 deletions(-) diff --git a/crates/bevy_scene/src/serde.rs b/crates/bevy_scene/src/serde.rs index 9bcd11c2df0af..0904502f2d4e4 100644 --- a/crates/bevy_scene/src/serde.rs +++ b/crates/bevy_scene/src/serde.rs @@ -693,32 +693,6 @@ mod tests { assert_eq!(1, dst_world.query::<&Baz>().iter(&dst_world).count()); } - // Regression test for https://github.com/bevyengine/bevy/issues/12462 - #[test] - fn should_reserialize() { - let mut world = create_world(); - - world.spawn(MyComponent { - foo: [1, 2, 3], - bar: (1.3, 3.7), - baz: MyEnum::Tuple("Hello World!".to_string()), - }); - - let registry = world.resource::(); - let scene = DynamicScene::from_world(&world); - let serialized_scene1 = scene.serialize_ron(®istry.0).unwrap(); - - let scene_deserializer = SceneDeserializer { - type_registry: ®istry.0.read(), - }; - let mut deserializer = ron::de::Deserializer::from_str(&serialized_scene1).unwrap(); - let deserialized_scene = scene_deserializer.deserialize(&mut deserializer).unwrap(); - - let serialized_scene2 = deserialized_scene.serialize_ron(®istry.0).unwrap(); - - assert_eq!(serialized_scene1, serialized_scene2); - } - #[test] fn should_roundtrip_with_later_generations_and_obsolete_references() { let mut world = create_world(); From 2119bb82cfe3b5c5e6d559b5566a5685d9867e24 Mon Sep 17 00:00:00 2001 From: Peter Hayman Date: Thu, 14 Mar 2024 15:27:31 +1100 Subject: [PATCH 8/9] test: use serde `alias` instead of `rename` --- crates/bevy_reflect/src/serde/de.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/crates/bevy_reflect/src/serde/de.rs b/crates/bevy_reflect/src/serde/de.rs index 69f960844a0e4..a121a7831c188 100644 --- a/crates/bevy_reflect/src/serde/de.rs +++ b/crates/bevy_reflect/src/serde/de.rs @@ -1120,7 +1120,7 @@ mod tests { #[reflect(Deserialize)] struct CustomDeserialize { value: usize, - #[serde(rename = "renamed")] + #[serde(alias = "renamed")] inner_struct: SomeDeserializableStruct, } @@ -1170,7 +1170,7 @@ mod tests { registry } - fn get_my_struct()->MyStruct{ + fn get_my_struct() -> MyStruct { let mut map = HashMap::new(); map.insert(64, 32); @@ -1206,7 +1206,6 @@ mod tests { #[test] fn should_deserialize() { - let expected = get_my_struct(); let registry = get_registry(); @@ -1437,7 +1436,6 @@ mod tests { // Regression test for https://github.com/bevyengine/bevy/issues/12462 #[test] fn should_reserialize() { - let registry = get_registry(); let input1 = get_my_struct(); @@ -1456,7 +1454,6 @@ mod tests { #[test] fn should_deserialize_non_self_describing_binary() { - let expected = get_my_struct(); let registry = get_registry(); @@ -1489,7 +1486,6 @@ mod tests { #[test] fn should_deserialize_self_describing_binary() { - let expected = get_my_struct(); let registry = get_registry(); From b6cd9d28075dcd555f1f5edf76f36a35f1042973 Mon Sep 17 00:00:00 2001 From: Peter Hayman Date: Thu, 14 Mar 2024 15:36:03 +1100 Subject: [PATCH 9/9] test: deduplicate identical MyStruct declarations --- crates/bevy_reflect/src/serde/ser.rs | 79 ++++------------------------ 1 file changed, 10 insertions(+), 69 deletions(-) diff --git a/crates/bevy_reflect/src/serde/ser.rs b/crates/bevy_reflect/src/serde/ser.rs index 2823f2f811617..c67b81e8cc2e2 100644 --- a/crates/bevy_reflect/src/serde/ser.rs +++ b/crates/bevy_reflect/src/serde/ser.rs @@ -574,12 +574,10 @@ mod tests { registry } - #[test] - fn should_serialize() { + fn get_my_struct() -> MyStruct { let mut map = HashMap::new(); map.insert(64, 32); - - let input = MyStruct { + MyStruct { primitive_value: 123, option_value: Some(String::from("Hello world!")), option_value_complex: Some(SomeStruct { foo: 123 }), @@ -606,9 +604,14 @@ mod tests { value: 100, inner_struct: SomeSerializableStruct { foo: 101 }, }, - }; + } + } + #[test] + fn should_serialize() { + let input = get_my_struct(); let registry = get_registry(); + let serializer = ReflectSerializer::new(&input, ®istry); let config = PrettyConfig::default() @@ -776,38 +779,7 @@ mod tests { #[test] fn should_serialize_non_self_describing_binary() { - let mut map = HashMap::new(); - map.insert(64, 32); - - let input = MyStruct { - primitive_value: 123, - option_value: Some(String::from("Hello world!")), - option_value_complex: Some(SomeStruct { foo: 123 }), - tuple_value: (PI, 1337), - list_value: vec![-2, -1, 0, 1, 2], - array_value: [-2, -1, 0, 1, 2], - map_value: map, - struct_value: SomeStruct { foo: 999999999 }, - tuple_struct_value: SomeTupleStruct(String::from("Tuple Struct")), - unit_struct: SomeUnitStruct, - unit_enum: SomeEnum::Unit, - newtype_enum: SomeEnum::NewType(123), - tuple_enum: SomeEnum::Tuple(1.23, 3.21), - struct_enum: SomeEnum::Struct { - foo: String::from("Struct variant value"), - }, - ignored_struct: SomeIgnoredStruct { ignored: 123 }, - ignored_tuple_struct: SomeIgnoredTupleStruct(123), - ignored_struct_variant: SomeIgnoredEnum::Struct { - foo: String::from("Struct Variant"), - }, - ignored_tuple_variant: SomeIgnoredEnum::Tuple(1.23, 3.45), - custom_serialize: CustomSerialize { - value: 100, - inner_struct: SomeSerializableStruct { foo: 101 }, - }, - }; - + let input = get_my_struct(); let registry = get_registry(); let serializer = ReflectSerializer::new(&input, ®istry); @@ -834,38 +806,7 @@ mod tests { #[test] fn should_serialize_self_describing_binary() { - let mut map = HashMap::new(); - map.insert(64, 32); - - let input = MyStruct { - primitive_value: 123, - option_value: Some(String::from("Hello world!")), - option_value_complex: Some(SomeStruct { foo: 123 }), - tuple_value: (PI, 1337), - list_value: vec![-2, -1, 0, 1, 2], - array_value: [-2, -1, 0, 1, 2], - map_value: map, - struct_value: SomeStruct { foo: 999999999 }, - tuple_struct_value: SomeTupleStruct(String::from("Tuple Struct")), - unit_struct: SomeUnitStruct, - unit_enum: SomeEnum::Unit, - newtype_enum: SomeEnum::NewType(123), - tuple_enum: SomeEnum::Tuple(1.23, 3.21), - struct_enum: SomeEnum::Struct { - foo: String::from("Struct variant value"), - }, - ignored_struct: SomeIgnoredStruct { ignored: 123 }, - ignored_tuple_struct: SomeIgnoredTupleStruct(123), - ignored_struct_variant: SomeIgnoredEnum::Struct { - foo: String::from("Struct Variant"), - }, - ignored_tuple_variant: SomeIgnoredEnum::Tuple(1.23, 3.45), - custom_serialize: CustomSerialize { - value: 100, - inner_struct: SomeSerializableStruct { foo: 101 }, - }, - }; - + let input = get_my_struct(); let registry = get_registry(); let serializer = ReflectSerializer::new(&input, ®istry);