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

[Merged by Bors] - add ReflectAsset and ReflectHandle #5923

Closed
wants to merge 18 commits into from

Conversation

jakobhellermann
Copy link
Contributor

Objective

image

^ enable this

Concretely, I need to

  • list all handle ids for an asset type
  • fetch the asset as dyn Reflect, given a HandleUntyped
  • when encountering a Handle<T>, find out what asset type that handle refers to (T's type id) and turn the handle into a HandleUntyped

Solution

  • add ReflectAsset type containing function pointers for working with assets
pub struct ReflectAsset {
    type_uuid: Uuid,
    assets_resource_type_id: TypeId, // TypeId of the `Assets<T>` resource

    get: fn(&World, HandleUntyped) -> Option<&dyn Reflect>,
    get_mut: fn(&mut World, HandleUntyped) -> Option<&mut dyn Reflect>,
    get_unchecked_mut: unsafe fn(&World, HandleUntyped) -> Option<&mut dyn Reflect>,
    add: fn(&mut World, &dyn Reflect) -> HandleUntyped,
    set: fn(&mut World, HandleUntyped, &dyn Reflect) -> HandleUntyped,
    len: fn(&World) -> usize,
    ids: for<'w> fn(&'w World) -> Box<dyn Iterator<Item = HandleId> + 'w>,
    remove: fn(&mut World, HandleUntyped) -> Option<Box<dyn Reflect>>,
}
  • add ReflectHandle type relating the handle back to the asset type and providing a way to create a HandleUntyped
pub struct ReflectHandle {
    type_uuid: Uuid,
    asset_type_id: TypeId,
    downcast_handle_untyped: fn(&dyn Any) -> Option<HandleUntyped>,
}
  • add the corresponding FromType impls
  • add a function app.register_asset_reflect which is supposed to be called after .add_asset and registers ReflectAsset and ReflectHandle in the type registry

Changelog

  • add ReflectAsset and ReflectHandle types, which allow code to use reflection to manipulate arbitrary assets without knowing their types at compile time

