f5210c54d2
6 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Gino Valente
|
f5210c54d2
|
bevy_reflect: Reflection-based cloning (#13432)
# Objective Using `Reflect::clone_value` can be somewhat confusing to those unfamiliar with how Bevy's reflection crate works. For example take the following code: ```rust let value: usize = 123; let clone: Box<dyn Reflect> = value.clone_value(); ``` What can we expect to be the underlying type of `clone`? If you guessed `usize`, then you're correct! Let's try another: ```rust #[derive(Reflect, Clone)] struct Foo(usize); let value: Foo = Foo(123); let clone: Box<dyn Reflect> = value.clone_value(); ``` What about this code? What is the underlying type of `clone`? If you guessed `Foo`, unfortunately you'd be wrong. It's actually `DynamicStruct`. It's not obvious that the generated `Reflect` impl actually calls `Struct::clone_dynamic` under the hood, which always returns `DynamicStruct`. There are already some efforts to make this a bit more apparent to the end-user: #7207 changes the signature of `Reflect::clone_value` to instead return `Box<dyn PartialReflect>`, signaling that we're potentially returning a dynamic type. But why _can't_ we return `Foo`? `Foo` can obviously be cloned— in fact, we already derived `Clone` on it. But even without the derive, this seems like something `Reflect` should be able to handle. Almost all types that implement `Reflect` either contain no data (trivially clonable), they contain a `#[reflect_value]` type (which, by definition, must implement `Clone`), or they contain another `Reflect` type (which recursively fall into one of these three categories). This PR aims to enable true reflection-based cloning where you get back exactly the type that you think you do. ## Solution Add a `Reflect::reflect_clone` method which returns `Result<Box<dyn Reflect>, ReflectCloneError>`, where the `Box<dyn Reflect>` is guaranteed to be the same type as `Self`. ```rust #[derive(Reflect)] struct Foo(usize); let value: Foo = Foo(123); let clone: Box<dyn Reflect> = value.reflect_clone().unwrap(); assert!(clone.is::<Foo>()); ``` Notice that we didn't even need to derive `Clone` for this to work: it's entirely powered via reflection! Under the hood, the macro generates something like this: ```rust fn reflect_clone(&self) -> Result<Box<dyn Reflect>, ReflectCloneError> { Ok(Box::new(Self { // The `reflect_clone` impl for `usize` just makes use of its `Clone` impl 0: Reflect::reflect_clone(&self.0)?.take().map_err(/* ... */)?, })) } ``` If we did derive `Clone`, we can tell `Reflect` to rely on that instead: ```rust #[derive(Reflect, Clone)] #[reflect(Clone)] struct Foo(usize); ``` <details> <summary>Generated Code</summary> ```rust fn reflect_clone(&self) -> Result<Box<dyn Reflect>, ReflectCloneError> { Ok(Box::new(Clone::clone(self))) } ``` </details> Or, we can specify our own cloning function: ```rust #[derive(Reflect)] #[reflect(Clone(incremental_clone))] struct Foo(usize); fn incremental_clone(value: &usize) -> usize { *value + 1 } ``` <details> <summary>Generated Code</summary> ```rust fn reflect_clone(&self) -> Result<Box<dyn Reflect>, ReflectCloneError> { Ok(Box::new(incremental_clone(self))) } ``` </details> Similarly, we can specify how fields should be cloned. This is important for fields that are `#[reflect(ignore)]`'d as we otherwise have no way to know how they should be cloned. ```rust #[derive(Reflect)] struct Foo { #[reflect(ignore, clone)] bar: usize, #[reflect(ignore, clone = "incremental_clone")] baz: usize, } fn incremental_clone(value: &usize) -> usize { *value + 1 } ``` <details> <summary>Generated Code</summary> ```rust fn reflect_clone(&self) -> Result<Box<dyn Reflect>, ReflectCloneError> { Ok(Box::new(Self { bar: Clone::clone(&self.bar), baz: incremental_clone(&self.baz), })) } ``` </details> If we don't supply a `clone` attribute for an ignored field, then the method will automatically return `Err(ReflectCloneError::FieldNotClonable {/* ... */})`. `Err` values "bubble up" to the caller. So if `Foo` contains `Bar` and the `reflect_clone` method for `Bar` returns `Err`, then the `reflect_clone` method for `Foo` also returns `Err`. ### Attribute Syntax You might have noticed the differing syntax between the container attribute and the field attribute. This was purely done for consistency with the current attributes. There are PRs aimed at improving this. #7317 aims at making the "special-cased" attributes more in line with the field attributes syntactically. And #9323 aims at moving away from the stringified paths in favor of just raw function paths. ### Compatibility with Unique Reflect This PR was designed with Unique Reflect (#7207) in mind. This method actually wouldn't change that much (if at all) under Unique Reflect. It would still exist on `Reflect` and it would still `Option<Box<dyn Reflect>>`. In fact, Unique Reflect would only _improve_ the user's understanding of what this method returns. We may consider moving what's currently `Reflect::clone_value` to `PartialReflect` and possibly renaming it to `partial_reflect_clone` or `clone_dynamic` to better indicate how it differs from `reflect_clone`. ## Testing You can test locally by running the following command: ``` cargo test --package bevy_reflect ``` --- ## Changelog - Added `Reflect::reflect_clone` method - Added `ReflectCloneError` error enum - Added `#[reflect(Clone)]` container attribute - Added `#[reflect(clone)]` field attribute |
||
|
Clar Fon
|
efda7f3f9c
|
Simpler lint fixes: makes ci lints work but disables a lint for now (#15376)
Takes the first two commits from #15375 and adds suggestions from this comment: https://github.com/bevyengine/bevy/pull/15375#issuecomment-2366968300 See #15375 for more reasoning/motivation. ## Rebasing (rerunning) ```rust git switch simpler-lint-fixes git reset --hard main cargo fmt --all -- --unstable-features --config normalize_comments=true,imports_granularity=Crate cargo fmt --all git add --update git commit --message "rustfmt" cargo clippy --workspace --all-targets --all-features --fix cargo fmt --all -- --unstable-features --config normalize_comments=true,imports_granularity=Crate cargo fmt --all git add --update git commit --message "clippy" git cherry-pick e6c0b94f6795222310fb812fa5c4512661fc7887 ``` |
||
|
Benjamin Brienen
|
67615c5051
|
split bevy_reflect::derive::utilities into proper modules (#15354)
# Objective - A utilities module is considered to be a bad practice and poor organization of code, so this fixes it. ## Solution - Split each struct into its own module - Move related lose functions into their own module - Move the last few bits into good places ## Testing - CI --------- Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com> |
||
|
Gino Valente
|
6183b56b5d
|
bevy_reflect: Reflect remote types (#6042)
# Objective The goal with this PR is to allow the use of types that don't implement `Reflect` within the reflection API. Rust's [orphan rule](https://doc.rust-lang.org/book/ch10-02-traits.html#implementing-a-trait-on-a-type) prevents implementing a trait on an external type when neither type nor trait are owned by the implementor. This means that if a crate, `cool_rust_lib`, defines a type, `Foo`, then a user cannot use it with reflection. What this means is that we have to ignore it most of the time: ```rust #[derive(Reflect)] struct SomeStruct { #[reflect(ignore)] data: cool_rust_lib::Foo } ``` Obviously, it's impossible to implement `Reflect` on `Foo`. But does it *have* to be? Most of reflection doesn't deal with concrete types— it's almost all using `dyn Reflect`. And being very metadata-driven, it should theoretically be possible. I mean, [`serde`](https://serde.rs/remote-derive.html) does it. ## Solution > Special thanks to @danielhenrymantilla for their help reviewing this PR and offering wisdom wrt safety. Taking a page out of `serde`'s book, this PR adds the ability to easily use "remote types" with reflection. In this context, a "remote type" is the external type for which we have no ability to implement `Reflect`. This adds the `#[reflect_remote(...)]` attribute macro, which is used to generate "remote type wrappers". All you have to do is define the wrapper exactly the same as the remote type's definition: ```rust // Pretend this is our external crate mod cool_rust_lib { #[derive(Default)] struct Foo { pub value: String } } #[reflect_remote(cool_rust_lib::Foo)] struct FooWrapper { pub value: String } ``` > **Note:** All fields in the external type *must* be public. This could be addressed with a separate getter/setter attribute either in this PR or in another one. The macro takes this user-defined item and transforms it into a newtype wrapper around the external type, marking it as `#[repr(transparent)]`. The fields/variants defined by the user are simply used to build out the reflection impls. Additionally, it generates an implementation of the new trait, `ReflectRemote`, which helps prevent accidental misuses of this API. Therefore, the output generated by the macro would look something like: ```rust #[repr(transparent)] struct FooWrapper(pub cool_rust_lib::Foo); impl ReflectRemote for FooWrapper { type Remote = cool_rust_lib::Foo; // transmutation methods... } // reflection impls... // these will acknowledge and make use of the `value` field ``` Internally, the reflection API will pass around the `FooWrapper` and [transmute](https://doc.rust-lang.org/std/mem/fn.transmute.html) it where necessary. All we have to do is then tell `Reflect` to do that. So rather than ignoring the field, we tell `Reflect` to use our wrapper using the `#[reflect(remote = ...)]` field attribute: ```rust #[derive(Reflect)] struct SomeStruct { #[reflect(remote = FooWrapper)] data: cool_rust_lib::Foo } ``` #### Other Macros & Type Data Because this macro consumes the defined item and generates a new one, we can't just put our macros anywhere. All macros that should be passed to the generated struct need to come *below* this macro. For example, to derive `Default` and register its associated type data: ```rust // ✅ GOOD #[reflect_remote(cool_rust_lib::Foo)] #[derive(Default)] #[reflect(Default)] struct FooWrapper { pub value: String } // ❌ BAD #[derive(Default)] #[reflect_remote(cool_rust_lib::Foo)] #[reflect(Default)] struct FooWrapper { pub value: String } ``` #### Generics Generics are forwarded to the generated struct as well. They should also be defined in the same order: ```rust #[reflect_remote(RemoteGeneric<'a, T1, T2>)] struct GenericWrapper<'a, T1, T2> { pub foo: &'a T1, pub bar: &'a T2, } ``` > Naming does *not* need to match the original definition's. Only order matters here. > Also note that the code above is just a demonstration and doesn't actually compile since we'd need to enforce certain bounds (e.g. `T1: Reflect`, `'a: 'static`, etc.) #### Nesting And, yes, you can nest remote types: ```rust #[reflect_remote(RemoteOuter)] struct OuterWrapper { #[reflect(remote = InnerWrapper)] pub inner: RemoteInner } #[reflect_remote(RemoteInner)] struct InnerWrapper(usize); ``` #### Assertions This macro will also generate some compile-time assertions to ensure that the correct types are used. It's important we catch this early so users don't have to wait for something to panic. And it also helps keep our `unsafe` a little safer. For example, a wrapper definition that does not match its corresponding remote type will result in an error: ```rust mod external_crate { pub struct TheirStruct(pub u32); } #[reflect_remote(external_crate::TheirStruct)] struct MyStruct(pub String); // ERROR: expected type `u32` but found `String` ``` <details> <summary>Generated Assertion</summary> ```rust const _: () = { #[allow(non_snake_case)] #[allow(unused_variables)] #[allow(unused_assignments)] #[allow(unreachable_patterns)] #[allow(clippy::multiple_bound_locations)] fn assert_wrapper_definition_matches_remote_type( mut __remote__: external_crate::TheirStruct, ) { __remote__.0 = (|| -> ::core::option::Option<String> { None })().unwrap(); } }; ``` </details> Additionally, using the incorrect type in a `#[reflect(remote = ...)]` attribute should result in an error: ```rust mod external_crate { pub struct TheirFoo(pub u32); pub struct TheirBar(pub i32); } #[reflect_remote(external_crate::TheirFoo)] struct MyFoo(pub u32); #[reflect_remote(external_crate::TheirBar)] struct MyBar(pub i32); #[derive(Reflect)] struct MyStruct { #[reflect(remote = MyBar)] // ERROR: expected type `TheirFoo` but found struct `TheirBar` foo: external_crate::TheirFoo } ``` <details> <summary>Generated Assertion</summary> ```rust const _: () = { struct RemoteFieldAssertions; impl RemoteFieldAssertions { #[allow(non_snake_case)] #[allow(clippy::multiple_bound_locations)] fn assert__foo__is_valid_remote() { let _: <MyBar as bevy_reflect::ReflectRemote>::Remote = (|| -> ::core::option::Option<external_crate::TheirFoo> { None })().unwrap(); } } }; ``` </details> ### Discussion There are a couple points that I think still need discussion or validation. - [x] 1. `Any` shenanigans ~~If we wanted to downcast our remote type from a `dyn Reflect`, we'd have to first downcast to the wrapper then extract the inner type. This PR has a [commit](b840db9f74cb6d357f951cb11b150d46bac89ee2) that addresses this by making all the `Reflect::*any` methods return the inner type rather than the wrapper type. This allows us to downcast directly to our remote type.~~ ~~However, I'm not sure if this is something we want to do. For unknowing users, it could be confusing and seemingly inconsistent. Is it worth keeping? Or should this behavior be removed?~~ I think this should be fine. The remote wrapper is an implementation detail and users should not need to downcast to the wrapper type. Feel free to let me know if there are other opinions on this though! - [x] 2. Implementing `Deref/DerefMut` and `From` ~~We don't currently do this, but should we implement other traits on the generated transparent struct? We could implement `Deref`/`DerefMut` to easily access the inner type. And we could implement `From` for easier conversion between the two types (e.g. `T: Into<Foo>`).~~ As mentioned in the comments, we probably don't need to do this. Again, the remote wrapper is an implementation detail, and should generally not be used directly. - [x] 3. ~~Should we define a getter/setter field attribute in this PR as well or leave it for a future one?~~ I think this should be saved for a future PR - [ ] 4. Any foreseeable issues with this implementation? #### Alternatives One alternative to defining our own `ReflectRemote` would be to use [bytemuck's `TransparentWrapper`](https://docs.rs/bytemuck/1.13.1/bytemuck/trait.TransparentWrapper.html) (as suggested by @danielhenrymantilla). This is definitely a viable option, as `ReflectRemote` is pretty much the same thing as `TransparentWrapper`. However, the cost would be bringing in a new crate— though, it is already in use in a few other sub-crates like bevy_render. I think we're okay just defining `ReflectRemote` ourselves, but we can go the bytemuck route if we'd prefer offloading that work to another crate. --- ## Changelog * Added the `#[reflect_remote(...)]` attribute macro to allow `Reflect` to be used on remote types * Added `ReflectRemote` trait for ensuring proper remote wrapper usage |
||
|
Gino Valente
|
5db52663b3
|
bevy_reflect: Custom attributes (#11659)
# Objective As work on the editor starts to ramp up, it might be nice to start allowing types to specify custom attributes. These can be used to provide certain functionality to fields, such as ranges or controlling how data is displayed. A good example of this can be seen in [`bevy-inspector-egui`](https://github.com/jakobhellermann/bevy-inspector-egui) with its [`InspectorOptions`](https://docs.rs/bevy-inspector-egui/0.22.1/bevy_inspector_egui/struct.InspectorOptions.html): ```rust #[derive(Reflect, Default, InspectorOptions)] #[reflect(InspectorOptions)] struct Slider { #[inspector(min = 0.0, max = 1.0)] value: f32, } ``` Normally, as demonstrated in the example above, these attributes are handled by a derive macro and stored in a corresponding `TypeData` struct (i.e. `ReflectInspectorOptions`). Ideally, we would have a good way of defining this directly via reflection so that users don't need to create and manage a whole proc macro just to allow these sorts of attributes. And note that this doesn't have to just be for inspectors and editors. It can be used for things done purely on the code side of things. ## Solution Create a new method for storing attributes on fields via the `Reflect` derive. These custom attributes are stored in type info (e.g. `NamedField`, `StructInfo`, etc.). ```rust #[derive(Reflect)] struct Slider { #[reflect(@0.0..=1.0)] value: f64, } let TypeInfo::Struct(info) = Slider::type_info() else { panic!("expected struct info"); }; let field = info.field("value").unwrap(); let range = field.get_attribute::<RangeInclusive<f64>>().unwrap(); assert_eq!(*range, 0.0..=1.0); ``` ## TODO - [x] ~~Bikeshed syntax~~ Went with a type-based approach, prefixed by `@` for ease of parsing and flexibility - [x] Add support for custom struct/tuple struct field attributes - [x] Add support for custom enum variant field attributes - [x] ~~Add support for custom enum variant attributes (maybe?)~~ ~~Will require a larger refactor. Can be saved for a future PR if we really want it.~~ Actually, we apparently still have support for variant attributes despite not using them, so it was pretty easy to add lol. - [x] Add support for custom container attributes - [x] Allow custom attributes to store any reflectable value (not just `Lit`) - [x] ~~Store attributes in registry~~ This PR used to store these in attributes in the registry, however, it has since switched over to storing them in type info - [x] Add example ## Bikeshedding > [!note] > This section was made for the old method of handling custom attributes, which stored them by name (i.e. `some_attribute = 123`). The PR has shifted away from that, to a more type-safe approach. > > This section has been left for reference. There are a number of ways we can syntactically handle custom attributes. Feel free to leave a comment on your preferred one! Ideally we want one that is clear, readable, and concise since these will potentially see _a lot_ of use. Below is a small, non-exhaustive list of them. Note that the `skip_serializing` reflection attribute is added to demonstrate how each case plays with existing reflection attributes. <details> <summary>List</summary> ##### 1. `@(name = value)` > The `@` was chosen to make them stand out from other attributes and because the "at" symbol is a subtle pneumonic for "attribute". Of course, other symbols could be used (e.g. `$`, `#`, etc.). ```rust #[derive(Reflect)] struct Slider { #[reflect(@(min = 0.0, max = 1.0), skip_serializing)] #[[reflect(@(bevy_editor::hint = "Range: 0.0 to 1.0"))] value: f32, } ``` ##### 2. `@name = value` > This is my personal favorite. ```rust #[derive(Reflect)] struct Slider { #[reflect(@min = 0.0, @max = 1.0, skip_serializing)] #[[reflect(@bevy_editor::hint = "Range: 0.0 to 1.0")] value: f32, } ``` ##### 3. `custom_attr(name = value)` > `custom_attr` can be anything. Other possibilities include `with` or `tag`. ```rust #[derive(Reflect)] struct Slider { #[reflect(custom_attr(min = 0.0, max = 1.0), skip_serializing)] #[[reflect(custom_attr(bevy_editor::hint = "Range: 0.0 to 1.0"))] value: f32, } ``` ##### 4. `reflect_attr(name = value)` ```rust #[derive(Reflect)] struct Slider { #[reflect(skip_serializing)] #[reflect_attr(min = 0.0, max = 1.0)] #[[reflect_attr(bevy_editor::hint = "Range: 0.0 to 1.0")] value: f32, } ``` </details> --- ## Changelog - Added support for custom attributes on reflected types (i.e. `#[reflect(@Foo::new("bar")]`) |
||
|
BD103
|
22305acf66
|
Rename bevy_reflect_derive folder to derive (#13269)
# Objective - Some of the "large" crates have sub-crates, usually for things such as macros. - For an example, see [`bevy_ecs_macros` at `bevy_ecs/macros`]( |