e9418b3845
379 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
aecsocket
|
57931ce42f
|
bevy_reflect: Add ReflectDeserializerProcessor (#15482)
**NOTE: Also see https://github.com/bevyengine/bevy/pull/15548 for the serializer equivalent** # Objective The current `ReflectDeserializer` and `TypedReflectDeserializer` use the `TypeRegistration` and/or `ReflectDeserialize` of a given type in order to determine how to deserialize a value of that type. However, there is currently no way to statefully override deserialization of a given type when using these two deserializers - that is, to have some local data in the same scope as the `ReflectDeserializer`, and make use of that data when deserializing. The motivating use case for this came up when working on [`bevy_animation_graph`](https://github.com/aecsocket/bevy_animation_graph/tree/feat/dynamic-nodes), when loading an animation graph asset. The `AnimationGraph` stores `Vec<Box<dyn NodeLike>>`s which we have to load in. Those `Box<dyn NodeLike>`s may store `Handle`s to e.g. `Handle<AnimationClip>`. I want to trigger a `load_context.load()` for that handle when it's loaded. ```rs #[derive(Reflect)] struct Animation { clips: Vec<Handle<AnimationClip>>, } ``` ```rs ( clips: [ "animation_clips/walk.animclip.ron", "animation_clips/run.animclip.ron", "animation_clips/jump.animclip.ron", ], ) ```` Currently, if this were deserialized from an asset loader, this would be deserialized as a vec of `Handle::default()`s, which isn't useful since we also need to `load_context.load()` those handles for them to be used. With this processor field, a processor can detect when `Handle<T>`s are being loaded, then actually load them in. ## Solution ```rs trait ReflectDeserializerProcessor { fn try_deserialize<'de, D>( &mut self, registration: &TypeRegistration, deserializer: D, ) -> Result<Result<Box<dyn PartialReflect>, D>, D::Error> where D: serde::Deserializer<'de>; } ``` ```diff - pub struct ReflectDeserializer<'a> { + pub struct ReflectDeserializer<'a, P = ()> { // also for ReflectTypedDeserializer registry: &'a TypeRegistry, + processor: Option<&'a mut P>, } ``` ```rs impl<'a, P: ReflectDeserializerProcessor> ReflectDeserializer<'a, P> { // also for ReflectTypedDeserializer pub fn with_processor(registry: &'a TypeRegistry, processor: &'a mut P) -> Self { Self { registry, processor: Some(processor), } } } ``` This does not touch the existing `fn new`s. This `processor` field is also added to all internal visitor structs. When `TypedReflectDeserializer` runs, it will first try to deserialize a value of this type by passing the `TypeRegistration` and deserializer to the processor, and fallback to the default logic. This processor runs the earliest, and takes priority over all other deserialization logic. ## Testing Added unit tests to `bevy_reflect::serde::de`. Also using almost exactly the same implementation in [my fork of `bevy_animation_graph`](https://github.com/aecsocket/bevy_animation_graph/tree/feat/dynamic-nodes). ## Migration Guide (Since I added `P = ()`, I don't think this is actually a breaking change anymore, but I'll leave this in) `bevy_reflect`'s `ReflectDeserializer` and `TypedReflectDeserializer` now take a `ReflectDeserializerProcessor` as the type parameter `P`, which allows you to customize deserialization for specific types when they are found. However, the rest of the API surface (`new`) remains the same. <details> <summary>Original implementation</summary> Add `ReflectDeserializerProcessor`: ```rs struct ReflectDeserializerProcessor { pub can_deserialize: Box<dyn FnMut(&TypeRegistration) -> bool + 'p>, pub deserialize: Box< dyn FnMut( &TypeRegistration, &mut dyn erased_serde::Deserializer, ) -> Result<Box<dyn PartialReflect>, erased_serde::Error> + 'p, } ``` Along with `ReflectDeserializer::new_with_processor` and `TypedReflectDeserializer::new_with_processor`. This does not touch the public API of the existing `new` fns. This is stored as an `Option<&mut ReflectDeserializerProcessor>` on the deserializer and any of the private `-Visitor` structs, and when we attempt to deserialize a value, we first pass it through this processor. Also added a very comprehensive doc test to `ReflectDeserializerProcessor`, which is actually a scaled down version of the code for the `bevy_animation_graph` loader. This should give users a good motivating example for when and why to use this feature. ### Why `Box<dyn ..>`? When I originally implemented this, I added a type parameter to `ReflectDeserializer` to determine the processor used, with `()` being "no processor". However when using this, I kept running into rustc errors where it failed to validate certain type bounds and led to overflows. I then switched to a dynamic dispatch approach. The dynamic dispatch should not be that expensive, nor should it be a performance regression, since it's only used if there is `Some` processor. (Note: I have not benchmarked this, I am just speculating.) Also, it means that we don't infect the rest of the code with an extra type parameter, which is nicer to maintain. ### Why the `'p` on `ReflectDeserializerProcessor<'p>`? Without a lifetime here, the `Box`es would automatically become `Box<dyn FnMut(..) + 'static>`. This makes them practically useless, since any local data you would want to pass in must then be `'static`. In the motivating example, you couldn't pass in that `&mut LoadContext` to the function. This means that the `'p` infects the rest of the Visitor types, but this is acceptable IMO. This PR also elides the lifetimes in the `impl<'de> Visitor<'de> for -Visitor` blocks where possible. ### Future possibilities I think it's technically possible to turn the processor into a trait, and make the deserializers generic over that trait. This would also open the door to an API like: ```rs type Seed; fn seed_deserialize(&mut self, r: &TypeRegistration) -> Option<Self::Seed>; fn deserialize(&mut self, r: &TypeRegistration, d: &mut dyn erased_serde::Deserializer, s: Self::Seed) -> ...; ``` A similar processor system should also be added to the serialization side, but that's for another PR. Ideally, both PRs will be in the same release, since one isn't very useful without the other. ## Testing Added unit tests to `bevy_reflect::serde::de`. Also using almost exactly the same implementation in [my fork of `bevy_animation_graph`](https://github.com/aecsocket/bevy_animation_graph/tree/feat/dynamic-nodes). ## Migration Guide `bevy_reflect`'s `ReflectDeserializer` and `TypedReflectDeserializer` now take a second lifetime parameter `'p` for storing the `ReflectDeserializerProcessor` field lifetimes. However, the rest of the API surface (`new`) remains the same, so if you are not storing these deserializers or referring to them with lifetimes, you should not have to make any changes. </details> |
||
|
Rob Parrett
|
30d84519a2
|
Use en-us locale for typos (#16037)
# Objective Bevy seems to want to standardize on "American English" spellings. Not sure if this is laid out anywhere in writing, but see also #15947. While perusing the docs for `typos`, I noticed that it has a `locale` config option and tried it out. ## Solution Switch to `en-us` locale in the `typos` config and run `typos -w` ## Migration Guide The following methods or fields have been renamed from `*dependants*` to `*dependents*`. - `ProcessorAssetInfo::dependants` - `ProcessorAssetInfos::add_dependant` - `ProcessorAssetInfos::non_existent_dependants` - `AssetInfo::dependants_waiting_on_load` - `AssetInfo::dependants_waiting_on_recursive_dep_load` - `AssetInfos::loader_dependants` - `AssetInfos::remove_dependants_and_labels` |
||
|
Stepan Koltsov
|
405fa3e8ea
|
Mute non-local definition warnings in bevy_reflect (#16013)
# Objective
```
cargo check -p bevy_reflect
```
outputs a lot of warnings like:
```
warning: non-local `impl` definition, `impl` blocks should be written at the same level as their item
--> crates/bevy_reflect/src/impls/std.rs:223:13
|
223 | impl_type_path!($ty);
| ^-------------------
| |
| `TypePath` is not local
| move the `impl` block outside of this constant `_` and up 2 bodies
...
346 | / impl_reflect_for_atomic!(
347 | | ::core::sync::atomic::AtomicIsize,
| | --------------------------------- `AtomicIsize` is not local
348 | | ::core::sync::atomic::Ordering::SeqCst
349 | | );
| |_- in this macro invocation
|
= note: the macro `impl_type_path` defines the non-local `impl`, and may need to be changed
= note: the macro `impl_type_path` may come from an old version of the `bevy_reflect_derive` crate, try updating your dependency with `cargo update -p bevy_reflect_derive`
= note: an `impl` is never scoped, even when it is nested inside an item, as it may impact type checking outside of that item, which can be the case if neither the trait or the self type are at the same nesting level as the `impl`
= note: items in an anonymous const item (`const _: () = { ... }`) are treated as in the same scope as the anonymous const's declaration for the purpose of this lint
= note: `#[warn(non_local_definitions)]` on by default
= note: this warning originates in the macro `impl_type_path` which comes from the expansion of the macro `impl_reflect_for_atomic` (in Nightly builds, run with -Z macro-backtrace for more info)
```
## Solution
Move `impl_type_path!` into global scope. Warnings no longer pop up.
## Testing
CI
|
||
|
Torstein Grindvik
|
9f5f5d3d41
|
bevy_reflect: get_represented_kind_info APIs for reflected kinds (#14380)
# Objective Fixes #14378 --------- Signed-off-by: Torstein Grindvik <torstein.grindvik@muybridge.com> Co-authored-by: Torstein Grindvik <torstein.grindvik@muybridge.com> Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com> Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> |
||
|
Clar Fon
|
e79bc7811d
|
Fix *most* clippy lints (#15906)
# Objective Another clippy-lint fix: the goal is so that `ci lints` actually displays the problems that a contributor caused, and not a bunch of existing stuff in the repo. (when run on nightly) ## Solution This fixes all but the `clippy::needless_lifetimes` lint, which will result in substantially more fixes and be in other PR(s). I also explicitly allow `non_local_definitions` since it is [not working correctly, but will be fixed](https://github.com/rust-lang/rust/issues/131643). A few things were manually fixed: for example, some places had an explicitly defined `div_ceil` function that was used, which is no longer needed since this function is stable on unsigned integers. Also, empty lines in doc comments were handled individually. ## Testing I ran `cargo clippy --workspace --all-targets --all-features --fix --allow-staged` with the `clippy::needless_lifetimes` lint marked as `allow` in `Cargo.toml` to avoid fixing that too. It now passes with all but the listed lint. |
||
|
Nathan Lilienthal
|
0b2e0cfaca
|
bevy_reflect: Add crate level functions feature docs (#15086)
Adds the missing section for the `functions` cargo feature of the `bevy_reflect` crate. |
||
|
Zachary Harrold
|
3cc1527e9e
|
Remove thiserror from bevy_reflect (#15766)
# Objective - Contributes to #15460 ## Solution - Removed `thiserror` from `bevy_reflect` |
||
|
notmd
|
cab00766d9
|
Serialize and deserialize tuple struct with one field as newtype struct (#15628)
# Objective - fix https://github.com/bevyengine/bevy/issues/15623 ## Solution - Checking field length of tuple struct before ser/der ## Testing - CI should pass ## Migration Guide - Reflection now will serialize and deserialize tuple struct with single field as newtype struct. Consider this code. ```rs #[derive(Reflect, Serialize)] struct Test(usize); let reflect = Test(3); let serializer = TypedReflectSerializer::new(reflect.as_partial_reflect(), ®istry); return serde_json::to_string(&serializer) ``` Old behavior will return `["3"]`. New behavior will return `"3"`. If you were relying on old behavior you need to update your logic. Especially with `serde_json`. `ron` doesn't affect from this. |
||
|
Benjamin Brienen
|
c841dd92a1
|
Documentation for variadics (#15387)
# Objective Relevant: #15208 ## Solution I went ahead and added the variadics documentation in all applicable locations. ## Testing - I built the documentation and inspected it to see whether the feature is there. |
||
|
Gino Valente
|
eaa37f3b45
|
bevy_reflect: Add DeserializeWithRegistry and SerializeWithRegistry (#8611)
# Objective
### The Problem
Currently, the reflection deserializers give little control to users for
how a type is deserialized. The most control a user can have is to
register `ReflectDeserialize`, which will use a type's `Deserialize`
implementation.
However, there are times when a type may require slightly more control.
For example, let's say we want to make Bevy's `Mesh` easier to
deserialize via reflection (assume `Mesh` actually implemented
`Reflect`). Since we want this to be extensible, we'll make it so users
can use their own types so long as they satisfy `Into<Mesh>`. The end
result should allow users to define a RON file like:
```rust
{
"my_game::meshes::Sphere": (
radius: 2.5
)
}
```
### The Current Solution
Since we don't know the types ahead of time, we'll need to use
reflection. Luckily, we can access type information dynamically via the
type registry. Let's make a custom type data struct that users can
register on their types:
```rust
pub struct ReflectIntoMesh {
// ...
}
impl<T: FromReflect + Into<Mesh>> FromType<T> for ReflectIntoMesh {
fn from_type() -> Self {
// ...
}
}
```
Now we'll need a way to use this type data during deserialization.
Unfortunately, we can't use `Deserialize` since we need access to the
registry. This is where `DeserializeSeed` comes in handy:
```rust
pub struct MeshDeserializer<'a> {
pub registry: &'a TypeRegistry
}
impl<'a, 'de> DeserializeSeed<'de> for MeshDeserializer<'a> {
type Value = Mesh;
fn deserialize<D>(self, deserializer: D) -> Result<Self::Value, D::Error>
where
D: serde::Deserializer<'de>,
{
struct MeshVisitor<'a> {
registry: &'a TypeRegistry
}
impl<'a, 'de> Visitor<'de> for MeshVisitor<'a> {
fn expecting(&self, formatter: &mut Formatter) -> std::fmt::Result {
write!(formatter, "map containing mesh information")
}
fn visit_map<A>(self, mut map: A) -> Result<Self::Value, serde:🇩🇪:Error> where A: MapAccess<'de> {
// Parse the type name
let type_name = map.next_key::<String>()?.unwrap();
// Deserialize the value based on the type name
let registration = self.registry
.get_with_name(&type_name)
.expect("should be registered");
let value = map.next_value_seed(TypedReflectDeserializer {
registration,
registry: self.registry,
})?;
// Convert the deserialized value into a `Mesh`
let into_mesh = registration.data::<ReflectIntoMesh>().unwrap();
Ok(into_mesh.into(value))
}
}
}
}
```
### The Problem with the Current Solution
The solution above works great when all we need to do is deserialize
`Mesh` directly. But now, we want to be able to deserialize a struct
like this:
```rust
struct Fireball {
damage: f32,
mesh: Mesh,
}
```
This might look simple enough and should theoretically be no problem for
the reflection deserializer to handle, but this is where our
`MeshDeserializer` solution starts to break down.
In order to use `MeshDeserializer`, we need to have access to the
registry. The reflection deserializers have access to that, but we have
no way of borrowing it for our own deserialization since they have no
way of knowing about `MeshDeserializer`.
This means we need to implement _another_ `DeserializeSeed`— this time
for `Fireball`!
And if we decided to put `Fireball` inside another type, well now we
need one for that type as well.
As you can see, this solution does not scale well and results in a lot
of unnecessary boilerplate for the user.
## Solution
> [!note]
> This PR originally only included the addition of
`DeserializeWithRegistry`. Since then, a corresponding
`SerializeWithRegistry` trait has also been added. The reasoning and
usage is pretty much the same as the former so I didn't bother to update
the full PR description.
Created the `DeserializeWithRegistry` trait and
`ReflectDeserializeWithRegistry` type data.
The `DeserializeWithRegistry` trait works like a standard `Deserialize`
but provides access to the registry. And by registering the
`ReflectDeserializeWithRegistry` type data, the reflection deserializers
will automatically use the `DeserializeWithRegistry` implementation,
just like it does for `Deserialize`.
All we need to do is make the following changes:
```diff
#[derive(Reflect)]
+ #[reflect(DeserializeWithRegistry)]
struct Mesh {
// ...
}
- impl<'a, 'de> DeserializeSeed<'de> for MeshDeserializer<'a> {
- type Value = Mesh;
- fn deserialize<D>(self, deserializer: D) -> Result<Self::Value, D::Error>
+ impl<'de> DeserializeWithRegistry<'de> for Mesh {
+ fn deserialize<D>(deserializer: D, registry: &TypeRegistry) -> Result<Self, D::Error>
where
D: serde::Deserializer<'de>,
{
// ...
}
}
```
Now, any time the reflection deserializer comes across `Mesh`, it will
opt to use its `DeserializeWithRegistry` implementation. And this means
we no longer need to create a whole slew of `DeserializeSeed` types just
to deserialize `Mesh`.
### Why not a trait like `DeserializeSeed`?
While this would allow for anyone to define a deserializer for `Mesh`,
the problem is that it means __anyone can define a deserializer for
`Mesh`.__ This has the unfortunate consequence that users can never be
certain that their registration of `ReflectDeserializeSeed` is the one
that will actually be used.
We could consider adding something like that in the future, but I think
this PR's solution is much safer and follows the example set by
`ReflectDeserialize`.
### What if we made the `TypeRegistry` globally available?
This is one potential solution and has been discussed before (#6101).
However, that change is much more controversial and comes with its own
set of disadvantages (can't have multiple registries such as with
multiple worlds, likely some added performance cost with each access,
etc.).
### Followup Work
Once this PR is merged, we should consider merging `ReflectDeserialize`
into `DeserializeWithRegistry`. ~~There is already a blanket
implementation to make this transition generally pretty
straightforward.~~ The blanket implementations were removed for the sake
of this PR and will need to be re-added in the followup. I would propose
that we first mark `ReflectDeserialize` as deprecated, though, before we
outright remove it in a future release.
---
## Changelog
- Added the `DeserializeReflect` trait and `ReflectDeserializeReflect`
type data
- Added the `SerializeReflect` trait and `ReflectSerializeReflect` type
data
- Added `TypedReflectDeserializer::of` convenience constructor
---------
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: aecsocket <43144841+aecsocket@users.noreply.github.com>
|
||
|
Gino Valente
|
397f20e835
|
bevy_reflect: Generic parameter info (#15475)
# Objective Currently, reflecting a generic type provides no information about the generic parameters. This means that you can't get access to the type of `T` in `Foo<T>` without creating custom type data (we do this for [`ReflectHandle`](https://docs.rs/bevy/0.14.2/bevy/asset/struct.ReflectHandle.html#method.asset_type_id)). ## Solution This PR makes it so that generic type parameters and generic const parameters are tracked in a `Generics` struct stored on the `TypeInfo` for a type. For example, `struct Foo<T, const N: usize>` will store `T` and `N` as a `TypeParamInfo` and `ConstParamInfo`, respectively. The stored information includes: - The name of the generic parameter (i.e. `T`, `N`, etc.) - The type of the generic parameter (remember that we're dealing with monomorphized types, so this will actually be a concrete type) - The default type/value, if any (e.g. `f32` in `T = f32` or `10` in `const N: usize = 10`) ### Caveats The only requirement for this to work is that the user does not opt-out of the automatic `TypePath` derive with `#[reflect(type_path = false)]`. Doing so prevents the macro code from 100% knowing that the generic type implements `TypePath`. This in turn means the generated `Typed` impl can't add generics to the type. There are two solutions for this—both of which I think we should explore in a future PR: 1. We could just not use `TypePath`. This would mean that we can't store the `Type` of the generic, but we can at least store the `TypeId`. 2. We could provide a way to opt out of the automatic `Typed` derive with a `#[reflect(typed = false)]` attribute. This would allow users to manually implement `Typed` to add whatever generic information they need (e.g. skipping a parameter that can't implement `TypePath` while the rest can). I originally thought about making `Generics` an enum with `Generic`, `NonGeneric`, and `Unavailable` variants to signify whether there are generics, no generics, or generics that cannot be added due to opting out of `TypePath`. I ultimately decided against this as I think it adds a bit too much complexity for such an uncommon problem. Additionally, user's don't necessarily _have_ to know the generics of a type, so just skipping them should generally be fine for now. ## Testing You can test locally by running: ``` cargo test --package bevy_reflect ``` --- ## Showcase You can now access generic parameters via `TypeInfo`! ```rust #[derive(Reflect)] struct MyStruct<T, const N: usize>([T; N]); let generics = MyStruct::<f32, 10>::type_info().generics(); // Get by index: let t = generics.get(0).unwrap(); assert_eq!(t.name(), "T"); assert!(t.ty().is::<f32>()); assert!(!t.is_const()); // Or by name: let n = generics.get_named("N").unwrap(); assert_eq!(n.name(), "N"); assert!(n.ty().is::<usize>()); assert!(n.is_const()); ``` You can even access parameter defaults: ```rust #[derive(Reflect)] struct MyStruct<T = String, const N: usize = 10>([T; N]); let generics = MyStruct::<f32, 5>::type_info().generics(); let GenericInfo::Type(info) = generics.get_named("T").unwrap() else { panic!("expected a type parameter"); }; let default = info.default().unwrap(); assert!(default.is::<String>()); let GenericInfo::Const(info) = generics.get_named("N").unwrap() else { panic!("expected a const parameter"); }; let default = info.default().unwrap(); assert_eq!(default.downcast_ref::<usize>().unwrap(), &10); ``` |
||
|
andriyDev
|
04d5685889
|
Make drain take a mutable borrow instead of Box<Self> for reflected Map, List, and Set. (#15406)
# Objective Fixes #15185. # Solution Change `drain` to take a `&mut self` for most reflected types. Some notable exceptions to this change are `Array` and `Tuple`. These types don't make sense with `drain` taking a mutable borrow since they can't get "smaller". Also `BTreeMap` doesn't have a `drain` function, so we have to pop elements off one at a time. ## Testing - The existing tests are sufficient. --- ## Migration Guide - `reflect::Map`, `reflect::List`, and `reflect::Set` all now take a `&mut self` instead of a `Box<Self>`. Callers of these traits should add `&mut` before their boxes, and implementers of these traits should update to match. |
||
|
Giacomo Stevanato
|
0d751e8809
|
Use HashTable in DynamicMap and fix bug in remove (#15158)
# Objective - `DynamicMap` currently uses an `HashMap` from a `u64` hash to the entry index in a `Vec`. This is incorrect in the presence of hash collisions, so let's fix it; - `DynamicMap::remove` was also buggy, as it didn't fix up the indexes of the other elements after removal. Fix that up as well and add a regression test. ## Solution - Use `HashTable` in `DynamicMap` to distinguish entries that have the same hash by using `reflect_partial_eq`, bringing it more in line with what `DynamicSet` does; - Reimplement `DynamicMap::remove` to properly fix up the index of moved elements after the removal. ## Testing - A regression test was added for the `DynamicMap::remove` issue. --- Some kinda related considerations: the use of a separate `Vec` for storing the entries adds some complications that I'm not sure are worth. This is mainly used to implement an efficient `get_at`, which is relied upon by `MapIter`. However both `HashMap` and `BTreeMap` implement `get_at` inefficiently (and cannot do so efficiently), leading to a `O(N^2)` complexity for iterating them. This could be removed in favor of a `Box<dyn Iterator>` like it's done in `DynamicSet`. |
||
|
hshrimp
|
7ee5143d45
|
Remove Return::Unit variant (#15484)
# Objective - Fixes #15447 ## Solution - Remove the `Return::Unit` variant and use a `Return::Owned` variant holding a unit `()` type. ## Migration Guide - Removed the `Return::Unit` variant; use `Return::unit()` instead. --------- Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com> |
||
|
JohnTheCoolingFan
|
1175cf7920
|
Fix ReflectKind description wording (#15498)
# Objective The "zero-sized" description was outdated and misleading. ## Solution Changed the description to just say that it's an enumeration (an enum) |
||
|
Zachary Harrold
|
d70595b667
|
Add core and alloc over std Lints (#15281)
# Objective - Fixes #6370 - Closes #6581 ## Solution - Added the following lints to the workspace: - `std_instead_of_core` - `std_instead_of_alloc` - `alloc_instead_of_core` - Used `cargo +nightly fmt` with [item level use formatting](https://rust-lang.github.io/rustfmt/?version=v1.6.0&search=#Item%5C%3A) to split all `use` statements into single items. - Used `cargo clippy --workspace --all-targets --all-features --fix --allow-dirty` to _attempt_ to resolve the new linting issues, and intervened where the lint was unable to resolve the issue automatically (usually due to needing an `extern crate alloc;` statement in a crate root). - Manually removed certain uses of `std` where negative feature gating prevented `--all-features` from finding the offending uses. - Used `cargo +nightly fmt` with [crate level use formatting](https://rust-lang.github.io/rustfmt/?version=v1.6.0&search=#Crate%5C%3A) to re-merge all `use` statements matching Bevy's previous styling. - Manually fixed cases where the `fmt` tool could not re-merge `use` statements due to conditional compilation attributes. ## Testing - Ran CI locally ## Migration Guide The MSRV is now 1.81. Please update to this version or higher. ## Notes - This is a _massive_ change to try and push through, which is why I've outlined the semi-automatic steps I used to create this PR, in case this fails and someone else tries again in the future. - Making this change has no impact on user code, but does mean Bevy contributors will be warned to use `core` and `alloc` instead of `std` where possible. - This lint is a critical first step towards investigating `no_std` options for Bevy. --------- Co-authored-by: François Mockers <francois.mockers@vleue.com> |
||
|
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` |
||
|
Gino Valente
|
aa241672e1
|
bevy_reflect: Nested TypeInfo getters (#13321)
# Objective
Right now, `TypeInfo` can be accessed directly from a type using either
`Typed::type_info` or `Reflect::get_represented_type_info`.
However, once that `TypeInfo` is accessed, any nested types must be
accessed via the `TypeRegistry`.
```rust
#[derive(Reflect)]
struct Foo {
bar: usize
}
let registry = TypeRegistry::default();
let TypeInfo::Struct(type_info) = Foo::type_info() else {
panic!("expected struct info");
};
let field = type_info.field("bar").unwrap();
let field_info = registry.get_type_info(field.type_id()).unwrap();
assert!(field_info.is::<usize>());;
```
## Solution
Enable nested types within a `TypeInfo` to be retrieved directly.
```rust
#[derive(Reflect)]
struct Foo {
bar: usize
}
let TypeInfo::Struct(type_info) = Foo::type_info() else {
panic!("expected struct info");
};
let field = type_info.field("bar").unwrap();
let field_info = field.type_info().unwrap();
assert!(field_info.is::<usize>());;
```
The particular implementation was chosen for two reasons.
Firstly, we can't just store `TypeInfo` inside another `TypeInfo`
directly. This is because some types are recursive and would result in a
deadlock when trying to create the `TypeInfo` (i.e. it has to create the
`TypeInfo` before it can use it, but it also needs the `TypeInfo` before
it can create it). Therefore, we must instead store the function so it
can be retrieved lazily.
I had considered also using a `OnceLock` or something to lazily cache
the info, but I figured we can look into optimizations later. The API
should remain the same with or without the `OnceLock`.
Secondly, a new wrapper trait had to be introduced: `MaybeTyped`. Like
`RegisterForReflection`, this trait is `#[doc(hidden)]` and only exists
so that we can properly handle dynamic type fields without requiring
them to implement `Typed`. We don't want dynamic types to implement
`Typed` due to the fact that it would make the return type
`Option<&'static TypeInfo>` for all types even though only the dynamic
types ever need to return `None` (see #6971 for details).
Users should never have to interact with this trait as it has a blanket
impl for all `Typed` types. And `Typed` is automatically implemented
when deriving `Reflect` (as it is required).
The one downside is we do need to return `Option<&'static TypeInfo>`
from all these new methods so that we can handle the dynamic cases. If
we didn't have to, we'd be able to get rid of the `Option` entirely. But
I think that's an okay tradeoff for this one part of the API, and keeps
the other APIs intact.
## Testing
This PR contains tests to verify everything works as expected. You can
test locally by running:
```
cargo test --package bevy_reflect
```
---
## Changelog
### Public Changes
- Added `ArrayInfo::item_info` method
- Added `NamedField::type_info` method
- Added `UnnamedField::type_info` method
- Added `ListInfo::item_info` method
- Added `MapInfo::key_info` method
- Added `MapInfo::value_info` method
- All active fields now have a `Typed` bound (remember that this is
automatically satisfied for all types that derive `Reflect`)
### Internal Changes
- Added `MaybeTyped` trait
## Migration Guide
All active fields for reflected types (including lists, maps, tuples,
etc.), must implement `Typed`. For the majority of users this won't have
any visible impact.
However, users implementing `Reflect` manually may need to update their
types to implement `Typed` if they weren't already.
Additionally, custom dynamic types will need to implement the new hidden
`MaybeTyped` trait.
|
||
|
Gino Valente
|
e512cb602c
|
bevy_reflect: TypeInfo casting methods (#13320)
# Objective
There are times when we might know the type of a `TypeInfo` ahead of
time. Or we may have already checked it one way or another.
In such cases, it's a bit cumbersome to have to pattern match every time
we want to access the nested info:
```rust
if let TypeInfo::List(info) = <Vec<i32>>::type_info() {
// ...
} else {
panic!("expected list info");
}
```
Ideally, there would be a way to simply perform the cast down to
`ListInfo` since we already know it will succeed.
Or even if we don't, perhaps we just want a cleaner way of exiting a
function early (i.e. with the `?` operator).
## Solution
Taking a bit from
[`mirror-mirror`](https://docs.rs/mirror-mirror/latest/mirror_mirror/struct.TypeDescriptor.html#implementations),
`TypeInfo` now has methods for attempting a cast into the variant's info
type.
```rust
let info = <Vec<i32>>::type_info().as_list().unwrap();
// ...
```
These new conversion methods return a `Result` where the error type is a
new `TypeInfoError` enum.
A `Result` was chosen as the return type over `Option` because if we do
choose to `unwrap` it, the error message will give us some indication of
what went wrong. In other words, it can truly replace those instances
where we were panicking in the `else` case.
### Open Questions
1. Should the error types instead be a struct? I chose an enum for
future-proofing, but right now it only has one error state.
Alternatively, we could make it a reflect-wide casting error so it could
be used for similar methods on `ReflectRef` and friends.
2. I was going to do it in a separate PR but should I just go ahead and
add similar methods to `ReflectRef`, `ReflectMut`, and `ReflectOwned`? 🤔
3. Should we name these `try_as_***` instead of `as_***` since they
return a `Result`?
## Testing
You can test locally by running:
```
cargo test --package bevy_reflect
```
---
## Changelog
### Added
- `TypeInfoError` enum
- `TypeInfo::kind` method
- `TypeInfo::as_struct` method
- `TypeInfo::as_tuple_struct` method
- `TypeInfo::as_tuple` method
- `TypeInfo::as_list` method
- `TypeInfo::as_array` method
- `TypeInfo::as_map` method
- `TypeInfo::as_enum` method
- `TypeInfo::as_value` method
- `VariantInfoError` enum
- `VariantInfo::variant_type` method
- `VariantInfo::as_unit_variant` method
- `VariantInfo::as_tuple_variant` method
- `VariantInfo::as_struct_variant` method
|
||
|
Gino Valente
|
99c9218b56
|
bevy_reflect: Feature-gate function reflection (#14174)
# Objective Function reflection requires a lot of macro code generation in the form of several `all_tuples!` invocations, as well as impls generated in the `Reflect` derive macro. Seeing as function reflection is currently a bit more niche, it makes sense to gate it all behind a feature. ## Solution Add a `functions` feature to `bevy_reflect`, which can be enabled in Bevy using the `reflect_functions` feature. ## Testing You can test locally by running: ``` cargo test --package bevy_reflect ``` That should ensure that everything still works with the feature disabled. To test with the feature on, you can run: ``` cargo test --package bevy_reflect --features functions ``` --- ## Changelog - Moved function reflection behind a Cargo feature (`bevy/reflect_functions` and `bevy_reflect/functions`) - Add `IntoFunction` export in `bevy_reflect::prelude` ## 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. Function reflection is now gated behind a feature. To use function reflection, enable the feature: - If using `bevy_reflect` directly, enable the `functions` feature - If using `bevy`, enable the `reflect_functions` feature |
||
|
Giacomo Stevanato
|
d7080369a7
|
Fix intra-doc links and make CI test them (#14076)
# Objective - Bevy currently has lot of invalid intra-doc links, let's fix them! - Also make CI test them, to avoid future regressions. - Helps with #1983 (but doesn't fix it, as there could still be explicit links to docs.rs that are broken) ## Solution - Make `cargo r -p ci -- doc-check` check fail on warnings (could also be changed to just some specific lints) - Manually fix all the warnings (note that in some cases it was unclear to me what the fix should have been, I'll try to highlight them in a self-review) |
||
|
Rob Parrett
|
3594c4f2f5
|
Fix doc list indentation (#14225)
# Objective Fixes #14221 ## Solution Add indentation as suggested. ## Testing Confirmed that - This makes Clippy happy with rust beta - Built docs visually look the same before/after |
||
|
Lura
|
856b39d821
|
Apply Clippy lints regarding lazy evaluation and closures (#14015)
# Objective - Lazily evaluate [default](https://rust-lang.github.io/rust-clippy/master/index.html#/unwrap_or_default)~~/[or](https://rust-lang.github.io/rust-clippy/master/index.html#/or_fun_call)~~ values where it makes sense - ~~`unwrap_or(foo())` -> `unwrap_or_else(|| foo())`~~ - `unwrap_or(Default::default())` -> `unwrap_or_default()` - etc. - Avoid creating [redundant closures](https://rust-lang.github.io/rust-clippy/master/index.html#/redundant_closure), even for [method calls](https://rust-lang.github.io/rust-clippy/master/index.html#/redundant_closure_for_method_calls) - `map(|something| something.into())` -> `map(Into:into)` ## Solution - Apply Clippy lints: - ~~[or_fun_call](https://rust-lang.github.io/rust-clippy/master/index.html#/or_fun_call)~~ - [unwrap_or_default](https://rust-lang.github.io/rust-clippy/master/index.html#/unwrap_or_default) - [redundant_closure_for_method_calls](https://rust-lang.github.io/rust-clippy/master/index.html#/redundant_closure_for_method_calls) ([redundant closures](https://rust-lang.github.io/rust-clippy/master/index.html#/redundant_closure) is already enabled) ## Testing - Tested on Windows 11 (`stable-x86_64-pc-windows-gnu`, 1.79.0) - Bevy compiles without errors or warnings and examples seem to work as intended - `cargo clippy` ✅ - `cargo run -p ci -- compile` ✅ --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> |
||
|
Gino Valente
|
276dd04001
|
bevy_reflect: Function reflection (#13152)
# Objective
We're able to reflect types sooooooo... why not functions?
The goal of this PR is to make functions callable within a dynamic
context, where type information is not readily available at compile
time.
For example, if we have a function:
```rust
fn add(left: i32, right: i32) -> i32 {
left + right
}
```
And two `Reflect` values we've already validated are `i32` types:
```rust
let left: Box<dyn Reflect> = Box::new(2_i32);
let right: Box<dyn Reflect> = Box::new(2_i32);
```
We should be able to call `add` with these values:
```rust
// ?????
let result: Box<dyn Reflect> = add.call_dynamic(left, right);
```
And ideally this wouldn't just work for functions, but methods and
closures too!
Right now, users have two options:
1. Manually parse the reflected data and call the function themselves
2. Rely on registered type data to handle the conversions for them
For a small function like `add`, this isn't too bad. But what about for
more complex functions? What about for many functions?
At worst, this process is error-prone. At best, it's simply tedious.
And this is assuming we know the function at compile time. What if we
want to accept a function dynamically and call it with our own
arguments?
It would be much nicer if `bevy_reflect` could alleviate some of the
problems here.
## Solution
Added function reflection!
This adds a `DynamicFunction` type to wrap a function dynamically. This
can be called with an `ArgList`, which is a dynamic list of
`Reflect`-containing `Arg` arguments. It returns a `FunctionResult`
which indicates whether or not the function call succeeded, returning a
`Reflect`-containing `Return` type if it did succeed.
Many functions can be converted into this `DynamicFunction` type thanks
to the `IntoFunction` trait.
Taking our previous `add` example, this might look something like
(explicit types added for readability):
```rust
fn add(left: i32, right: i32) -> i32 {
left + right
}
let mut function: DynamicFunction = add.into_function();
let args: ArgList = ArgList::new().push_owned(2_i32).push_owned(2_i32);
let result: Return = function.call(args).unwrap();
let value: Box<dyn Reflect> = result.unwrap_owned();
assert_eq!(value.take::<i32>().unwrap(), 4);
```
And it also works on closures:
```rust
let add = |left: i32, right: i32| left + right;
let mut function: DynamicFunction = add.into_function();
let args: ArgList = ArgList::new().push_owned(2_i32).push_owned(2_i32);
let result: Return = function.call(args).unwrap();
let value: Box<dyn Reflect> = result.unwrap_owned();
assert_eq!(value.take::<i32>().unwrap(), 4);
```
As well as methods:
```rust
#[derive(Reflect)]
struct Foo(i32);
impl Foo {
fn add(&mut self, value: i32) {
self.0 += value;
}
}
let mut foo = Foo(2);
let mut function: DynamicFunction = Foo::add.into_function();
let args: ArgList = ArgList::new().push_mut(&mut foo).push_owned(2_i32);
function.call(args).unwrap();
assert_eq!(foo.0, 4);
```
### Limitations
While this does cover many functions, it is far from a perfect system
and has quite a few limitations. Here are a few of the limitations when
using `IntoFunction`:
1. The lifetime of the return value is only tied to the lifetime of the
first argument (useful for methods). This means you can't have a
function like `(a: i32, b: &i32) -> &i32` without creating the
`DynamicFunction` manually.
2. Only 15 arguments are currently supported. If the first argument is a
(mutable) reference, this number increases to 16.
3. Manual implementations of `Reflect` will need to implement the new
`FromArg`, `GetOwnership`, and `IntoReturn` traits in order to be used
as arguments/return types.
And some limitations of `DynamicFunction` itself:
1. All arguments share the same lifetime, or rather, they will shrink to
the shortest lifetime.
2. Closures that capture their environment may need to have their
`DynamicFunction` dropped before accessing those variables again (there
is a `DynamicFunction::call_once` to make this a bit easier)
3. All arguments and return types must implement `Reflect`. While not a
big surprise coming from `bevy_reflect`, this implementation could
actually still work by swapping `Reflect` out with `Any`. Of course,
that makes working with the arguments and return values a bit harder.
4. Generic functions are not supported (unless they have been manually
monomorphized)
And general, reflection gotchas:
1. `&str` does not implement `Reflect`. Rather, `&'static str`
implements `Reflect` (the same is true for `&Path` and similar types).
This means that `&'static str` is considered an "owned" value for the
sake of generating arguments. Additionally, arguments and return types
containing `&str` will assume it's `&'static str`, which is almost never
the desired behavior. In these cases, the only solution (I believe) is
to use `&String` instead.
### Followup Work
This PR is the first of two PRs I intend to work on. The second PR will
aim to integrate this new function reflection system into the existing
reflection traits and `TypeInfo`. The goal would be to register and call
a reflected type's methods dynamically.
I chose not to do that in this PR since the diff is already quite large.
I also want the discussion for both PRs to be focused on their own
implementation.
Another followup I'd like to do is investigate allowing common container
types as a return type, such as `Option<&[mut] T>` and `Result<&[mut] T,
E>`. This would allow even more functions to opt into this system. I
chose to not include it in this one, though, for the same reasoning as
previously mentioned.
### Alternatives
One alternative I had considered was adding a macro to convert any
function into a reflection-based counterpart. The idea would be that a
struct that wraps the function would be created and users could specify
which arguments and return values should be `Reflect`. It could then be
called via a new `Function` trait.
I think that could still work, but it will be a fair bit more involved,
requiring some slightly more complex parsing. And it of course is a bit
more work for the user, since they need to create the type via macro
invocation.
It also makes registering these functions onto a type a bit more
complicated (depending on how it's implemented).
For now, I think this is a fairly simple, yet powerful solution that
provides the least amount of friction for users.
---
## Showcase
Bevy now adds support for storing and calling functions dynamically
using reflection!
```rust
// 1. Take a standard Rust function
fn add(left: i32, right: i32) -> i32 {
left + right
}
// 2. Convert it into a type-erased `DynamicFunction` using the `IntoFunction` trait
let mut function: DynamicFunction = add.into_function();
// 3. Define your arguments from reflected values
let args: ArgList = ArgList::new().push_owned(2_i32).push_owned(2_i32);
// 4. Call the function with your arguments
let result: Return = function.call(args).unwrap();
// 5. Extract the return value
let value: Box<dyn Reflect> = result.unwrap_owned();
assert_eq!(value.take::<i32>().unwrap(), 4);
```
## Changelog
#### TL;DR
- Added support for function reflection
- Added a new `Function Reflection` example:
|
||
|
Gino Valente
|
53910e07ae
|
bevy_reflect: Improve reflection serialization error messages (#13867)
# Objective The error messages that appear when a value cannot be serialized or deserialized via reflection could be slightly improved. When one of these operations fails, some users are confused about how to resolve the issue. I've spoken with a few who didn't know they could register `ReflectSerialize` themselves. We should try to clarify this to some degree in the error messages. ## Solution Add some more detail to the error messages. For example, replacing this: ``` Type 'core::ops::RangeInclusive<f32>' did not register ReflectSerialize ``` with this: ``` Type `core::ops::RangeInclusive<f32>` did not register the `ReflectSerialize` type data. For certain types, this may need to be registered manually using `register_type_data` ``` I also added a separate error message if the type was not registered in the type registry at all: ``` Type `core::ops::RangeInclusive<f32>` is not registered in the type registry ``` ## Testing You can test locally by running: ``` cargo test --package bevy_reflect ``` --- ## Changelog - Added error message for missing type registration when serializing reflect data - Changed error message for missing `ReflectSerialize` registration when serializing reflect data - Changed error message for missing `ReflectDeserialize` registration when deserializing reflect data |
||
|
Jan Hohenheim
|
6273227e09
|
Fix lints introduced in Rust beta 1.80 (#13899)
Resolves #13895 Mostly just involves being more explicit about which parts of the docs belong to a list and which begin a new paragraph. - found a few docs that were malformed because of exactly this, so I fixed that by introducing a paragraph - added indentation to nearly all multiline lists - fixed a few minor typos - added `#[allow(dead_code)]` to types that are needed to test annotations but are never constructed ([here](https://github.com/bevyengine/bevy/pull/13899/files#diff-b02b63604e569c8577c491e7a2030d456886d8f6716eeccd46b11df8aac75dafR1514) and [here](https://github.com/bevyengine/bevy/pull/13899/files#diff-b02b63604e569c8577c491e7a2030d456886d8f6716eeccd46b11df8aac75dafR1523)) - verified that `cargo +beta run -p ci -- lints` passes - verified that `cargo +beta run -p ci -- test` passes |
||
|
Wuketuke
|
d45bcfd043
|
improved the error message by insert_boxed (issue #13646) (again) (#13706)
previously I worked on fixing issue #13646, back when the error message did not include the type at all. But that error message had room for improvement, so I included the feedback of @alice-i-cecile and @MrGVSV. The error message will now read `the given key (of type bevy_reflect::tests::Foo) does not support hashing` or 'the given key (of type bevy_reflect::DynamicStruct) does not support hashing' in case of a dynamic struct that represents a hashable struct i also added a new unit test for this new behaviour (`reflect_map_no_hash_dynamic`). Fixes #13646 (again) --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com> |
||
|
Wuketuke
|
2eb9d5cc38
|
hashing error in bevy_reflect now includes the type (bevyengine#13646) (#13691)
# Objective If you try to add an object to the hashmap that is not capable of hashing, the program panics. For easier debugging, the type for that object should be included in the error message. Fixes #13646. ## Solution initially i tried calling std::any::type_name_of_val. this had the problem that it would print something like dyn Box<dyn Reflect>, not helpful. But since these objects all implement Reflect, i used Reflect::type_path() instead. Previously, the error message was part of a constant called HASH_ERROR. i changed that to a macro called hash_error to print the type of that object more easily ## Testing i adapted the unit test reflect_map_no_hash to expect the type in that panic aswell since this is my first contribution, please let me know if i have done everything properly |
||
|
Alice Cecile
|
ec7b3490f6
|
Add on_unimplemented Diagnostics to Most Public Traits (#13347) (#13662)
# Objective - #13414 did not have the intended effect. - #13404 is still blocked ## Solution - Re-adds #13347. Co-authored-by: Zachary Harrold <zac@harrold.com.au> Co-authored-by: Jamie Ridding <Themayu@users.noreply.github.com> Co-authored-by: BD103 <59022059+BD103@users.noreply.github.com> |
||
|
Olle Lukowski
|
8c7f73ab81
|
Move bevy_math Reflect impls (#13520)
# Objective Fixes #13456 ## Solution Moved `bevy_math`'s `Reflect` impls from `bevy_reflect` to `bevy_math`. ### Quick note I accidentally used the same commit message while resolving a merge conflict (first time I had to resolve a conflict). Sorry about that. |
||
|
Salvador Carvalhinho
|
7d843e0c08
|
Implement Rhombus 2D primitive. (#13501)
# Objective - Create a new 2D primitive, Rhombus, also knows as "Diamond Shape" - Simplify the creation and handling of isometric projections - Extend Bevy's arsenal of 2D primitives ## Testing - New unit tests created in bevy_math/ primitives and bev_math/ bounding - Tested translations, rotations, wireframe, bounding sphere, aabb and creation parameters --------- Co-authored-by: Luís Figueiredo <luispcfigueiredo@tecnico.ulisboa.pt> |
||
|
Gino Valente
|
5db52663b3
|
bevy_reflect: Custom attributes (#11659)
# Objective As work on the editor starts to ramp up, it might be nice to start allowing types to specify custom attributes. These can be used to provide certain functionality to fields, such as ranges or controlling how data is displayed. A good example of this can be seen in [`bevy-inspector-egui`](https://github.com/jakobhellermann/bevy-inspector-egui) with its [`InspectorOptions`](https://docs.rs/bevy-inspector-egui/0.22.1/bevy_inspector_egui/struct.InspectorOptions.html): ```rust #[derive(Reflect, Default, InspectorOptions)] #[reflect(InspectorOptions)] struct Slider { #[inspector(min = 0.0, max = 1.0)] value: f32, } ``` Normally, as demonstrated in the example above, these attributes are handled by a derive macro and stored in a corresponding `TypeData` struct (i.e. `ReflectInspectorOptions`). Ideally, we would have a good way of defining this directly via reflection so that users don't need to create and manage a whole proc macro just to allow these sorts of attributes. And note that this doesn't have to just be for inspectors and editors. It can be used for things done purely on the code side of things. ## Solution Create a new method for storing attributes on fields via the `Reflect` derive. These custom attributes are stored in type info (e.g. `NamedField`, `StructInfo`, etc.). ```rust #[derive(Reflect)] struct Slider { #[reflect(@0.0..=1.0)] value: f64, } let TypeInfo::Struct(info) = Slider::type_info() else { panic!("expected struct info"); }; let field = info.field("value").unwrap(); let range = field.get_attribute::<RangeInclusive<f64>>().unwrap(); assert_eq!(*range, 0.0..=1.0); ``` ## TODO - [x] ~~Bikeshed syntax~~ Went with a type-based approach, prefixed by `@` for ease of parsing and flexibility - [x] Add support for custom struct/tuple struct field attributes - [x] Add support for custom enum variant field attributes - [x] ~~Add support for custom enum variant attributes (maybe?)~~ ~~Will require a larger refactor. Can be saved for a future PR if we really want it.~~ Actually, we apparently still have support for variant attributes despite not using them, so it was pretty easy to add lol. - [x] Add support for custom container attributes - [x] Allow custom attributes to store any reflectable value (not just `Lit`) - [x] ~~Store attributes in registry~~ This PR used to store these in attributes in the registry, however, it has since switched over to storing them in type info - [x] Add example ## Bikeshedding > [!note] > This section was made for the old method of handling custom attributes, which stored them by name (i.e. `some_attribute = 123`). The PR has shifted away from that, to a more type-safe approach. > > This section has been left for reference. There are a number of ways we can syntactically handle custom attributes. Feel free to leave a comment on your preferred one! Ideally we want one that is clear, readable, and concise since these will potentially see _a lot_ of use. Below is a small, non-exhaustive list of them. Note that the `skip_serializing` reflection attribute is added to demonstrate how each case plays with existing reflection attributes. <details> <summary>List</summary> ##### 1. `@(name = value)` > The `@` was chosen to make them stand out from other attributes and because the "at" symbol is a subtle pneumonic for "attribute". Of course, other symbols could be used (e.g. `$`, `#`, etc.). ```rust #[derive(Reflect)] struct Slider { #[reflect(@(min = 0.0, max = 1.0), skip_serializing)] #[[reflect(@(bevy_editor::hint = "Range: 0.0 to 1.0"))] value: f32, } ``` ##### 2. `@name = value` > This is my personal favorite. ```rust #[derive(Reflect)] struct Slider { #[reflect(@min = 0.0, @max = 1.0, skip_serializing)] #[[reflect(@bevy_editor::hint = "Range: 0.0 to 1.0")] value: f32, } ``` ##### 3. `custom_attr(name = value)` > `custom_attr` can be anything. Other possibilities include `with` or `tag`. ```rust #[derive(Reflect)] struct Slider { #[reflect(custom_attr(min = 0.0, max = 1.0), skip_serializing)] #[[reflect(custom_attr(bevy_editor::hint = "Range: 0.0 to 1.0"))] value: f32, } ``` ##### 4. `reflect_attr(name = value)` ```rust #[derive(Reflect)] struct Slider { #[reflect(skip_serializing)] #[reflect_attr(min = 0.0, max = 1.0)] #[[reflect_attr(bevy_editor::hint = "Range: 0.0 to 1.0")] value: f32, } ``` </details> --- ## Changelog - Added support for custom attributes on reflected types (i.e. `#[reflect(@Foo::new("bar")]`) |
||
|
Alice Cecile
|
ee6dfd35c9
|
Revert "Add on_unimplemented Diagnostics to Most Public Traits" (#13413)
# Objective - Rust 1.78 breaks all Android support, see https://github.com/bevyengine/bevy/issues/13331 - We should not bump the MSRV to 1.78 until that's resolved in #13366. ## Solution - Temporarily revert https://github.com/bevyengine/bevy/pull/13347 Co-authored-by: Alice Cecile <alice.i.cecil@gmail.com> |
||
|
Zachary Harrold
|
11f0a2dcde
|
Add on_unimplemented Diagnostics to Most Public Traits (#13347)
# Objective - Fixes #12377 ## Solution Added simple `#[diagnostic::on_unimplemented(...)]` attributes to some critical public traits providing a more approachable initial error message. Where appropriate, a `note` is added indicating that a `derive` macro is available. ## Examples <details> <summary>Examples hidden for brevity</summary> Below is a collection of examples showing the new error messages produced by this change. In general, messages will start with a more Bevy-centric error message (e.g., _`MyComponent` is not a `Component`_), and a note directing the user to an available derive macro where appropriate. ### Missing `#[derive(Resource)]` <details> <summary>Example Code</summary> ```rust use bevy::prelude::*; struct MyResource; fn main() { App::new() .insert_resource(MyResource) .run(); } ``` </details> <details> <summary>Error Generated</summary> ```error error[E0277]: `MyResource` is not a `Resource` --> examples/app/empty.rs:7:26 | 7 | .insert_resource(MyResource) | --------------- ^^^^^^^^^^ invalid `Resource` | | | required by a bound introduced by this call | = help: the trait `Resource` is not implemented for `MyResource` = note: consider annotating `MyResource` with `#[derive(Resource)]` = help: the following other types implement trait `Resource`: AccessibilityRequested ManageAccessibilityUpdates bevy::bevy_a11y::Focus DiagnosticsStore FrameCount bevy::prelude::State<S> SystemInfo bevy::prelude::Axis<T> and 141 others note: required by a bound in `bevy::prelude::App::insert_resource` --> C:\Users\Zac\Documents\GitHub\bevy\crates\bevy_app\src\app.rs:419:31 | 419 | pub fn insert_resource<R: Resource>(&mut self, resource: R) -> &mut Self { | ^^^^^^^^ required by this bound in `App::insert_resource` ``` </details> ### Putting A `QueryData` in a `QueryFilter` Slot <details> <summary>Example Code</summary> ```rust use bevy::prelude::*; #[derive(Component)] struct A; #[derive(Component)] struct B; fn my_system(_query: Query<&A, &B>) {} fn main() { App::new() .add_systems(Update, my_system) .run(); } ``` </details> <details> <summary>Error Generated</summary> ```error error[E0277]: `&B` is not a valid `Query` filter --> examples/app/empty.rs:9:22 | 9 | fn my_system(_query: Query<&A, &B>) {} | ^^^^^^^^^^^^^ invalid `Query` filter | = help: the trait `QueryFilter` is not implemented for `&B` = help: the following other types implement trait `QueryFilter`: With<T> Without<T> bevy::prelude::Or<()> bevy::prelude::Or<(F0,)> bevy::prelude::Or<(F0, F1)> bevy::prelude::Or<(F0, F1, F2)> bevy::prelude::Or<(F0, F1, F2, F3)> bevy::prelude::Or<(F0, F1, F2, F3, F4)> and 28 others note: required by a bound in `bevy::prelude::Query` --> C:\Users\Zac\Documents\GitHub\bevy\crates\bevy_ecs\src\system\query.rs:349:51 | 349 | pub struct Query<'world, 'state, D: QueryData, F: QueryFilter = ()> { | ^^^^^^^^^^^ required by this bound in `Query` ``` </details> ### Missing `#[derive(Component)]` <details> <summary>Example Code</summary> ```rust use bevy::prelude::*; struct A; fn my_system(mut commands: Commands) { commands.spawn(A); } fn main() { App::new() .add_systems(Startup, my_system) .run(); } ``` </details> <details> <summary>Error Generated</summary> ```error error[E0277]: `A` is not a `Bundle` --> examples/app/empty.rs:6:20 | 6 | commands.spawn(A); | ----- ^ invalid `Bundle` | | | required by a bound introduced by this call | = help: the trait `bevy::prelude::Component` is not implemented for `A`, which is required by `A: Bundle` = note: consider annotating `A` with `#[derive(Component)]` or `#[derive(Bundle)]` = help: the following other types implement trait `Bundle`: TransformBundle SceneBundle DynamicSceneBundle AudioSourceBundle<Source> SpriteBundle SpriteSheetBundle Text2dBundle MaterialMesh2dBundle<M> and 34 others = note: required for `A` to implement `Bundle` note: required by a bound in `bevy::prelude::Commands::<'w, 's>::spawn` --> C:\Users\Zac\Documents\GitHub\bevy\crates\bevy_ecs\src\system\commands\mod.rs:243:21 | 243 | pub fn spawn<T: Bundle>(&mut self, bundle: T) -> EntityCommands { | ^^^^^^ required by this bound in `Commands::<'w, 's>::spawn` ``` </details> ### Missing `#[derive(Asset)]` <details> <summary>Example Code</summary> ```rust use bevy::prelude::*; struct A; fn main() { App::new() .init_asset::<A>() .run(); } ``` </details> <details> <summary>Error Generated</summary> ```error error[E0277]: `A` is not an `Asset` --> examples/app/empty.rs:7:23 | 7 | .init_asset::<A>() | ---------- ^ invalid `Asset` | | | required by a bound introduced by this call | = help: the trait `Asset` is not implemented for `A` = note: consider annotating `A` with `#[derive(Asset)]` = help: the following other types implement trait `Asset`: Font AnimationGraph DynamicScene Scene AudioSource Pitch bevy::bevy_gltf::Gltf GltfNode and 17 others note: required by a bound in `init_asset` --> C:\Users\Zac\Documents\GitHub\bevy\crates\bevy_asset\src\lib.rs:307:22 | 307 | fn init_asset<A: Asset>(&mut self) -> &mut Self; | ^^^^^ required by this bound in `AssetApp::init_asset` ``` </details> ### Mismatched Input and Output on System Piping <details> <summary>Example Code</summary> ```rust use bevy::prelude::*; fn producer() -> u32 { 123 } fn consumer(_: In<u16>) {} fn main() { App::new() .add_systems(Update, producer.pipe(consumer)) .run(); } ``` </details> <details> <summary>Error Generated</summary> ```error error[E0277]: `fn(bevy::prelude::In<u16>) {consumer}` is not a valid system with input `u32` and output `_` --> examples/app/empty.rs:11:44 | 11 | .add_systems(Update, producer.pipe(consumer)) | ---- ^^^^^^^^ invalid system | | | required by a bound introduced by this call | = help: the trait `bevy::prelude::IntoSystem<u32, _, _>` is not implemented for fn item `fn(bevy::prelude::In<u16>) {consumer}` = note: expecting a system which consumes `u32` and produces `_` note: required by a bound in `pipe` --> C:\Users\Zac\Documents\GitHub\bevy\crates\bevy_ecs\src\system\mod.rs:168:12 | 166 | fn pipe<B, Final, MarkerB>(self, system: B) -> PipeSystem<Self::System, B::System> | ---- required by a bound in this associated function 167 | where 168 | B: IntoSystem<Out, Final, MarkerB>, | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `IntoSystem::pipe` ``` </details> ### Missing Reflection <details> <summary>Example Code</summary> ```rust use bevy::prelude::*; #[derive(Component)] struct MyComponent; fn main() { App::new() .register_type::<MyComponent>() .run(); } ``` </details> <details> <summary>Error Generated</summary> ```error error[E0277]: `MyComponent` does not provide type registration information --> examples/app/empty.rs:8:26 | 8 | .register_type::<MyComponent>() | ------------- ^^^^^^^^^^^ the trait `GetTypeRegistration` is not implemented for `MyComponent` | | | required by a bound introduced by this call | = note: consider annotating `MyComponent` with `#[derive(Reflect)]` = help: the following other types implement trait `GetTypeRegistration`: bool char isize i8 i16 i32 i64 i128 and 443 others note: required by a bound in `bevy::prelude::App::register_type` --> C:\Users\Zac\Documents\GitHub\bevy\crates\bevy_app\src\app.rs:619:29 | 619 | pub fn register_type<T: bevy_reflect::GetTypeRegistration>(&mut self) -> &mut Self { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `App::register_type` ``` </details> ### Missing `#[derive(States)]` Implementation <details> <summary>Example Code</summary> ```rust use bevy::prelude::*; #[derive(Debug, Clone, Copy, Default, Eq, PartialEq, Hash)] enum AppState { #[default] Menu, InGame { paused: bool, turbo: bool, }, } fn main() { App::new() .init_state::<AppState>() .run(); } ``` </details> <details> <summary>Error Generated</summary> ```error error[E0277]: the trait bound `AppState: FreelyMutableState` is not satisfied --> examples/app/empty.rs:15:23 | 15 | .init_state::<AppState>() | ---------- ^^^^^^^^ the trait `FreelyMutableState` is not implemented for `AppState` | | | required by a bound introduced by this call | = note: consider annotating `AppState` with `#[derive(States)]` note: required by a bound in `bevy::prelude::App::init_state` --> C:\Users\Zac\Documents\GitHub\bevy\crates\bevy_app\src\app.rs:282:26 | 282 | pub fn init_state<S: FreelyMutableState + FromWorld>(&mut self) -> &mut Self { | ^^^^^^^^^^^^^^^^^^ required by this bound in `App::init_state` ``` </details> ### Adding a `System` with Unhandled Output <details> <summary>Example Code</summary> ```rust use bevy::prelude::*; fn producer() -> u32 { 123 } fn main() { App::new() .add_systems(Update, consumer) .run(); } ``` </details> <details> <summary>Error Generated</summary> ```error error[E0277]: `fn() -> u32 {producer}` does not describe a valid system configuration --> examples/app/empty.rs:9:30 | 9 | .add_systems(Update, producer) | ----------- ^^^^^^^^ invalid system configuration | | | required by a bound introduced by this call | = help: the trait `IntoSystem<(), (), _>` is not implemented for fn item `fn() -> u32 {producer}`, which is required by `fn() -> u32 {producer}: IntoSystemConfigs<_>` = help: the following other types implement trait `IntoSystemConfigs<Marker>`: <Box<(dyn bevy::prelude::System<In = (), Out = ()> + 'static)> as IntoSystemConfigs<()>> <NodeConfigs<Box<(dyn bevy::prelude::System<In = (), Out = ()> + 'static)>> as IntoSystemConfigs<()>> <(S0,) as IntoSystemConfigs<(SystemConfigTupleMarker, P0)>> <(S0, S1) as IntoSystemConfigs<(SystemConfigTupleMarker, P0, P1)>> <(S0, S1, S2) as IntoSystemConfigs<(SystemConfigTupleMarker, P0, P1, P2)>> <(S0, S1, S2, S3) as IntoSystemConfigs<(SystemConfigTupleMarker, P0, P1, P2, P3)>> <(S0, S1, S2, S3, S4) as IntoSystemConfigs<(SystemConfigTupleMarker, P0, P1, P2, P3, P4)>> <(S0, S1, S2, S3, S4, S5) as IntoSystemConfigs<(SystemConfigTupleMarker, P0, P1, P2, P3, P4, P5)>> and 14 others = note: required for `fn() -> u32 {producer}` to implement `IntoSystemConfigs<_>` note: required by a bound in `bevy::prelude::App::add_systems` --> C:\Users\Zac\Documents\GitHub\bevy\crates\bevy_app\src\app.rs:342:23 | 339 | pub fn add_systems<M>( | ----------- required by a bound in this associated function ... 342 | systems: impl IntoSystemConfigs<M>, | ^^^^^^^^^^^^^^^^^^^^ required by this bound in `App::add_systems` ``` </details> </details> ## Testing CI passed locally. ## Migration Guide Upgrade to version 1.78 (or higher) of Rust. ## Future Work - Currently, hints are not supported in this diagnostic. Ideally, suggestions like _"consider using ..."_ would be in a hint rather than a note, but that is the best option for now. - System chaining and other `all_tuples!(...)`-based traits have bad error messages due to the slightly different error message format. --------- Co-authored-by: Jamie Ridding <Themayu@users.noreply.github.com> Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> Co-authored-by: BD103 <59022059+BD103@users.noreply.github.com> |
||
|
Ben Harper
|
be03ba1b68
|
Add reflect impls for bevy_math curve structs (#13348)
# Objective Fixes #13189 ## Solution To add the reflect impls I needed to make all the struct fields pub. I don't think there's any harm for these types, but just a note for review. --------- Co-authored-by: Ben Harper <ben@tukom.org> |
||
|
rmsthebest
|
278380394f
|
Avoid bevy_reflect::List::iter wrapping in release mode (#13271)
# Objective Fixes #13230 ## Solution Uses solution described in #13230 They mention a worry about adding a branch, but I'm not sure there is one. This code ```Rust #[no_mangle] pub fn next_if_some(num: i32, b: Option<bool>) -> i32 { num + b.is_some() as i32 } ``` produces this assembly with opt-level 3 ```asm next_if_some: xor eax, eax cmp sil, 2 setne al add eax, edi ret ``` ## Testing Added test from #13230, tagged it as ignore as it is only useful in release mode and very slow if you accidentally invoke it in debug mode. --- ## Changelog Iterationg of ListIter will no longer overflow and wrap around ## Migration Guide |
||
|
Brezak
|
9c4ac7c297
|
Finish the work on try_apply (#12646)
# Objective Finish the `try_apply` implementation started in #6770 by @feyokorenhof. Supersedes and closes #6770. Closes #6182 ## Solution Add `try_apply` to `Reflect` and implement it in all the places that implement `Reflect`. --- ## Changelog Added `try_apply` to `Reflect`. --------- Co-authored-by: Feyo Korenhof <feyokorenhof@gmail.com> Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com> |
||
|
Marcel Müller
|
6d25545c51
|
Implement Reflect for Result<T, E> as enum (#13182)
# Objective - Make `Result<T, E>` implement Reflect such that it is an Enum rather than a Value - Fixes #13178 ## Solution - Use the correct macro ## Testing - Did you test these changes? I tried it out locally, and it does what it says on the tin. Not sure how to test it in context of the crate? --- ## Changelog ### Changed - Result now uses `ReflectKind::Enum` rather than `ReflectKind::Value`, allowing for inspection of its constituents ## Migration Guide `Result<T, E>` has had its `Reflect` implementation changed to align it with `Option<T>` and its intended semantics: A carrier of either an `Ok` or `Err` value, and the ability to access it. To achieve this it is no longer a `ReflectKind::Value` but rather a `ReflectKind::Enum` and as such carries these changes with it: For `Result<T, E>` - Both `T` and `E` no longer require to be `Clone` and now require to be `FromReflect` - `<Result<T, E> as Reflect>::reflect_*` now returns a `ReflectKind::Enum`, so any code that previously relied on it being a `Value` kind will have to be adapted. - `Result<T, E>` now implements `Enum` Since the migration is highly dependent on the previous usage, no automatic upgrade path can be given. Signed-off-by: Marcel Müller <neikos@neikos.email> |
||
|
Mateusz Wachowiak
|
64b987921c
|
iter_with_data (#13102)
# Objective - Provide a way to iterate over the registered TypeData. ## Solution - a new method on the `TypeRegistry` that iterates over `TypeRegistrations` with theirs `TypeData` --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com> |
||
|
targrub
|
8316166622
|
Fix uses of "it's" vs "its". (#13033)
Grammar changes only. |
||
|
andristarr
|
2b3e3341d6
|
separating finite and infinite 3d planes (#12426)
# Objective Fixes #12388 ## Solution - Removing the plane3d and adding rect3d primitive mesh |
||
|
Robert Walter
|
2532447dcb
|
impl Reflect for EntityHashSet (#12971)
`EntityHashSet` doesn't seem to implement `Reflect` which seems weird! Especially since `EntityHashMap` implements `Reflect`. This PR just added an extra `impl_reflect_value!` for `EntityHashSet` and this seems to do the trick. I left out doing the same for `StableHashSet` since it's marked as deprecated. --- I'm really wondering what was the issue here. If anyone can explain why `EntityHashSet` can't use the `Reflect` impl of `bevy_utils::HashSet` similar to how it's the case with the `...HashMap`s I'd be interested! |
||
|
BD103
|
7b8d502083
|
Fix beta lints (#12980)
# Objective - Fixes #12976 ## Solution This one is a doozy. - Run `cargo +beta clippy --workspace --all-targets --all-features` and fix all issues - This includes: - Moving inner attributes to be outer attributes, when the item in question has both inner and outer attributes - Use `ptr::from_ref` in more scenarios - Extend the valid idents list used by `clippy:doc_markdown` with more names - Use `Clone::clone_from` when possible - Remove redundant `ron` import - Add backticks to **so many** identifiers and items - I'm sorry whoever has to review this --- ## Changelog - Added links to more identifiers in documentation. |
||
|
BD103
|
aa2ebbb43f
|
Fix some nightly Clippy lints (#12927)
# Objective - I daily drive nightly Rust when developing Bevy, so I notice when new warnings are raised by `cargo check` and Clippy. - `cargo +nightly clippy` raises a few of these new warnings. ## Solution - Fix most warnings from `cargo +nightly clippy` - I skipped the docs-related warnings because some were covered by #12692. - Use `Clone::clone_from` in applicable scenarios, which can sometimes avoid an extra allocation. - Implement `Default` for structs that have a `pub const fn new() -> Self` method. - Fix an occurrence where generic constraints were defined in both `<C: Trait>` and `where C: Trait`. - Removed generic constraints that were implied by the `Bundle` trait. --- ## Changelog - `BatchingStrategy`, `NonGenericTypeCell`, and `GenericTypeCell` now implement `Default`. |
||
|
Matty
|
c8aa3ac7d1
|
Meshing for Annulus primitive (#12734)
# Objective Related to #10572 Allow the `Annulus` primitive to be meshed. ## Solution We introduce a `Meshable` structure, `AnnulusMeshBuilder`, which allows the `Annulus` primitive to be meshed, leaving optional configuration of the number of angular sudivisions to the user. Here is a picture of the annulus's UV-mapping: <img width="1440" alt="Screenshot 2024-03-26 at 10 39 48 AM" src="https://github.com/bevyengine/bevy/assets/2975848/b170291d-cba7-441b-90ee-2ad6841eaedb"> Other features are essentially identical to the implementations for `Circle`/`Ellipse`. --- ## Changelog - Introduced `AnnulusMeshBuilder` - Implemented `Meshable` for `Annulus` with `Output = AnnulusMeshBuilder` - Implemented `From<Annulus>` and `From<AnnulusMeshBuilder>` for `Mesh` - Added `impl_reflect!` declaration for `Annulus` and `Triangle3d` in `bevy_reflect` --- ## Discussion ### Design considerations The only interesting wrinkle here is that the existing UV-mapping of `Ellipse` (and hence of `Circle` and `RegularPolygon`) is non-radial (it's skew-free, created by situating the mesh in a bounding rectangle), so the UV-mapping of `Annulus` doesn't limit to that of `Circle` as its inner radius tends to zero, for instance. I don't see this as a real issue for `Annulus`, which should almost certainly have this kind of UV-mapping, but I think we ought to at least consider allowing mesh configuration for `Circle`/`Ellipse` that performs radial UV-mapping instead. (In these cases in particular, it would be especially easy, since we wouldn't need a different parameter set in the builder.) --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> |
||
|
Jakub Marcowski
|
20ee56e719
|
Add Tetrahedron primitive to bevy_math::primitives (#12688)
# Objective - #10572 There is no 3D primitive available for the common shape of a tetrahedron (3-simplex). ## Solution This PR introduces a new type to the existing math primitives: - `Tetrahedron`: a polyhedron composed of four triangular faces, six straight edges, and four vertices --- ## Changelog ### Added - `Tetrahedron` primitive to the `bevy_math` crate - `Tetrahedron` tests (`area`, `volume` methods) - `impl_reflect!` declaration for `Tetrahedron` in the `bevy_reflect` crate |
||
|
BD103
|
84363f2fab
|
Remove redundant imports (#12817)
# Objective - There are several redundant imports in the tests and examples that are not caught by CI because additional flags need to be passed. ## Solution - Run `cargo check --workspace --tests` and `cargo check --workspace --examples`, then fix all warnings. - Add `test-check` to CI, which will be run in the check-compiles job. This should catch future warnings for tests. Examples are already checked, but I'm not yet sure why they weren't caught. ## Discussion - Should the `--tests` and `--examples` flags be added to CI, so this is caught in the future? - If so, #12818 will need to be merged first. It was also a warning raised by checking the examples, but I chose to split off into a separate PR. --------- Co-authored-by: François Mockers <francois.mockers@vleue.com> |
||
|
James Liu
|
56bcbb0975
|
Forbid unsafe in most crates in the engine (#12684)
# Objective Resolves #3824. `unsafe` code should be the exception, not the norm in Rust. It's obviously needed for various use cases as it's interfacing with platforms and essentially running the borrow checker at runtime in the ECS, but the touted benefits of Bevy is that we are able to heavily leverage Rust's safety, and we should be holding ourselves accountable to that by minimizing our unsafe footprint. ## Solution Deny `unsafe_code` workspace wide. Add explicit exceptions for the following crates, and forbid it in almost all of the others. * bevy_ecs - Obvious given how much unsafe is needed to achieve performant results * bevy_ptr - Works with raw pointers, even more low level than bevy_ecs. * bevy_render - due to needing to integrate with wgpu * bevy_window - due to needing to integrate with raw_window_handle * bevy_utils - Several unsafe utilities used by bevy_ecs. Ideally moved into bevy_ecs instead of made publicly usable. * bevy_reflect - Required for the unsafe type casting it's doing. * bevy_transform - for the parallel transform propagation * bevy_gizmos - For the SystemParam impls it has. * bevy_assets - To support reflection. Might not be required, not 100% sure yet. * bevy_mikktspace - due to being a conversion from a C library. Pending safe rewrite. * bevy_dynamic_plugin - Inherently unsafe due to the dynamic loading nature. Several uses of unsafe were rewritten, as they did not need to be using them: * bevy_text - a case of `Option::unchecked` could be rewritten as a normal for loop and match instead of an iterator. * bevy_color - the Pod/Zeroable implementations were replaceable with bytemuck's derive macros. |
||
|
Gino Valente
|
0265436fff
|
bevy_reflect: Rename UntypedReflectDeserializer to ReflectDeserializer (#12721)
# Objective We have `ReflectSerializer` and `TypedReflectSerializer`. The former is the one users will most often use since the latter takes a bit more effort to deserialize. However, our deserializers are named `UntypedReflectDeserializer` and `TypedReflectDeserializer`. There is no obvious indication that `UntypedReflectDeserializer` must be used with `ReflectSerializer` since the names don't quite match up. ## Solution Rename `UntypedReflectDeserializer` back to `ReflectDeserializer` (initially changed as part of #5723). Also update the docs for both deserializers (as they were pretty out of date) and include doc examples. I also updated the docs for the serializers, too, just so that everything is consistent. --- ## Changelog - Renamed `UntypedReflectDeserializer` to `ReflectDeserializer` - Updated docs for `ReflectDeserializer`, `TypedReflectDeserializer`, `ReflectSerializer`, and `TypedReflectSerializer` ## Migration Guide `UntypedReflectDeserializer` has been renamed to `ReflectDeserializer`. Usages will need to be updated accordingly. ```diff - let reflect_deserializer = UntypedReflectDeserializer::new(®istry); + let reflect_deserializer = ReflectDeserializer::new(®istry); ``` |
||
|
James Liu
|
f096ad4155
|
Set the logo and favicon for all of Bevy's published crates (#12696)
# Objective Currently the built docs only shows the logo and favicon for the top level `bevy` crate. This makes views like https://docs.rs/bevy_ecs/latest/bevy_ecs/ look potentially unrelated to the project at first glance. ## Solution Reproduce the docs attributes for every crate that Bevy publishes. Ideally this would be done with some workspace level Cargo.toml control, but AFAICT, such support does not exist. |
||
|
Ame
|
72c51cdab9
|
Make feature(doc_auto_cfg) work (#12642)
# Objective - In #12366 `![cfg_attr(docsrs, feature(doc_auto_cfg))] `was added. But to apply it it needs `--cfg=docsrs` in rustdoc-args. ## Solution - Apply `--cfg=docsrs` to all crates and CI. I also added `[package.metadata.docs.rs]` to all crates to avoid adding code behind a feature and forget adding the metadata. Before:  After:  |