Make TypeRegistry global available
#6101
Labels
A-Reflection
Runtime information about types
C-Usability
A targeted quality-of-life change that makes Bevy easier to use
Comments
afonsolage
added
C-Feature
Nilirad
added
C-Usability
One of the foreseeable complications with this is the performance of acquiring a write lock on the registry every time we need to register types and type data. Maybe we remove TypeRegistry::register_scope(|registry| {
// Only need to get the lock once
registry.register::<Foo>();
registry.register::<Bar>();
// ...
}); |
|
I'm almost always against globals, especially for "logical constructs" because they disallow app-specific config in a context where there are multiple apps at the same time. They also complicate (and sometimes break) things like unit tests, which we're already seeing the consequences of from #2250. I don't think optimizing the ergonomics of internal serializers is worth that loss of flexibility. Forgive me for throwing in a slippery slope argument, but if we are willing to make this call here, it feels like we could justify globals for pretty much everything. Then theres @MrGVSV's point about threading. We've already solved this problem via the Resource abstraction (in combination with systems). I think we should let Bevy do its thing here. And we've already optimized the type registration ergonomics from the users' perspective. Accessing the type registry for type registration is fully abstracted out. |
github-merge-queue bot
pushed a commit
that referenced
this issue
Oct 1, 2024
…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 <[email protected]> Co-authored-by: aecsocket <[email protected]>
github-merge-queue bot
pushed a commit
that referenced
this issue
Oct 2, 2024
…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 <[email protected]> Co-authored-by: aecsocket <[email protected]>
robtfm
pushed a commit
to robtfm/bevy
that referenced
this issue
Oct 4, 2024
…y` (bevyengine#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 (bevyengine#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 <[email protected]> Co-authored-by: aecsocket <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
What problem does this solve or what need does it fill?
bevy_reflectusesTypeRegistryto keep track of metadata used to dynamic interact withReflecttypes. This is also used by ser/de operations, likebevy_sceneand any networking.Users should keep a reference to a single
TypeRegistrythat contains all types registered and pass it around toScenes,ReflectSerializer,*ReflectDeserializerand any type which needs to deal with its metadata, which is very verbose to carry around.Also this make it hard to have custom
ReflectSerializeandReflectDeserializeimplementations, since there is noTypeRegistryavailable, where in this case, users should make theTypeRegistryglobal available themselves.Usually the
TypeRegistryis written once, at theAppstartup, either by plugins or by any custom logic, and then read by anybevy_reflectoperations which needs to know it's metadata.What solution would you like?
Add a default global available
TypeRegistry, to be used when a singleTypeRegistryis required (which should cover most use cases), but keepTypeRegistryto be instantiated and be used individually when required (something likeReflectSerializer::with_registry).TypeRegistryglobal available;ReflectSerializer/*ReflectDeserializer; to use globalTypeRegistryby default;TypeRegistryon:ReflectSerializer;*ReflectDeserializer;What alternative(s) have you considered?
Let users create a global available
TypeRegistry, to be built after app startup and use it. This will solve the problem of accessingTypeRegistryinside a customReflectSerialize/ReflectDeserializebut keeping the verbosity of carrying around theTypeRegistry.Additional context
I'll do some researches on this subject, but there are some questions which needs to be solved, mostly to know how far should we go with this enhancement:
DynamicSceneto also use globalTypeRegistryby default?App::register_typein favor of a staticTypeRegistry::register_type?TypeRegistryfromApp?Also, another questions to be answered:
TypeRegistryshould be kept around?The text was updated successfully, but these errors were encountered: