Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
bevy_reflect: Add
DeserializeWithRegistry and `SerializeWithRegistr…
…y` (#8611) # Objective ### The Problem Currently, the reflection deserializers give little control to users for how a type is deserialized. The most control a user can have is to register `ReflectDeserialize`, which will use a type's `Deserialize` implementation. However, there are times when a type may require slightly more control. For example, let's say we want to make Bevy's `Mesh` easier to deserialize via reflection (assume `Mesh` actually implemented `Reflect`). Since we want this to be extensible, we'll make it so users can use their own types so long as they satisfy `Into<Mesh>`. The end result should allow users to define a RON file like: ```rust { "my_game::meshes::Sphere": ( radius: 2.5 ) } ``` ### The Current Solution Since we don't know the types ahead of time, we'll need to use reflection. Luckily, we can access type information dynamically via the type registry. Let's make a custom type data struct that users can register on their types: ```rust pub struct ReflectIntoMesh { // ... } impl<T: FromReflect + Into<Mesh>> FromType<T> for ReflectIntoMesh { fn from_type() -> Self { // ... } } ``` Now we'll need a way to use this type data during deserialization. Unfortunately, we can't use `Deserialize` since we need access to the registry. This is where `DeserializeSeed` comes in handy: ```rust pub struct MeshDeserializer<'a> { pub registry: &'a TypeRegistry } impl<'a, 'de> DeserializeSeed<'de> for MeshDeserializer<'a> { type Value = Mesh; fn deserialize<D>(self, deserializer: D) -> Result<Self::Value, D::Error> where D: serde::Deserializer<'de>, { struct MeshVisitor<'a> { registry: &'a TypeRegistry } impl<'a, 'de> Visitor<'de> for MeshVisitor<'a> { fn expecting(&self, formatter: &mut Formatter) -> std::fmt::Result { write!(formatter, "map containing mesh information") } fn visit_map<A>(self, mut map: A) -> Result<Self::Value, serde::de::Error> where A: MapAccess<'de> { // Parse the type name let type_name = map.next_key::<String>()?.unwrap(); // Deserialize the value based on the type name let registration = self.registry .get_with_name(&type_name) .expect("should be registered"); let value = map.next_value_seed(TypedReflectDeserializer { registration, registry: self.registry, })?; // Convert the deserialized value into a `Mesh` let into_mesh = registration.data::<ReflectIntoMesh>().unwrap(); Ok(into_mesh.into(value)) } } } } ``` ### The Problem with the Current Solution The solution above works great when all we need to do is deserialize `Mesh` directly. But now, we want to be able to deserialize a struct like this: ```rust struct Fireball { damage: f32, mesh: Mesh, } ``` This might look simple enough and should theoretically be no problem for the reflection deserializer to handle, but this is where our `MeshDeserializer` solution starts to break down. In order to use `MeshDeserializer`, we need to have access to the registry. The reflection deserializers have access to that, but we have no way of borrowing it for our own deserialization since they have no way of knowing about `MeshDeserializer`. This means we need to implement _another_ `DeserializeSeed`— this time for `Fireball`! And if we decided to put `Fireball` inside another type, well now we need one for that type as well. As you can see, this solution does not scale well and results in a lot of unnecessary boilerplate for the user. ## Solution > [!note] > This PR originally only included the addition of `DeserializeWithRegistry`. Since then, a corresponding `SerializeWithRegistry` trait has also been added. The reasoning and usage is pretty much the same as the former so I didn't bother to update the full PR description. Created the `DeserializeWithRegistry` trait and `ReflectDeserializeWithRegistry` type data. The `DeserializeWithRegistry` trait works like a standard `Deserialize` but provides access to the registry. And by registering the `ReflectDeserializeWithRegistry` type data, the reflection deserializers will automatically use the `DeserializeWithRegistry` implementation, just like it does for `Deserialize`. All we need to do is make the following changes: ```diff #[derive(Reflect)] + #[reflect(DeserializeWithRegistry)] struct Mesh { // ... } - impl<'a, 'de> DeserializeSeed<'de> for MeshDeserializer<'a> { - type Value = Mesh; - fn deserialize<D>(self, deserializer: D) -> Result<Self::Value, D::Error> + impl<'de> DeserializeWithRegistry<'de> for Mesh { + fn deserialize<D>(deserializer: D, registry: &TypeRegistry) -> Result<Self, D::Error> where D: serde::Deserializer<'de>, { // ... } } ``` Now, any time the reflection deserializer comes across `Mesh`, it will opt to use its `DeserializeWithRegistry` implementation. And this means we no longer need to create a whole slew of `DeserializeSeed` types just to deserialize `Mesh`. ### Why not a trait like `DeserializeSeed`? While this would allow for anyone to define a deserializer for `Mesh`, the problem is that it means __anyone can define a deserializer for `Mesh`.__ This has the unfortunate consequence that users can never be certain that their registration of `ReflectDeserializeSeed` is the one that will actually be used. We could consider adding something like that in the future, but I think this PR's solution is much safer and follows the example set by `ReflectDeserialize`. ### What if we made the `TypeRegistry` globally available? This is one potential solution and has been discussed before (#6101). However, that change is much more controversial and comes with its own set of disadvantages (can't have multiple registries such as with multiple worlds, likely some added performance cost with each access, etc.). ### Followup Work Once this PR is merged, we should consider merging `ReflectDeserialize` into `DeserializeWithRegistry`. ~~There is already a blanket implementation to make this transition generally pretty straightforward.~~ The blanket implementations were removed for the sake of this PR and will need to be re-added in the followup. I would propose that we first mark `ReflectDeserialize` as deprecated, though, before we outright remove it in a future release. --- ## Changelog - Added the `DeserializeReflect` trait and `ReflectDeserializeReflect` type data - Added the `SerializeReflect` trait and `ReflectSerializeReflect` type data - Added `TypedReflectDeserializer::of` convenience constructor --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> Co-authored-by: aecsocket <43144841+aecsocket@users.noreply.github.com>
- Loading branch information
3 people
authored
Oct 2, 2024
1 parent
f86ee32
commit eaa37f3