@jakobhellermann jakobhellermann added C-Feature A new feature, making something new possible A-Assets Load files from disk to use for things like images, models, and sounds A-Reflection Runtime information about types labels Sep 9, 2022
@alice-i-cecile alice-i-cecile added this to the Bevy 0.9 milestone Sep 9, 2022
@@ -111,6 +112,8 @@ pub struct StandardMaterial {
pub double_sided: bool,
/// Whether to cull the "front", "back" or neither side of a mesh
/// defaults to `Face::Back`
// TODO: include this in reflection somehow (maybe via remote types like serde https://serde.rs/remote-derive.html)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use a blanket impl for Option in bevy_reflect? Or do we not own the Face type either :(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Face is the problem, because it is defined in wgpu-types, not in bevy.

We can

  1. add a feature to bevy_reflect to implement Reflect for wgpu-types
  2. add a feature to wgpu-types to implement Reflect (if the maintainers agree that this is worth it)
  3. solve this similar to what serde does, and what @MrGVSV experimented with in https://discordapp.com/channels/691052431525675048/1002362493634629796/1016608961224511538

@alice-i-cecile
Copy link
Member

I'm in favor of this! Just some small comments around ergonomics and docs.

Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking really good! Just some comments/questions/suggestions.

/// and adds [`ReflectAsset`] type data to `T` and [`ReflectHandle`] type data to `Handle<T>` in the type registry.
///
/// This enables reflection code to access assets. For detailed information, see the docs on [`ReflectAsset`] and [`ReflectHandle`].
fn register_asset_reflect<T>(&mut self) -> &mut Self
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this should also perform the add_asset call internally. Then users only need to write one or the other, not both. In that case, I might also recommend bikeshedding this to add_reflectable_asset or something similar.

Any thoughts on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. I think my preference is to have these two methods separately, because then you can search for add_asset globally and easily find all asset registration. Registering reflect also feels like something that is done "additionally", not like a completely different way of adding an asset.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, true. I guess it also means you can quickly comment out the line if you need to test it without the reflection. Save on those keystrokes haha

I think I lean more towards keeping it as is now, but if anyone else has any thoughts, I'd love to hear it!

crates/bevy_asset/src/assets.rs Show resolved Hide resolved
crates/bevy_asset/src/assets.rs Outdated Show resolved Hide resolved
crates/bevy_asset/src/reflect.rs Outdated Show resolved Hide resolved
crates/bevy_asset/src/reflect.rs Show resolved Hide resolved
crates/bevy_asset/src/reflect.rs Outdated Show resolved Hide resolved
crates/bevy_pbr/src/alpha.rs Outdated Show resolved Hide resolved
crates/bevy_pbr/src/pbr_material.rs Outdated Show resolved Hide resolved
crates/bevy_sprite/src/mesh2d/color_material.rs Outdated Show resolved Hide resolved
crates/bevy_sprite/src/texture_atlas.rs Show resolved Hide resolved
@jakobhellermann
Copy link
Contributor Author

One question I just thought of is if we want to represent owned values in these kinds of APIs as &Reflect with the expectation that we will just call FromReflect on it, or as Box<dyn Reflect> which we assume to be containing exactly the wanted type.
If the later, we probably need ReflectFromReflect for people who want to use FromReflect dynamically (part of #6056).

@zicklag
Copy link
Member

zicklag commented Oct 6, 2022

Big 👍 to this! This is exactly what I was needing for asset management in scripts.

@zicklag
Copy link
Member

zicklag commented Oct 6, 2022

One question I just thought of is if we want to represent owned values in these kinds of APIs as &Reflect with the expectation that we will just call FromReflect on it, or as Box which we assume to be containing exactly the wanted type.

My instinct is that I should have a Box<dyn Reflect> for owned values, and ReflectFromReflect was another thing I ran into the need for earlier.

To work around not having ReflectFromReflect I ended up trying to combine ReflectDefault to create the value and Reflect.apply() to patch the value.

@MrGVSV
Copy link
Member

MrGVSV commented Oct 25, 2022

@jakobhellermann I think this PR needs to be rebased

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Definitely in line with the patterns we've already established. A bit unfortunate that we need to revert to reflect_value and reflect(ignore) in some cases, but thats not your fault and the solutions are pretty clear at this point (and we can punt that until later).

@cart
Copy link
Member

cart commented Oct 28, 2022

bors r+

bors bot pushed a commit that referenced this pull request Oct 28, 2022
# Objective
![image](https://user-images.githubusercontent.com/22177966/189350194-639a0211-e984-4f73-ae62-0ede44891eb9.png)

^ enable this

Concretely, I need to
- list all handle ids for an asset type
- fetch the asset as `dyn Reflect`, given a `HandleUntyped`
- when encountering a `Handle<T>`, find out what asset type that handle refers to (`T`'s type id) and turn the handle into a `HandleUntyped`

## Solution

- add `ReflectAsset` type containing function pointers for working with assets
```rust
pub struct ReflectAsset {
    type_uuid: Uuid,
    assets_resource_type_id: TypeId, // TypeId of the `Assets<T>` resource

    get: fn(&World, HandleUntyped) -> Option<&dyn Reflect>,
    get_mut: fn(&mut World, HandleUntyped) -> Option<&mut dyn Reflect>,
    get_unchecked_mut: unsafe fn(&World, HandleUntyped) -> Option<&mut dyn Reflect>,
    add: fn(&mut World, &dyn Reflect) -> HandleUntyped,
    set: fn(&mut World, HandleUntyped, &dyn Reflect) -> HandleUntyped,
    len: fn(&World) -> usize,
    ids: for<'w> fn(&'w World) -> Box<dyn Iterator<Item = HandleId> + 'w>,
    remove: fn(&mut World, HandleUntyped) -> Option<Box<dyn Reflect>>,
}
```
- add `ReflectHandle` type relating the handle back to the asset type and providing a way to create a `HandleUntyped`
```rust
pub struct ReflectHandle {
    type_uuid: Uuid,
    asset_type_id: TypeId,
    downcast_handle_untyped: fn(&dyn Any) -> Option<HandleUntyped>,
}
```
- add the corresponding `FromType` impls
- add a function `app.register_asset_reflect` which is supposed to be called after `.add_asset` and registers `ReflectAsset` and `ReflectHandle` in the type registry
---

## Changelog

- add `ReflectAsset` and `ReflectHandle` types, which allow code to use reflection to manipulate arbitrary assets without knowing their types at compile time
@bors bors bot changed the title add ReflectAsset and ReflectHandle [Merged by Bors] - add ReflectAsset and ReflectHandle Oct 28, 2022
@bors bors bot closed this Oct 28, 2022
@jakobhellermann jakobhellermann deleted the reflect-asset branch October 29, 2022 08:50
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective
![image](https://user-images.githubusercontent.com/22177966/189350194-639a0211-e984-4f73-ae62-0ede44891eb9.png)

^ enable this

Concretely, I need to
- list all handle ids for an asset type
- fetch the asset as `dyn Reflect`, given a `HandleUntyped`
- when encountering a `Handle<T>`, find out what asset type that handle refers to (`T`'s type id) and turn the handle into a `HandleUntyped`

## Solution

- add `ReflectAsset` type containing function pointers for working with assets
```rust
pub struct ReflectAsset {
    type_uuid: Uuid,
    assets_resource_type_id: TypeId, // TypeId of the `Assets<T>` resource

    get: fn(&World, HandleUntyped) -> Option<&dyn Reflect>,
    get_mut: fn(&mut World, HandleUntyped) -> Option<&mut dyn Reflect>,
    get_unchecked_mut: unsafe fn(&World, HandleUntyped) -> Option<&mut dyn Reflect>,
    add: fn(&mut World, &dyn Reflect) -> HandleUntyped,
    set: fn(&mut World, HandleUntyped, &dyn Reflect) -> HandleUntyped,
    len: fn(&World) -> usize,
    ids: for<'w> fn(&'w World) -> Box<dyn Iterator<Item = HandleId> + 'w>,
    remove: fn(&mut World, HandleUntyped) -> Option<Box<dyn Reflect>>,
}
```
- add `ReflectHandle` type relating the handle back to the asset type and providing a way to create a `HandleUntyped`
```rust
pub struct ReflectHandle {
    type_uuid: Uuid,
    asset_type_id: TypeId,
    downcast_handle_untyped: fn(&dyn Any) -> Option<HandleUntyped>,
}
```
- add the corresponding `FromType` impls
- add a function `app.register_asset_reflect` which is supposed to be called after `.add_asset` and registers `ReflectAsset` and `ReflectHandle` in the type registry
---

## Changelog

- add `ReflectAsset` and `ReflectHandle` types, which allow code to use reflection to manipulate arbitrary assets without knowing their types at compile time
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds A-Reflection Runtime information about types C-Feature A new feature, making something new possible
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants