Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bevy_reflect: Add ReflectFromReflect #4147

Closed
wants to merge 7 commits into from

Conversation

MrGVSV
Copy link
Member

@MrGVSV MrGVSV commented Mar 8, 2022

Objective

FromReflect requires a concrete type in order for it to be invoked:

let foo = <MyStruct as FromReflect>::from_reflect(&my_dyn_struct).unwrap();

However, we might not always have access to the concrete type. One area this comes up is in deserialization of trait objects. If we have a something like:

#[derive(Reflect, FromReflect)]
#[reflect(MyTrait)]
struct MyStruct {
  foo: i32
}

And some serialized data:

(
  "type": "MyStruct",
  "struct": {
    "foo": {
      "type": "i32",
      "value": 123
    }
  }
)

How do we deserialize this into something like &dyn MyTrait? (or Box<dyn MyTrait> if #4120 gets merged.)

The ReflectDeserializer gives us a Box<dyn Reflect> over a DynamicStruct, but now we need to convert that DynamicStruct to the actual type. We technically know the type— we can get its registration and type information easily. But how do we go about the conversion?

The only way to convert a DynamicStruct to its underlying type (as far as I'm aware) is by using FromReflect. If all we have is the TypeId of the data or its type name, then we can't do much about this.

I believe bevy_scene solves this problem for deserializing scene components by just iterating through the registered components (more-or-less).

Solution

To use FromReflect dynamically, I added a ReflectFromReflect struct (its naming follows the convention of those generated by the #[reflect_trait] macro). This struct contains a method for converting a Dynamic*** object to a reflected concrete one (as a Box<dyn Reflect>). This allows us to deserialize our MyStruct into a &dyn MyTrait like:

let rfr: &ReflectFromReflect = registry
  .get_type_data::<ReflectFromReflect>(type_id)
  .unwrap();

let reflected: Box<dyn Reflect> = rfr
  .from_reflect(&my_dynamic_struct)
  .unwrap();

let my_trait: &ReflectMyTrait = registry
  .get_type_data::<ReflectMyTrait>(type_id)
  .unwrap();

let my_trait_obj: &dyn MyTrait = my_trait
  .get(reflected.as_ref())
  .unwrap();

Limitations

Like other Reflect*** type data, you need to specify its existence using the #[reflect(...)] attribute. This means that any time you derive FromReflect, you'll want to specify that trait as well.

#[derive(Reflect, FromReflect)]
#[reflect(MyTrait, FromReflect)]
struct MyStruct {
  foo: i32
}

This is an annoying quirk that I'm not too sure how to solve since it's not guaranteed that all Reflect types implement FromReflect (see this comment). In other words, we can't just have it always registered in the Reflect derive macro.

This limitation might be too inconvenient, annoying, and/or confusing. If it is, this PR will probably need to be closed until a different solution is found.

Usefulness

In most cases when deserializing, we probably don't need this since we typically have access to the actual type. The biggest use case for this is supporting deserialization of trait objects. However, this can also be useful outside of just handling deserialization, such as for scenarios when only the DynamicStruct is given and a method needs to be called on it.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 8, 2022
@MrGVSV MrGVSV changed the title Reflect from reflect Add ReflectFromReflect Mar 8, 2022
@inodentry inodentry added A-Reflection Runtime information about types C-Feature A new feature, making something new possible C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Mar 9, 2022
@MrGVSV MrGVSV changed the title Add ReflectFromReflect bevy_reflect: Add ReflectFromReflect Mar 28, 2022
@MrGVSV
Copy link
Member Author

MrGVSV commented Apr 22, 2022

I’m currently feeling iffy about this. Mainly because:

  • It adds boilerplate/confusion
  • It's really only useful for trait objects
  • We don't even deserialize to the actual type with our current deserializer (just a reflected Dynamic***)

So I’m leaning towards closing this in favor of finding a better solution. But if anyone disagrees or thinks this could be useful, I'll keep it open.

@alice-i-cecile
Copy link
Member

@MrGVSV can you make an issue that links to this PR about the challenges we run into working with trait objects? Once that's done, I think we can close this out and revive it later.

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Apr 25, 2022
bors bot pushed a commit that referenced this pull request Dec 11, 2022
# Objective

Resolves #4597 (based on the work from #6056 and a refresh of #4147)

When using reflection, we may often end up in a scenario where we have a Dynamic representing a certain type. Unfortunately, we can't just call `MyType::from_reflect` as we do not have knowledge of the concrete type (`MyType`) at runtime.

Such scenarios happen when we call `Reflect::clone_value`, use the reflection deserializers, or create the Dynamic type ourselves.

## Solution

Add a `ReflectFromReflect` type data struct.

This struct allows us to easily convert Dynamic representations of our types into their respective concrete instances.

```rust
#[derive(Reflect, FromReflect)]
#[reflect(FromReflect)] // <- Register `ReflectFromReflect`
struct MyStruct(String);

let type_id = TypeId::of::<MyStruct>();

// Register our type
let mut registry = TypeRegistry::default();
registry.register::<MyStruct>();

// Create a concrete instance
let my_struct = MyStruct("Hello world".to_string());

// `Reflect::clone_value` will generate a `DynamicTupleStruct` for tuple struct types
let dynamic_value: Box<dyn Reflect> = my_struct.clone_value();
assert!(!dynamic_value.is::<MyStruct>());

// Get the `ReflectFromReflect` type data from the registry
let rfr: &ReflectFromReflect = registry
  .get_type_data::<ReflectFromReflect>(type_id)
  .unwrap();

// Call `FromReflect::from_reflect` on our Dynamic value
let concrete_value: Box<dyn Reflect> = rfr.from_reflect(&dynamic_value);
assert!(concrete_value.is::<MyStruct>());
```

### Why this PR?

###### Why now?

The three main reasons I closed #4147 were that:

1. Registering `ReflectFromReflect` is clunky (deriving `FromReflect` *and* registering `ReflectFromReflect`)
2. The ecosystem and Bevy itself didn't seem to pay much attention to deriving `FromReflect`
3. I didn't see a lot of desire from the community for such a feature

However, as time has passed it seems 2 and 3 are not really true anymore. Bevy is internally adding lots more `FromReflect` derives, which should make this feature all the more useful. Additionally, I have seen a growing number of people look for something like `ReflectFromReflect`.

I think 1 is still an issue, but not a horrible one. Plus it could be made much, much better using #6056. And I think splitting this feature out of #6056 could lead to #6056 being adopted sooner (or at least make the need more clear to users).

###### Why not just re-open #4147?

The main reason is so that this PR can garner more attention than simply re-opening the old one. This helps bring fresh eyes to the PR for potentially more perspectives/reviews.

---

## Changelog

* Added `ReflectFromReflect`

Co-authored-by: Gino Valente <[email protected]>
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
# Objective

Resolves bevyengine#4597 (based on the work from bevyengine#6056 and a refresh of bevyengine#4147)

When using reflection, we may often end up in a scenario where we have a Dynamic representing a certain type. Unfortunately, we can't just call `MyType::from_reflect` as we do not have knowledge of the concrete type (`MyType`) at runtime.

Such scenarios happen when we call `Reflect::clone_value`, use the reflection deserializers, or create the Dynamic type ourselves.

## Solution

Add a `ReflectFromReflect` type data struct.

This struct allows us to easily convert Dynamic representations of our types into their respective concrete instances.

```rust
#[derive(Reflect, FromReflect)]
#[reflect(FromReflect)] // <- Register `ReflectFromReflect`
struct MyStruct(String);

let type_id = TypeId::of::<MyStruct>();

// Register our type
let mut registry = TypeRegistry::default();
registry.register::<MyStruct>();

// Create a concrete instance
let my_struct = MyStruct("Hello world".to_string());

// `Reflect::clone_value` will generate a `DynamicTupleStruct` for tuple struct types
let dynamic_value: Box<dyn Reflect> = my_struct.clone_value();
assert!(!dynamic_value.is::<MyStruct>());

// Get the `ReflectFromReflect` type data from the registry
let rfr: &ReflectFromReflect = registry
  .get_type_data::<ReflectFromReflect>(type_id)
  .unwrap();

// Call `FromReflect::from_reflect` on our Dynamic value
let concrete_value: Box<dyn Reflect> = rfr.from_reflect(&dynamic_value);
assert!(concrete_value.is::<MyStruct>());
```

### Why this PR?

###### Why now?

The three main reasons I closed bevyengine#4147 were that:

1. Registering `ReflectFromReflect` is clunky (deriving `FromReflect` *and* registering `ReflectFromReflect`)
2. The ecosystem and Bevy itself didn't seem to pay much attention to deriving `FromReflect`
3. I didn't see a lot of desire from the community for such a feature

However, as time has passed it seems 2 and 3 are not really true anymore. Bevy is internally adding lots more `FromReflect` derives, which should make this feature all the more useful. Additionally, I have seen a growing number of people look for something like `ReflectFromReflect`.

I think 1 is still an issue, but not a horrible one. Plus it could be made much, much better using bevyengine#6056. And I think splitting this feature out of bevyengine#6056 could lead to bevyengine#6056 being adopted sooner (or at least make the need more clear to users).

###### Why not just re-open bevyengine#4147?

The main reason is so that this PR can garner more attention than simply re-opening the old one. This helps bring fresh eyes to the PR for potentially more perspectives/reviews.

---

## Changelog

* Added `ReflectFromReflect`

Co-authored-by: Gino Valente <[email protected]>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

Resolves bevyengine#4597 (based on the work from bevyengine#6056 and a refresh of bevyengine#4147)

When using reflection, we may often end up in a scenario where we have a Dynamic representing a certain type. Unfortunately, we can't just call `MyType::from_reflect` as we do not have knowledge of the concrete type (`MyType`) at runtime.

Such scenarios happen when we call `Reflect::clone_value`, use the reflection deserializers, or create the Dynamic type ourselves.

## Solution

Add a `ReflectFromReflect` type data struct.

This struct allows us to easily convert Dynamic representations of our types into their respective concrete instances.

```rust
#[derive(Reflect, FromReflect)]
#[reflect(FromReflect)] // <- Register `ReflectFromReflect`
struct MyStruct(String);

let type_id = TypeId::of::<MyStruct>();

// Register our type
let mut registry = TypeRegistry::default();
registry.register::<MyStruct>();

// Create a concrete instance
let my_struct = MyStruct("Hello world".to_string());

// `Reflect::clone_value` will generate a `DynamicTupleStruct` for tuple struct types
let dynamic_value: Box<dyn Reflect> = my_struct.clone_value();
assert!(!dynamic_value.is::<MyStruct>());

// Get the `ReflectFromReflect` type data from the registry
let rfr: &ReflectFromReflect = registry
  .get_type_data::<ReflectFromReflect>(type_id)
  .unwrap();

// Call `FromReflect::from_reflect` on our Dynamic value
let concrete_value: Box<dyn Reflect> = rfr.from_reflect(&dynamic_value);
assert!(concrete_value.is::<MyStruct>());
```

### Why this PR?

###### Why now?

The three main reasons I closed bevyengine#4147 were that:

1. Registering `ReflectFromReflect` is clunky (deriving `FromReflect` *and* registering `ReflectFromReflect`)
2. The ecosystem and Bevy itself didn't seem to pay much attention to deriving `FromReflect`
3. I didn't see a lot of desire from the community for such a feature

However, as time has passed it seems 2 and 3 are not really true anymore. Bevy is internally adding lots more `FromReflect` derives, which should make this feature all the more useful. Additionally, I have seen a growing number of people look for something like `ReflectFromReflect`.

I think 1 is still an issue, but not a horrible one. Plus it could be made much, much better using bevyengine#6056. And I think splitting this feature out of bevyengine#6056 could lead to bevyengine#6056 being adopted sooner (or at least make the need more clear to users).

###### Why not just re-open bevyengine#4147?

The main reason is so that this PR can garner more attention than simply re-opening the old one. This helps bring fresh eyes to the PR for potentially more perspectives/reviews.

---

## Changelog

* Added `ReflectFromReflect`

Co-authored-by: Gino Valente <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types C-Feature A new feature, making something new possible C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants