From 9bf549e104b4be7b3909e17caf732614cf13b159 Mon Sep 17 00:00:00 2001 From: Jakob Hellermann Date: Mon, 20 Jun 2022 17:18:58 +0000 Subject: [PATCH] `bevy_reflect`: put `serialize` into external `ReflectSerialize` type (#4782) builds on top of #4780 # Objective `Reflect` and `Serialize` are currently very tied together because `Reflect` has a `fn serialize(&self) -> Option>` method. Because of that, we can either implement `Reflect` for types like `Option` with `T: Serialize` and have `fn serialize` be implemented, or without the bound but having `fn serialize` return `None`. By separating `ReflectSerialize` into a separate type (like how it already is for `ReflectDeserialize`, `ReflectDefault`), we could separately `.register::>()` and `.register_data::, ReflectSerialize>()` only if the type `T: Serialize`. This PR does not change the registration but allows it to be changed in a future PR. ## Solution - add the type ```rust struct ReflectSerialize { .. } impl FromType for ReflectSerialize { .. } ``` - remove `#[reflect(Serialize)]` special casing. - when serializing reflect value types, look for `ReflectSerialize` in the `TypeRegistry` instead of calling `value.serialize()` --- crates/bevy_asset/src/handle.rs | 2 +- crates/bevy_asset/src/path.rs | 2 +- crates/bevy_core_pipeline/src/clear_color.rs | 2 +- .../src/core_3d/camera_3d.rs | 2 +- crates/bevy_ecs/src/reflect.rs | 1 + .../src/container_attributes.rs | 24 --------------- .../bevy_reflect_derive/src/impls.rs | 9 ------ crates/bevy_reflect/src/array.rs | 7 +---- crates/bevy_reflect/src/impls/glam.rs | 2 +- crates/bevy_reflect/src/impls/std.rs | 30 ++++++++----------- crates/bevy_reflect/src/lib.rs | 7 +++-- crates/bevy_reflect/src/list.rs | 8 ++--- crates/bevy_reflect/src/serde/ser.rs | 26 +++++++++------- crates/bevy_reflect/src/type_registry.rs | 30 +++++++++++++++++++ crates/bevy_render/src/camera/projection.rs | 4 ++- crates/bevy_render/src/color/mod.rs | 2 +- crates/bevy_ui/src/focus.rs | 2 +- crates/bevy_ui/src/widget/image.rs | 2 +- 18 files changed, 77 insertions(+), 85 deletions(-) diff --git a/crates/bevy_asset/src/handle.rs b/crates/bevy_asset/src/handle.rs index 3cc5df54b069e3..e929371e2fdbcb 100644 --- a/crates/bevy_asset/src/handle.rs +++ b/crates/bevy_asset/src/handle.rs @@ -10,7 +10,7 @@ use crate::{ Asset, Assets, }; use bevy_ecs::{component::Component, reflect::ReflectComponent}; -use bevy_reflect::{FromReflect, Reflect, ReflectDeserialize}; +use bevy_reflect::{FromReflect, Reflect, ReflectDeserialize, ReflectSerialize}; use bevy_utils::Uuid; use crossbeam_channel::{Receiver, Sender}; use serde::{Deserialize, Serialize}; diff --git a/crates/bevy_asset/src/path.rs b/crates/bevy_asset/src/path.rs index df99eaee19da1a..cb39b3f5ca1274 100644 --- a/crates/bevy_asset/src/path.rs +++ b/crates/bevy_asset/src/path.rs @@ -1,4 +1,4 @@ -use bevy_reflect::{Reflect, ReflectDeserialize}; +use bevy_reflect::{Reflect, ReflectDeserialize, ReflectSerialize}; use bevy_utils::AHasher; use serde::{Deserialize, Serialize}; use std::{ diff --git a/crates/bevy_core_pipeline/src/clear_color.rs b/crates/bevy_core_pipeline/src/clear_color.rs index 903316a753fc43..2e071dc9745b46 100644 --- a/crates/bevy_core_pipeline/src/clear_color.rs +++ b/crates/bevy_core_pipeline/src/clear_color.rs @@ -1,6 +1,6 @@ use bevy_derive::{Deref, DerefMut}; use bevy_ecs::prelude::*; -use bevy_reflect::{Reflect, ReflectDeserialize}; +use bevy_reflect::{Reflect, ReflectDeserialize, ReflectSerialize}; use bevy_render::{color::Color, extract_resource::ExtractResource}; use serde::{Deserialize, Serialize}; diff --git a/crates/bevy_core_pipeline/src/core_3d/camera_3d.rs b/crates/bevy_core_pipeline/src/core_3d/camera_3d.rs index a9bc38464f3b40..bca403cc0303c9 100644 --- a/crates/bevy_core_pipeline/src/core_3d/camera_3d.rs +++ b/crates/bevy_core_pipeline/src/core_3d/camera_3d.rs @@ -1,6 +1,6 @@ use crate::clear_color::ClearColorConfig; use bevy_ecs::{prelude::*, query::QueryItem}; -use bevy_reflect::{Reflect, ReflectDeserialize}; +use bevy_reflect::{Reflect, ReflectDeserialize, ReflectSerialize}; use bevy_render::{ camera::{Camera, CameraRenderGraph, Projection}, extract_component::ExtractComponent, diff --git a/crates/bevy_ecs/src/reflect.rs b/crates/bevy_ecs/src/reflect.rs index 4881dd83f0edde..bcd41971c7c9e5 100644 --- a/crates/bevy_ecs/src/reflect.rs +++ b/crates/bevy_ecs/src/reflect.rs @@ -8,6 +8,7 @@ use crate::{ }; use bevy_reflect::{ impl_from_reflect_value, impl_reflect_value, FromType, Reflect, ReflectDeserialize, + ReflectSerialize, }; #[derive(Clone)] diff --git a/crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs b/crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs index dea47069a112d9..5f2d16c4a6af97 100644 --- a/crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs +++ b/crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs @@ -18,7 +18,6 @@ use syn::{Meta, NestedMeta, Path}; const DEBUG_ATTR: &str = "Debug"; const PARTIAL_EQ_ATTR: &str = "PartialEq"; const HASH_ATTR: &str = "Hash"; -const SERIALIZE_ATTR: &str = "Serialize"; // The traits listed below are not considered "special" (i.e. they use the `ReflectMyTrait` syntax) // but useful to know exist nonetheless @@ -54,7 +53,6 @@ impl Default for TraitImpl { /// * `Debug` /// * `Hash` /// * `PartialEq` -/// * `Serialize` /// /// When registering a trait, there are a few things to keep in mind: /// * Traits must have a valid `Reflect{}` struct in scope. For example, `Default` @@ -110,7 +108,6 @@ pub(crate) struct ReflectTraits { debug: TraitImpl, hash: TraitImpl, partial_eq: TraitImpl, - serialize: TraitImpl, idents: Vec, } @@ -133,7 +130,6 @@ impl ReflectTraits { DEBUG_ATTR => traits.debug = TraitImpl::Implemented, PARTIAL_EQ_ATTR => traits.partial_eq = TraitImpl::Implemented, HASH_ATTR => traits.hash = TraitImpl::Implemented, - SERIALIZE_ATTR => traits.serialize = TraitImpl::Implemented, // We only track reflected idents for traits not considered special _ => traits.idents.push(utility::get_reflect_ident(&ident)), } @@ -156,7 +152,6 @@ impl ReflectTraits { DEBUG_ATTR => traits.debug = trait_func_ident, PARTIAL_EQ_ATTR => traits.partial_eq = trait_func_ident, HASH_ATTR => traits.hash = trait_func_ident, - SERIALIZE_ATTR => traits.serialize = trait_func_ident, _ => {} } } @@ -230,25 +225,6 @@ impl ReflectTraits { } } - /// Returns the implementation of `Reflect::serializable` as a `TokenStream`. - /// - /// If `Serialize` was not registered, returns `None`. - pub fn get_serialize_impl(&self, bevy_reflect_path: &Path) -> Option { - match &self.serialize { - TraitImpl::Implemented => Some(quote! { - fn serializable(&self) -> Option<#bevy_reflect_path::serde::Serializable> { - Some(#bevy_reflect_path::serde::Serializable::Borrowed(self)) - } - }), - TraitImpl::Custom(impl_fn) => Some(quote! { - fn serializable(&self) -> Option<#bevy_reflect_path::serde::Serializable> { - Some(#impl_fn(self)) - } - }), - TraitImpl::NotImplemented => None, - } - } - /// Returns the implementation of `Reflect::debug` as a `TokenStream`. /// /// If `Debug` was not registered, returns `None`. diff --git a/crates/bevy_reflect/bevy_reflect_derive/src/impls.rs b/crates/bevy_reflect/bevy_reflect_derive/src/impls.rs index 95b8d6f0ac7519..7aa4ea373df72d 100644 --- a/crates/bevy_reflect/bevy_reflect_derive/src/impls.rs +++ b/crates/bevy_reflect/bevy_reflect_derive/src/impls.rs @@ -40,7 +40,6 @@ pub(crate) fn impl_struct(derive_data: &ReflectDeriveData) -> TokenStream { let field_indices = (0..field_count).collect::>(); let hash_fn = derive_data.traits().get_hash_impl(bevy_reflect_path); - let serialize_fn = derive_data.traits().get_serialize_impl(bevy_reflect_path); let partial_eq_fn = derive_data .traits() .get_partial_eq_impl(bevy_reflect_path) @@ -192,8 +191,6 @@ pub(crate) fn impl_struct(derive_data: &ReflectDeriveData) -> TokenStream { #partial_eq_fn #debug_fn - - #serialize_fn } }) } @@ -216,7 +213,6 @@ pub(crate) fn impl_tuple_struct(derive_data: &ReflectDeriveData) -> TokenStream let field_indices = (0..field_count).collect::>(); let hash_fn = derive_data.traits().get_hash_impl(bevy_reflect_path); - let serialize_fn = derive_data.traits().get_serialize_impl(bevy_reflect_path); let partial_eq_fn = derive_data .traits() .get_partial_eq_impl(bevy_reflect_path) @@ -344,8 +340,6 @@ pub(crate) fn impl_tuple_struct(derive_data: &ReflectDeriveData) -> TokenStream #partial_eq_fn #debug_fn - - #serialize_fn } }) } @@ -359,7 +353,6 @@ pub(crate) fn impl_value( reflect_traits: &ReflectTraits, ) -> TokenStream { let hash_fn = reflect_traits.get_hash_impl(bevy_reflect_path); - let serialize_fn = reflect_traits.get_serialize_impl(bevy_reflect_path); let partial_eq_fn = reflect_traits.get_partial_eq_impl(bevy_reflect_path); let debug_fn = reflect_traits.get_debug_impl(); @@ -445,8 +438,6 @@ pub(crate) fn impl_value( #partial_eq_fn #debug_fn - - #serialize_fn } }) } diff --git a/crates/bevy_reflect/src/array.rs b/crates/bevy_reflect/src/array.rs index f94e0e6cceaa4a..bc8c08a108e80f 100644 --- a/crates/bevy_reflect/src/array.rs +++ b/crates/bevy_reflect/src/array.rs @@ -1,6 +1,5 @@ use crate::{ - serde::Serializable, utility::NonGenericTypeInfoCell, DynamicInfo, Reflect, ReflectMut, - ReflectRef, TypeInfo, Typed, + utility::NonGenericTypeInfoCell, DynamicInfo, Reflect, ReflectMut, ReflectRef, TypeInfo, Typed, }; use std::{ any::{Any, TypeId}, @@ -217,10 +216,6 @@ unsafe impl Reflect for DynamicArray { fn reflect_partial_eq(&self, value: &dyn Reflect) -> Option { array_partial_eq(self, value) } - - fn serializable(&self) -> Option { - None - } } impl Array for DynamicArray { diff --git a/crates/bevy_reflect/src/impls/glam.rs b/crates/bevy_reflect/src/impls/glam.rs index 101f5e6597d072..2310e640eded14 100644 --- a/crates/bevy_reflect/src/impls/glam.rs +++ b/crates/bevy_reflect/src/impls/glam.rs @@ -1,7 +1,7 @@ use crate as bevy_reflect; use crate::prelude::ReflectDefault; use crate::reflect::Reflect; -use crate::ReflectDeserialize; +use crate::{ReflectDeserialize, ReflectSerialize}; use bevy_reflect_derive::{impl_from_reflect_value, impl_reflect_struct, impl_reflect_value}; use glam::*; diff --git a/crates/bevy_reflect/src/impls/std.rs b/crates/bevy_reflect/src/impls/std.rs index c5fed20da26ec8..13dae6c6345809 100644 --- a/crates/bevy_reflect/src/impls/std.rs +++ b/crates/bevy_reflect/src/impls/std.rs @@ -1,8 +1,8 @@ use crate as bevy_reflect; use crate::{ - map_partial_eq, serde::Serializable, Array, ArrayInfo, ArrayIter, DynamicMap, FromReflect, - FromType, GetTypeRegistration, List, ListInfo, Map, MapInfo, MapIter, Reflect, - ReflectDeserialize, ReflectMut, ReflectRef, TypeInfo, TypeRegistration, Typed, ValueInfo, + map_partial_eq, Array, ArrayInfo, ArrayIter, DynamicMap, FromReflect, FromType, + GetTypeRegistration, List, ListInfo, Map, MapInfo, MapIter, Reflect, ReflectDeserialize, + ReflectMut, ReflectRef, ReflectSerialize, TypeInfo, TypeRegistration, Typed, ValueInfo, }; use crate::utility::{GenericTypeInfoCell, NonGenericTypeInfoCell}; @@ -159,10 +159,6 @@ unsafe impl Reflect for Vec { fn reflect_partial_eq(&self, value: &dyn Reflect) -> Option { crate::list_partial_eq(self, value) } - - fn serializable(&self) -> Option { - None - } } impl Typed for Vec { @@ -420,11 +416,6 @@ unsafe impl Reflect for [T; N] { fn reflect_partial_eq(&self, value: &dyn Reflect) -> Option { crate::array_partial_eq(self, value) } - - #[inline] - fn serializable(&self) -> Option { - None - } } impl FromReflect for [T; N] { @@ -541,10 +532,6 @@ unsafe impl Reflect for Cow<'static, str> { Some(false) } } - - fn serializable(&self) -> Option { - Some(Serializable::Borrowed(self)) - } } impl Typed for Cow<'static, str> { @@ -558,6 +545,7 @@ impl GetTypeRegistration for Cow<'static, str> { fn get_type_registration() -> TypeRegistration { let mut registration = TypeRegistration::of::>(); registration.insert::(FromType::>::from_type()); + registration.insert::(FromType::>::from_type()); registration } } @@ -570,13 +558,19 @@ impl FromReflect for Cow<'static, str> { #[cfg(test)] mod tests { - use crate::Reflect; + use crate::{Reflect, ReflectSerialize, TypeRegistry}; use bevy_utils::HashMap; use std::f32::consts::{PI, TAU}; #[test] fn can_serialize_duration() { - assert!(std::time::Duration::ZERO.serializable().is_some()); + let mut type_registry = TypeRegistry::default(); + type_registry.register::(); + + let reflect_serialize = type_registry + .get_type_data::(std::any::TypeId::of::()) + .unwrap(); + let _serializable = reflect_serialize.get_serializable(&std::time::Duration::ZERO); } #[test] diff --git a/crates/bevy_reflect/src/lib.rs b/crates/bevy_reflect/src/lib.rs index 2abfdc8d65f93a..22833c557b871d 100644 --- a/crates/bevy_reflect/src/lib.rs +++ b/crates/bevy_reflect/src/lib.rs @@ -34,8 +34,8 @@ pub mod prelude { pub use crate::std_traits::*; #[doc(hidden)] pub use crate::{ - reflect_trait, GetField, GetTupleStructField, Reflect, ReflectDeserialize, Struct, - TupleStruct, + reflect_trait, GetField, GetTupleStructField, Reflect, ReflectDeserialize, + ReflectSerialize, Struct, TupleStruct, }; } @@ -877,7 +877,8 @@ bevy_reflect::tests::should_reflect_debug::Test { let v = vec3(12.0, 3.0, -6.9); let mut registry = TypeRegistry::default(); - registry.add_registration(Vec3::get_type_registration()); + registry.register::(); + registry.register::(); let ser = ReflectSerializer::new(&v, ®istry); diff --git a/crates/bevy_reflect/src/list.rs b/crates/bevy_reflect/src/list.rs index 66d3816707adb9..6e86e453be9a1d 100644 --- a/crates/bevy_reflect/src/list.rs +++ b/crates/bevy_reflect/src/list.rs @@ -3,8 +3,8 @@ use std::fmt::{Debug, Formatter}; use crate::utility::NonGenericTypeInfoCell; use crate::{ - serde::Serializable, Array, ArrayIter, DynamicArray, DynamicInfo, FromReflect, Reflect, - ReflectMut, ReflectRef, TypeInfo, Typed, + Array, ArrayIter, DynamicArray, DynamicInfo, FromReflect, Reflect, ReflectMut, ReflectRef, + TypeInfo, Typed, }; /// An ordered, mutable list of [Reflect] items. This corresponds to types like [`std::vec::Vec`]. @@ -229,10 +229,6 @@ unsafe impl Reflect for DynamicList { list_partial_eq(self, value) } - fn serializable(&self) -> Option { - None - } - fn debug(&self, f: &mut Formatter<'_>) -> std::fmt::Result { write!(f, "DynamicList(")?; list_debug(self, f)?; diff --git a/crates/bevy_reflect/src/serde/ser.rs b/crates/bevy_reflect/src/serde/ser.rs index 180bac1c810694..de0ad7760a6fab 100644 --- a/crates/bevy_reflect/src/serde/ser.rs +++ b/crates/bevy_reflect/src/serde/ser.rs @@ -1,6 +1,6 @@ use crate::{ - serde::type_fields, Array, List, Map, Reflect, ReflectRef, Struct, Tuple, TupleStruct, - TypeRegistry, + serde::type_fields, Array, List, Map, Reflect, ReflectRef, ReflectSerialize, Struct, Tuple, + TupleStruct, TypeRegistry, }; use serde::{ ser::{SerializeMap, SerializeSeq}, @@ -22,13 +22,19 @@ impl<'a> Serializable<'a> { } } -fn get_serializable(reflect_value: &dyn Reflect) -> Result { - reflect_value.serializable().ok_or_else(|| { - serde::ser::Error::custom(format_args!( - "Type '{}' does not support ReflectValue serialization", - reflect_value.type_name() - )) - }) +fn get_serializable<'a, E: serde::ser::Error>( + reflect_value: &'a dyn Reflect, + type_registry: &TypeRegistry, +) -> Result, E> { + let reflect_serialize = type_registry + .get_type_data::(reflect_value.type_id()) + .ok_or_else(|| { + serde::ser::Error::custom(format_args!( + "Type '{}' did not register ReflectSerialize", + reflect_value.type_name() + )) + })?; + Ok(reflect_serialize.get_serializable(reflect_value)) } pub struct ReflectSerializer<'a> { @@ -101,7 +107,7 @@ impl<'a> Serialize for ReflectValueSerializer<'a> { state.serialize_entry(type_fields::TYPE, self.value.type_name())?; state.serialize_entry( type_fields::VALUE, - get_serializable::(self.value)?.borrow(), + get_serializable::(self.value, self.registry)?.borrow(), )?; state.end() } diff --git a/crates/bevy_reflect/src/type_registry.rs b/crates/bevy_reflect/src/type_registry.rs index 60c29a88556262..f793ab6f8fca4a 100644 --- a/crates/bevy_reflect/src/type_registry.rs +++ b/crates/bevy_reflect/src/type_registry.rs @@ -1,3 +1,4 @@ +use crate::serde::Serializable; use crate::{Reflect, TypeInfo, Typed}; use bevy_utils::{HashMap, HashSet}; use downcast_rs::{impl_downcast, Downcast}; @@ -389,6 +390,35 @@ pub trait FromType { fn from_type() -> Self; } +/// A struct used to serialize reflected instances of a type. +/// +/// A `ReflectSerialize` for type `T` can be obtained via +/// [`FromType::from_type`]. +#[derive(Clone)] +pub struct ReflectSerialize { + get_serializable: for<'a> fn(value: &'a dyn Reflect) -> Serializable, +} + +impl FromType for ReflectSerialize { + fn from_type() -> Self { + ReflectSerialize { + get_serializable: |value| { + let value = value.downcast_ref::().unwrap_or_else(|| { + panic!("ReflectSerialize::get_serialize called with type `{}`, even though it was created for `{}`", value.type_name(), std::any::type_name::()) + }); + Serializable::Borrowed(value) + }, + } + } +} + +impl ReflectSerialize { + /// Turn the value into a serializable representation + pub fn get_serializable<'a>(&self, value: &'a dyn Reflect) -> Serializable<'a> { + (self.get_serializable)(value) + } +} + /// A struct used to deserialize reflected instances of a type. /// /// A `ReflectDeserialize` for type `T` can be obtained via diff --git a/crates/bevy_render/src/camera/projection.rs b/crates/bevy_render/src/camera/projection.rs index bd40d52a620a52..aaf0e761bba370 100644 --- a/crates/bevy_render/src/camera/projection.rs +++ b/crates/bevy_render/src/camera/projection.rs @@ -4,7 +4,9 @@ use super::DepthCalculation; use bevy_app::{App, CoreStage, Plugin, StartupStage}; use bevy_ecs::{prelude::*, reflect::ReflectComponent}; use bevy_math::Mat4; -use bevy_reflect::{std_traits::ReflectDefault, GetTypeRegistration, Reflect, ReflectDeserialize}; +use bevy_reflect::{ + std_traits::ReflectDefault, GetTypeRegistration, Reflect, ReflectDeserialize, ReflectSerialize, +}; use bevy_window::ModifiesWindows; use serde::{Deserialize, Serialize}; diff --git a/crates/bevy_render/src/color/mod.rs b/crates/bevy_render/src/color/mod.rs index 20bedac8ad8fa0..b249cb1887bce2 100644 --- a/crates/bevy_render/src/color/mod.rs +++ b/crates/bevy_render/src/color/mod.rs @@ -4,7 +4,7 @@ pub use colorspace::*; use crate::color::{HslRepresentation, SrgbColorSpace}; use bevy_math::{Vec3, Vec4}; -use bevy_reflect::{FromReflect, Reflect, ReflectDeserialize}; +use bevy_reflect::{FromReflect, Reflect, ReflectDeserialize, ReflectSerialize}; use serde::{Deserialize, Serialize}; use std::ops::{Add, AddAssign, Mul, MulAssign}; use thiserror::Error; diff --git a/crates/bevy_ui/src/focus.rs b/crates/bevy_ui/src/focus.rs index 03a269c690dca7..881376b0232801 100644 --- a/crates/bevy_ui/src/focus.rs +++ b/crates/bevy_ui/src/focus.rs @@ -7,7 +7,7 @@ use bevy_ecs::{ }; use bevy_input::{mouse::MouseButton, touch::Touches, Input}; use bevy_math::Vec2; -use bevy_reflect::{Reflect, ReflectDeserialize}; +use bevy_reflect::{Reflect, ReflectDeserialize, ReflectSerialize}; use bevy_transform::components::GlobalTransform; use bevy_utils::FloatOrd; use bevy_window::Windows; diff --git a/crates/bevy_ui/src/widget/image.rs b/crates/bevy_ui/src/widget/image.rs index 21f7d6288e803b..57be01e1ccd4b6 100644 --- a/crates/bevy_ui/src/widget/image.rs +++ b/crates/bevy_ui/src/widget/image.rs @@ -7,7 +7,7 @@ use bevy_ecs::{ reflect::ReflectComponent, system::{Query, Res}, }; -use bevy_reflect::{Reflect, ReflectDeserialize}; +use bevy_reflect::{Reflect, ReflectDeserialize, ReflectSerialize}; use bevy_render::texture::Image; use serde::{Deserialize, Serialize};