f04406ccce
62 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Gino Valente
|
c2854a2a05
|
bevy_reflect: Deprecate PartialReflect::clone_value (#18284)
# Objective #13432 added proper reflection-based cloning. This is a better method than cloning via `clone_value` for reasons detailed in the description of that PR. However, it may not be immediately apparent to users why one should be used over the other, and what the gotchas of `clone_value` are. ## Solution This PR marks `PartialReflect::clone_value` as deprecated, with the deprecation notice pointing users to `PartialReflect::reflect_clone`. However, it also suggests using a new method introduced in this PR: `PartialReflect::to_dynamic`. `PartialReflect::to_dynamic` is essentially a renaming of `PartialReflect::clone_value`. By naming it `to_dynamic`, we make it very obvious that what's returned is a dynamic type. The one caveat to this is that opaque types still use `reflect_clone` as they have no corresponding dynamic type. Along with changing the name, the method is now optional, and comes with a default implementation that calls out to the respective reflection subtrait method. This was done because there was really no reason to require manual implementors provide a method that almost always calls out to a known set of methods. Lastly, to make this default implementation work, this PR also did a similar thing with the `clone_dynamic ` methods on the reflection subtraits. For example, `Struct::clone_dynamic` has been marked deprecated and is superseded by `Struct::to_dynamic_struct`. This was necessary to avoid the "multiple names in scope" issue. ### Open Questions This PR maintains the original signature of `clone_value` on `to_dynamic`. That is, it takes `&self` and returns `Box<dyn PartialReflect>`. However, in order for this to work, it introduces a panic if the value is opaque and doesn't override the default `reflect_clone` implementation. One thing we could do to avoid the panic would be to make the conversion fallible, either returning `Option<Box<dyn PartialReflect>>` or `Result<Box<dyn PartialReflect>, ReflectCloneError>`. This makes using the method a little more involved (i.e. users have to either unwrap or handle the rare possibility of an error), but it would set us up for a world where opaque types don't strictly need to be `Clone`. Right now this bound is sort of implied by the fact that `clone_value` is a required trait method, and the default behavior of the macro is to use `Clone` for opaque types. Alternatively, we could keep the signature but make the method required. This maintains that implied bound where manual implementors must provide some way of cloning the value (or YOLO it and just panic), but also makes the API simpler to use. Finally, we could just leave it with the panic. It's unlikely this would occur in practice since our macro still requires `Clone` for opaque types, and thus this would only ever be an issue if someone were to manually implement `PartialReflect` without a valid `to_dynamic` or `reflect_clone` method. ## Testing You can test locally using the following command: ``` cargo test --package bevy_reflect --all-features ``` --- ## Migration Guide `PartialReflect::clone_value` is being deprecated. Instead, use `PartialReflect::to_dynamic` if wanting to create a new dynamic instance of the reflected value. Alternatively, use `PartialReflect::reflect_clone` to attempt to create a true clone of the underlying value. Similarly, the following methods have been deprecated and should be replaced with these alternatives: - `Array::clone_dynamic` → `Array::to_dynamic_array` - `Enum::clone_dynamic` → `Enum::to_dynamic_enum` - `List::clone_dynamic` → `List::to_dynamic_list` - `Map::clone_dynamic` → `Map::to_dynamic_map` - `Set::clone_dynamic` → `Set::to_dynamic_set` - `Struct::clone_dynamic` → `Struct::to_dynamic_struct` - `Tuple::clone_dynamic` → `Tuple::to_dynamic_tuple` - `TupleStruct::clone_dynamic` → `TupleStruct::to_dynamic_tuple_struct` |
||
|
Zachary Harrold
|
76e9bf9c99
|
Automatically enable portable-atomic when required (#17570)
# Objective - Contributes to #15460 - Reduce quantity and complexity of feature gates across Bevy ## Solution - Used `target_has_atomic` configuration variable to automatically detect impartial atomic support and automatically switch to `portable-atomic` over the standard library on an as-required basis. ## Testing - CI ## Notes To explain the technique employed here, consider getting `Arc` either from `alloc::sync` _or_ `portable-atomic-util`. First, we can inspect the `alloc` crate to see that you only have access to `Arc` _if_ `target_has_atomic = "ptr"`. We add a target dependency for this particular configuration _inverted_: ```toml [target.'cfg(not(target_has_atomic = "ptr"))'.dependencies] portable-atomic-util = { version = "0.2.4", default-features = false } ``` This ensures we only have the dependency when it is needed, and it is entirely excluded from the dependency graph when it is not. Next, we adjust our configuration flags to instead of checking for `feature = "portable-atomic"` to instead check for `target_has_atomic = "ptr"`: ```rust // `alloc` feature flag hidden for brevity #[cfg(not(target_has_atomic = "ptr"))] use portable_atomic_util as arc; #[cfg(target_has_atomic = "ptr")] use alloc::sync as arc; pub use arc::{Arc, Weak}; ``` The benefits of this technique are three-fold: 1. For platforms without full atomic support, the functionality is enabled automatically. 2. For platforms with atomic support, the dependency is never included, even if a feature was enabled using `--all-features` (for example) 3. The `portable-atomic` feature no longer needs to virally spread to all user-facing crates, it's instead something handled within `bevy_platform_support` (with some extras where other dependencies also need their features enabled). |
||
|
Zachary Harrold
|
5241e09671
|
Upgrade to Rust Edition 2024 (#17967)
# Objective - Fixes #17960 ## Solution - Followed the [edition upgrade guide](https://doc.rust-lang.org/edition-guide/editions/transitioning-an-existing-project-to-a-new-edition.html) ## Testing - CI --- ## Summary of Changes ### Documentation Indentation When using lists in documentation, proper indentation is now linted for. This means subsequent lines within the same list item must start at the same indentation level as the item. ```rust /* Valid */ /// - Item 1 /// Run-on sentence. /// - Item 2 struct Foo; /* Invalid */ /// - Item 1 /// Run-on sentence. /// - Item 2 struct Foo; ``` ### Implicit `!` to `()` Conversion `!` (the never return type, returned by `panic!`, etc.) no longer implicitly converts to `()`. This is particularly painful for systems with `todo!` or `panic!` statements, as they will no longer be functions returning `()` (or `Result<()>`), making them invalid systems for functions like `add_systems`. The ideal fix would be to accept functions returning `!` (or rather, _not_ returning), but this is blocked on the [stabilisation of the `!` type itself](https://doc.rust-lang.org/std/primitive.never.html), which is not done. The "simple" fix would be to add an explicit `-> ()` to system signatures (e.g., `|| { todo!() }` becomes `|| -> () { todo!() }`). However, this is _also_ banned, as there is an existing lint which (IMO, incorrectly) marks this as an unnecessary annotation. So, the "fix" (read: workaround) is to put these kinds of `|| -> ! { ... }` closuers into variables and give the variable an explicit type (e.g., `fn()`). ```rust // Valid let system: fn() = || todo!("Not implemented yet!"); app.add_systems(..., system); // Invalid app.add_systems(..., || todo!("Not implemented yet!")); ``` ### Temporary Variable Lifetimes The order in which temporary variables are dropped has changed. The simple fix here is _usually_ to just assign temporaries to a named variable before use. ### `gen` is a keyword We can no longer use the name `gen` as it is reserved for a future generator syntax. This involved replacing uses of the name `gen` with `r#gen` (the raw-identifier syntax). ### Formatting has changed Use statements have had the order of imports changed, causing a substantial +/-3,000 diff when applied. For now, I have opted-out of this change by amending `rustfmt.toml` ```toml style_edition = "2021" ``` This preserves the original formatting for now, reducing the size of this PR. It would be a simple followup to update this to 2024 and run `cargo fmt`. ### New `use<>` Opt-Out Syntax Lifetimes are now implicitly included in RPIT types. There was a handful of instances where it needed to be added to satisfy the borrow checker, but there may be more cases where it _should_ be added to avoid breakages in user code. ### `MyUnitStruct { .. }` is an invalid pattern Previously, you could match against unit structs (and unit enum variants) with a `{ .. }` destructuring. This is no longer valid. ### Pretty much every use of `ref` and `mut` are gone Pattern binding has changed to the point where these terms are largely unused now. They still serve a purpose, but it is far more niche now. ### `iter::repeat(...).take(...)` is bad New lint recommends using the more explicit `iter::repeat_n(..., ...)` instead. ## Migration Guide The lifetimes of functions using return-position impl-trait (RPIT) are likely _more_ conservative than they had been previously. If you encounter lifetime issues with such a function, please create an issue to investigate the addition of `+ use<...>`. ## Notes - Check the individual commits for a clearer breakdown for what _actually_ changed. --------- Co-authored-by: François Mockers <francois.mockers@vleue.com> |
||
|
raldone01
|
1b7db895b7
|
Harden proc macro path resolution and add integration tests. (#17330)
This pr uses the `extern crate self as` trick to make proc macros behave the same way inside and outside bevy. # Objective - Removes noise introduced by `crate as` in the whole bevy repo. - Fixes #17004. - Hardens proc macro path resolution. ## TODO - [x] `BevyManifest` needs cleanup. - [x] Cleanup remaining `crate as`. - [x] Add proper integration tests to the ci. ## Notes - `cargo-manifest-proc-macros` is written by me and based/inspired by the old `BevyManifest` implementation and [`bkchr/proc-macro-crate`](https://github.com/bkchr/proc-macro-crate). - What do you think about the new integration test machinery I added to the `ci`? More and better integration tests can be added at a later stage. The goal of these integration tests is to simulate an actual separate crate that uses bevy. Ideally they would lightly touch all bevy crates. ## Testing - Needs RA test - Needs testing from other users - Others need to run at least `cargo run -p ci integration-test` and verify that they work. --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> |
||
|
aecsocket
|
f232674291
|
Remove unnecessary PartialReflect bound on DeserializeWithRegistry (#17560)
# Objective
The new `DeserializeWithRegistry` trait in 0.15 was designed to be used
with reflection, and so has a `trait DeserializeWithRegistry:
PartialReflect` bound. However, this bound is not actually necessary for
the trait to function properly. And this `PartialReflect` bound already
exists on:
```rs
impl<T: PartialReflect + for<'de> DeserializeWithRegistry<'de>> FromType<T>
for ReflectDeserializeWithRegistry
```
So there is no point in constraining the trait itself with this bound as
well.
This lets me use `DeserializeWithRegistry` with non-`Reflect` types,
which I want to do to avoid making a bunch of `FooDeserializer` structs
and `impl DeserializeSeed` on them.
## Solution
Removes this unnecessary bound.
## Testing
Trivial change, does not break compilation or `bevy_reflect` tests.
## Migration Guide
`DeserializeWithRegistry` types are no longer guaranteed to be
`PartialReflect` as well. If you were relying on this type bound, you
should add it to your own bounds manually.
```diff
- impl<T: DeserializeWithRegistry> Foo for T { .. }
+ impl<T: DeserializeWithRegistry + PartialReflect> Foo for T { .. }
```
|
||
|
Zachary Harrold
|
9bc0ae33c3
|
Move hashbrown and foldhash out of bevy_utils (#17460)
# Objective - Contributes to #16877 ## Solution - Moved `hashbrown`, `foldhash`, and related types out of `bevy_utils` and into `bevy_platform_support` - Refactored the above to match the layout of these types in `std`. - Updated crates as required. ## Testing - CI --- ## Migration Guide - The following items were moved out of `bevy_utils` and into `bevy_platform_support::hash`: - `FixedState` - `DefaultHasher` - `RandomState` - `FixedHasher` - `Hashed` - `PassHash` - `PassHasher` - `NoOpHash` - The following items were moved out of `bevy_utils` and into `bevy_platform_support::collections`: - `HashMap` - `HashSet` - `bevy_utils::hashbrown` has been removed. Instead, import from `bevy_platform_support::collections` _or_ take a dependency on `hashbrown` directly. - `bevy_utils::Entry` has been removed. Instead, import from `bevy_platform_support::collections::hash_map` or `bevy_platform_support::collections::hash_set` as appropriate. - All of the above equally apply to `bevy::utils` and `bevy::platform_support`. ## Notes - I left `PreHashMap`, `PreHashMapExt`, and `TypeIdMap` in `bevy_utils` as they might be candidates for micro-crating. They can always be moved into `bevy_platform_support` at a later date if desired. |
||
|
Zachary Harrold
|
a64446b77e
|
Create bevy_platform_support Crate (#17250)
# Objective - Contributes to #16877 ## Solution - Initial creation of `bevy_platform_support` crate. - Moved `bevy_utils::Instant` into new `bevy_platform_support` crate. - Moved `portable-atomic`, `portable-atomic-util`, and `critical-section` into new `bevy_platform_support` crate. ## Testing - CI --- ## Showcase Instead of needing code like this to import an `Arc`: ```rust #[cfg(feature = "portable-atomic")] use portable_atomic_util::Arc; #[cfg(not(feature = "portable-atomic"))] use alloc::sync::Arc; ``` We can now use: ```rust use bevy_platform_support::sync::Arc; ``` This applies to many other types, but the goal is overall the same: allowing crates to use `std`-like types without the boilerplate of conditional compilation and platform-dependencies. ## Migration Guide - Replace imports of `bevy_utils::Instant` with `bevy_platform_support::time::Instant` - Replace imports of `bevy::utils::Instant` with `bevy::platform_support::time::Instant` ## Notes - `bevy_platform_support` hasn't been reserved on `crates.io` - ~~`bevy_platform_support` is not re-exported from `bevy` at this time. It may be worthwhile exporting this crate, but I am unsure of a reasonable name to export it under (`platform_support` may be a bit wordy for user-facing).~~ - I've included an implementation of `Instant` which is suitable for `no_std` platforms that are not Wasm for the sake of eliminating feature gates around its use. It may be a controversial inclusion, so I'm happy to remove it if required. - There are many other items (`spin`, `bevy_utils::Sync(Unsafe)Cell`, etc.) which should be added to this crate. I have kept the initial scope small to demonstrate utility without making this too unwieldy. --------- Co-authored-by: TimJentzsch <TimJentzsch@users.noreply.github.com> Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com> Co-authored-by: François Mockers <francois.mockers@vleue.com> |
||
|
MichiRecRoom
|
120b733ab5
|
bevy_reflect: Apply #[deny(clippy::allow_attributes, clippy::allow_attributes_without_reason)] (#17092)
# Objective We want to deny the following lints: * `clippy::allow_attributes` - Because there's no reason to `#[allow(...)]` an attribute if it wouldn't lint against anything; you should always use `#[expect(...)]` * `clippy::allow_attributes_without_reason` - Because documenting the reason for allowing/expecting a lint is always good ## Solution Set the `clippy::allow_attributes` and `clippy::allow_attributes_without_reason` lints to `deny`, and bring `bevy_reflect` in line with the new restrictions. No code changes have been made - except if a lint that was previously `allow(...)`'d could be removed via small code changes. For example, `unused_variables` can be handled by adding a `_` to the beginning of a field's name. ## Testing I ran `cargo clippy`, and received no errors. |
||
|
Zachary Harrold
|
0403948aa2
|
Remove Implicit std Prelude from no_std Crates (#17086)
# Background In `no_std` compatible crates, there is often an `std` feature which will allow access to the standard library. Currently, with the `std` feature _enabled_, the [`std::prelude`](https://doc.rust-lang.org/std/prelude/index.html) is implicitly imported in all modules. With the feature _disabled_, instead the [`core::prelude`](https://doc.rust-lang.org/core/prelude/index.html) is implicitly imported. This creates a subtle and pervasive issue where `alloc` items _may_ be implicitly included (if `std` is enabled), or must be explicitly included (if `std` is not enabled). # Objective - Make the implicit imports for `no_std` crates consistent regardless of what features are/not enabled. ## Solution - Replace the `cfg_attr` "double negative" `no_std` attribute with conditional compilation to _include_ `std` as an external crate. ```rust // Before #![cfg_attr(not(feature = "std"), no_std)] // After #![no_std] #[cfg(feature = "std")] extern crate std; ``` - Fix imports that are currently broken but are only now visible with the above fix. ## Testing - CI ## Notes I had previously used the "double negative" version of `no_std` based on general consensus that it was "cleaner" within the Rust embedded community. However, this implicit prelude issue likely was considered when forming this consensus. I believe the reason why is the items most affected by this issue are provided by the `alloc` crate, which is rarely used within embedded but extensively used within Bevy. |
||
|
ickshonpe
|
7a5a734452
|
Replace map + unwrap_or(false) with is_some_and (#17067)
# Objective The `my_option.map(|inner| inner.is_whatever).unwrap_or(false)` pattern is fragile and ugly. Replace it with `is_some_and` everywhere. |
||
|
Benjamin Brienen
|
64efd08e13
|
Prefer Display over Debug (#16112)
# Objective Fixes #16104 ## Solution I removed all instances of `:?` and put them back one by one where it caused an error. I removed some bevy_utils helper functions that were only used in 2 places and don't add value. See: #11478 ## Testing CI should catch the mistakes ## Migration Guide `bevy::utils::{dbg,info,warn,error}` were removed. Use `bevy::utils::tracing::{debug,info,warn,error}` instead. --------- Co-authored-by: SpecificProtagonist <vincentjunge@posteo.net> |
||
|
Clar Fon
|
711246aa34
|
Update hashbrown to 0.15 (#15801)
Updating dependencies; adopted version of #15696. (Supercedes #15696.) Long answer: hashbrown is no longer using ahash by default, meaning that we can't use the default-hasher methods with ahasher. So, we have to use the longer-winded versions instead. This takes the opportunity to also switch our default hasher as well, but without actually enabling the default-hasher feature for hashbrown, meaning that we'll be able to change our hasher more easily at the cost of all of these method calls being obnoxious forever. One large change from 0.15 is that `insert_unique_unchecked` is now `unsafe`, and for cases where unsafe code was denied at the crate level, I replaced it with `insert`. ## Migration Guide `bevy_utils` has updated its version of `hashbrown` to 0.15 and now defaults to `foldhash` instead of `ahash`. This means that if you've hard-coded your hasher to `bevy_utils::AHasher` or separately used the `ahash` crate in your code, you may need to switch to `foldhash` to ensure that everything works like it does in Bevy. |
||
|
Zachary Harrold
|
bf765e61b5
|
Add no_std support to bevy_reflect (#16256)
# Objective - Contributes to #15460 ## Solution - Added `std` feature (enabled by default) ## Testing - CI - `cargo check -p bevy_reflect --no-default-features --target "x86_64-unknown-none"` - UEFI demo application runs with this branch of `bevy_reflect`, allowing `derive(Reflect)` ## Notes - The [`spin`](https://crates.io/crates/spin) crate has been included to provide `RwLock` and `Once` (as an alternative to `OnceLock`) when the `std` feature is not enabled. Another alternative may be more desirable, please provide feedback if you have a strong opinion here! - Certain items (`Box`, `String`, `ToString`) provided by `alloc` have been added to `__macro_exports` as a way to avoid `alloc` vs `std` namespacing. I'm personally quite annoyed that we can't rely on `alloc` as a crate name in `std` environments within macros. I'd love an alternative to my approach here, but I suspect it's the least-bad option. - I would've liked to have an `alloc` feature (for allocation-free `bevy_reflect`), unfortunately, `erased_serde` unconditionally requires access to `Box`. Maybe one day we could design around this, but for now it just means `bevy_reflect` requires `alloc`. --------- Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com> Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> |
||
|
aecsocket
|
17c4b070ab
|
bevy_reflect: Add ReflectSerializerProcessor (#15548)
**NOTE: This is based on, and should be merged alongside, https://github.com/bevyengine/bevy/pull/15482.** I'll leave this in draft until that PR is merged. # Objective Equivalent of https://github.com/bevyengine/bevy/pull/15482 but for serialization. See that issue for the motivation. Also part of this tracking issue: https://github.com/bevyengine/bevy/issues/15518 This PR is non-breaking, just like the deserializer PR (because the new type parameter `P` has a default `P = ()`). ## Solution Identical solution to the deserializer PR. ## Testing Added unit tests and a very comprehensive doc test outlining a clear example and use case. |
||
|
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> |
||
|
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. |
||
|
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>
|
||
|
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
|
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. |
||
|
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! ``` |
||
|
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. |
||
|
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
|
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)
|
||
|
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(); ``` |
||
|
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
|
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 ``` |
||
|
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!` |
||
|
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
|
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 |
||
|
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); ``` |
||
|
Peter Hayman
|
d3e44325b4
|
Fix: deserialize DynamicEnum using index (#12464)
# Objective - Addresses #12462 - When we serialize an enum, deserialize it, then reserialize it, the correct variant should be selected. ## Solution - Change `dynamic_enum.set_variant` to `dynamic_enum.set_variant_with_index` in `EnumVisitor` |
||
|
Gino Valente
|
4c47e31be6
|
bevy_reflect: Remove U32Visitor (#12433)
# Objective The `U32Visitor` struct has been unused since its introduction in #6140. It's made itself known now by causing a recent [CI failure](https://github.com/bevyengine/bevy/actions/runs/8243333274/job/22543736624). ## Solution Remove the unused `U32Visitor` struct. Also removed `PrepassLightsViewFlush` as it was causing a [similar CI failure](https://github.com/bevyengine/bevy/actions/runs/8243838066/job/22545103746?pr=12433#step:6:269) on this PR. |
||
|
radiish
|
2b7a3b2a55
|
reflect: treat proxy types correctly when serializing (#12024)
# Objective - Fixes #12001. - Note this PR doesn't change any feature flags, however flaky the issue revealed they are. ## Solution - Use `FromReflect` to convert proxy types to concrete ones in `ReflectSerialize::get_serializable`. - Use `get_represented_type_info() -> type_id()` to get the correct type id to interact with the registry in `bevy_reflect::serde::ser::get_serializable`. --- ## Changelog - Registering `ReflectSerialize` now imposes additional `FromReflect` and `TypePath` bounds. ## Migration Guide - If `ReflectSerialize` is registered on a type, but `TypePath` or `FromReflect` implementations are omitted (perhaps by `#[reflect(type_path = false)` or `#[reflect(from_reflect = false)]`), the traits must now be implemented. --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com> |
||
|
eri
|
5f8f3b532c
|
Check cfg during CI and fix feature typos (#12103)
# Objective - Add the new `-Zcheck-cfg` checks to catch more warnings - Fixes #12091 ## Solution - Create a new `cfg-check` to the CI that runs `cargo check -Zcheck-cfg --workspace` using cargo nightly (and fails if there are warnings) - Fix all warnings generated by the new check --- ## Changelog - Remove all redundant imports - Fix cfg wasm32 targets - Add 3 dead code exceptions (should StandardColor be unused?) - Convert ios_simulator to a feature (I'm not sure if this is the right way to do it, but the check complained before) ## Migration Guide No breaking changes --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> |
||
|
Doonv
|
189ceaf0d3
|
Replace or document ignored doctests (#11040)
# Objective There are a lot of doctests that are `ignore`d for no documented reason. And that should be fixed. ## Solution I searched the bevy repo with the regex ` ```[a-z,]*ignore ` in order to find all `ignore`d doctests. For each one of the `ignore`d doctests, I did the following steps: 1. Attempt to remove the `ignored` attribute while still passing the test. I did this by adding hidden dummy structs and imports. 2. If step 1 doesn't work, attempt to replace the `ignored` attribute with the `no_run` attribute while still passing the test. 3. If step 2 doesn't work, keep the `ignored` attribute but add documentation for why the `ignored` attribute was added. --------- Co-authored-by: François <mockersf@gmail.com> |
||
|
Tygyh
|
1568d4a415
|
Reorder impl to be the same as the trait (#11076)
# Objective - Make the implementation order consistent between all sources to fit the order in the trait. ## Solution - Change the implementation order. |
||
|
tygyh
|
fd308571c4
|
Remove unnecessary path prefixes (#10749)
# Objective - Shorten paths by removing unnecessary prefixes ## Solution - Remove the prefixes from many paths which do not need them. Finding the paths was done automatically using built-in refactoring tools in Jetbrains RustRover. |
||
|
Hank Jordan
|
e85af0e366
|
Fix issue with Option serialization (#10705)
# Objective - Fix #10499 ## Solution - Use `.get_represented_type_info()` module path and type ident instead of `.reflect_*` module path and type ident when serializing the `Option` enum --- ## Changelog - Fix serialization bug - Add simple test - Add `serde_json` dev dependency - Add `serde` with `derive` feature dev dependency (wouldn't compile for me without it) --------- Co-authored-by: hank <hank@hank.co.in> Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com> |
||
|
Gino Valente
|
60773e6787
|
bevy_reflect: Fix ignored/skipped field order (#7575)
# Objective Fixes #5101 Alternative to #6511 ## Solution Corrected the behavior for ignored fields in `FromReflect`, which was previously using the incorrect field indexes. Similarly, fields marked with `#[reflect(skip_serializing)]` no longer break when using `FromReflect` after deserialization. This was done by modifying `SerializationData` to store a function pointer that can later be used to generate a default instance of the skipped field during deserialization. The function pointer points to a function generated by the derive macro using the behavior designated by `#[reflect(default)]` (or just `Default` if none provided). The entire output of the macro is now wrapped in an [unnamed constant](https://doc.rust-lang.org/stable/reference/items/constant-items.html#unnamed-constant) which keeps this behavior hygienic. #### Rationale The biggest downside to this approach is that it requires fields marked `#[reflect(skip_serializing)]` to provide the ability to create a default instance— either via a `Default` impl or by specifying a custom one. While this isn't great, I think it might be justified by the fact that we really need to create this value when using `FromReflect` on a deserialized object. And we need to do this _during_ deserialization because after that (at least for tuples and tuple structs) we lose information about which field is which: _"is the value at index 1 in this `DynamicTupleStruct` the actual value for index 1 or is it really the value for index 2 since index 1 is skippable...?"_ #### Alternatives An alternative would be to store `Option<Box<dyn Reflect>>` within `DynamicTuple` and `DynamicTupleStruct` instead of just `Box<dyn Reflect>`. This would allow us to insert "empty"/"missing" fields during deserialization, thus saving the positional information of the skipped fields. However, this may require changing the API of `Tuple` and `TupleStruct` such that they can account for their dynamic counterparts returning `None` for a skipped field. In practice this would probably mean exposing the `Option`-ness of the dynamics onto implementors via methods like `Tuple::drain` or `TupleStruct::field`. Personally, I think requiring `Default` would be better than muddying up the API to account for these special cases. But I'm open to trying out this other approach if the community feels that it's better. --- ## Changelog ### Public Changes #### Fixed - The behaviors of `#[reflect(ignore)]` and `#[reflect(skip_serializing)]` are no longer dependent on field order #### Changed - Fields marked with `#[reflect(skip_serializing)]` now need to either implement `Default` or specify a custom default function using `#[reflect(default = "path::to::some_func")]` - Deserializing a type with fields marked `#[reflect(skip_serializing)]` will now include that field initialized to its specified default value - `SerializationData::new` now takes the new `SkippedField` struct along with the skipped field index - Renamed `SerializationData::is_ignored_field` to `SerializationData::is_field_skipped` #### Added - Added `SkippedField` struct - Added methods `SerializationData::generate_default` and `SerializationData::iter_skipped` ### Internal Changes #### Changed - Replaced `members_to_serialization_denylist` and `BitSet<u32>` with `SerializationDataDef` - The `Reflect` derive is more hygienic as it now outputs within an [unnamed constant](https://doc.rust-lang.org/stable/reference/items/constant-items.html#unnamed-constant) - `StructField::index` has been split up into `StructField::declaration_index` and `StructField::reflection_index` #### Removed - Removed `bitset` dependency ## Migration Guide * Fields marked `#[reflect(skip_serializing)]` now must implement `Default` or specify a custom default function with `#[reflect(default = "path::to::some_func")]` ```rust #[derive(Reflect)] struct MyStruct { #[reflect(skip_serializing)] #[reflect(default = "get_foo_default")] foo: Foo, // <- `Foo` does not impl `Default` so requires a custom function #[reflect(skip_serializing)] bar: Bar, // <- `Bar` impls `Default` } #[derive(Reflect)] struct Foo(i32); #[derive(Reflect, Default)] struct Bar(i32); fn get_foo_default() -> Foo { Foo(123) } ``` * `SerializationData::new` has been changed to expect an iterator of `(usize, SkippedField)` rather than one of just `usize` ```rust // BEFORE SerializationData::new([0, 3].into_iter()); // AFTER SerializationData::new([ (0, SkippedField::new(field_0_default_fn)), (3, SkippedField::new(field_3_default_fn)), ].into_iter()); ``` * `Serialization::is_ignored_field` has been renamed to `Serialization::is_field_skipped` * Fields marked `#[reflect(skip_serializing)]` are now included in deserialization output. This may affect logic that expected those fields to be absent. |
||
|
Gino Valente
|
01b910a148
|
bevy_reflect: Fix dynamic type serialization (#10103)
# Objective Fixes #10086 ## Solution Instead of serializing via `DynamicTypePath::reflect_type_path`, now uses the `TypePath` found on the `TypeInfo` returned by `Reflect::get_represented_type_info`. This issue was happening because the dynamic types implement `TypePath` themselves and do not (and cannot) forward their proxy's `TypePath` data. The solution was to access the proxy's type information in order to get the correct `TypePath` data. ## Changed - The `Debug` impl for `TypePathTable` now includes output for all fields. |
||
|
radiish
|
262846e702
|
reflect: TypePath part 2 (#8768)
# Objective
- Followup to #7184.
- ~Deprecate `TypeUuid` and remove its internal references.~ No longer
part of this PR.
- Use `TypePath` for the type registry, and (de)serialisation instead of
`std::any::type_name`.
- Allow accessing type path information behind proxies.
## Solution
- Introduce methods on `TypeInfo` and friends for dynamically querying
type path. These methods supersede the old `type_name` methods.
- Remove `Reflect::type_name` in favor of `DynamicTypePath::type_path`
and `TypeInfo::type_path_table`.
- Switch all uses of `std::any::type_name` in reflection, non-debugging
contexts to use `TypePath`.
---
## Changelog
- Added `TypePathTable` for dynamically accessing methods on `TypePath`
through `TypeInfo` and the type registry.
- Removed `type_name` from all `TypeInfo`-like structs.
- Added `type_path` and `type_path_table` methods to all `TypeInfo`-like
structs.
- Removed `Reflect::type_name` in favor of
`DynamicTypePath::reflect_type_path` and `TypeInfo::type_path`.
- Changed the signature of all `DynamicTypePath` methods to return
strings with a static lifetime.
## Migration Guide
- Rely on `TypePath` instead of `std::any::type_name` for all stability
guarantees and for use in all reflection contexts, this is used through
with one of the following APIs:
- `TypePath::type_path` if you have a concrete type and not a value.
- `DynamicTypePath::reflect_type_path` if you have an `dyn Reflect`
value without a concrete type.
- `TypeInfo::type_path` for use through the registry or if you want to
work with the represented type of a `DynamicFoo`.
- Remove `type_name` from manual `Reflect` implementations.
- Use `type_path` and `type_path_table` in place of `type_name` on
`TypeInfo`-like structs.
- Use `get_with_type_path(_mut)` over `get_with_type_name(_mut)`.
## Note to reviewers
I think if anything we were a little overzealous in merging #7184 and we
should take that extra care here.
In my mind, this is the "point of no return" for `TypePath` and while I
think we all agree on the design, we should carefully consider if the
finer details and current implementations are actually how we want them
moving forward.
For example [this incorrect `TypePath` implementation for
`String`](
|
||
|
Joseph
|
8eb6ccdd87
|
Remove useless single tuples and trailing commas (#9720)
# Objective Title |
||
|
Gino Valente
|
aeeb20ec4c
|
bevy_reflect: FromReflect Ergonomics Implementation (#6056)
# Objective **This implementation is based on https://github.com/bevyengine/rfcs/pull/59.** --- Resolves #4597 Full details and motivation can be found in the RFC, but here's a brief summary. `FromReflect` is a very powerful and important trait within the reflection API. It allows Dynamic types (e.g., `DynamicList`, etc.) to be formed into Real ones (e.g., `Vec<i32>`, etc.). This mainly comes into play concerning deserialization, where the reflection deserializers both return a `Box<dyn Reflect>` that almost always contain one of these Dynamic representations of a Real type. To convert this to our Real type, we need to use `FromReflect`. It also sneaks up in other ways. For example, it's a required bound for `T` in `Vec<T>` so that `Vec<T>` as a whole can be made `FromReflect`. It's also required by all fields of an enum as it's used as part of the `Reflect::apply` implementation. So in other words, much like `GetTypeRegistration` and `Typed`, it is very much a core reflection trait. The problem is that it is not currently treated like a core trait and is not automatically derived alongside `Reflect`. This makes using it a bit cumbersome and easy to forget. ## Solution Automatically derive `FromReflect` when deriving `Reflect`. Users can then choose to opt-out if needed using the `#[reflect(from_reflect = false)]` attribute. ```rust #[derive(Reflect)] struct Foo; #[derive(Reflect)] #[reflect(from_reflect = false)] struct Bar; fn test<T: FromReflect>(value: T) {} test(Foo); // <-- OK test(Bar); // <-- Panic! Bar does not implement trait `FromReflect` ``` #### `ReflectFromReflect` This PR also automatically adds the `ReflectFromReflect` (introduced in #6245) registration to the derived `GetTypeRegistration` impl— if the type hasn't opted out of `FromReflect` of course. <details> <summary><h4>Improved Deserialization</h4></summary> > **Warning** > This section includes changes that have since been descoped from this PR. They will likely be implemented again in a followup PR. I am mainly leaving these details in for archival purposes, as well as for reference when implementing this logic again. And since we can do all the above, we might as well improve deserialization. We can now choose to deserialize into a Dynamic type or automatically convert it using `FromReflect` under the hood. `[Un]TypedReflectDeserializer::new` will now perform the conversion and return the `Box`'d Real type. `[Un]TypedReflectDeserializer::new_dynamic` will work like what we have now and simply return the `Box`'d Dynamic type. ```rust // Returns the Real type let reflect_deserializer = UntypedReflectDeserializer::new(®istry); let mut deserializer = ron:🇩🇪:Deserializer::from_str(input)?; let output: SomeStruct = reflect_deserializer.deserialize(&mut deserializer)?.take()?; // Returns the Dynamic type let reflect_deserializer = UntypedReflectDeserializer::new_dynamic(®istry); let mut deserializer = ron:🇩🇪:Deserializer::from_str(input)?; let output: DynamicStruct = reflect_deserializer.deserialize(&mut deserializer)?.take()?; ``` </details> --- ## Changelog * `FromReflect` is now automatically derived within the `Reflect` derive macro * This includes auto-registering `ReflectFromReflect` in the derived `GetTypeRegistration` impl * ~~Renamed `TypedReflectDeserializer::new` and `UntypedReflectDeserializer::new` to `TypedReflectDeserializer::new_dynamic` and `UntypedReflectDeserializer::new_dynamic`, respectively~~ **Descoped** * ~~Changed `TypedReflectDeserializer::new` and `UntypedReflectDeserializer::new` to automatically convert the deserialized output using `FromReflect`~~ **Descoped** ## Migration Guide * `FromReflect` is now automatically derived within the `Reflect` derive macro. Items with both derives will need to remove the `FromReflect` one. ```rust // OLD #[derive(Reflect, FromReflect)] struct Foo; // NEW #[derive(Reflect)] struct Foo; ``` If using a manual implementation of `FromReflect` and the `Reflect` derive, users will need to opt-out of the automatic implementation. ```rust // OLD #[derive(Reflect)] struct Foo; impl FromReflect for Foo {/* ... */} // NEW #[derive(Reflect)] #[reflect(from_reflect = false)] struct Foo; impl FromReflect for Foo {/* ... */} ``` <details> <summary><h4>Removed Migrations</h4></summary> > **Warning** > This section includes changes that have since been descoped from this PR. They will likely be implemented again in a followup PR. I am mainly leaving these details in for archival purposes, as well as for reference when implementing this logic again. * The reflect deserializers now perform a `FromReflect` conversion internally. The expected output of `TypedReflectDeserializer::new` and `UntypedReflectDeserializer::new` is no longer a Dynamic (e.g., `DynamicList`), but its Real counterpart (e.g., `Vec<i32>`). ```rust let reflect_deserializer = UntypedReflectDeserializer::new_dynamic(®istry); let mut deserializer = ron:🇩🇪:Deserializer::from_str(input)?; // OLD let output: DynamicStruct = reflect_deserializer.deserialize(&mut deserializer)?.take()?; // NEW let output: SomeStruct = reflect_deserializer.deserialize(&mut deserializer)?.take()?; ``` Alternatively, if this behavior isn't desired, use the `TypedReflectDeserializer::new_dynamic` and `UntypedReflectDeserializer::new_dynamic` methods instead: ```rust // OLD let reflect_deserializer = UntypedReflectDeserializer::new(®istry); // NEW let reflect_deserializer = UntypedReflectDeserializer::new_dynamic(®istry); ``` </details> --------- Co-authored-by: Carter Anderson <mcanders1@gmail.com> |
||
|
Giacomo Stevanato
|
0f4d16aa3c
|
Don't ignore additional entries in UntypedReflectDeserializerVisitor (#7112)
# Objective
Currently when `UntypedReflectDeserializerVisitor` deserializes a
`Box<dyn Reflect>` it only considers the first entry of the map,
silently ignoring any additional entries. For example the following RON
data:
```json
{
"f32": 1.23,
"u32": 1,
}
```
is successfully deserialized as a `f32`, completly ignoring the `"u32":
1` part.
## Solution
`UntypedReflectDeserializerVisitor` was changed to check if any other
key could be deserialized, and in that case returns an error.
---
## Changelog
`UntypedReflectDeserializer` now errors on malformed inputs instead of
silently disgarding additional data.
## Migration Guide
If you were deserializing `Box<dyn Reflect>` values with multiple
entries (i.e. entries other than `"type": { /* fields */ }`) you should
remove them or deserialization will fail.
|
||
|
Gino Valente
|
75130bd5ec
|
bevy_reflect: Better proxies (#6971)
# Objective > This PR is based on discussion from #6601 The Dynamic types (e.g. `DynamicStruct`, `DynamicList`, etc.) act as both: 1. Dynamic containers which may hold any arbitrary data 2. Proxy types which may represent any other type Currently, the only way we can represent the proxy-ness of a Dynamic is by giving it a name. ```rust // This is just a dynamic container let mut data = DynamicStruct::default(); // This is a "proxy" data.set_name(std::any::type_name::<Foo>()); ``` This type name is the only way we check that the given Dynamic is a proxy of some other type. When we need to "assert the type" of a `dyn Reflect`, we call `Reflect::type_name` on it. However, because we're only using a string to denote the type, we run into a few gotchas and limitations. For example, hashing a Dynamic proxy may work differently than the type it proxies: ```rust #[derive(Reflect, Hash)] #[reflect(Hash)] struct Foo(i32); let concrete = Foo(123); let dynamic = concrete.clone_dynamic(); let concrete_hash = concrete.reflect_hash(); let dynamic_hash = dynamic.reflect_hash(); // The hashes are not equal because `concrete` uses its own `Hash` impl // while `dynamic` uses a reflection-based hashing algorithm assert_ne!(concrete_hash, dynamic_hash); ``` Because the Dynamic proxy only knows about the name of the type, it's unaware of any other information about it. This means it also differs on `Reflect::reflect_partial_eq`, and may include ignored or skipped fields in places the concrete type wouldn't. ## Solution Rather than having Dynamics pass along just the type name of proxied types, we can instead have them pass around the `TypeInfo`. Now all Dynamic types contain an `Option<&'static TypeInfo>` rather than a `String`: ```diff pub struct DynamicTupleStruct { - type_name: String, + represented_type: Option<&'static TypeInfo>, fields: Vec<Box<dyn Reflect>>, } ``` By changing `Reflect::get_type_info` to `Reflect::represented_type_info`, hopefully we make this behavior a little clearer. And to account for `None` values on these dynamic types, `Reflect::represented_type_info` now returns `Option<&'static TypeInfo>`. ```rust let mut data = DynamicTupleStruct::default(); // Not proxying any specific type assert!(dyn_tuple_struct.represented_type_info().is_none()); let type_info = <Foo as Typed>::type_info(); dyn_tuple_struct.set_represented_type(Some(type_info)); // Alternatively: // let dyn_tuple_struct = foo.clone_dynamic(); // Now we're proxying `Foo` assert!(dyn_tuple_struct.represented_type_info().is_some()); ``` This means that we can have full access to all the static type information for the proxied type. Future work would include transitioning more static type information (trait impls, attributes, etc.) over to the `TypeInfo` so it can actually be utilized by Dynamic proxies. ### Alternatives & Rationale > **Note** > These alternatives were written when this PR was first made using a `Proxy` trait. This trait has since been removed. <details> <summary>View</summary> #### Alternative: The `Proxy<T>` Approach I had considered adding something like a `Proxy<T>` type where `T` would be the Dynamic and would contain the proxied type information. This was nice in that it allows us to explicitly determine whether something is a proxy or not at a type level. `Proxy<DynamicStruct>` proxies a struct. Makes sense. The reason I didn't go with this approach is because (1) tuples, (2) complexity, and (3) `PartialReflect`. The `DynamicTuple` struct allows us to represent tuples at runtime. It also allows us to do something you normally can't with tuples: add new fields. Because of this, adding a field immediately invalidates the proxy (e.g. our info for `(i32, i32)` doesn't apply to `(i32, i32, NewField)`). By going with this PR's approach, we can just remove the type info on `DynamicTuple` when that happens. However, with the `Proxy<T>` approach, it becomes difficult to represent this behavior— we'd have to completely control how we access data for `T` for each `T`. Secondly, it introduces some added complexities (aside from the manual impls for each `T`). Does `Proxy<T>` impl `Reflect`? Likely yes, if we want to represent it as `dyn Reflect`. What `TypeInfo` do we give it? How would we forward reflection methods to the inner type (remember, we don't have specialization)? How do we separate this from Dynamic types? And finally, how do all this in a way that's both logical and intuitive for users? Lastly, introducing a `Proxy` trait rather than a `Proxy<T>` struct is actually more inline with the [Unique Reflect RFC](https://github.com/bevyengine/rfcs/pull/56). In a way, the `Proxy` trait is really one part of the `PartialReflect` trait introduced in that RFC (it's technically not in that RFC but it fits well with it), where the `PartialReflect` serves as a way for proxies to work _like_ concrete types without having full access to everything a concrete `Reflect` type can do. This would help bridge the gap between the current state of the crate and the implementation of that RFC. All that said, this is still a viable solution. If the community believes this is the better path forward, then we can do that instead. These were just my reasons for not initially going with it in this PR. #### Alternative: The Type Registry Approach The `Proxy` trait is great and all, but how does it solve the original problem? Well, it doesn't— yet! The goal would be to start moving information from the derive macro and its attributes to the generated `TypeInfo` since these are known statically and shouldn't change. For example, adding `ignored: bool` to `[Un]NamedField` or a list of impls. However, there is another way of storing this information. This is, of course, one of the uses of the `TypeRegistry`. If we're worried about Dynamic proxies not aligning with their concrete counterparts, we could move more type information to the registry and require its usage. For example, we could replace `Reflect::reflect_hash(&self)` with `Reflect::reflect_hash(&self, registry: &TypeRegistry)`. That's not the _worst_ thing in the world, but it is an ergonomics loss. Additionally, other attributes may have their own requirements, further restricting what's possible without the registry. The `Reflect::apply` method will require the registry as well now. Why? Well because the `map_apply` function used for the `Reflect::apply` impls on `Map` types depends on `Map::insert_boxed`, which (at least for `DynamicMap`) requires `Reflect::reflect_hash`. The same would apply when adding support for reflection-based diffing, which will require `Reflect::reflect_partial_eq`. Again, this is a totally viable alternative. I just chose not to go with it for the reasons above. If we want to go with it, then we can close this PR and we can pursue this alternative instead. #### Downsides Just to highlight a quick potential downside (likely needs more investigation): retrieving the `TypeInfo` requires acquiring a lock on the `GenericTypeInfoCell` used by the `Typed` impls for generic types (non-generic types use a `OnceBox which should be faster). I am not sure how much of a performance hit that is and will need to run some benchmarks to compare against. </details> ### Open Questions 1. Should we use `Cow<'static, TypeInfo>` instead? I think that might be easier for modding? Perhaps, in that case, we need to update `Typed::type_info` and friends as well? 2. Are the alternatives better than the approach this PR takes? Are there other alternatives? --- ## Changelog ### Changed - `Reflect::get_type_info` has been renamed to `Reflect::represented_type_info` - This method now returns `Option<&'static TypeInfo>` rather than just `&'static TypeInfo` ### Added - Added `Reflect::is_dynamic` method to indicate when a type is dynamic - Added a `set_represented_type` method on all dynamic types ### Removed - Removed `TypeInfo::Dynamic` (use `Reflect::is_dynamic` instead) - Removed `Typed` impls for all dynamic types ## Migration Guide - The Dynamic types no longer take a string type name. Instead, they require a static reference to `TypeInfo`: ```rust #[derive(Reflect)] struct MyTupleStruct(f32, f32); let mut dyn_tuple_struct = DynamicTupleStruct::default(); dyn_tuple_struct.insert(1.23_f32); dyn_tuple_struct.insert(3.21_f32); // BEFORE: let type_name = std::any::type_name::<MyTupleStruct>(); dyn_tuple_struct.set_name(type_name); // AFTER: let type_info = <MyTupleStruct as Typed>::type_info(); dyn_tuple_struct.set_represented_type(Some(type_info)); ``` - `Reflect::get_type_info` has been renamed to `Reflect::represented_type_info` and now also returns an `Option<&'static TypeInfo>` (instead of just `&'static TypeInfo`): ```rust // BEFORE: let info: &'static TypeInfo = value.get_type_info(); // AFTER: let info: &'static TypeInfo = value.represented_type_info().unwrap(); ``` - `TypeInfo::Dynamic` and `DynamicInfo` has been removed. Use `Reflect::is_dynamic` instead: ```rust // BEFORE: if matches!(value.get_type_info(), TypeInfo::Dynamic) { // ... } // AFTER: if value.is_dynamic() { // ... } ``` --------- Co-authored-by: radiish <cb.setho@gmail.com> |
||
|
张林伟
|
0d2cdb450d |
Fix beta clippy lints (#7154)
# Objective - When I run `cargo run -p ci` for my pr locally using latest beta toolchain, the ci failed due to [uninlined_format_args](https://rust-lang.github.io/rust-clippy/master/index.html#uninlined_format_args) and [needless_lifetimes](https://rust-lang.github.io/rust-clippy/master/index.html#needless_lifetimes) lints ## Solution - Fix lints according to clippy suggestions. |
||
|
Giacomo Stevanato
|
871c80c103 |
Add TypeRegistrationDeserializer and remove BorrowedStr (#7094)
# Objective This a follow-up to #6894, see https://github.com/bevyengine/bevy/pull/6894#discussion_r1045203113 The goal is to avoid cloning any string when getting a `&TypeRegistration` corresponding to a string which is being deserialized. As a bonus code duplication is also reduced. ## Solution The manual deserialization of a string and lookup into the type registry has been moved into a separate `TypeRegistrationDeserializer` type, which implements `DeserializeSeed` with a `Visitor` that accepts any string with `visit_str`, even ones that may not live longer than that function call. `BorrowedStr` has been removed since it's no longer used. --- ## Changelog - The type `TypeRegistrationDeserializer` has been added, which simplifies getting a `&TypeRegistration` while deserializing a string. |
||
|
Gino Valente
|
717def2ccf |
bevy_reflect: Fix deserialization with readers (#6894)
# Objective Fixes #6891 ## Solution Replaces deserializing map keys as `&str` with deserializing them as `String`. This bug seems to occur when using something like `File` or `BufReader` rather than bytes or a string directly (I only tested `File` and `BufReader` for `rmp-serde` and `serde_json`). This might be an issue with other `Read` impls as well (except `&[u8]` it seems). We already had passing tests for Message Pack but none that use a `File` or `BufReader`. This PR also adds or modifies tests to check for this in the future. This change was also based on [feedback](https://github.com/bevyengine/bevy/pull/4561#discussion_r957385136) I received in a previous PR. --- ## Changelog - Fix bug where scene deserialization using certain readers could fail (e.g. `BufReader`, `File`, etc.) |
||
|
Gino Valente
|
4e2374334f |
bevy_reflect: Fix binary deserialization not working for unit structs (#6722)
# Objective Fixes #6713 Binary deserialization is failing for unit structs as well as structs with all ignored/skipped fields. ## Solution Add a check for the number of possible fields in a struct before deserializing. If empty, don't attempt to deserialize any fields (as there will be none). Note: ~~This does not apply to enums as they do not properly handle skipped fields (see #6721).~~ Enums still do not properly handle skipped fields, but I decided to include the logic for it anyways to account for `#[reflect(ignore)]`'d fields in the meantime. --- ## Changelog - Fix bug where deserializing unit structs would fail for non-self-describing formats |