0fe33c3bba
313 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
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 ``` |
||
|
Gino Valente
|
1a41c736b3
|
bevy_reflect: Update EulerRot to match glam 0.29 (#15402)
# Objective #15349 added an `impl_reflect!` for `glam::EulerRot`. This was done by copying and pasting the enum definition from `glam` into `bevy_reflect` so that the macro could interpret the variants. However, as mentioned in the description for that PR, this would need to be updated for `glam` 0.29, as it had not been updated yet. #15249 came and updated `glam` to 0.29, but did not change these impls. This is understandable as failing to do so doesn't cause any compile errors. This PR updates the definition and aims to make this silent breakage a little less silent. ## Solution Firstly, I updated the definition for `EulerRot` to match the one from `glam`. Secondly, I added the `assert_type_match` crate, which I created specifically to solve this problem. By using this crate, we'll get a compile time error if `glam` ever decides to change `EulerRot` again. In the future we can consider using it for other types with this problem, including in other crates (I'm pretty sure `bevy_window` and/or `bevy_winit` also copy+paste some types). I made sure to use as few dependencies as possible so everything should already be in-tree (it's just `quote`, `proc-macro2`, and `syn` with default features). ## Testing No tests added. CI should pass. --- ## Migration Guide The reflection implementation for `EulerRot` has been updated to align with `glam` 0.29. Please update any reflection-based usages accordingly. |
||
|
Gino Valente
|
83356b12c9
|
bevy_reflect: Replace "value" terminology with "opaque" (#15240)
# Objective Currently, the term "value" in the context of reflection is a bit overloaded. For one, it can be used synonymously with "data" or "variable". An example sentence would be "this function takes a reflected value". However, it is also used to refer to reflected types which are `ReflectKind::Value`. These types are usually either primitives, opaque types, or types that don't fall into any other `ReflectKind` (or perhaps could, but don't due to some limitation/difficulty). An example sentence would be "this function takes a reflected value type". This makes it difficult to write good documentation or other learning material without causing some amount of confusion to readers. Ideally, we'd be able to move away from the `ReflectKind::Value` usage and come up with a better term. ## Solution This PR replaces the terminology of "value" with "opaque" across `bevy_reflect`. This includes in documentation, type names, variant names, and macros. The term "opaque" was chosen because that's essentially how the type is treated within the reflection API. In other words, its internal structure is hidden. All we can do is work with the type itself. ### Primitives While primitives are not technically opaque types, I think it's still clearer to refer to them as "opaque" rather than keep the confusing "value" terminology. We could consider adding another concept for primitives (e.g. `ReflectKind::Primitive`), but I'm not sure that provides a lot of benefit right now. In most circumstances, they'll be treated just like an opaque type. They would also likely use the same macro (or two copies of the same macro but with different names). ## Testing You can test locally by running: ``` cargo test --package bevy_reflect --all-features ``` --- ## Migration Guide The reflection concept of "value type" has been replaced with a clearer "opaque type". The following renames have been made to account for this: - `ReflectKind::Value` → `ReflectKind::Opaque` - `ReflectRef::Value` → `ReflectRef::Opaque` - `ReflectMut::Value` → `ReflectMut::Opaque` - `ReflectOwned::Value` → `ReflectOwned::Opaque` - `TypeInfo::Value` → `TypeInfo::Opaque` - `ValueInfo` → `OpaqueInfo` - `impl_reflect_value!` → `impl_reflect_opaque!` - `impl_from_reflect_value!` → `impl_from_reflect_opaque!` Additionally, declaring your own opaque types no longer uses `#[reflect_value]`. This attribute has been replaced by `#[reflect(opaque)]`: ```rust // BEFORE #[derive(Reflect)] #[reflect_value(Default)] struct MyOpaqueType(u32); // AFTER #[derive(Reflect)] #[reflect(opaque)] #[reflect(Default)] struct MyOpaqueType(u32); ``` Note that the order in which `#[reflect(opaque)]` appears does not matter. |
||
|
Benjamin Brienen
|
8a6d0b063c
|
Use crate: disqualified (#15372)
# Objective Fixes #15351 ## Solution - Created new external crate and ported over the code ## Testing - CI ## Migration guide Replace references to `bevy_utils::ShortName` with `disqualified::ShortName`. |
||
|
Clar Fon
|
2c5be2ef4c
|
Reflect for TextureFormat (#15355)
# Objective In order to derive `Reflect`, all of a struct's fields must implement `FromReflect`. [As part of looking into some of the work mentioned here](https://github.com/bevyengine/bevy/issues/13713#issuecomment-2364786694), I noticed that `TextureFormat` doesn't implement `Reflect`, and decided to split that into a separate PR. ## Solution I decided that `TextureFormat` should be a `reflect_value` since, although one variant has fields, most users will treat this as an opaque value set explicitly. It also substantially reduces the complexity of the implementation. For now, this implementation isn't actually used by any crates, so, I decided to not preemptively enable the feature on anything. But it's technically an option, now, and more `wgpu` types can be added in the future. ## Testing Everything compiles okay, and I can't really see how this could be done incorrectly given the above constraints. |
||
|
aecsocket
|
fb324f0e89
|
impl_reflect! for EulerRot instead of treating it as an opaque value (#15349)
# Objective
Currently, Bevy implements reflection for `glam::EulerRot` using:
```rs
impl_reflect_value!(::glam::EulerRot(Debug, Default, Deserialize, Serialize));
```
Treating it as an opaque type. However, it's useful to expose the
EulerRot enum variants directly, which I make use of from a drop down
selection box in `bevy_egui`. This PR changes this to use
`impl_reflect!`.
**Importantly**, Bevy currently uses glam 0.28.0, in which `EulerRot`
has just 6 variants. In glam 0.29.0, this is exanded to 24 variants, see
|
||
|
Gino Valente
|
51accd34ed
|
bevy_reflect: Add dynamic type data access and iteration to TypeRegistration (#15347)
# Objective There's currently no way to iterate through all the type data in a `TypeRegistration`. While these are all type-erased, it can still be useful to see what types (by `TypeId`) are registered for a given type. Additionally, it might be good to have ways of dynamically working with `TypeRegistration`. ## Solution Added a way to iterate through all type data on a given `TypeRegistration`. This PR also adds methods for working with type data dynamically as well as methods for conveniently checking if a given type data exists on the registration. I also took this opportunity to reorganize the methods on `TypeRegistration` as it has always bothered me haha (i.e. the constructor not being at the top, etc.). ## Testing You can test locally by running: ``` cargo test --package bevy_reflect ``` --- ## Showcase The type-erased type data on a `TypeRegistration` can now be iterated! ```rust #[derive(Reflect)] struct Foo; #[derive(Clone)] struct DataA(i32); #[derive(Clone)] struct DataB(i32); let mut registration = TypeRegistration::of::<Foo>(); registration.insert(DataA(123)); registration.insert(DataB(456)); let mut iter = registration.iter(); let (id, data) = iter.next().unwrap(); assert_eq!(id, TypeId::of::<DataA>()); assert_eq!(data.downcast_ref::<DataA>().unwrap().0, 123); let (id, data) = iter.next().unwrap(); assert_eq!(id, TypeId::of::<DataB>()); assert_eq!(data.downcast_ref::<DataB>().unwrap().0, 456); assert!(iter.next().is_none()); ``` |
||
|
Patrick Walton
|
8154164f1b
|
Allow animation clips to animate arbitrary properties. (#15282)
Currently, Bevy restricts animation clips to animating
`Transform::translation`, `Transform::rotation`, `Transform::scale`, or
`MorphWeights`, which correspond to the properties that glTF can
animate. This is insufficient for many use cases such as animating UI,
as the UI layout systems expect to have exclusive control over UI
elements' `Transform`s and therefore the `Style` properties must be
animated instead.
This commit fixes this, allowing for `AnimationClip`s to animate
arbitrary properties. The `Keyframes` structure has been turned into a
low-level trait that can be implemented to achieve arbitrary animation
behavior. Along with `Keyframes`, this patch adds a higher-level trait,
`AnimatableProperty`, that simplifies the task of animating single
interpolable properties. Built-in `Keyframes` implementations exist for
translation, rotation, scale, and morph weights. For the most part, you
can migrate by simply changing your code from
`Keyframes::Translation(...)` to `TranslationKeyframes(...)`, and
likewise for rotation, scale, and morph weights.
An example `AnimatableProperty` implementation for the font size of a
text section follows:
#[derive(Reflect)]
struct FontSizeProperty;
impl AnimatableProperty for FontSizeProperty {
type Component = Text;
type Property = f32;
fn get_mut(component: &mut Self::Component) -> Option<&mut
Self::Property> {
Some(&mut component.sections.get_mut(0)?.style.font_size)
}
}
In order to keep this patch relatively small, this patch doesn't include
an implementation of `AnimatableProperty` on top of the reflection
system. That can be a follow-up.
This patch builds on top of the new `EntityMutExcept<>` type in order to
widen the `AnimationTarget` query to include write access to all
components. Because `EntityMutExcept<>` has some performance overhead
over an explicit query, we continue to explicitly query `Transform` in
order to avoid regressing the performance of skeletal animation, such as
the `many_foxes` benchmark. I've measured the performance of that
benchmark and have found no significant regressions.
A new example, `animated_ui`, has been added. This example shows how to
use Bevy's built-in animation infrastructure to animate font size and
color, which wasn't possible before this patch.
## Showcase
https://github.com/user-attachments/assets/1fa73492-a9ce-405a-a8f2-4aacd7f6dc97
## Migration Guide
* Animation keyframes are now an extensible trait, not an enum. Replace
`Keyframes::Translation(...)`, `Keyframes::Scale(...)`,
`Keyframes::Rotation(...)`, and `Keyframes::Weights(...)` with
`Box::new(TranslationKeyframes(...))`, `Box::new(ScaleKeyframes(...))`,
`Box::new(RotationKeyframes(...))`, and
`Box::new(MorphWeightsKeyframes(...))` respectively.
|
||
|
Gino Valente
|
6e95f297ea
|
bevy_reflect: Automatic arg count validation (#15145)
# Objective Functions created into `DynamicFunction[Mut]` do not currently validate the number of arguments they are given before calling the function. I originally did this because I felt users would want to validate this themselves in the function rather than have it be done behind-the-scenes. I'm now realizing, however, that we could remove this boilerplate and if users wanted to check again then they would still be free to do so (it'd be more of a sanity check at that point). ## Solution Automatically validate the number of arguments passed to `DynamicFunction::call` and `DynamicFunctionMut::call[_once]`. This is a pretty trivial change since we just need to compare the length of the `ArgList` to the length of the `[ArgInfo]` in the function's `FunctionInfo`. I also ran the benchmarks just in case and saw no regression by doing this. ## Testing You can test locally by running: ``` cargo test --package bevy_reflect --all-features ``` |
||
|
Gino Valente
|
4d0961cc8a
|
bevy_reflect: Add ReflectRef/ReflectMut/ReflectOwned convenience casting methods (#15235)
# Objective #13320 added convenience methods for casting a `TypeInfo` into its respective variant: ```rust let info: &TypeInfo = <Vec<i32> as Typed>::type_info(); // We know `info` contains a `ListInfo`, so we can simply cast it: let list_info: &ListInfo = info.as_list().unwrap(); ``` This is especially helpful when you have already verified a type is a certain kind via `ReflectRef`, `ReflectMut`, `ReflectOwned`, or `ReflectKind`. As mentioned in that PR, though, it would be useful to add similar convenience methods to those types as well. ## Solution Added convenience casting methods to `ReflectRef`, `ReflectMut`, and `ReflectOwned`. With these methods, I was able to reduce our nesting in certain places throughout the crate. Additionally, I took this opportunity to move these types (and `ReflectKind`) to their own module to help clean up the `reflect` module. ## Testing You can test locally by running: ``` cargo test --package bevy_reflect --all-features ``` --- ## Showcase Convenience methods for casting `ReflectRef`, `ReflectMut`, and `ReflectOwned` into their respective variants has been added! This allows you to write cleaner code if you already know the kind of your reflected data: ```rust // BEFORE let ReflectRef::List(list) = list.reflect_ref() else { panic!("expected list"); }; // AFTER let list = list.reflect_ref().as_list().unwrap(); ``` --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> Co-authored-by: Pablo Reinhardt <126117294+pablo-lua@users.noreply.github.com> |
||
|
Gino Valente
|
59c0521690
|
bevy_reflect: Add Function trait (#15205)
# Objective While #13152 added function reflection, it didn't really make functions reflectable. Instead, it made it so that they can be called with reflected arguments and return reflected data. But functions themselves cannot be reflected. In other words, we can't go from `DynamicFunction` to `dyn PartialReflect`. ## Solution Allow `DynamicFunction` to actually be reflected. This PR adds the `Function` reflection subtrait (and corresponding `ReflectRef`, `ReflectKind`, etc.). With this new trait, we're able to implement `PartialReflect` on `DynamicFunction`. ### Implementors `Function` is currently only implemented for `DynamicFunction<'static>`. This is because we can't implement it generically over all functions—even those that implement `IntoFunction`. What about `DynamicFunctionMut`? Well, this PR does **not** implement `Function` for `DynamicFunctionMut`. The reasons for this are a little complicated, but it boils down to mutability. `DynamicFunctionMut` requires `&mut self` to be invoked since it wraps a `FnMut`. However, we can't really model this well with `Function`. And if we make `DynamicFunctionMut` wrap its internal `FnMut` in a `Mutex` to allow for `&self` invocations, then we run into either concurrency issues or recursion issues (or, in the worst case, both). So for the time-being, we won't implement `Function` for `DynamicFunctionMut`. It will be better to evaluate it on its own. And we may even consider the possibility of removing it altogether if it adds too much complexity to the crate. ### Dynamic vs Concrete One of the issues with `DynamicFunction` is the fact that it's both a dynamic representation (like `DynamicStruct` or `DynamicList`) and the only way to represent a function. Because of this, it's in a weird middle ground where we can't easily implement full-on `Reflect`. That would require `Typed`, but what static `TypeInfo` could it provide? Just that it's a `DynamicFunction`? None of the other dynamic types implement `Typed`. However, by not implementing `Reflect`, we lose the ability to downcast back to our `DynamicStruct`. Our only option is to call `Function::clone_dynamic`, which clones the data rather than by simply downcasting. This works in favor of the `PartialReflect::try_apply` implementation since it would have to clone anyways, but is definitely not ideal. This is also the reason I had to add `Debug` as a supertrait on `Function`. For now, this PR chooses not to implement `Reflect` for `DynamicFunction`. We may want to explore this in a followup PR (or even this one if people feel strongly that it's strictly required). The same is true for `FromReflect`. We may decide to add an implementation there as well, but it's likely out-of-scope of this PR. ## Testing You can test locally by running: ``` cargo test --package bevy_reflect --all-features ``` --- ## Showcase You can now pass around a `DynamicFunction` as a `dyn PartialReflect`! This also means you can use it as a field on a reflected type without having to ignore it (though you do need to opt out of `FromReflect`). ```rust #[derive(Reflect)] #[reflect(from_reflect = false)] struct ClickEvent { callback: DynamicFunction<'static>, } let event: Box<dyn Struct> = Box::new(ClickEvent { callback: (|| println!("Clicked!")).into_function(), }); // We can access our `DynamicFunction` as a `dyn PartialReflect` let callback: &dyn PartialReflect = event.field("callback").unwrap(); // And access function-related methods via the new `Function` trait let ReflectRef::Function(callback) = callback.reflect_ref() else { unreachable!() }; // Including calling the function callback.reflect_call(ArgList::new()).unwrap(); // Prints: Clicked! ``` |
||
|
Benjamin Brienen
|
02a9ed4b0b
|
move ShortName to bevy_reflect (#15340)
# Objective - Goal is to minimize bevy_utils #11478 ## Solution - Move the file short_name wholesale into bevy_reflect ## Testing - Unit tests - CI ## Migration Guide - References to `bevy_utils::ShortName` should instead now be `bevy_reflect::ShortName`. --------- Co-authored-by: François Mockers <francois.mockers@vleue.com> |
||
|
VitalyR
|
661ab1ab41
|
Fix warnings triggered by elided_named_lifetimes lint (#15328)
# Objective Eliminate some warnings introduced by the new rust lint [elided_named_lifetimes](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/builtin/static.ELIDED_NAMED_LIFETIMES.html), fix #15326. ## Solution - Add or remove lifetime markers to not trigger the lint. ## Testing - When the lint comes to stable, the CI will fail and this PR could fix that. |
||
|
Rich Churcher
|
fd329c0426
|
Allow to expect (adopted) (#15301)
# Objective > Rust 1.81 released the #[expect(...)] attribute, which works like #[allow(...)] but throws a warning if the lint isn't raised. This is preferred to #[allow(...)] because it tells us when it can be removed. - Adopts the parts of #15118 that are complete, and updates the branch so it can be merged. - There were a few conflicts, let me know if I misjudged any of 'em. Alice's [recommendation](https://github.com/bevyengine/bevy/issues/15059#issuecomment-2349263900) seems well-taken, let's do this crate by crate now that @BD103 has done the lion's share of this! (Relates to, but doesn't yet completely finish #15059.) Crates this _doesn't_ cover: - bevy_input - bevy_gilrs - bevy_window - bevy_winit - bevy_state - bevy_render - bevy_picking - bevy_core_pipeline - bevy_sprite - bevy_text - bevy_pbr - bevy_ui - bevy_gltf - bevy_gizmos - bevy_dev_tools - bevy_internal - bevy_dylib --------- Co-authored-by: BD103 <59022059+BD103@users.noreply.github.com> Co-authored-by: Ben Frankel <ben.frankel7@gmail.com> Co-authored-by: Antony <antony.m.3012@gmail.com> |
||
|
Gino Valente
|
ebb57c5511
|
bevy_reflect: Add FunctionRegistry::call (#15148)
# Objective There may be times where a function in the `FunctionRegistry` doesn't need to be fully retrieved. A user may just need to call it with a set of arguments. We should provide a shortcut for doing this. ## Solution Add the `FunctionRegistry::call` method to directly call a function in the registry with the given name and arguments. ## Testing You can test locally by running: ``` cargo test --package bevy_reflect --all-features ``` |
||
|
Zachary Harrold
|
fcfa60844a
|
Remove allocation in get_short_name (#15294)
`ShortName` is lazily evaluated and does not allocate, instead providing
`Display` and `Debug` implementations which write directly to a
formatter using the original algorithm. When using `ShortName` in format
strings (`panic`, `dbg`, `format`, etc.) you can directly use the
`ShortName` type. If you require a `String`, simply call
`ShortName(...).to_string()`.
# Objective
- Remove the requirement for allocation when using `get_short_name`
## Solution
- Added new type `ShortName` which wraps a name and provides its own
`Debug` and `Display` implementations, using the original
`get_short_name` algorithm without the need for allocating.
- Removed `get_short_name`, as `ShortName(...)` is more performant and
ergonomic.
- Added `ShortName::of::<T>` method to streamline the common use-case
for name shortening.
## Testing
- CI
## Migration Guide
### For `format!`, `dbg!`, `panic!`, etc.
```rust
// Before
panic!("{} is too short!", get_short_name(name));
// After
panic!("{} is too short!", ShortName(name));
```
### Need a `String` Value
```rust
// Before
let short: String = get_short_name(name);
// After
let short: String = ShortName(name).to_string();
```
## Notes
`ShortName` lazily evaluates, and directly writes to a formatter via
`Debug` and `Display`, which removes the need to allocate a `String`
when printing a shortened type name. Because the implementation has been
moved into the `fmt` method, repeated printing of the `ShortName` type
may be less performant than converting it into a `String`. However, no
instances of this are present in Bevy, and the user can get the original
behaviour by calling `.to_string()` at no extra cost.
---------
Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>
|
||
|
Gino Valente
|
69541462c5
|
bevy_reflect: Add Reflectable trait (#5772)
# Objective
When deriving `Reflect`, users will notice that their generic arguments
also need to implement `Reflect`:
```rust
#[derive(Reflect)]
struct Foo<T: Reflect> {
value: T
}
```
This works well for now. However, as we want to do more with `Reflect`,
these bounds might need to change. For example, to get #4154 working, we
likely need to enforce the `GetTypeRegistration` trait. So now we have:
```rust
#[derive(Reflect)]
struct Foo<T: Reflect + GetTypeRegistration> {
value: T
}
```
Not great, but not horrible. However, we might then want to do something
as suggested in
[this](https://github.com/bevyengine/bevy/issues/5745#issuecomment-1221389131)
comment and add a `ReflectTypeName` trait for stable type name support.
Well now we have:
```rust
#[derive(Reflect)]
struct Foo<T: Reflect + GetTypeRegistration + ReflectTypeName> {
value: T
}
```
Now imagine that for even two or three generic types. Yikes!
As the API changes it would be nice if users didn't need to manually
migrate their generic type bounds like this.
A lot of these traits are (or will/might be) core to the entire
reflection API. And although `Reflect` can't add them as supertraits for
object-safety reasons, they are still indirectly required for things to
function properly (manual implementors will know how easy it is to
forget to implement `GetTypeRegistration`). And they should all be
automatically implemented for user types anyways as long they're using
`#[derive(Reflect)]`.
## Solution
Add a "catch-all" trait called `Reflectable` whose supertraits are a
select handful of core reflection traits.
This allows us to consolidate all the examples above into this:
```rust
#[derive(Reflect)]
struct Foo<T: Reflectable> {
value: T
}
```
And as we experiment with the API, users can rest easy knowing they
don't need to migrate dozens upon dozens of types. It should all be
automatic!
## Discussion
1. Thoughts on the name `Reflectable`? Is it too easily confused with
`Reflect`? Or does it at least accurately describe that this contains
the core traits? If not, maybe `BaseReflect`?
---
## Changelog
- Added the `Reflectable` trait
---------
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
|
||
|
Wybe Westra
|
612731edfb
|
Add DynamicTyped link to TypeInfo docs (#15188) (#15259)
Also added a bit to the paragraph to explain when to use the new function. Fixes #15188. |
||
|
Al M.
|
2ea51fc60f
|
Use FromReflect when extracting entities in dynamic scenes (#15174)
# Objective Fix #10284. ## Solution When `DynamicSceneBuilder` extracts entities, they are cloned via `PartialReflect::clone_value`, making them into dynamic versions of the original components. This loses any custom `ReflectSerialize` type data. Dynamic scenes are deserialized with the original types, not the dynamic versions, and so any component with a custom serialize may fail. In this case `Rect` and `Vec2`. The dynamic version includes the field names 'x' and 'y' but the `Serialize` impl doesn't, hence the "expect float" error. The solution here: Instead of using `clone_value` to clone the components, `FromReflect` clones and retains the original information needed to serialize with any custom `Serialize` impls. I think using something like `reflect_clone` from (https://github.com/bevyengine/bevy/pull/13432) might make this more efficient. I also did the same when deserializing dynamic scenes to appease some of the round-trip tests which use `ReflectPartialEq`, which requires the types be the same and not a unique/proxy pair. I'm not sure it's otherwise necessary. Maybe this would also be more efficient when spawning dynamic scenes with `reflect_clone` instead of `FromReflect` again? An alternative solution would be to fall back to the dynamic version when deserializing `DynamicScene`s if the custom version fails. I think that's possible. Or maybe simply always deserializing via the dynamic route for dynamic scenes? ## Testing This example is similar to the original test case in #10284: ``` rust #![allow(missing_docs)] use bevy::{prelude::*, scene::SceneInstanceReady}; fn main() { App::new() .add_plugins(DefaultPlugins) .add_systems(Startup, (save, load).chain()) .observe(check) .run(); } static SAVEGAME_SAVE_PATH: &str = "savegame.scn.ron"; fn save(world: &mut World) { let entity = world.spawn(OrthographicProjection::default()).id(); let scene = DynamicSceneBuilder::from_world(world) .extract_entity(entity) .build(); if let Some(registry) = world.get_resource::<AppTypeRegistry>() { let registry = registry.read(); let serialized_scene = scene.serialize(®istry).unwrap(); // println!("{}", serialized_scene); std::fs::write(format!("assets/{SAVEGAME_SAVE_PATH}"), serialized_scene).unwrap(); } world.entity_mut(entity).despawn_recursive(); } fn load(mut commands: Commands, asset_server: Res<AssetServer>) { commands.spawn(DynamicSceneBundle { scene: asset_server.load(SAVEGAME_SAVE_PATH), ..default() }); } fn check(_trigger: Trigger<SceneInstanceReady>, query: Query<&OrthographicProjection>) { dbg!(query.single()); } ``` ## Migration Guide The `DynamicScene` format is changed to use custom serialize impls so old scene files will need updating: Old: ```ron ( resources: {}, entities: { 4294967299: ( components: { "bevy_render:📷:projection::OrthographicProjection": ( near: 0.0, far: 1000.0, viewport_origin: ( x: 0.5, y: 0.5, ), scaling_mode: WindowSize(1.0), scale: 1.0, area: ( min: ( x: -1.0, y: -1.0, ), max: ( x: 1.0, y: 1.0, ), ), ), }, ), }, ) ``` New: ```ron ( resources: {}, entities: { 4294967299: ( components: { "bevy_render:📷:projection::OrthographicProjection": ( near: 0.0, far: 1000.0, viewport_origin: (0.5, 0.5), scaling_mode: WindowSize(1.0), scale: 1.0, area: ( min: (-1.0, -1.0), max: (1.0, 1.0), ), ), }, ), }, ) ``` --------- Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com> |
||
|
Cole Varner
|
cf55e6cb22
|
ParsedPath::try_from<&str> (#15180)
# Objective - implements ParsedPath::try_from<&str> - resolves #14438 ## Testing - Added unit test for ParsedPath::try_from<&str> Note: I don't claim to be an expert on lifetimes! That said I think it makes sense that the error shares a lifetime with input string as deeper down it is used to construct it. |
||
|
Gino Valente
|
37443e0f3f
|
bevy_reflect: Add DynamicTyped trait (#15108)
# Objective Thanks to #7207, we now have a way to validate at the type-level that a reflected value is actually the type it says it is and not just a dynamic representation of that type. `dyn PartialReflect` values _might_ be a dynamic type, but `dyn Reflect` values are guaranteed to _not_ be a dynamic type. Therefore, we can start to add methods to `Reflect` that weren't really possible before. For example, we should now be able to always get a `&'static TypeInfo`, and not just an `Option<&'static TypeInfo>`. ## Solution Add the `DynamicTyped` trait. This trait is similar to `DynamicTypePath` in that it provides a way to use the non-object-safe `Typed` trait in an object-safe way. And since all types that derive `Reflect` will also derive `Typed`, we can safely add `DynamicTyped` as a supertrait of `Reflect`. This allows us to use it when just given a `dyn Reflect` trait object. ## Testing You can test locally by running: ``` cargo test --package bevy_reflect ``` --- ## Showcase `Reflect` now has a supertrait of `DynamicTyped`, allowing `TypeInfo` to be retrieved from a `dyn Reflect` trait object without having to unwrap anything! ```rust let value: Box<dyn Reflect> = Box::new(String::from("Hello!")); // BEFORE let info: &'static TypeInfo = value.get_represented_type_info().unwrap(); // AFTER let info: &'static TypeInfo = value.reflect_type_info(); ``` ## Migration Guide `Reflect` now has a supertrait of `DynamicTyped`. If you were manually implementing `Reflect` and did not implement `Typed`, you will now need to do so. |
||
|
mamekoro
|
15e246eff8
|
Fix typo in bevy_reflect/src/reflect.rs (#15157)
Corrected a typo "enumuration" to "enumeration". |
||
|
Gino Valente
|
75343ef584
|
bevy_reflect: Mention FunctionRegistry in bevy_reflect::func docs (#15147)
# Objective The module docs for `bevy_reflect::func` don't mention the `FunctionRegistry`. ## Solution Add a section about the `FunctionRegistry` to the module-level documentation. ## Testing You can test locally by running: ``` cargo test --doc --package bevy_reflect --all-features ``` |
||
|
Gino Valente
|
90bb1adeb2
|
bevy_reflect: Contextual serialization error messages (#13888)
# Objective Reflection serialization can be difficult to debug. A lot of times a type fails to be serialized and the user is left wondering where that type came from. This is most often encountered with Bevy's scenes. Attempting to serialize all resources in the world will fail because some resources can't be serialized. For example, users will often get complaints about `bevy_utils::Instant` not registering `ReflectSerialize`. Well, `Instant` can't be serialized, so the only other option is to exclude the resource that contains it. But what resource contains it? This is where reflection serialization can get a little tricky (it's `Time<Real>` btw). ## Solution Add the `debug_stack` feature to `bevy_reflect`. When enabled, the reflection serializers and deserializers will keep track of the current type stack. And this stack will be used in error messages to help with debugging. Now, if we unknowingly try to serialize `Time<Real>`, we'll get the following error: ``` type `bevy_utils::Instant` did not register the `ReflectSerialize` type data. For certain types, this may need to be registered manually using `register_type_data` (stack: `bevy_time::time::Time<bevy_time::real::Real>` -> `bevy_time::real::Real` -> `bevy_utils::Instant`) ``` ### Implementation This makes use of `thread_local!` to manage an internal `TypeInfoStack` which holds a stack of `&'static TypeInfo`. We push to the stack before a type is (de)serialized and pop from the stack afterwards. Using a thread-local should be fine since we know two (de)serializers can't be running at the same time (and if they're running on separate threads, then we're still good). The only potential issue would be if a user went through one of the sub-serializers, like `StructSerializer`. However, I don't think many users are going through these types (I don't even know if we necessarily want to keep those public either, but we'll save that for a different PR). Additionally, this is just a debug feature that only affects error messages, so it wouldn't have any drastically negative effect. It would just result in the stack not being cleared properly if there were any errors. Lastly, this is not the most performant implementation since we now fetch the `TypeInfo` an extra time. But I figured that for a debug tool, it wouldn't matter too much. ### Feature This also adds a `debug` feature, which enables the `debug_stack` feature. I added it because I think we may want to potentially add more debug tools in the future, and this gives us a good framework for adding those. Users who want all debug features, present and future, can just set `debug`. If they only want this feature, then they can just use `debug_stack`. I also made the `debug` feature default to help capture the widest audience (i.e. the users who want this feature but don't know they do). However, if we think it's better as a non-default feature, I can change it! And if there's any bikeshedding around the name `debug_stack`, let me know! ## Testing Run the following command: ``` cargo test --package bevy_reflect --features debug_stack ``` --- ## Changelog - Added the `debug` and `debug_stack` features to `bevy_reflect` - Updated the error messages returned by the reflection serializers and deserializers to include more contextual information when the `debug_stack` or `debug` feature is enabled |
||
|
Gino Valente
|
245d03a78a
|
bevy_reflect: Update on_unimplemented attributes (#15110)
# Objective
Some of the new compile error messages are a little unclear (at least to
me). For example:
```
error[E0277]: `tests::foo::Bar` can not be created through reflection
--> crates/bevy_reflect/src/lib.rs:679:18
|
679 | #[derive(Reflect)]
| ^^^^^^^ the trait `from_reflect::FromReflect` is not implemented for `tests::foo::Bar`
|
= note: consider annotating `tests::foo::Bar` with `#[derive(Reflect)]` or `#[derive(FromReflect)]`
```
While the annotation makes it clear that `FromReflect` is missing, it's
not very clear from the main error message.
My IDE lists errors with only their message immediately present:
<p align="center">
<img width="700" alt="Image of said IDE listing errors with only their
message immediately present. These errors are as follows:
\"`tests::foo::Bar` can not be created through reflection\", \"The trait
bound `tests::foo::Bar: RegisterForReflection` is not satisfied\", and
\"The trait bound `tests::foo::Bar: type_info::MaybeTyped` is not
satisfied\""
src="https://github.com/user-attachments/assets/42c24051-9e8e-4555-8477-51a9407446aa">
</p>
This makes it hard to tell at a glance why my code isn't compiling.
## Solution
Updated all `on_unimplemented` attributes in `bevy_reflect` to mention
the relevant trait—either the actual trait or the one users actually
need to implement—as well as a small snippet of what not implementing
them means.
For example, failing to implement `TypePath` now mentions missing a
`TypePath` implementation. And failing to implement `DynamicTypePath`
now also mentions missing a `TypePath` implementation, since that's the
actual trait users need to implement (i.e. they shouldn't implement
`DynamicTypePath` directly).
Lastly, I also added some missing `on_unimplemented` attributes for
`MaybeTyped` and `RegisterForReflection` (which you can see in the image
above).
Here's how this looks in my IDE now:
<p align="center">
<img width="700" alt="Similar image as before showing the errors listed
by the IDE. This time the errors read as follows: \"`tests::foo::Bar`
does not implement `FromReflect` so cannot be reified through
reflection\", \"`tests::foo::Bar` does not implement
`GetTypeRegistration` so cannot be registered for reflection\", and
\"`tests::foo::Bar` does not implement `Typed` so cannot provide static
type information\""
src="https://github.com/user-attachments/assets/f6f8501f-0450-4f78-b84f-00e7a18d0533">
</p>
## Testing
You can test by adding the following code and verifying the compile
errors are correct:
```rust
#[derive(Reflect)]
struct Foo(Bar);
struct Bar;
```
|
||
|
Christian Hughes
|
e939d6c33f
|
Remove remnant EntityHash and related types from bevy_utils (#15039)
# Objective `EntityHash` and related types were moved from `bevy_utils` to `bevy_ecs` in #11498, but seemed to have been accidentally reintroduced a week later in #11707. ## Solution Remove the old leftover code. --- ## Migration Guide - Uses of `bevy::utils::{EntityHash, EntityHasher, EntityHashMap, EntityHashSet}` now have to be imported from `bevy::ecs::entity`. |
||
|
Gino Valente
|
ba3d9b3fb6
|
bevy_reflect: Refactor serde module (#15107)
# Objective
The `ser` and `de` modules in `bevy_reflect/serde` are very long and
difficult to navigate.
## Solution
Refactor both modules into many smaller modules that each have a single
primary focus (i.e. a `structs` module that only handles struct
serialization/deserialization).
I chose to keep the `ser` and `de` modules separate. We could have
instead broken it up kind (e.g. lists, maps, etc.), but I think this is
a little cleaner. Serialization and deserialization, while related, can
be very different. So keeping them separated makes sense for
organizational purposes.
That being said, if people disagree and think we should structure this a
different way, I am open to changing it.
Note that this PR's changes are mainly structural. There are a few
places I refactored code to reduce duplication and to make things a bit
cleaner, but these are largely cosmetic and shouldn't have any impact on
behavior.
### Other Details
This PR also hides a lot of the internal logic from being exported.
These were originally public, but it's unlikely they really saw any use
outside of these modules. In fact, you don't really gain anything by
using them outside of this module either.
By privatizing these fields and items, we also set ourselves up for more
easily changing internal logic around without involving a breaking
change.
I also chose not to mess around with tests since that would really blow
up the diff haha.
## Testing
You can test locally by running:
```
cargo test --package bevy_reflect --all-features
```
---
## Migration Guide
The fields on `ReflectSerializer` and `TypedReflectSerializer` are now
private. To instantiate, the corresponding constructor must be used:
```rust
// BEFORE
let serializer = ReflectSerializer {
value: &my_value,
registry: &type_registry,
};
// AFTER
let serializer = ReflectSerializer::new(&my_value, &type_registry);
```
Additionally, the following types are no longer public:
- `ArraySerializer`
- `EnumSerializer`
- `ListSerializer`
- `MapSerializer`
- `ReflectValueSerializer` (fully removed)
- `StructSerializer`
- `TupleSerializer`
- `TupleStructSerializer`
As well as the following traits:
- `DeserializeValue` (fully removed)
|
||
|
BD103
|
6ec6a55645
|
Unify crate-level preludes (#15080)
# Objective
- Crate-level prelude modules, such as `bevy_ecs::prelude`, are plagued
with inconsistency! Let's fix it!
## Solution
Format all preludes based on the following rules:
1. All preludes should have brief documentation in the format of:
> The _name_ prelude.
>
> This includes the most common types in this crate, re-exported for
your convenience.
2. All documentation should be outer, not inner. (`///` instead of
`//!`.)
3. No prelude modules should be annotated with `#[doc(hidden)]`. (Items
within them may, though I'm not sure why this was done.)
## Testing
- I manually searched for the term `mod prelude` and updated all
occurrences by hand. 🫠
---------
Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>
|
||
|
Zachary Harrold
|
547b1c7a7a
|
Reflect SmolStr's De/Serialize implementation (#14982)
# Objective - Fixes #14969 ## Solution - Added `Deserialize` to the list of reflected traits for `SmolStr` ## Testing - CI passed locally. --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> |
||
|
Zachary Harrold
|
bc13161416
|
Migrated NonZero* to NonZero<*> (#14978)
# Objective - Fixes #14974 ## Solution - Replace all* instances of `NonZero*` with `NonZero<*>` ## Testing - CI passed locally. --- ## Notes Within the `bevy_reflect` implementations for `std` types, `impl_reflect_value!()` will continue to use the type aliases instead, as it inappropriately parses the concrete type parameter as a generic argument. If the `ZeroablePrimitive` trait was stable, or the macro could be modified to accept a finite list of types, then we could fully migrate. |
||
|
Zachary Harrold
|
44620dd6ae
|
Split GenericTypeCell::get_or_insert into smaller pieces (#14865)
# Objective Based on the discussion in #14864, I wanted to experiment with the core `GenericTypeCell` type, whose `get_or_insert` method accounted for 2% of the final binary size of the `3d_scene` example. The reason for this large percentage is likely because the type is fundamental to the rest of Bevy while having 3 generic parameters (the type stored `T`, the type to retrieve `G`, and the function used to insert a new value `F`). - Acts on #14864 ## Solution - Split `get_or_insert` into smaller functions with minimised parameterisation. These new functions are private as to preserve the public facing API, but could be exposed if desired. ## Testing - Ran CI locally. - Used `cargo bloat --release --example 3d_scene -n 100000 --message-format json > out.json` and @cart's [bloat analyzer](https://gist.github.com/cart/722756ba3da0e983d207633e0a48a8ab) to measure a 428KiB reduction in binary size when compiling on Windows 10. - ~I have _not_ benchmarked to determine if this improves/hurts performance.~ See [below](https://github.com/bevyengine/bevy/pull/14865#issuecomment-2306083606). ## Notes In my opinion this seems like a good test-case for the concept of debloating generics within the Bevy codebase. I believe the performance impact here is negligible in either direction (at runtime and compile time), but the binary reduction is measurable and quite significant for a relatively minor change in code. --------- Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com> |
||
|
Gino Valente
|
3892adcb47
|
bevy_reflect: Add Type type (#14838)
# Objective Closes #7622. I was working on adding support for reflecting generic functions and found that I wanted to use an argument's `TypeId` for hashing and comparison, but its `TypePath` for debugging and error messaging. While I could just keep them separate, place them in a tuple or a local struct or something, I think I see an opportunity to make a dedicate type for this. Additionally, we can use this type to clean up some duplication amongst the type info structs in a manner similar to #7622. ## Solution Added the `Type` type. This should be seen as the most basic representation of a type apart from `TypeId`. It stores both the `TypeId` of the type as well as its `TypePathTable`. The `Hash` and `PartialEq` implementations rely on the `TypeId`, while the `Debug` implementation relies on the `TypePath`. This makes it especially useful as a key in a `HashMap` since we get the speed of the `TypeId` hashing/comparisons with the readability of `TypePath`. With this type, we're able to reduce the duplication across the type info structs by removing individual fields for `TypeId` and `TypePathTable`, replacing them with a single `Type` field. Similarly, we can remove many duplicate methods and replace it with a macro that delegates to the stored `Type`. ### Caveats It should be noted that this type is currently 3x larger than `TypeId`. On my machine, it's 48 bytes compared to `TypeId`'s 16. While this doesn't matter for `TypeInfo` since it would contain that data regardless, it is something to keep in mind when using elsewhere. ## Testing All tests should pass as normal: ``` cargo test --package bevy_reflect ``` --- ## Showcase `bevy_reflect` now exports a `Type` struct. This type contains both the `TypeId` and the `TypePathTable` of the given type, allowing it to be used like `TypeId` but have the debuggability of `TypePath`. ```rust // We can create this for any type implementing `TypePath`: let ty = Type::of::<String>(); // It has `Hash` and `Eq` impls powered by `TypeId`, making it useful for maps: let mut map = HashMap::<Type, i32>::new(); map.insert(ty, 25); // And it has a human-readable `Debug` representation: let debug = format!("{:?}", map); assert_eq!(debug, "{alloc::string::String: 25}"); ``` ## Migration Guide Certain type info structs now only return their item types as `Type` instead of exposing direct methods on them. The following methods have been removed: - `ArrayInfo::item_type_path_table` - `ArrayInfo::item_type_id` - `ArrayInfo::item_is` - `ListInfo::item_type_path_table` - `ListInfo::item_type_id` - `ListInfo::item_is` - `SetInfo::value_type_path_table` - `SetInfo::value_type_id` - `SetInfo::value_is` - `MapInfo::key_type_path_table` - `MapInfo::key_type_id` - `MapInfo::key_is` - `MapInfo::value_type_path_table` - `MapInfo::value_type_id` - `MapInfo::value_is` Instead, access the `Type` directly using one of the new methods: - `ArrayInfo::item_ty` - `ListInfo::item_ty` - `SetInfo::value_ty` - `MapInfo::key_ty` - `MapInfo::value_ty` For example: ```rust // BEFORE let type_id = array_info.item_type_id(); // AFTER let type_id = array_info.item_ty().id(); ``` |
||
|
Lubba64
|
f9fbd08f9f
|
Implement Reflect for std::ops::Bound (#14861)
# Objective - Fixes #14844 ## Solution - implement reflect using the `impl_reflect_value` macro ## Testing - I wrote a test locally to understand and learn how reflection worked on a basic level and to confirm that yes indeed the bound struct could use the reflection traits that have been implemented for it. note: I did remove a line that asked for bound to not have reflect implemented in a test, since that's the point of this PR and the test worked without the line so I am not sure what that was about, not sure if that uncovers a deeper issue or not. |
||
|
EdJoPaTo
|
938d810766
|
Apply unused_qualifications lint (#14828)
# Objective Fixes #14782 ## Solution Enable the lint and fix all upcoming hints (`--fix`). Also tried to figure out the false-positive (see review comment). Maybe split this PR up into multiple parts where only the last one enables the lint, so some can already be merged resulting in less many files touched / less potential for merge conflicts? Currently, there are some cases where it might be easier to read the code with the qualifier, so perhaps remove the import of it and adapt its cases? In the current stage it's just a plain adoption of the suggestions in order to have a base to discuss. ## Testing `cargo clippy` and `cargo run -p ci` are happy. |
||
|
Gino Valente
|
2b4180ca8f
|
bevy_reflect: Function reflection terminology refactor (#14813)
# Objective One of the changes in #14704 made `DynamicFunction` effectively the same as `DynamicClosure<'static>`. This change meant that the de facto function type would likely be `DynamicClosure<'static>` instead of the intended `DynamicFunction`, since the former is much more flexible. We _could_ explore ways of making `DynamicFunction` implement `Copy` using some unsafe code, but it likely wouldn't be worth it. And users would likely still reach for the convenience of `DynamicClosure<'static>` over the copy-ability of `DynamicFunction`. The goal of this PR is to fix this confusion between the two types. ## Solution Firstly, the `DynamicFunction` type was removed. Again, it was no different than `DynamicClosure<'static>` so it wasn't a huge deal to remove. Secondly, `DynamicClosure<'env>` and `DynamicClosureMut<'env>` were renamed to `DynamicFunction<'env>` and `DynamicFunctionMut<'env>`, respectively. Yes, we still ultimately kept the naming of `DynamicFunction`, but changed its behavior to that of `DynamicClosure<'env>`. We need a term to refer to both functions and closures, and "function" was the best option. [Originally](https://discord.com/channels/691052431525675048/1002362493634629796/1274091992162242710), I was going to go with "callable" as the replacement term to encompass both functions and closures (e.g. `DynamciCallable<'env>`). However, it was [suggested](https://discord.com/channels/691052431525675048/1002362493634629796/1274653581777047625) by @SkiFire13 that the simpler "function" term could be used instead. While "callable" is perhaps the better umbrella term—being truly ambiguous over functions and closures— "function" is more familiar, used more often, easier to discover, and is subjectively just "better-sounding". ## Testing Most changes are purely swapping type names or updating documentation, but you can verify everything still works by running the following command: ``` cargo test --package bevy_reflect ``` |
||
|
Gino Valente
|
423285cf1c
|
bevy_reflect: Store functions as DynamicClosure<'static> in FunctionRegistry (#14704)
# Objective #14098 added the `FunctionRegistry` for registering functions such that they can be retrieved by name and used dynamically. One thing we chose to leave out in that initial PR is support for closures. Why support closures? Mainly, we don't want to prohibit users from injecting environmental data into their registered functions. This allows these functions to not leak their internals to the public API. For example, let's say we're writing a library crate that allows users to register callbacks for certain actions. We want to perform some actions before invoking the user's callback so we can't just call it directly. We need a closure for this: ```rust registry.register("my_lib::onclick", move |event: ClickEvent| { // ...other work... user_onclick.call(event); // <-- Captured variable }); ``` We could have made our callback take a reference to the user's callback. This would remove the need for the closure, but it would change our desired API to place the burden of fetching the correct callback on the caller. ## Solution Modify the `FunctionRegistry` to store registered functions as `DynamicClosure<'static>` instead of `DynamicFunction` (now using `IntoClosure` instead of `IntoFunction`). Due to limitations in Rust and how function reflection works, `DynamicClosure<'static>` is functionally equivalent to `DynamicFunction`. And a normal function is considered a subset of closures (it's a closure that doesn't capture anything), so there shouldn't be any difference in usage: all functions that satisfy `IntoFunction` should satisfy `IntoClosure`. This means that the registration API introduced in #14098 should require little-to-no changes on anyone following `main`. ### Closures vs Functions One consideration here is whether we should keep closures and functions separate. This PR unifies them into `DynamicClosure<'static>`, but we can consider splitting them up. The reasons we might want to do so are: - Simplifies mental model and terminology (users don't have to understand that functions turn into closures) - If Rust ever improves its function model, we may be able to add additional guarantees to `DynamicFunction` that make it useful to separate the two - Adding support for generic functions may be less confusing for users since closures in Rust technically can't be generic The reasons behind this PR's unification approach are: - Reduces the number of methods needed on `FunctionRegistry` - Reduces the number of lookups a user may have to perform (i.e. "`get_function` or else `get_closure`") - Establishes `DynamicClosure<'static>` as the de facto dynamic callable (similar to how most APIs in Rust code tend to prefer `impl Fn() -> String` over `fn() -> String`) I'd love to hear feedback on this matter, and whether we should continue with this PR's approach or switch to a split model. ## Testing You can test locally by running: ``` cargo test --package bevy_reflect ``` --- ## Showcase Closures can now be registered into the `FunctionRegistry`: ```rust let punct = String::from("!!!"); registry.register_with_name("my_crate::punctuate", move |text: String| { format!("{}{}", text, punct) }); ``` |
||
|
eckz
|
a44278aee6
|
Making DynamicEnum::is_dynamic() return true (#14732)
# Objective - Right now `DynamicEnum::is_dynamic()` is returning `false`. I don't think this was expected, since the rest of `Dynamic*` types return `true`. ## Solution - Making `DynamicEnum::is_dynamic()` return true ## Testing - Added an extra unit test to verify that `.is_dynamic()` returns `true`. |
||
|
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 |
||
|
Tau Gärtli
|
aab1f8e435
|
Use #[doc(fake_variadic)] to improve docs readability (#14703)
# Objective - Fixes #14697 ## Solution This PR modifies the existing `all_tuples!` macro to optionally accept a `#[doc(fake_variadic)]` attribute in its input. If the attribute is present, each invocation of the impl macro gets the correct attributes (i.e. the first impl receives `#[doc(fake_variadic)]` while the other impls are hidden using `#[doc(hidden)]`. Impls for the empty tuple (unit type) are left untouched (that's what the [standard library](https://doc.rust-lang.org/std/cmp/trait.PartialEq.html#impl-PartialEq-for-()) and [serde](https://docs.rs/serde/latest/serde/trait.Serialize.html#impl-Serialize-for-()) do). To work around https://github.com/rust-lang/cargo/issues/8811 and to get impls on re-exports to correctly show up as variadic, `--cfg docsrs_dep` is passed when building the docs for the toplevel `bevy` crate. `#[doc(fake_variadic)]` only works on tuples and fn pointers, so impls for structs like `AnyOf<(T1, T2, ..., Tn)>` are unchanged. ## Testing I built the docs locally using `RUSTDOCFLAGS='--cfg docsrs' RUSTFLAGS='--cfg docsrs_dep' cargo +nightly doc --no-deps --workspace` and checked the documentation page of a trait both in its original crate and the re-exported version in `bevy`. The description should correctly mention for how many tuple items the trait is implemented. I added `rustc-args` for docs.rs to the `bevy` crate, I hope there aren't any other notable crates that re-export `#[doc(fake_variadic)]` traits. --- ## Showcase `bevy_ecs::query::QueryData`: <img width="1015" alt="Screenshot 2024-08-12 at 16 41 28" src="https://github.com/user-attachments/assets/d40136ed-6731-475f-91a0-9df255cd24e3"> `bevy::ecs::query::QueryData` (re-export): <img width="1005" alt="Screenshot 2024-08-12 at 16 42 57" src="https://github.com/user-attachments/assets/71d44cf0-0ab0-48b0-9a51-5ce332594e12"> ## Original Description <details> Resolves #14697 Submitting as a draft for now, very WIP. Unfortunately, the docs don't show the variadics nicely when looking at reexported items. For example: `bevy_ecs::bundle::Bundle` correctly shows the variadic impl:  while `bevy::ecs::bundle::Bundle` (the reexport) shows all the impls (not good):  Built using `RUSTDOCFLAGS='--cfg docsrs' cargo +nightly doc --workspace --no-deps` (`--no-deps` because of wgpu-core). Maybe I missed something or this is a limitation in the *totally not private* `#[doc(fake_variadic)]` thingy. In any case I desperately need some sleep now :)) </details> |
||
|
radiish
|
6ab8767d3b
|
reflect: implement the unique reflect rfc (#7207)
# Objective
- Implements the [Unique Reflect
RFC](https://github.com/nicopap/rfcs/blob/bevy-reflect-api/rfcs/56-better-reflect.md).
## Solution
- Implements the RFC.
- This implementation differs in some ways from the RFC:
- In the RFC, it was suggested `Reflect: Any` but `PartialReflect:
?Any`. During initial implementation I tried this, but we assume the
`PartialReflect: 'static` in a lot of places and the changes required
crept out of the scope of this PR.
- `PartialReflect::try_into_reflect` originally returned `Option<Box<dyn
Reflect>>` but i changed this to `Result<Box<dyn Reflect>, Box<dyn
PartialReflect>>` since the method takes by value and otherwise there
would be no way to recover the type. `as_full` and `as_full_mut` both
still return `Option<&(mut) dyn Reflect>`.
---
## Changelog
- Added `PartialReflect`.
- `Reflect` is now a subtrait of `PartialReflect`.
- Moved most methods on `Reflect` to the new `PartialReflect`.
- Added `PartialReflect::{as_partial_reflect, as_partial_reflect_mut,
into_partial_reflect}`.
- Added `PartialReflect::{try_as_reflect, try_as_reflect_mut,
try_into_reflect}`.
- Added `<dyn PartialReflect>::{try_downcast_ref, try_downcast_mut,
try_downcast, try_take}` supplementing the methods on `dyn Reflect`.
## Migration Guide
- Most instances of `dyn Reflect` should be changed to `dyn
PartialReflect` which is less restrictive, however trait bounds should
generally stay as `T: Reflect`.
- The new `PartialReflect::{as_partial_reflect, as_partial_reflect_mut,
into_partial_reflect, try_as_reflect, try_as_reflect_mut,
try_into_reflect}` methods as well as `Reflect::{as_reflect,
as_reflect_mut, into_reflect}` will need to be implemented for manual
implementors of `Reflect`.
## Future Work
- This PR is designed to be followed up by another "Unique Reflect Phase
2" that addresses the following points:
- Investigate making serialization revolve around `Reflect` instead of
`PartialReflect`.
- [Remove the `try_*` methods on `dyn PartialReflect` since they are
stop
gaps](https://github.com/bevyengine/bevy/pull/7207#discussion_r1083476050).
- Investigate usages like `ReflectComponent`. In the places they
currently use `PartialReflect`, should they be changed to use `Reflect`?
- Merging this opens the door to lots of reflection features we haven't
been able to implement.
- We could re-add [the `Reflectable`
trait](
|
||
|
Gino Valente
|
297c0a3954
|
bevy_reflect: Add DynamicSet to dynamic_types example (#14665)
# Objective The `dynamic_types` example was missing a reference to the newly added `DynamicSet` type. ## Solution Add `DynamicSet` to the `dynamic_types` example. For parity with the other dynamic types, I also implemented `FromIterator<T: Reflect>`, `FromIterator<Box<dyn Reflect>>`, and `IntoIterator for &DynamicSet`. ## Testing You can run the example locally: ``` cargo run --example dynamic_types ``` |
||
|
Gino Valente
|
aeef1c0f20
|
bevy_reflect: Update internal docs regarding anonymous function type names (#14666)
# Objective As pointed out by @SkiFire13 on [Discord](https://discord.com/channels/691052431525675048/1002362493634629796/1270624366119485441), I was incorrect in #14641 regarding the type name of anonymous functions. I had stated that they will return something like `fn(i32, i32) -> i32`, but this is wrong. They actually behave like closures (despite not technically being closures) and return something more like `foo::bar::{{closure}}`. This isn't a major issue because the reasoning behind #14641 still stands. However, the internal documentation should probably be updated so future contributors don't believe the lies I left behind. ## Solution Updated the internal documentation for `create_info` to reflect the actual type name of an anonymous function. In that same module, I also added a test for function pointers and updated all tests to include sanity checks for the `std::any::type_name` of each category of callable. ## Testing You can test locally by running: ``` cargo test --package bevy_reflect ``` |
||
|
Gino Valente
|
a0cc636ea3
|
bevy_reflect: Anonymous function parsing (#14641)
# Objective ### TL;DR #14098 added the `FunctionRegistry` but had some last minute complications due to anonymous functions. It ended up going with a "required name" approach to ensure anonymous functions would always have a name. However, this approach isn't ideal for named functions since, by definition, they will always have a name. Therefore, this PR aims to modify function reflection such that we can make function registration easier for named functions, while still allowing anonymous functions to be registered as well. ### Context Function registration (#14098) ran into a little problem: anonymous functions. Anonymous functions, including function pointers, have very non-unique type names. For example, the anonymous function `|a: i32, b: i32| a + b` has the type name of `fn(i32, i32) -> i32`. This obviously means we'd conflict with another function like `|a: i32, b: i32| a - b`. The solution that #14098 landed on was to always require a name during function registration. The downside with this is that named functions (e.g. `fn add(a: i32, b: i32) -> i32 { a + b }`) had to redundantly provide a name. Additionally, manually constructed `DynamicFunction`s also ran into this ergonomics issue. I don't entirely know how the function registry will be used, but I have a strong suspicion that most of its registrations will either be named functions or manually constructed `DynamicFunction`s, with anonymous functions only being used here and there for quick prototyping or adding small functionality. Why then should the API prioritize the anonymous function use case by always requiring a name during registration? #### Telling Functions Apart Rust doesn't provide a lot of out-of-the-box tools for reflecting functions. One of the biggest hurdles in attempting to solve the problem outlined above would be to somehow tell the different kinds of functions apart. Let's briefly recap on the categories of functions in Rust: | Category | Example | | ------------------ | ----------------------------------------- | | Named function | `fn add(a: i32, b: i32) -> i32 { a + b }` | | Closure | `\|a: i32\| a + captured_variable` | | Anonymous function | `\|a: i32, b: i32\| a + b` | | Function pointer | `fn(i32, i32) -> i32` | My first thought was to try and differentiate these categories based on their size. However, we can see that this doesn't quite work: | Category | `size_of` | | ------------------ | --------- | | Named function | 0 | | Closure | 0+ | | Anonymous function | 0 | | Function pointer | 8 | Not only does this not tell anonymous functions from named ones, but it struggles with pretty much all of them. My second then was to differentiate based on type name: | Category | `type_name` | | ------------------ | ----------------------- | | Named function | `foo::bar::baz` | | Closure | `foo::bar::{{closure}}` | | Anonymous function | `fn() -> String` | | Function pointer | `fn() -> String` | This is much better. While it can't distinguish between function pointers and anonymous functions, this doesn't matter too much since we only care about whether we can _name_ the function. So why didn't we implement this in #14098? #### Relying on `type_name` While this solution was known about while working on #14098, it was left out from that PR due to it being potentially controversial. The [docs](https://doc.rust-lang.org/stable/std/any/fn.type_name.html) for `std::any::type_name` state: > The returned string must not be considered to be a unique identifier of a type as multiple types may map to the same type name. Similarly, there is no guarantee that all parts of a type will appear in the returned string: for example, lifetime specifiers are currently not included. In addition, the output may change between versions of the compiler. So that's it then? We can't use `type_name`? Well, this statement isn't so much a rule as it is a guideline. And Bevy is no stranger to bending the rules to make things work or to improve ergonomics. Remember that before `TypePath`, Bevy's scene system was entirely dependent on `type_name`. Not to mention that `type_name` is being used as a key into both the `TypeRegistry` and the `FunctionRegistry`. Bevy's practices aside, can we reliably use `type_name` for this? My answer would be "yes". Anonymous functions are anonymous. They have no name. There's nothing Rust could do to give them a name apart from generating a random string of characters. But remember that this is a diagnostic tool, it doesn't make sense to obfuscate the type by randomizing the output. So changing it to be anything other than what it is now is very unlikely. The only changes that I could potentially see happening are: 1. Closures replace `{{closure}}` with the name of their variable 2. Lifetimes are included in the output I don't think the first is likely to happen, but if it does then it actually works out in our favor: closures are now named! The second point is probably the likeliest. However, adding lifetimes doesn't mean we can't still rely on `type_name` to determine whether or not a function is named. So we should be okay in this case as well. ## Solution Parse the `type_name` of the function in the `TypedFunction` impl to determine if the function is named or anonymous. This once again makes `FunctionInfo::name` optional. For manual constructions of `DynamicFunction`, `FunctionInfo::named` or ``FunctionInfo::anonymous` can be used. The `FunctionRegistry` API has also been reworked to account for this change. `FunctionRegistry::register` no longer takes a name and instead takes it from the supplied function, returning a `FunctionRegistrationError::MissingName` error if the name is `None`. This also doubles as a replacement for the old `FunctionRegistry::register_dynamic` method, which has been removed. To handle anonymous functions, a `FunctionRegistry::register_with_name` method has been added. This works in the same way `FunctionRegistry::register` used to work before this PR. The overwriting methods have been updated in a similar manner, with modifications to `FunctionRegistry::overwrite_registration`, the removal of `FunctionRegistry::overwrite_registration_dynamic`, and the addition of `FunctionRegistry::overwrite_registration_with_name`. This PR also updates the methods on `App` in a similar way: `App::register_function` no longer requires a name argument and `App::register_function_with_name` has been added to handle anonymous functions (and eventually closures). ## Testing You can run the tests locally by running: ``` cargo test --package bevy_reflect --features functions ``` --- ## Internal Migration Guide > [!important] > Function reflection was introduced as part of the 0.15 dev cycle. This migration guide was written for developers relying on `main` during this cycle, and is not a breaking change coming from 0.14. > [!note] > This list is not exhaustive. It only contains some of the most important changes. `FunctionRegistry::register` no longer requires a name string for named functions. Anonymous functions, however, need to be registered using `FunctionRegistry::register_with_name`. ```rust // BEFORE registry .register(std::any::type_name_of_val(&foo), foo)? .register("bar", || println!("Hello world!")); // AFTER registry .register(foo)? .register_with_name("bar", || println!("Hello world!")); ``` `FunctionInfo::name` is now optional. Anonymous functions and closures will now have their name set to `None` by default. Additionally, `FunctionInfo::new` has been renamed to `FunctionInfo::named`. |
||
|
Gino Valente
|
0caeaa2ca9
|
bevy_reflect: Update serde tests for Set (#14616)
# Objective Support for reflecting set-like types (e.g. `HashSet`) was added in #13014. However, we didn't add any serialization tests to verify that serialization works as expected. ## Solution Update the serde tests. ## Testing You can test locally by running: ``` cargo test --package bevy_reflect ``` |
||
|
Gino Valente
|
df61117850
|
bevy_reflect: Function registry (#14098)
# Objective #13152 added support for reflecting functions. Now, we need a way to register those functions such that they may be accessed anywhere within the ECS. ## Solution Added a `FunctionRegistry` type similar to `TypeRegistry`. This allows a function to be registered and retrieved by name. ```rust fn foo() -> i32 { 123 } let mut registry = FunctionRegistry::default(); registry.register("my_function", foo); let function = registry.get_mut("my_function").unwrap(); let value = function.call(ArgList::new()).unwrap().unwrap_owned(); assert_eq!(value.downcast_ref::<i32>(), Some(&123)); ``` Additionally, I added an `AppFunctionRegistry` resource which wraps a `FunctionRegistryArc`. Functions can be registered into this resource using `App::register_function` or by getting a mutable reference to the resource itself. ### Limitations #### `Send + Sync` In order to get this registry to work across threads, it needs to be `Send + Sync`. This means that `DynamicFunction` needs to be `Send + Sync`, which means that its internal function also needs to be `Send + Sync`. In most cases, this won't be an issue because standard Rust functions (the type most likely to be registered) are always `Send + Sync`. Additionally, closures tend to be `Send + Sync` as well, granted they don't capture any `!Send` or `!Sync` variables. This PR adds this `Send + Sync` requirement, but as mentioned above, it hopefully shouldn't be too big of an issue. #### Closures Unfortunately, closures can't be registered yet. This will likely be explored and added in a followup PR. ### Future Work Besides addressing the limitations listed above, another thing we could look into is improving the lookup of registered functions. One aspect is in the performance of hashing strings. The other is in the developer experience of having to call `std::any::type_name_of_val` to get the name of their function (assuming they didn't give it a custom name). ## Testing You can run the tests locally with: ``` cargo test --package bevy_reflect ``` --- ## Changelog - Added `FunctionRegistry` - Added `AppFunctionRegistry` (a `Resource` available from `bevy_ecs`) - Added `FunctionRegistryArc` - Added `FunctionRegistrationError` - Added `reflect_functions` feature to `bevy_ecs` and `bevy_app` - `FunctionInfo` is no longer `Default` - `DynamicFunction` now requires its wrapped function be `Send + Sync` ## Internal Migration Guide > [!important] > Function reflection was introduced as part of the 0.15 dev cycle. This migration guide was written for developers relying on `main` during this cycle, and is not a breaking change coming from 0.14. `DynamicFunction` (both those created manually and those created with `IntoFunction`), now require `Send + Sync`. All standard Rust functions should meet that requirement. Closures, on the other hand, may not if they capture any `!Send` or `!Sync` variables from its environment. |
||
|
recatek
|
87b63af864
|
bevy_reflect: Adding support for Atomic values (#14419)
Fixes #14418 Note that this does not add AtomicPtr, which would need its own special casing support, just the regular value types. Also, I was forced to be opinionated about which Ordering to use, so I chose SeqCst as the strictest by default. |
||
|
Robert Walter
|
52a2a3b146
|
Dedicated Reflect implementation for Set-like things (#13014)
# Objective I just wanted to inspect `HashSet`s in `bevy-inspector-egui` but I noticed that it didn't work for some reason. A few minutes later I found myself looking into the bevy reflect impls noticing that `HashSet`s have been covered only rudimentary up until now. ## Solution I'm not sure if this is overkill (especially the first bullet), but here's a list of the changes: - created a whole new trait and enum variants for `ReflectRef` and the like called `Set` - mostly oriented myself at the `Map` trait and made the necessary changes until RA was happy - create macro `impl_reflect_for_hashset!` and call it on `std::HashSet` and `hashbrown::HashSet` Extra notes: - no `get_mut` or `get_mut_at` mirroring the `std::HashSet` - `insert[_boxed]` and `remove` return `bool` mirroring `std::HashSet`, additionally that bool is reflect as I thought that would be how we handle things in bevy reflect, but I'm not sure on this - ser/de are handled via `SeqAccess` - I'm not sure about the general deduplication property of this impl of `Set` that is generally expected? I'm also not sure yet if `Map` does provide this. This mainly refers to the `Dynamic[...]` structs - I'm not sure if there are other methods missing from the `trait`, I felt like `contains` or the set-operations (union/diff/...) could've been helpful, but I wanted to get out the bare minimum for feedback first --- ## Changelog ### Added - `Set` trait for `bevy_reflect` ### Changed - `std::collections::HashSet` and `bevy_utils::hashbrown::HashSet` now implement a more complete set of reflect functionalities instead of "just" `reflect_value` - `TypeInfo` contains a new variant `Set` that contains `SetInfo` - `ReflectKind` contains a new variant `Set` - `ReflectRef` contains a new variant `Set` - `ReflectMut` contains a new variant `Set` - `ReflectOwned` contains a new variant `Set` ## Migration Guide - The new `Set` variants on the enums listed in the change section should probably be considered by people working with this level of the lib ### Help wanted! I'm not sure if this change is able to break code. From my understanding it shouldn't since we just add functionality but I'm not sure yet if theres anything missing from my impl that would be normally provided by `impl_reflect_value!` |
||
|
Gino Valente
|
af865e76a3
|
bevy_reflect: Improve DynamicFunction ergonomics (#14201)
# Objective
Many functions can be converted to `DynamicFunction` using
`IntoFunction`. Unfortunately, we are limited by Rust itself and the
implementations are far from exhaustive. For example, we can't convert
functions with more than 16 arguments. Additionally, we can't handle
returns with lifetimes not tied to the lifetime of the first argument.
In such cases, users will have to create their `DynamicFunction`
manually.
Let's take the following function:
```rust
fn get(index: usize, list: &Vec<String>) -> &String {
&list[index]
}
```
This function cannot be converted to a `DynamicFunction` via
`IntoFunction` due to the lifetime of the return value being tied to the
second argument. Therefore, we need to construct the `DynamicFunction`
manually:
```rust
DynamicFunction::new(
|mut args, info| {
let list = args
.pop()
.unwrap()
.take_ref::<Vec<String>>(&info.args()[1])?;
let index = args.pop().unwrap().take_owned::<usize>(&info.args()[0])?;
Ok(Return::Ref(get(index, list)))
},
FunctionInfo::new()
.with_name("get")
.with_args(vec![
ArgInfo:🆕:<usize>(0).with_name("index"),
ArgInfo:🆕:<&Vec<String>>(1).with_name("list"),
])
.with_return_info(ReturnInfo:🆕:<&String>()),
);
```
While still a small and straightforward snippet, there's a decent amount
going on here. There's a lot of room for improvements when it comes to
ergonomics and readability.
The goal of this PR is to address those issues.
## Solution
Improve the ergonomics and readability of manually created
`DynamicFunction`s.
Some of the major changes:
1. Removed the need for `&ArgInfo` when reifying arguments (i.e. the
`&info.args()[1]` calls)
2. Added additional `pop` methods on `ArgList` to handle both popping
and casting
3. Added `take` methods on `ArgList` for taking the arguments out in
order
4. Removed the need for `&FunctionInfo` in the internal closure (Change
1 made it no longer necessary)
5. Added methods to automatically handle generating `ArgInfo` and
`ReturnInfo`
With all these changes in place, we get something a lot nicer to both
write and look at:
```rust
DynamicFunction::new(
|mut args| {
let index = args.take::<usize>()?;
let list = args.take::<&Vec<String>>()?;
Ok(Return::Ref(get(index, list)))
},
FunctionInfo::new()
.with_name("get")
.with_arg::<usize>("index")
.with_arg::<&Vec<String>>("list")
.with_return::<&String>(),
);
```
Alternatively, to rely on type inference for taking arguments, you could
do:
```rust
DynamicFunction::new(
|mut args| {
let index = args.take_owned()?;
let list = args.take_ref()?;
Ok(Return::Ref(get(index, list)))
},
FunctionInfo::new()
.with_name("get")
.with_arg::<usize>("index")
.with_arg::<&Vec<String>>("list")
.with_return::<&String>(),
);
```
## Testing
You can test locally by running:
```
cargo test --package bevy_reflect
```
---
## Changelog
- Removed `&ArgInfo` argument from `FromArg::from_arg` trait method
- Removed `&ArgInfo` argument from `Arg::take_***` methods
- Added `ArgValue`
- `Arg` is now a struct containing an `ArgValue` and an argument `index`
- `Arg::take_***` methods now require `T` is also `TypePath`
- Added `Arg::new`, `Arg::index`, `Arg::value`, `Arg::take_value`, and
`Arg::take` methods
- Replaced `ArgId` in `ArgError` with just the argument `index`
- Added `ArgError::EmptyArgList`
- Renamed `ArgList::push` to `ArgList::push_arg`
- Added `ArgList::pop_arg`, `ArgList::pop_owned`, `ArgList::pop_ref`,
and `ArgList::pop_mut`
- Added `ArgList::take_arg`, `ArgList::take_owned`, `ArgList::take_ref`,
`ArgList::take_mut`, and `ArgList::take`
- `ArgList::pop` is now generic
- Renamed `FunctionError::InvalidArgCount` to
`FunctionError::ArgCountMismatch`
- The closure given to `DynamicFunction::new` no longer has a
`&FunctionInfo` argument
- Added `FunctionInfo::with_arg`
- Added `FunctionInfo::with_return`
## Internal Migration Guide
> [!important]
> Function reflection was introduced as part of the 0.15 dev cycle. This
migration guide was written for developers relying on `main` during this
cycle, and is not a breaking change coming from 0.14.
* The `FromArg::from_arg` trait method and the `Arg::take_***` methods
no longer take a `&ArgInfo` argument.
* What used to be `Arg` is now `ArgValue`. `Arg` is now a struct which
contains an `ArgValue`.
* `Arg::take_***` methods now require `T` is also `TypePath`
* Instances of `id: ArgId` in `ArgError` have been replaced with `index:
usize`
* `ArgList::push` is now `ArgList::push_arg`. It also takes the new
`ArgValue` type.
* `ArgList::pop` has become `ArgList::pop_arg` and now returns
`ArgValue`. `Arg::pop` now takes a generic type and downcasts to that
type. It's recommended to use `ArgList::take` and friends instead since
they allow removing the arguments from the list in the order they were
pushed (rather than reverse order).
* `FunctionError::InvalidArgCount` is now
`FunctionError::ArgCountMismatch`
* The closure given to `DynamicFunction::new` no longer has a
`&FunctionInfo` argument. This argument can be removed.
|
||
|
Gino Valente
|
1042f09c2e
|
bevy_reflect: Add DynamicClosure and DynamicClosureMut (#14141)
# Objective As mentioned in [this](https://github.com/bevyengine/bevy/pull/13152#issuecomment-2198387297) comment, creating a function registry (see #14098) is a bit difficult due to the requirements of `DynamicFunction`. Internally, a `DynamicFunction` contains a `Box<dyn FnMut>` (the function that reifies reflected arguments and calls the actual function), which requires `&mut self` in order to be called. This means that users would require a mutable reference to the function registry for it to be useful— which isn't great. And they can't clone the `DynamicFunction` either because cloning an `FnMut` isn't really feasible (wrapping it in an `Arc` would allow it to be cloned but we wouldn't be able to call the clone since we need a mutable reference to the `FnMut`, which we can't get with multiple `Arc`s still alive, requiring us to also slap in a `Mutex`, which adds additional overhead). And we don't want to just replace the `dyn FnMut` with `dyn Fn` as that would prevent reflecting closures that mutate their environment. Instead, we need to introduce a new type to split the requirements of `DynamicFunction`. ## Solution Introduce new types for representing closures. Specifically, this PR introduces `DynamicClosure` and `DynamicClosureMut`. Similar to how `IntoFunction` exists for `DynamicFunction`, two new traits were introduced: `IntoClosure` and `IntoClosureMut`. Now `DynamicFunction` stores a `dyn Fn` with a `'static` lifetime. `DynamicClosure` also uses a `dyn Fn` but has a lifetime, `'env`, tied to its environment. `DynamicClosureMut` is most like the old `DynamicFunction`, keeping the `dyn FnMut` and also typing its lifetime, `'env`, to the environment Here are some comparison tables: | | `DynamicFunction` | `DynamicClosure` | `DynamicClosureMut` | | - | ----------------- | ---------------- | ------------------- | | Callable with `&self` | ✅ | ✅ | ❌ | | Callable with `&mut self` | ✅ | ✅ | ✅ | | Allows for non-`'static` lifetimes | ❌ | ✅ | ✅ | | | `IntoFunction` | `IntoClosure` | `IntoClosureMut` | | - | -------------- | ------------- | ---------------- | | Convert `fn` functions | ✅ | ✅ | ✅ | | Convert `fn` methods | ✅ | ✅ | ✅ | | Convert anonymous functions | ✅ | ✅ | ✅ | | Convert closures that capture immutable references | ❌ | ✅ | ✅ | | Convert closures that capture mutable references | ❌ | ❌ | ✅ | | Convert closures that capture owned values | ❌[^1] | ✅ | ✅ | [^1]: Due to limitations in Rust, `IntoFunction` can't be implemented for just functions (unless we forced users to manually coerce them to function pointers first). So closures that meet the trait requirements _can technically_ be converted into a `DynamicFunction` as well. To both future-proof and reduce confusion, though, we'll just pretend like this isn't a thing. ```rust let mut list: Vec<i32> = vec![1, 2, 3]; // `replace` is a closure that captures a mutable reference to `list` let mut replace = |index: usize, value: i32| -> i32 { let old_value = list[index]; list[index] = value; old_value }; // Convert the closure into a dynamic closure using `IntoClosureMut::into_closure_mut` let mut func: DynamicClosureMut = replace.into_closure_mut(); // Dynamically call the closure: let args = ArgList::default().push_owned(1_usize).push_owned(-2_i32); let value = func.call_once(args).unwrap().unwrap_owned(); // Check the result: assert_eq!(value.take::<i32>().unwrap(), 2); assert_eq!(list, vec![1, -2, 3]); ``` ### `ReflectFn`/`ReflectFnMut` To make extending the function reflection system easier (the blanket impls for `IntoFunction`, `IntoClosure`, and `IntoClosureMut` are all incredibly short), this PR generalizes callables with two new traits: `ReflectFn` and `ReflectFnMut`. These traits mimic `Fn` and `FnMut` but allow for being called via reflection. In fact, their blanket implementations are identical save for `ReflectFn` being implemented over `Fn` types and `ReflectFnMut` being implemented over `FnMut` types. And just as `Fn` is a subtrait of `FnMut`, `ReflectFn` is a subtrait of `ReflectFnMut`. So anywhere that expects a `ReflectFnMut` can also be given a `ReflectFn`. To reiterate, these traits aren't 100% necessary. They were added in purely for extensibility. If we decide to split things up differently or add new traits/types in the future, then those changes should be much simpler to implement. ### `TypedFunction` Because of the split into `ReflectFn` and `ReflectFnMut`, we needed a new way to access the function type information. This PR moves that concept over into `TypedFunction`. Much like `Typed`, this provides a way to access a function's `FunctionInfo`. By splitting this trait out, it helps to ensure the other traits are focused on a single responsibility. ### Internal Macros The original function PR (#13152) implemented `IntoFunction` using a macro which was passed into an `all_tuples!` macro invocation. Because we needed the same functionality for these new traits, this PR has copy+pasted that code for `ReflectFn`, `ReflectFnMut`, and `TypedFunction`— albeit with some differences between them. Originally, I was going to try and macro-ify the impls and where clauses such that we wouldn't have to straight up duplicate a lot of this logic. However, aside from being more complex in general, autocomplete just does not play nice with such heavily nested macros (tried in both RustRover and VSCode). And both of those problems told me that it just wasn't worth it: we need to ensure the crate is easily maintainable, even at the cost of duplicating code. So instead, I made sure to simplify the macro code by removing all fully-qualified syntax and cutting the where clauses down to the bare essentials, which helps to clean up a lot of the visual noise. I also tried my best to document the macro logic in certain areas (I may even add a bit more) to help with maintainability for future devs. ### Documentation Documentation for this module was a bit difficult for me. So many of these traits and types are very interconnected. And each trait/type has subtle differences that make documenting it in a single place, like at the module level, difficult to do cleanly. Describing the valid signatures is also challenging to do well. Hopefully what I have here is okay. I think I did an okay job, but let me know if there any thoughts on ways to improve it. We can also move such a task to a followup PR for more focused discussion. ## Testing You can test locally by running: ``` cargo test --package bevy_reflect ``` --- ## Changelog - Added `DynamicClosure` struct - Added `DynamicClosureMut` struct - Added `IntoClosure` trait - Added `IntoClosureMut` trait - Added `ReflectFn` trait - Added `ReflectFnMut` trait - Added `TypedFunction` trait - `IntoFunction` now only works for standard Rust functions - `IntoFunction` no longer takes a lifetime parameter - `DynamicFunction::call` now only requires `&self` - Removed `DynamicFunction::call_once` - Changed the `IntoReturn::into_return` signature to include a where clause ## Internal Migration Guide > [!important] > Function reflection was introduced as part of the 0.15 dev cycle. This migration guide was written for developers relying on `main` during this cycle, and is not a breaking change coming from 0.14. ### `IntoClosure` `IntoFunction` now only works for standard Rust functions. Calling `IntoFunction::into_function` on a closure that captures references to its environment (either mutable or immutable), will no longer compile. Instead, you will need to use either `IntoClosure::into_closure` to create a `DynamicClosure` or `IntoClosureMut::into_closure_mut` to create a `DynamicClosureMut`, depending on your needs: ```rust let punct = String::from("!"); let print = |value: String| { println!("{value}{punct}"); }; // BEFORE let func: DynamicFunction = print.into_function(); // AFTER let func: DynamicClosure = print.into_closure(); ``` ### `IntoFunction` lifetime Additionally, `IntoFunction` no longer takes a lifetime parameter as it always expects a `'static` lifetime. Usages will need to remove any lifetime parameters: ```rust // BEFORE fn execute<'env, F: IntoFunction<'env, Marker>, Marker>(f: F) {/* ... */} // AFTER fn execute<F: IntoFunction<Marker>, Marker>(f: F) {/* ... */} ``` ### `IntoReturn` `IntoReturn::into_return` now has a where clause. Any manual implementors will need to add this where clause to their implementation. |
||
|
SpecificProtagonist
|
ab255aefc6
|
Implement FromIterator/IntoIterator for dynamic types (#14250)
# Objective Implement FromIterator/IntoIterator for dynamic types where missing Note: - can't impl `IntoIterator` for `&Array` & co because of orphan rules - `into_iter().collect()` is a no-op for `Vec`s because of specialization --- ## Migration Guide - Change `DynamicArray::from_vec` to `DynamicArray::from_iter` |