bevy_reflect: Add `ReflectSerializerProcessor` #15548

aecsocket · 2024-09-30T20:21:17Z

NOTE: This is based on, and should be merged alongside, #15482. I'll leave this in draft until that PR is merged.

Objective

Equivalent of #15482 but for serialization. See that issue for the motivation.

Also part of this tracking issue: #15518

This PR is non-breaking, just like the deserializer PR (because the new type parameter P has a default P = ()).

Solution

Identical solution to the deserializer PR.

Testing

Added unit tests and a very comprehensive doc test outlining a clear example and use case.

…deserialize op

wip: reflect serializer processor wip: ser tests serialize processor unit tests serializer processor doc tests fix doc tests fix doc fix newlines on windows please ci i beg you

**NOTE: Also see #15548 for the serializer equivalent** # Objective The current `ReflectDeserializer` and `TypedReflectDeserializer` use the `TypeRegistration` and/or `ReflectDeserialize` of a given type in order to determine how to deserialize a value of that type. However, there is currently no way to statefully override deserialization of a given type when using these two deserializers - that is, to have some local data in the same scope as the `ReflectDeserializer`, and make use of that data when deserializing. The motivating use case for this came up when working on [`bevy_animation_graph`](https://github.com/aecsocket/bevy_animation_graph/tree/feat/dynamic-nodes), when loading an animation graph asset. The `AnimationGraph` stores `Vec<Box<dyn NodeLike>>`s which we have to load in. Those `Box<dyn NodeLike>`s may store `Handle`s to e.g. `Handle<AnimationClip>`. I want to trigger a `load_context.load()` for that handle when it's loaded. ```rs #[derive(Reflect)] struct Animation { clips: Vec<Handle<AnimationClip>>, } ``` ```rs ( clips: [ "animation_clips/walk.animclip.ron", "animation_clips/run.animclip.ron", "animation_clips/jump.animclip.ron", ], ) ```` Currently, if this were deserialized from an asset loader, this would be deserialized as a vec of `Handle::default()`s, which isn't useful since we also need to `load_context.load()` those handles for them to be used. With this processor field, a processor can detect when `Handle<T>`s are being loaded, then actually load them in. ## Solution ```rs trait ReflectDeserializerProcessor { fn try_deserialize<'de, D>( &mut self, registration: &TypeRegistration, deserializer: D, ) -> Result<Result<Box<dyn PartialReflect>, D>, D::Error> where D: serde::Deserializer<'de>; } ``` ```diff - pub struct ReflectDeserializer<'a> { + pub struct ReflectDeserializer<'a, P = ()> { // also for ReflectTypedDeserializer registry: &'a TypeRegistry, + processor: Option<&'a mut P>, } ``` ```rs impl<'a, P: ReflectDeserializerProcessor> ReflectDeserializer<'a, P> { // also for ReflectTypedDeserializer pub fn with_processor(registry: &'a TypeRegistry, processor: &'a mut P) -> Self { Self { registry, processor: Some(processor), } } } ``` This does not touch the existing `fn new`s. This `processor` field is also added to all internal visitor structs. When `TypedReflectDeserializer` runs, it will first try to deserialize a value of this type by passing the `TypeRegistration` and deserializer to the processor, and fallback to the default logic. This processor runs the earliest, and takes priority over all other deserialization logic. ## Testing Added unit tests to `bevy_reflect::serde::de`. Also using almost exactly the same implementation in [my fork of `bevy_animation_graph`](https://github.com/aecsocket/bevy_animation_graph/tree/feat/dynamic-nodes). ## Migration Guide (Since I added `P = ()`, I don't think this is actually a breaking change anymore, but I'll leave this in) `bevy_reflect`'s `ReflectDeserializer` and `TypedReflectDeserializer` now take a `ReflectDeserializerProcessor` as the type parameter `P`, which allows you to customize deserialization for specific types when they are found. However, the rest of the API surface (`new`) remains the same. <details> <summary>Original implementation</summary> Add `ReflectDeserializerProcessor`: ```rs struct ReflectDeserializerProcessor { pub can_deserialize: Box<dyn FnMut(&TypeRegistration) -> bool + 'p>, pub deserialize: Box< dyn FnMut( &TypeRegistration, &mut dyn erased_serde::Deserializer, ) -> Result<Box<dyn PartialReflect>, erased_serde::Error> + 'p, } ``` Along with `ReflectDeserializer::new_with_processor` and `TypedReflectDeserializer::new_with_processor`. This does not touch the public API of the existing `new` fns. This is stored as an `Option<&mut ReflectDeserializerProcessor>` on the deserializer and any of the private `-Visitor` structs, and when we attempt to deserialize a value, we first pass it through this processor. Also added a very comprehensive doc test to `ReflectDeserializerProcessor`, which is actually a scaled down version of the code for the `bevy_animation_graph` loader. This should give users a good motivating example for when and why to use this feature. ### Why `Box<dyn ..>`? When I originally implemented this, I added a type parameter to `ReflectDeserializer` to determine the processor used, with `()` being "no processor". However when using this, I kept running into rustc errors where it failed to validate certain type bounds and led to overflows. I then switched to a dynamic dispatch approach. The dynamic dispatch should not be that expensive, nor should it be a performance regression, since it's only used if there is `Some` processor. (Note: I have not benchmarked this, I am just speculating.) Also, it means that we don't infect the rest of the code with an extra type parameter, which is nicer to maintain. ### Why the `'p` on `ReflectDeserializerProcessor<'p>`? Without a lifetime here, the `Box`es would automatically become `Box<dyn FnMut(..) + 'static>`. This makes them practically useless, since any local data you would want to pass in must then be `'static`. In the motivating example, you couldn't pass in that `&mut LoadContext` to the function. This means that the `'p` infects the rest of the Visitor types, but this is acceptable IMO. This PR also elides the lifetimes in the `impl<'de> Visitor<'de> for -Visitor` blocks where possible. ### Future possibilities I think it's technically possible to turn the processor into a trait, and make the deserializers generic over that trait. This would also open the door to an API like: ```rs type Seed; fn seed_deserialize(&mut self, r: &TypeRegistration) -> Option<Self::Seed>; fn deserialize(&mut self, r: &TypeRegistration, d: &mut dyn erased_serde::Deserializer, s: Self::Seed) -> ...; ``` A similar processor system should also be added to the serialization side, but that's for another PR. Ideally, both PRs will be in the same release, since one isn't very useful without the other. ## Testing Added unit tests to `bevy_reflect::serde::de`. Also using almost exactly the same implementation in [my fork of `bevy_animation_graph`](https://github.com/aecsocket/bevy_animation_graph/tree/feat/dynamic-nodes). ## Migration Guide `bevy_reflect`'s `ReflectDeserializer` and `TypedReflectDeserializer` now take a second lifetime parameter `'p` for storing the `ReflectDeserializerProcessor` field lifetimes. However, the rest of the API surface (`new`) remains the same, so if you are not storing these deserializers or referring to them with lifetimes, you should not have to make any changes. </details>

