mirror of
https://github.com/bevyengine/bevy
synced 2024-11-21 20:23:28 +00:00
57931ce42f
**NOTE: Also see https://github.com/bevyengine/bevy/pull/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> |
||
|---|---|---|
| .. | ||
| bevy_a11y | ||
| bevy_animation | ||
| bevy_app | ||
| bevy_asset | ||
| bevy_audio | ||
| bevy_color | ||
| bevy_core | ||
| bevy_core_pipeline | ||
| bevy_derive | ||
| bevy_dev_tools | ||
| bevy_diagnostic | ||
| bevy_dylib | ||
| bevy_ecs | ||
| bevy_encase_derive | ||
| bevy_gilrs | ||
| bevy_gizmos | ||
| bevy_gltf | ||
| bevy_hierarchy | ||
| bevy_image | ||
| bevy_input | ||
| bevy_internal | ||
| bevy_log | ||
| bevy_macro_utils | ||
| bevy_math | ||
| bevy_mesh | ||
| bevy_mikktspace | ||
| bevy_pbr | ||
| bevy_picking | ||
| bevy_ptr | ||
| bevy_reflect | ||
| bevy_remote | ||
| bevy_render | ||
| bevy_scene | ||
| bevy_sprite | ||
| bevy_state | ||
| bevy_tasks | ||
| bevy_text | ||
| bevy_time | ||
| bevy_transform | ||
| bevy_ui | ||
| bevy_utils | ||
| bevy_window | ||
| bevy_winit | ||