MrGVSV

Just a few comments.

Also, could you rebase this to get rid of the already-merged changes?

MrGVSV · 2024-11-16T00:33:27Z

crates/bevy_reflect/src/serde/ser/serializer.rs

+///
+/// [`try_serialize`]: Self::try_serialize
+/// [`SerializeWithRegistry`]: crate::serde::SerializeWithRegistry
+pub trait ReflectSerializerProcessor {


Nit: Might be worth adding a "see also" doc link to ReflectDeserializerProcessor (and vice versa). Maybe also with a short comment on how the other can be used to roundtrip the whole process?

Added links between ReflectDe/SerializerProcessor types. Although I'm not sure what kind of comment I can write wrt round-tripping.

I think what you have is fine for now. We can always expand upon it later if needed.

crates/bevy_reflect/src/serde/ser/serializer.rs

MrGVSV · 2024-11-16T00:47:59Z

crates/bevy_reflect/src/serde/ser/serializer.rs

+    ///         let Some(value) = value.try_as_reflect() else {
+    ///             // this value isn't `Reflect`, just do the default serialization
+    ///             return Ok(Err(serializer));
+    ///         };
+    ///         // actually read the type ID of this value
+    ///         let type_id = value.reflect_type_info().type_id();
+    ///
+    ///         if type_id == TypeId::of::<i32>() {
+    ///             let value_as_string = format!("{value:?}");
+    ///             serializer.serialize_str(&value_as_string).map(Ok)
+    ///         } else {
+    ///             Ok(Err(serializer))
+    ///         }


Nit: I know part of the point of this example is to show how to get the type info, but it would be more idiomatic to just use try_downcast_ref on the original value. And to be honest, I'm not sure we really need to go that in-depth here. Maybe a future example can give more of those details, but for this example, I think we should keep it simple:

Suggested change

/// let Some(value) = value.try_as_reflect() else {

/// // this value isn't `Reflect`, just do the default serialization

/// return Ok(Err(serializer));

/// };

/// // actually read the type ID of this value

/// let type_id = value.reflect_type_info().type_id();

///

/// if type_id == TypeId::of::<i32>() {

/// let value_as_string = format!("{value:?}");

/// serializer.serialize_str(&value_as_string).map(Ok)

/// } else {

/// Ok(Err(serializer))

/// }

/// if let Some(value) = value.try_downcast_ref::<i32>() {

/// let value_as_string = format!("{value:?}");

/// serializer.serialize_str(&value_as_string).map(Ok)

/// } else {

/// // Not an `i32`, just do the default serialization

/// Ok(Err(serializer))

/// }

Also, is the .map(Ok) necessary or could we also do:

Suggested change

/// let Some(value) = value.try_as_reflect() else {

/// // this value isn't `Reflect`, just do the default serialization

/// return Ok(Err(serializer));

/// };

/// // actually read the type ID of this value

/// let type_id = value.reflect_type_info().type_id();

///

/// if type_id == TypeId::of::<i32>() {

/// let value_as_string = format!("{value:?}");

/// serializer.serialize_str(&value_as_string).map(Ok)

/// } else {

/// Ok(Err(serializer))

/// }

/// if let Some(value) = value.try_downcast_ref::<i32>() {

/// let value_as_string = format!("{value:?}");

/// Ok(Ok(serializer.serialize_str(&value_as_string)?));

/// } else {

/// // Not an `i32`, just do the default serialization

/// Ok(Err(serializer))

/// }

try_downcast_ref looks much better, I didn't realise you could use it here. 100% agree on reducing complexity and just using that.

For the map(Ok): you need to wrap the result in another Ok to satisfy Result<Result<..>>. Either map(Ok) or Ok(Ok(..)), either approach works. I think the double Ok makes it clearer what type you're returning, so I'll use that, and replace it elsewhere as well.

MrGVSV

Looks great!

MrGVSV · 2024-11-16T18:22:05Z

crates/bevy_reflect/src/serde/de/processor.rs

+/// # Serialization equivalent
+///
+/// The serialization equivalent of this is [`ReflectSerializerProcessor`].
+///


Nit: Can we move this up to above # Compared to ... and remove the section header like you did for ReflectSerializerProcessor?

I did that change already, but didn't save the file 🤦 Fixed and pushed.

**NOTE: Also see #15548 for the serializer equivalent** # Objective The current `ReflectDeserializer` and `TypedReflectDeserializer` use the `TypeRegistration` and/or `ReflectDeserialize` of a given type in order to determine how to deserialize a value of that type. However, there is currently no way to statefully override deserialization of a given type when using these two deserializers - that is, to have some local data in the same scope as the `ReflectDeserializer`, and make use of that data when deserializing. The motivating use case for this came up when working on [`bevy_animation_graph`](https://github.com/aecsocket/bevy_animation_graph/tree/feat/dynamic-nodes), when loading an animation graph asset. The `AnimationGraph` stores `Vec<Box<dyn NodeLike>>`s which we have to load in. Those `Box<dyn NodeLike>`s may store `Handle`s to e.g. `Handle<AnimationClip>`. I want to trigger a `load_context.load()` for that handle when it's loaded. ```rs #[derive(Reflect)] struct Animation { clips: Vec<Handle<AnimationClip>>, } ``` ```rs ( clips: [ "animation_clips/walk.animclip.ron", "animation_clips/run.animclip.ron", "animation_clips/jump.animclip.ron", ], ) ```` Currently, if this were deserialized from an asset loader, this would be deserialized as a vec of `Handle::default()`s, which isn't useful since we also need to `load_context.load()` those handles for them to be used. With this processor field, a processor can detect when `Handle<T>`s are being loaded, then actually load them in. ## Solution ```rs trait ReflectDeserializerProcessor { fn try_deserialize<'de, D>( &mut self, registration: &TypeRegistration, deserializer: D, ) -> Result<Result<Box<dyn PartialReflect>, D>, D::Error> where D: serde::Deserializer<'de>; } ``` ```diff - pub struct ReflectDeserializer<'a> { + pub struct ReflectDeserializer<'a, P = ()> { // also for ReflectTypedDeserializer registry: &'a TypeRegistry, + processor: Option<&'a mut P>, } ``` ```rs impl<'a, P: ReflectDeserializerProcessor> ReflectDeserializer<'a, P> { // also for ReflectTypedDeserializer pub fn with_processor(registry: &'a TypeRegistry, processor: &'a mut P) -> Self { Self { registry, processor: Some(processor), } } } ``` This does not touch the existing `fn new`s. This `processor` field is also added to all internal visitor structs. When `TypedReflectDeserializer` runs, it will first try to deserialize a value of this type by passing the `TypeRegistration` and deserializer to the processor, and fallback to the default logic. This processor runs the earliest, and takes priority over all other deserialization logic. ## Testing Added unit tests to `bevy_reflect::serde::de`. Also using almost exactly the same implementation in [my fork of `bevy_animation_graph`](https://github.com/aecsocket/bevy_animation_graph/tree/feat/dynamic-nodes). ## Migration Guide (Since I added `P = ()`, I don't think this is actually a breaking change anymore, but I'll leave this in) `bevy_reflect`'s `ReflectDeserializer` and `TypedReflectDeserializer` now take a `ReflectDeserializerProcessor` as the type parameter `P`, which allows you to customize deserialization for specific types when they are found. However, the rest of the API surface (`new`) remains the same. <details> <summary>Original implementation</summary> Add `ReflectDeserializerProcessor`: ```rs struct ReflectDeserializerProcessor { pub can_deserialize: Box<dyn FnMut(&TypeRegistration) -> bool + 'p>, pub deserialize: Box< dyn FnMut( &TypeRegistration, &mut dyn erased_serde::Deserializer, ) -> Result<Box<dyn PartialReflect>, erased_serde::Error> + 'p, } ``` Along with `ReflectDeserializer::new_with_processor` and `TypedReflectDeserializer::new_with_processor`. This does not touch the public API of the existing `new` fns. This is stored as an `Option<&mut ReflectDeserializerProcessor>` on the deserializer and any of the private `-Visitor` structs, and when we attempt to deserialize a value, we first pass it through this processor. Also added a very comprehensive doc test to `ReflectDeserializerProcessor`, which is actually a scaled down version of the code for the `bevy_animation_graph` loader. This should give users a good motivating example for when and why to use this feature. ### Why `Box<dyn ..>`? When I originally implemented this, I added a type parameter to `ReflectDeserializer` to determine the processor used, with `()` being "no processor". However when using this, I kept running into rustc errors where it failed to validate certain type bounds and led to overflows. I then switched to a dynamic dispatch approach. The dynamic dispatch should not be that expensive, nor should it be a performance regression, since it's only used if there is `Some` processor. (Note: I have not benchmarked this, I am just speculating.) Also, it means that we don't infect the rest of the code with an extra type parameter, which is nicer to maintain. ### Why the `'p` on `ReflectDeserializerProcessor<'p>`? Without a lifetime here, the `Box`es would automatically become `Box<dyn FnMut(..) + 'static>`. This makes them practically useless, since any local data you would want to pass in must then be `'static`. In the motivating example, you couldn't pass in that `&mut LoadContext` to the function. This means that the `'p` infects the rest of the Visitor types, but this is acceptable IMO. This PR also elides the lifetimes in the `impl<'de> Visitor<'de> for -Visitor` blocks where possible. ### Future possibilities I think it's technically possible to turn the processor into a trait, and make the deserializers generic over that trait. This would also open the door to an API like: ```rs type Seed; fn seed_deserialize(&mut self, r: &TypeRegistration) -> Option<Self::Seed>; fn deserialize(&mut self, r: &TypeRegistration, d: &mut dyn erased_serde::Deserializer, s: Self::Seed) -> ...; ``` A similar processor system should also be added to the serialization side, but that's for another PR. Ideally, both PRs will be in the same release, since one isn't very useful without the other. ## Testing Added unit tests to `bevy_reflect::serde::de`. Also using almost exactly the same implementation in [my fork of `bevy_animation_graph`](https://github.com/aecsocket/bevy_animation_graph/tree/feat/dynamic-nodes). ## Migration Guide `bevy_reflect`'s `ReflectDeserializer` and `TypedReflectDeserializer` now take a second lifetime parameter `'p` for storing the `ReflectDeserializerProcessor` field lifetimes. However, the rest of the API surface (`new`) remains the same, so if you are not storing these deserializers or referring to them with lifetimes, you should not have to make any changes. </details>

**NOTE: This is based on, and should be merged alongside, #15482 I'll leave this in draft until that PR is merged. # Objective Equivalent of #15482 but for serialization. See that issue for the motivation. Also part of this tracking issue: #15518 This PR is non-breaking, just like the deserializer PR (because the new type parameter `P` has a default `P = ()`). ## Solution Identical solution to the deserializer PR. ## Testing Added unit tests and a very comprehensive doc test outlining a clear example and use case.

**NOTE: Also see bevyengine#15548 for the serializer equivalent** # Objective The current `ReflectDeserializer` and `TypedReflectDeserializer` use the `TypeRegistration` and/or `ReflectDeserialize` of a given type in order to determine how to deserialize a value of that type. However, there is currently no way to statefully override deserialization of a given type when using these two deserializers - that is, to have some local data in the same scope as the `ReflectDeserializer`, and make use of that data when deserializing. The motivating use case for this came up when working on [`bevy_animation_graph`](https://github.com/aecsocket/bevy_animation_graph/tree/feat/dynamic-nodes), when loading an animation graph asset. The `AnimationGraph` stores `Vec<Box<dyn NodeLike>>`s which we have to load in. Those `Box<dyn NodeLike>`s may store `Handle`s to e.g. `Handle<AnimationClip>`. I want to trigger a `load_context.load()` for that handle when it's loaded. ```rs #[derive(Reflect)] struct Animation { clips: Vec<Handle<AnimationClip>>, } ``` ```rs ( clips: [ "animation_clips/walk.animclip.ron", "animation_clips/run.animclip.ron", "animation_clips/jump.animclip.ron", ], ) ```` Currently, if this were deserialized from an asset loader, this would be deserialized as a vec of `Handle::default()`s, which isn't useful since we also need to `load_context.load()` those handles for them to be used. With this processor field, a processor can detect when `Handle<T>`s are being loaded, then actually load them in. ## Solution ```rs trait ReflectDeserializerProcessor { fn try_deserialize<'de, D>( &mut self, registration: &TypeRegistration, deserializer: D, ) -> Result<Result<Box<dyn PartialReflect>, D>, D::Error> where D: serde::Deserializer<'de>; } ``` ```diff - pub struct ReflectDeserializer<'a> { + pub struct ReflectDeserializer<'a, P = ()> { // also for ReflectTypedDeserializer registry: &'a TypeRegistry, + processor: Option<&'a mut P>, } ``` ```rs impl<'a, P: ReflectDeserializerProcessor> ReflectDeserializer<'a, P> { // also for ReflectTypedDeserializer pub fn with_processor(registry: &'a TypeRegistry, processor: &'a mut P) -> Self { Self { registry, processor: Some(processor), } } } ``` This does not touch the existing `fn new`s. This `processor` field is also added to all internal visitor structs. When `TypedReflectDeserializer` runs, it will first try to deserialize a value of this type by passing the `TypeRegistration` and deserializer to the processor, and fallback to the default logic. This processor runs the earliest, and takes priority over all other deserialization logic. ## Testing Added unit tests to `bevy_reflect::serde::de`. Also using almost exactly the same implementation in [my fork of `bevy_animation_graph`](https://github.com/aecsocket/bevy_animation_graph/tree/feat/dynamic-nodes). ## Migration Guide (Since I added `P = ()`, I don't think this is actually a breaking change anymore, but I'll leave this in) `bevy_reflect`'s `ReflectDeserializer` and `TypedReflectDeserializer` now take a `ReflectDeserializerProcessor` as the type parameter `P`, which allows you to customize deserialization for specific types when they are found. However, the rest of the API surface (`new`) remains the same. <details> <summary>Original implementation</summary> Add `ReflectDeserializerProcessor`: ```rs struct ReflectDeserializerProcessor { pub can_deserialize: Box<dyn FnMut(&TypeRegistration) -> bool + 'p>, pub deserialize: Box< dyn FnMut( &TypeRegistration, &mut dyn erased_serde::Deserializer, ) -> Result<Box<dyn PartialReflect>, erased_serde::Error> + 'p, } ``` Along with `ReflectDeserializer::new_with_processor` and `TypedReflectDeserializer::new_with_processor`. This does not touch the public API of the existing `new` fns. This is stored as an `Option<&mut ReflectDeserializerProcessor>` on the deserializer and any of the private `-Visitor` structs, and when we attempt to deserialize a value, we first pass it through this processor. Also added a very comprehensive doc test to `ReflectDeserializerProcessor`, which is actually a scaled down version of the code for the `bevy_animation_graph` loader. This should give users a good motivating example for when and why to use this feature. ### Why `Box<dyn ..>`? When I originally implemented this, I added a type parameter to `ReflectDeserializer` to determine the processor used, with `()` being "no processor". However when using this, I kept running into rustc errors where it failed to validate certain type bounds and led to overflows. I then switched to a dynamic dispatch approach. The dynamic dispatch should not be that expensive, nor should it be a performance regression, since it's only used if there is `Some` processor. (Note: I have not benchmarked this, I am just speculating.) Also, it means that we don't infect the rest of the code with an extra type parameter, which is nicer to maintain. ### Why the `'p` on `ReflectDeserializerProcessor<'p>`? Without a lifetime here, the `Box`es would automatically become `Box<dyn FnMut(..) + 'static>`. This makes them practically useless, since any local data you would want to pass in must then be `'static`. In the motivating example, you couldn't pass in that `&mut LoadContext` to the function. This means that the `'p` infects the rest of the Visitor types, but this is acceptable IMO. This PR also elides the lifetimes in the `impl<'de> Visitor<'de> for -Visitor` blocks where possible. ### Future possibilities I think it's technically possible to turn the processor into a trait, and make the deserializers generic over that trait. This would also open the door to an API like: ```rs type Seed; fn seed_deserialize(&mut self, r: &TypeRegistration) -> Option<Self::Seed>; fn deserialize(&mut self, r: &TypeRegistration, d: &mut dyn erased_serde::Deserializer, s: Self::Seed) -> ...; ``` A similar processor system should also be added to the serialization side, but that's for another PR. Ideally, both PRs will be in the same release, since one isn't very useful without the other. ## Testing Added unit tests to `bevy_reflect::serde::de`. Also using almost exactly the same implementation in [my fork of `bevy_animation_graph`](https://github.com/aecsocket/bevy_animation_graph/tree/feat/dynamic-nodes). ## Migration Guide `bevy_reflect`'s `ReflectDeserializer` and `TypedReflectDeserializer` now take a second lifetime parameter `'p` for storing the `ReflectDeserializerProcessor` field lifetimes. However, the rest of the API surface (`new`) remains the same, so if you are not storing these deserializers or referring to them with lifetimes, you should not have to make any changes. </details>

**NOTE: This is based on, and should be merged alongside, bevyengine#15482 I'll leave this in draft until that PR is merged. # Objective Equivalent of bevyengine#15482 but for serialization. See that issue for the motivation. Also part of this tracking issue: bevyengine#15518 This PR is non-breaking, just like the deserializer PR (because the new type parameter `P` has a default `P = ()`). ## Solution Identical solution to the deserializer PR. ## Testing Added unit tests and a very comprehensive doc test outlining a clear example and use case.

**NOTE: Also see bevyengine#15548 for the serializer equivalent** # Objective The current `ReflectDeserializer` and `TypedReflectDeserializer` use the `TypeRegistration` and/or `ReflectDeserialize` of a given type in order to determine how to deserialize a value of that type. However, there is currently no way to statefully override deserialization of a given type when using these two deserializers - that is, to have some local data in the same scope as the `ReflectDeserializer`, and make use of that data when deserializing. The motivating use case for this came up when working on [`bevy_animation_graph`](https://github.com/aecsocket/bevy_animation_graph/tree/feat/dynamic-nodes), when loading an animation graph asset. The `AnimationGraph` stores `Vec<Box<dyn NodeLike>>`s which we have to load in. Those `Box<dyn NodeLike>`s may store `Handle`s to e.g. `Handle<AnimationClip>`. I want to trigger a `load_context.load()` for that handle when it's loaded. ```rs #[derive(Reflect)] struct Animation { clips: Vec<Handle<AnimationClip>>, } ``` ```rs ( clips: [ "animation_clips/walk.animclip.ron", "animation_clips/run.animclip.ron", "animation_clips/jump.animclip.ron", ], ) ```` Currently, if this were deserialized from an asset loader, this would be deserialized as a vec of `Handle::default()`s, which isn't useful since we also need to `load_context.load()` those handles for them to be used. With this processor field, a processor can detect when `Handle<T>`s are being loaded, then actually load them in. ## Solution ```rs trait ReflectDeserializerProcessor { fn try_deserialize<'de, D>( &mut self, registration: &TypeRegistration, deserializer: D, ) -> Result<Result<Box<dyn PartialReflect>, D>, D::Error> where D: serde::Deserializer<'de>; } ``` ```diff - pub struct ReflectDeserializer<'a> { + pub struct ReflectDeserializer<'a, P = ()> { // also for ReflectTypedDeserializer registry: &'a TypeRegistry, + processor: Option<&'a mut P>, } ``` ```rs impl<'a, P: ReflectDeserializerProcessor> ReflectDeserializer<'a, P> { // also for ReflectTypedDeserializer pub fn with_processor(registry: &'a TypeRegistry, processor: &'a mut P) -> Self { Self { registry, processor: Some(processor), } } } ``` This does not touch the existing `fn new`s. This `processor` field is also added to all internal visitor structs. When `TypedReflectDeserializer` runs, it will first try to deserialize a value of this type by passing the `TypeRegistration` and deserializer to the processor, and fallback to the default logic. This processor runs the earliest, and takes priority over all other deserialization logic. ## Testing Added unit tests to `bevy_reflect::serde::de`. Also using almost exactly the same implementation in [my fork of `bevy_animation_graph`](https://github.com/aecsocket/bevy_animation_graph/tree/feat/dynamic-nodes). ## Migration Guide (Since I added `P = ()`, I don't think this is actually a breaking change anymore, but I'll leave this in) `bevy_reflect`'s `ReflectDeserializer` and `TypedReflectDeserializer` now take a `ReflectDeserializerProcessor` as the type parameter `P`, which allows you to customize deserialization for specific types when they are found. However, the rest of the API surface (`new`) remains the same. <details> <summary>Original implementation</summary> Add `ReflectDeserializerProcessor`: ```rs struct ReflectDeserializerProcessor { pub can_deserialize: Box<dyn FnMut(&TypeRegistration) -> bool + 'p>, pub deserialize: Box< dyn FnMut( &TypeRegistration, &mut dyn erased_serde::Deserializer, ) -> Result<Box<dyn PartialReflect>, erased_serde::Error> + 'p, } ``` Along with `ReflectDeserializer::new_with_processor` and `TypedReflectDeserializer::new_with_processor`. This does not touch the public API of the existing `new` fns. This is stored as an `Option<&mut ReflectDeserializerProcessor>` on the deserializer and any of the private `-Visitor` structs, and when we attempt to deserialize a value, we first pass it through this processor. Also added a very comprehensive doc test to `ReflectDeserializerProcessor`, which is actually a scaled down version of the code for the `bevy_animation_graph` loader. This should give users a good motivating example for when and why to use this feature. ### Why `Box<dyn ..>`? When I originally implemented this, I added a type parameter to `ReflectDeserializer` to determine the processor used, with `()` being "no processor". However when using this, I kept running into rustc errors where it failed to validate certain type bounds and led to overflows. I then switched to a dynamic dispatch approach. The dynamic dispatch should not be that expensive, nor should it be a performance regression, since it's only used if there is `Some` processor. (Note: I have not benchmarked this, I am just speculating.) Also, it means that we don't infect the rest of the code with an extra type parameter, which is nicer to maintain. ### Why the `'p` on `ReflectDeserializerProcessor<'p>`? Without a lifetime here, the `Box`es would automatically become `Box<dyn FnMut(..) + 'static>`. This makes them practically useless, since any local data you would want to pass in must then be `'static`. In the motivating example, you couldn't pass in that `&mut LoadContext` to the function. This means that the `'p` infects the rest of the Visitor types, but this is acceptable IMO. This PR also elides the lifetimes in the `impl<'de> Visitor<'de> for -Visitor` blocks where possible. ### Future possibilities I think it's technically possible to turn the processor into a trait, and make the deserializers generic over that trait. This would also open the door to an API like: ```rs type Seed; fn seed_deserialize(&mut self, r: &TypeRegistration) -> Option<Self::Seed>; fn deserialize(&mut self, r: &TypeRegistration, d: &mut dyn erased_serde::Deserializer, s: Self::Seed) -> ...; ``` A similar processor system should also be added to the serialization side, but that's for another PR. Ideally, both PRs will be in the same release, since one isn't very useful without the other. ## Testing Added unit tests to `bevy_reflect::serde::de`. Also using almost exactly the same implementation in [my fork of `bevy_animation_graph`](https://github.com/aecsocket/bevy_animation_graph/tree/feat/dynamic-nodes). ## Migration Guide `bevy_reflect`'s `ReflectDeserializer` and `TypedReflectDeserializer` now take a second lifetime parameter `'p` for storing the `ReflectDeserializerProcessor` field lifetimes. However, the rest of the API surface (`new`) remains the same, so if you are not storing these deserializers or referring to them with lifetimes, you should not have to make any changes. </details>

**NOTE: This is based on, and should be merged alongside, bevyengine#15482 I'll leave this in draft until that PR is merged. # Objective Equivalent of bevyengine#15482 but for serialization. See that issue for the motivation. Also part of this tracking issue: bevyengine#15518 This PR is non-breaking, just like the deserializer PR (because the new type parameter `P` has a default `P = ()`). ## Solution Identical solution to the deserializer PR. ## Testing Added unit tests and a very comprehensive doc test outlining a clear example and use case.

This was referenced Sep 30, 2024

Meta-issue for improving reflect-deserialized asset loading #15518

Open

bevy_reflect: Add ReflectDeserializerProcessor #15482

Merged

aecsocket force-pushed the feat/reflect-ser branch from 3cc5728 to afcd069 Compare September 30, 2024 20:35

alice-i-cecile added C-Feature A new feature, making something new possible A-Reflection Runtime information about types S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 30, 2024

aecsocket force-pushed the feat/reflect-ser branch from b1b2755 to 2e4683b Compare October 12, 2024 15:25

aecsocket force-pushed the feat/reflect-ser branch 2 times, most recently from 2c5bf0d to 904a41a Compare October 29, 2024 09:20

aecsocket marked this pull request as ready for review October 29, 2024 09:46

BenjaminBrienen added the D-Complex Quite challenging from either a design or technical perspective. Ask for help! label Oct 29, 2024

BenjaminBrienen approved these changes Oct 31, 2024

View reviewed changes

aecsocket added 18 commits October 31, 2024 17:57

Add ReflectDeserializerProcessor

9d59375

fix clippy

8f5cd7b

improve docs

6749256

address comments

38e618f

ReflectDeserializerProcessor should_deserialize returns the actual …

c8a3980

…deserialize op

wip: switch to type param for reflect deserializer processor

5e6d02c

fix processor

19bd7d8

get it working with the type system

56dbf02

fix docs for deserialize processor

4a4993e

fix doc tests

2c983cc

Add default P = () for ReflectDeserializer

1d5f725

add P = () to TypedReflectDeserializer as well

283bc15

Use new_internal in reflect deserializer

4a4ddaf

Provide access to TypeRegistry from processor

7b534db

doc fix

06e1a65

fix doc tests

428bfa2

Update for main

c9168b6

Address review comments

bb1bc39

aecsocket added 5 commits October 31, 2024 17:57

switch to incorrect spelling

7faf965

Add ReflectSerializerProcessor

b9eecfc

wip: reflect serializer processor wip: ser tests serialize processor unit tests serializer processor doc tests fix doc tests fix doc fix newlines on windows please ci i beg you

fix up rebase

e55a4c7

switch to incorrect spelling

a70451d

Add docs

d265b9b

aecsocket force-pushed the feat/reflect-ser branch from a9ca6fb to d265b9b Compare October 31, 2024 17:57

MrGVSV reviewed Nov 16, 2024

View reviewed changes

MrGVSV added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Nov 16, 2024

aecsocket added 4 commits November 16, 2024 11:04

Merge branch 'main' into feat/reflect-ser

fba8187

Address review comments

631d22e

replace map(Ok) with Ok(Ok(..)) wrapping

41384c9

fix doc

d99ca7c

MrGVSV approved these changes Nov 16, 2024

View reviewed changes

MrGVSV added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Nov 16, 2024

aecsocket added 2 commits November 16, 2024 19:50

doc cleanup

3078fcd

fix doc

dce23e6

alice-i-cecile added this to the 0.15 milestone Nov 17, 2024

alice-i-cecile added this pull request to the merge queue Nov 17, 2024

Merged via the queue into bevyengine:main with commit 17c4b07 Nov 17, 2024
29 checks passed

aecsocket mentioned this pull request Nov 18, 2024

0.15: ReflectDe/SerializerProcessor bevyengine/bevy-website#1818

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

bevy_reflect: Add `ReflectSerializerProcessor` #15548

bevy_reflect: Add `ReflectSerializerProcessor` #15548

Uh oh!

aecsocket commented Sep 30, 2024

Uh oh!

MrGVSV left a comment

Uh oh!

MrGVSV Nov 16, 2024

Uh oh!

aecsocket Nov 16, 2024

Uh oh!

MrGVSV Nov 16, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MrGVSV Nov 16, 2024

Uh oh!

aecsocket Nov 16, 2024

Uh oh!

MrGVSV left a comment

Uh oh!

MrGVSV Nov 16, 2024

Uh oh!

aecsocket Nov 16, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

bevy_reflect: Add ReflectSerializerProcessor #15548

bevy_reflect: Add ReflectSerializerProcessor #15548

Uh oh!

Conversation

aecsocket commented Sep 30, 2024

Objective

Solution

Testing

Uh oh!

MrGVSV left a comment

Choose a reason for hiding this comment

Uh oh!

MrGVSV Nov 16, 2024

Choose a reason for hiding this comment

Uh oh!

aecsocket Nov 16, 2024

Choose a reason for hiding this comment

Uh oh!

MrGVSV Nov 16, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MrGVSV Nov 16, 2024

Choose a reason for hiding this comment

Uh oh!

aecsocket Nov 16, 2024

Choose a reason for hiding this comment

Uh oh!

MrGVSV left a comment

Choose a reason for hiding this comment

Uh oh!

MrGVSV Nov 16, 2024

Choose a reason for hiding this comment

Uh oh!

aecsocket Nov 16, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bevy_reflect: Add `ReflectSerializerProcessor` #15548

bevy_reflect: Add `ReflectSerializerProcessor` #15548