fixes#17478
# Objective
- Complete #17558.
- the `insert_children` method was previously removed, and as #17478
points out, needs to be added back.
## Solution
- Add a `OrderedRelationshipSourceCollection`, which allows sorting,
ordering, rearranging, etc of a `RelationshipSourceCollection`.
- Implement `insert_related`
- Implement `insert_children`
- Tidy up some docs while I'm here.
## Testing
@bjoernp116 set up a unit test, and I added a doc test to
`OrderedRelationshipSourceCollection`.
---------
Co-authored-by: bjoernp116 <bjoernpollen@gmail.com>
Co-authored-by: Dmytro Banin <banind@cs.washington.edu>
Co-authored-by: Talin <viridia@gmail.com>
# Objective
#18173 allows components to be queued without being fully registered.
But much of bevy's debug logging contained
`components.get_name(id).unwrap()`. However, this panics when the id is
queued. This PR fixes this, allowing names to be retrieved for debugging
purposes, etc, even while they're still queued.
## Solution
We change `ComponentInfo::descriptor` to be `Arc<ComponentDescriptor>`
instead of not arc'd. This lets us pass the descriptor around (as a name
or otherwise) as needed. The alternative would require some form of
`MappedRwLockReadGuard`, which is unstable, and would be terribly
blocking. Putting it in an arc also signifies that it doesn't change,
which is a nice signal to users. This does mean there's an extra pointer
dereference, but I don't think that's an issue here, as almost all paths
that use this are for debugging purposes or one-time set ups.
## Testing
Existing tests.
## Migration Guide
`Components::get_name` now returns `Option<Cow<'_, str>` instead of
`Option<&str>`. This is because it now returns results for queued
components. If that behavior is not desired, or you know the component
is not queued, you can use
`components.get_info().map(ComponentInfo::name)` instead.
Similarly, `ScheduleGraph::conflicts_to_string` now returns `impl
Iterator<Item = (String, String, Vec<Cow<str>>)>` instead of `impl
Iterator<Item = (String, String, Vec<&str>)>`. Because `Cow<str>` derefs
to `&str`, most use cases can remain unchanged.
---------
Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com>
- Lots of nits, formatting, and rephrasing, with the goal of making
things more consistent.
- Fix outdated error handler explanation in `Commands` and
`EntityCommands` docs.
- Expand docs for system-related commands.
- Remove panic notes if the command only panics with the default error
handler.
- Update error handling notes for `try_` variants.
- Hide `prelude` import in most doctest examples, unless the example
uses something that people might not realize is in the prelude (like
`Name`).
- Remove a couple doctest examples that (in my opinion) didn't make
sense.
---------
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com>
# Objective
Improve error messages for missing resources.
The default error handler currently prints the `Debug` representation of
the error type instead of `Display`. Most error types use
`#[derive(Debug)]`, resulting in a dump of the structure, but will have
a user-friendly message for `Display`.
Follow-up to #18593
## Solution
Change the default error handler to use `Display` instead of `Debug`.
Change `BevyError` to include the backtrace in the `Display` format in
addition to `Debug` so that it is still included.
## Showcase
Before:
```
Encountered an error in system `system_name`: SystemParamValidationError { skipped: false, message: "Resource does not exist", param: "bevy_ecs::change_detection::Res<app_name::ResourceType>" }
Encountered an error in system `other_system_name`: "String message with\nmultiple lines."
```
After
```
Encountered an error in system `system_name`: Parameter `Res<ResourceType>` failed validation: Resource does not exist
Encountered an error in system `other_system_name`: String message with
multiple lines.
```
# Objective
- There's been several changes to `Query` for this release cycle, and
`Query`'s top-level documentation has gotten slightly out-of-date.
- Alternative to #18615.
## Solution
- Edit `Query`'s docs for consistency, clarity, and correctness.
- Make sure to group `get()` and `get_many()` together instead of
`single()` and `get_many()`, to enforce the distinction from
https://github.com/bevyengine/bevy/pull/18615#issuecomment-2764355672.
- Reformat doc tests so they would be readable if extracted into their
own file. (Which mainly involves adding more spacing.)
- Move link definitions to be nearer where they are used.
- Fix the tables so they are up-to-date and correctly escape square
brackets `\[ \]`.
## Testing
I ran `cargo doc -p bevy_ecs --no-deps` to view the docs and `cargo test
-p bevy_ecs --doc` to test the doc comments.
## Reviewing
The diff is difficult to read, so I don't recommend _just_ looking at
that. Instead, run `cargo doc -p bevy_ecs --no-deps` locally and read
through the new version. It should theoretically read smoother with less
super-technical jargon. :)
## Follow-up
I want to go through some of `Query`'s methods, such as `single()`,
`get()`, and `get_many()`, but I'll leave that for another PR.
---------
Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com>
# Objective
Fixes#9367.
Yet another follow-up to #16547.
These traits were initially based on `Borrow<Entity>` because that trait
was what they were replacing, and it felt close enough in meaning.
However, they ultimately don't quite match: `borrow` always returns
references, whereas `EntityBorrow` always returns a plain `Entity`.
Additionally, `EntityBorrow` can imply that we are borrowing an `Entity`
from the ECS, which is not what it does.
Due to its safety contract, `TrustedEntityBorrow` is important an
important and widely used trait for `EntitySet` functionality.
In contrast, the safe `EntityBorrow` does not see much use, because even
outside of `EntitySet`-related functionality, it is a better idea to
accept `TrustedEntityBorrow` over `EntityBorrow`.
Furthermore, as #9367 points out, abstracting over returning `Entity`
from pointers/structs that contain it can skip some ergonomic friction.
On top of that, there are aspects of #18319 and #18408 that are relevant
to naming:
We've run into the issue that relying on a type default can switch
generic order. This is livable in some contexts, but unacceptable in
others.
To remedy that, we'd need to switch to a type alias approach:
The "defaulted" `Entity` case becomes a
`UniqueEntity*`/`Entity*Map`/`Entity*Set` alias, and the base type
receives a more general name. `TrustedEntityBorrow` does not mesh
clearly with sensible base type names.
## Solution
Replace any `EntityBorrow` bounds with `TrustedEntityBorrow`.
+
Rename them as such:
`EntityBorrow` -> `ContainsEntity`
`TrustedEntityBorrow` -> `EntityEquivalent`
For `EntityBorrow` we produce a change in meaning; We designate it for
types that aren't necessarily strict wrappers around `Entity` or some
pointer to `Entity`, but rather any of the myriad of types that contain
a single associated `Entity`.
This pattern can already be seen in the common `entity`/`id` methods
across the engine.
We do not mean for `ContainsEntity` to be a trait that abstracts input
API (like how `AsRef<T>` is often used, f.e.), because eliding
`entity()` would be too implicit in the general case.
We prefix "Contains" to match the intuition of a struct with an `Entity`
field, like some contain a `length` or `capacity`.
It gives the impression of structure, which avoids the implication of a
relationship to the `ECS`.
`HasEntity` f.e. could be interpreted as "a currently live entity",
As an input trait for APIs like #9367 envisioned, `TrustedEntityBorrow`
is a better fit, because it *does* restrict itself to strict wrappers
and pointers. Which is why we replace any
`EntityBorrow`/`ContainsEntity` bounds with
`TrustedEntityBorrow`/`EntityEquivalent`.
Here, the name `EntityEquivalent` is a lot closer to its actual meaning,
which is "A type that is both equivalent to an `Entity`, and forms the
same total order when compared".
Prior art for this is the
[`Equivalent`](https://docs.rs/hashbrown/latest/hashbrown/trait.Equivalent.html)
trait in `hashbrown`, which utilizes both `Borrow` and `Eq` for its one
blanket impl!
Given that we lose the `Borrow` moniker, and `Equivalent` can carry
various meanings, we expand on the safety comment of `EntityEquivalent`
somewhat. That should help prevent the confusion we saw in
[#18408](https://github.com/bevyengine/bevy/pull/18408#issuecomment-2742094176).
The new name meshes a lot better with the type aliasing approach in
#18408, by aligning with the base name `EntityEquivalentHashMap`.
For a consistent scheme among all set types, we can use this scheme for
the `UniqueEntity*` wrapper types as well!
This allows us to undo the switched generic order that was introduced to
`UniqueEntityArray` by its `Entity` default.
Even without the type aliases, I think these renames are worth doing!
## Migration Guide
Any use of `EntityBorrow` becomes `ContainsEntity`.
Any use of `TrustedEntityBorrow` becomes `EntityEquivalent`.
# Objective
Unlike for their helper typers, the import paths for
`unique_array::UniqueEntityArray`, `unique_slice::UniqueEntitySlice`,
`unique_vec::UniqueEntityVec`, `hash_set::EntityHashSet`,
`hash_map::EntityHashMap`, `index_set::EntityIndexSet`,
`index_map::EntityIndexMap` are quite redundant.
When looking at the structure of `hashbrown`, we can also see that while
both `HashSet` and `HashMap` have their own modules, the main types
themselves are re-exported to the crate level.
## Solution
Re-export the types in their shared `entity` parent module, and simplify
the imports where they're used.
# Objective
Fixes#18515
After the recent changes to system param validation, the panic message
for a missing resource is currently:
```
Encountered an error in system `missing_resource_error::res_system`: SystemParamValidationError { skipped: false }
```
Add the parameter type name and a descriptive message, improving the
panic message to:
```
Encountered an error in system `missing_resource_error::res_system`: SystemParamValidationError { skipped: false, message: "Resource does not exist", param: "bevy_ecs::change_detection::Res<missing_resource_error::MissingResource>" }
```
## Solution
Add fields to `SystemParamValidationError` for error context. Include
the `type_name` of the param and a message.
Store them as `Cow<'static, str>` and only format them into a friendly
string in the `Display` impl. This lets us create errors using a
`&'static str` with no allocation or formatting, while still supporting
runtime `String` values if necessary.
Add a unit test that verifies the panic message.
## Future Work
If we change the default error handling to use `Display` instead of
`Debug`, and to use `ShortName` for the system name, the panic message
could be further improved to:
```
Encountered an error in system `res_system`: Parameter `Res<MissingResource>` failed validation: Resource does not exist
```
However, `BevyError` currently includes the backtrace in `Debug` but not
`Display`, and I didn't want to try to change that in this PR.
# Objective
#18555 added improved require syntax, but inline structs didn't support
`..Default::default()` syntax (for technical reasons we can't parse the
struct directly, so there is manual logic that missed this case).
## Solution
When a `{}` or `()` section is encountered for a required component,
rather than trying to parse the fields directly, just pass _all_ of the
tokens through. This ensures no tokens are dropped, protects us against
any future syntax changes, and optimizes our parsing logic (as we're
dropping the field parsing logic entirely).
# Objective
Requires are currently more verbose than they need to be. People would
like to define inline component values. Additionally, the current
`#[require(Foo(custom_constructor))]` and `#[require(Foo(|| Foo(10))]`
syntax doesn't really make sense within the context of the Rust type
system. #18309 was an attempt to improve ergonomics for some cases, but
it came at the cost of even more weirdness / unintuitive behavior. Our
approach as a whole needs a rethink.
## Solution
Rework the `#[require()]` syntax to make more sense. This is a breaking
change, but I think it will make the system easier to learn, while also
improving ergonomics substantially:
```rust
#[derive(Component)]
#[require(
A, // this will use A::default()
B(1), // inline tuple-struct value
C { value: 1 }, // inline named-struct value
D::Variant, // inline enum variant
E::SOME_CONST, // inline associated const
F::new(1), // inline constructor
G = returns_g(), // an expression that returns G
H = SomethingElse::new(), // expression returns SomethingElse, where SomethingElse: Into<H>
)]
struct Foo;
```
## Migration Guide
Custom-constructor requires should use the new expression-style syntax:
```rust
// before
#[derive(Component)]
#[require(A(returns_a))]
struct Foo;
// after
#[derive(Component)]
#[require(A = returns_a())]
struct Foo;
```
Inline-closure-constructor requires should use the inline value syntax
where possible:
```rust
// before
#[derive(Component)]
#[require(A(|| A(10))]
struct Foo;
// after
#[derive(Component)]
#[require(A(10)]
struct Foo;
```
In cases where that is not possible, use the expression-style syntax:
```rust
// before
#[derive(Component)]
#[require(A(|| A(10))]
struct Foo;
// after
#[derive(Component)]
#[require(A = A(10)]
struct Foo;
```
---------
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: François Mockers <mockersf@gmail.com>
# Objective
Fix panic in `run_system` when running an exclusive system wrapped in a
`PipeSystem` or `AdapterSystem`.
#18076 introduced a `System::run_without_applying_deferred` method. It
normally calls `System::run_unsafe`, but
`ExclusiveFunctionSystem::run_unsafe` panics, so it was overridden for
that type. Unfortunately, `PipeSystem::run_without_applying_deferred`
still calls `PipeSystem::run_unsafe`, which can then call
`ExclusiveFunctionSystem::run_unsafe` and panic.
## Solution
Make `ExclusiveFunctionSystem::run_unsafe` work instead of panicking.
Clarify the safety requirements that make this sound.
The alternative is to override `run_without_applying_deferred` in
`PipeSystem`, `CombinatorSystem`, `AdapterSystem`,
`InfallibleSystemWrapper`, and `InfallibleObserverWrapper`. That seems
like a lot of extra code just to preserve a confusing special case!
Remove some implementations of `System::run` that are no longer
necessary with this change. This slightly changes the behavior of
`PipeSystem` and `CombinatorSystem`: Currently `run` will call
`apply_deferred` on the first system before running the second, but
after this change it will only call it after *both* systems have run.
The new behavior is consistent with `run_unsafe` and
`run_without_applying_deferred`, and restores the behavior prior to
#11823.
The panic was originally necessary because [`run_unsafe` took
`&World`](https://github.com/bevyengine/bevy/pull/6083/files#diff-708dfc60ec5eef432b20a6f471357a7ea9bfb254dc2f918d5ed4a66deb0e85baR90).
Now that it takes `UnsafeWorldCell`, it is possible to make it work. See
also Cart's concerns at
https://github.com/bevyengine/bevy/pull/4166#discussion_r979140356,
although those also predate `UnsafeWorldCell`.
And see #6698 for a previous bug caused by this panic.
# Objective
Make it easier to short-circuit system parameter validation.
Simplify the API surface by combining `ValidationOutcome` with
`SystemParamValidationError`.
## Solution
Replace `ValidationOutcome` with `Result<(),
SystemParamValidationError>`. Move the docs from `ValidationOutcome` to
`SystemParamValidationError`.
Add a `skipped` field to `SystemParamValidationError` to distinguish the
`Skipped` and `Invalid` variants.
Use the `?` operator to short-circuit validation in tuples of system
params.
# Objective
- Fixes#16861
## Solution
- Added:
- `UnsafeEntityCell::get_mut_assume_mutable_by_id`
- `EntityMut::get_mut_assume_mutable_by_id`
- `EntityMut::get_mut_assume_mutable_by_id_unchecked`
- `EntityWorldMut::into_mut_assume_mutable_by_id`
- `EntityWorldMut::into_mut_assume_mutable`
- `EntityWorldMut::get_mut_assume_mutable_by_id`
- `EntityWorldMut::into_mut_assume_mutable_by_id`
- `EntityWorldMut::modify_component_by_id`
- `World::modify_component_by_id`
- `DeferredWorld::modify_component_by_id`
- Added `fetch_mut_assume_mutable` to `DynamicComponentFetch` trait
(this is a breaking change)
## Testing
- CI
---
## Migration Guide
If you had previously implemented `DynamicComponentFetch` you must now
include a definition for `fetch_mut_assume_mutable`. In general this
will be identical to `fetch_mut` using the relevant alternatives for
actually getting a component.
---
## Notes
All of the added methods are minor variations on existing functions and
should therefore be of low risk for inclusion during the RC process.
# Objective
fixes#18452.
## Solution
Spawning used to flush commands only, but those commands can reserve
entities. Now, spawning flushes everything, including reserved entities.
I checked, and this was the only place where `flush_commands` is used
instead of `flush` by mistake.
## Testing
I simplified the MRE from #18452 into its own test, which fails on main,
but passes on this branch.
# Objective
When introduced, `Single` was intended to simply be silently skipped,
allowing for graceful and efficient handling of systems during invalid
game states (such as when the player is dead).
However, this also caused missing resources to *also* be silently
skipped, leading to confusing and very hard to debug failures. In
0.15.1, this behavior was reverted to a panic, making missing resources
easier to debug, but largely making `Single` (and `Populated`)
worthless, as they would panic during expected game states.
Ultimately, the consensus is that this behavior should differ on a
per-system-param basis. However, there was no sensible way to *do* that
before this PR.
## Solution
Swap `SystemParam::validate_param` from a `bool` to:
```rust
/// The outcome of system / system param validation,
/// used by system executors to determine what to do with a system.
pub enum ValidationOutcome {
/// All system parameters were validated successfully and the system can be run.
Valid,
/// At least one system parameter failed validation, and an error must be handled.
/// By default, this will result in1 a panic. See [crate::error] for more information.
///
/// This is the default behavior, and is suitable for system params that should *always* be valid,
/// either because sensible fallback behavior exists (like [`Query`] or because
/// failures in validation should be considered a bug in the user's logic that must be immediately addressed (like [`Res`]).
Invalid,
/// At least one system parameter failed validation, but the system should be skipped due to [`ValidationBehavior::Skip`].
/// This is suitable for system params that are intended to only operate in certain application states, such as [`Single`].
Skipped,
}
```
Then, inside of each `SystemParam` implementation, return either Valid,
Invalid or Skipped.
Currently, only `Single`, `Option<Single>` and `Populated` use the
`Skipped` behavior. Other params (like resources) retain their current
failing
## Testing
Messed around with the fallible_params example. Added a pair of tests:
one for panicking when resources are missing, and another for properly
skipping `Single` and `Populated` system params.
## To do
- [x] get https://github.com/bevyengine/bevy/pull/18454 merged
- [x] fix the todo!() in the macro-powered tuple implementation (please
help 🥺)
- [x] test
- [x] write a migration guide
- [x] update the example comments
## Migration Guide
Various system and system parameter validation methods
(`SystemParam::validate_param`, `System::validate_param` and
`System::validate_param_unsafe`) now return and accept a
`ValidationOutcome` enum, rather than a `bool`. The previous `true`
values map to `ValidationOutcome::Valid`, while `false` maps to
`ValidationOutcome::Invalid`.
However, if you wrote a custom schedule executor, you should now respect
the new `ValidationOutcome::Skipped` parameter, skipping any systems
whose validation was skipped. By contrast, `ValidationOutcome::Invalid`
systems should also be skipped, but you should call the
`default_error_handler` on them first, which by default will result in a
panic.
If you are implementing a custom `SystemParam`, you should consider
whether failing system param validation is an error or an expected
state, and choose between `Invalid` and `Skipped` accordingly. In Bevy
itself, `Single` and `Populated` now once again skip the system when
their conditions are not met. This is the 0.15.0 behavior, but stands in
contrast to the 0.15.1 behavior, where they would panic.
---------
Co-authored-by: MiniaczQ <xnetroidpl@gmail.com>
Co-authored-by: Dmytro Banin <banind@cs.washington.edu>
Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com>
There are two related problems here:
1. Users should be able to change the fallback behavior of *all*
ECS-based errors in their application by setting the
`GLOBAL_ERROR_HANDLER`. See #18351 for earlier work in this vein.
2. The existing solution (#15500) for customizing this behavior is high
on boilerplate, not global and adds a great deal of complexity.
The consensus is that the default behavior when a parameter fails
validation should be set based on the kind of system parameter in
question: `Single` / `Populated` should silently skip the system, but
`Res` should panic. Setting this behavior at the system level is a
bandaid that makes getting to that ideal behavior more painful, and can
mask real failures (if a resource is missing but you've ignored a system
to make the Single stop panicking you're going to have a bad day).
I've removed the existing `ParamWarnPolicy`-based configuration, and
wired up the `GLOBAL_ERROR_HANDLER`/`default_error_handler` to the
various schedule executors to properly plumb through errors .
Additionally, I've done a small cleanup pass on the corresponding
example.
I've run the `fallible_params` example, with both the default and a
custom global error handler. The former panics (as expected), and the
latter spams the error console with warnings 🥲
1. Currently, failed system param validation will result in endless
console spam. Do you want me to implement a solution for warn_once-style
debouncing somehow?
2. Currently, the error reporting for failed system param validation is
very limited: all we get is that a system param failed validation and
the name of the system. Do you want me to implement improved error
reporting by bubbling up errors in this PR?
3. There is broad consensus that the default behavior for failed system
param validation should be set on a per-system param basis. Would you
like me to implement that in this PR?
My gut instinct is that we absolutely want to solve 2 and 3, but it will
be much easier to do that work (and review it) if we split the PRs
apart.
`ParamWarnPolicy` and the `WithParamWarnPolicy` have been removed
completely. Failures during system param validation are now handled via
the `GLOBAL_ERROR_HANDLER`: please see the `bevy_ecs::error` module docs
for more information.
---------
Co-authored-by: MiniaczQ <xnetroidpl@gmail.com>
# Objective
Bevy 0.15 used to have methods on `Children` for sorting and reordering
them. This is very important, because in certain situations, the order
of children matters. For example, in the context of UI nodes.
These methods are missing/omitted/forgotten in the current version,
after the Relationships rework.
Without them, it is impossible for me to upgrade `iyes_perf_ui` to Bevy
0.16.
## Solution
Reintroduce the methods. This PR simply copy-pastes them from Bevy 0.15.
# Objective
There are currently too many disparate ways to handle entity mapping,
especially after #17687. We now have MapEntities, VisitEntities,
VisitEntitiesMut, Component::visit_entities,
Component::visit_entities_mut.
Our only known use case at the moment for these is entity mapping. This
means we have significant consolidation potential.
Additionally, VisitEntitiesMut cannot be implemented for map-style
collections like HashSets, as you cant "just" mutate a `&mut Entity`.
Our current approach to Component mapping requires VisitEntitiesMut,
meaning this category of entity collection isn't mappable. `MapEntities`
is more generally applicable. Additionally, the _existence_ of the
blanket From impl on VisitEntitiesMut blocks us from implementing
MapEntities for HashSets (or any types we don't own), because the owner
could always add a conflicting impl in the future.
## Solution
Use `MapEntities` everywhere and remove all "visit entities" usages.
* Add `Component::map_entities`
* Remove `Component::visit_entities`, `Component::visit_entities_mut`,
`VisitEntities`, and `VisitEntitiesMut`
* Support deriving `Component::map_entities` in `#[derive(Coomponent)]`
* Add `#[derive(MapEntities)]`, and share logic with the
`Component::map_entities` derive.
* Add `ComponentCloneCtx::queue_deferred`, which is command-like logic
that runs immediately after normal clones. Reframe `FromWorld` fallback
logic in the "reflect clone" impl to use it. This cuts out a lot of
unnecessary work and I think justifies the existence of a pseudo-command
interface (given how niche, yet performance sensitive this is).
Note that we no longer auto-impl entity mapping for ` IntoIterator<Item
= &'a Entity>` types, as this would block our ability to implement cases
like `HashMap`. This means the onus is on us (or type authors) to add
explicit support for types that should be mappable.
Also note that the Component-related changes do not require a migration
guide as there hasn't been a release with them yet.
## Migration Guide
If you were previously implementing `VisitEntities` or
`VisitEntitiesMut` (likely via a derive), instead use `MapEntities`.
Those were almost certainly used in the context of Bevy Scenes or
reflection via `ReflectMapEntities`. If you have a case that uses
`VisitEntities` or `VisitEntitiesMut` directly, where `MapEntities` is
not a viable replacement, please let us know!
```rust
// before
#[derive(VisitEntities, VisitEntitiesMut)]
struct Inventory {
items: Vec<Entity>,
#[visit_entities(ignore)]
label: String,
}
// after
#[derive(MapEntities)]
struct Inventory {
#[entities]
items: Vec<Entity>,
label: String,
}
```
# Objective
Const values should be more ergonomic to insert, since this is too
verbose
``` rust
#[derive(Component)]
#[require(
LockedAxes(||LockedAxes::ROTATION_LOCKED),
)]
pub struct CharacterController;
```
instead, users can now abbreviate that nonsense like this
``` rust
#[derive(Component)]
#[require(
LockedAxes = ROTATION_LOCKED),
)]
pub struct CharacterController;
```
it also works for enum labels.
I chose to omit the type, since were trying to reduce typing here. The
alternative would have been this:
```rust
#[require(
LockedAxes = LockedAxes::ROTATION_LOCKED),
)]
```
This of course has its disadvantages, since the const has to be
associated, but the old closure method is still possible, so I dont
think its a problem.
- Fixes#16720
## Testing
I added one new test in the docs, which also explain the new change. I
also saw that the docs for the required components on line 165 was
missing an assertion, so I added it back in
---------
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
# Objective
While working on #18058 I realized I could use
`RelationshipTargetCollection::new`, so I added it.
## Solution
- Add `RelationshipTargetCollection::new`
- Add `RelationshipTargetCollection::reserve`. Could generally be useful
when doing micro-optimizations.
- Add `RelationshipTargetCollection::shrink_to_fit`. Rust collections
generally don't shrink when removing elements. Might be a good idea to
call this once in a while.
## Testing
`cargo clippy`
---
## Showcase
`RelationshipSourceCollection` now implements `new`, `reserve` and
`shrink_to_fit` to give greater control over how much memory it
consumes.
## Migration Guide
Any type implementing `RelationshipSourceCollection` now needs to also
implement `new`, `reserve` and `shrink_to_fit`. `reserve` and
`shrink_to_fit` can be made no-ops if they conceptually mean nothing to
a collection.
---------
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com>
# Objective
Add a way to efficiently replace a set of specifically related entities
with a new set.
Closes#18041
## Solution
Add new `replace_(related/children)` to `EntityWorldMut` and friends.
## Testing
Added a new test to `hierarchy.rs` that specifically check if
`replace_children` actually correctly replaces the children on a entity
while keeping the original one.
---
## Showcase
`EntityWorldMut` and `EntityCommands` can now be used to efficiently
replace the entities a entity is related to.
```rust
/// `parent` has 2 children. `entity_a` and `entity_b`.
assert_eq!([entity_a, entity_b], world.entity(parent).get::<Children>());
/// Replace `parent`s children with `entity_a` and `entity_c`
world.entity_mut(parent).replace_related(&[entity_a, entity_c]);
/// `parent` now has 2 children. `entity_a` and `entity_c`.
///
/// `replace_children` has saved time by not removing and reading
/// the relationship between `entity_a` and `parent`
assert_eq!([entity_a, entity_c], world.entity(parent).get::<Children>());
---------
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
## Objective
Fix `bevy_ecs` doc tests failing when used with `--all-features`.
```
---- crates\bevy_ecs\src\error\handler.rs - error::handler::GLOBAL_ERROR_HANDLER (line 87) stdout ----
error[E0425]: cannot find function `default_error_handler` in this scope
--> crates\bevy_ecs\src\error\handler.rs:92:24
|
8 | let error_handler = default_error_handler();
| ^^^^^^^^^^^^^^^^^^^^^ not found in this scope
```
I happened to come across this while testing #12207. I'm not sure it
actually needs fixing but seemed worth a go
## Testing
```
cargo test --doc -p bevy_ecs --all-features
```
## Side Notes
The CI misses this error as it doesn't use `--all-features`. Perhaps it
should?
I tried adding `--all-features` to `ci/src/commands/doc_tests.rs` but
this triggered a linker error:
```
Compiling bevy_dylib v0.16.0-dev (C:\Projects\bevy\crates\bevy_dylib)
error: linking with `link.exe` failed: exit code: 1189
= note: LINK : fatal error LNK1189: library limit of 65535 objects exceeded␍
```
# Objective
- ECS error handling is a lovely flagship feature for Bevy 0.16, all in
the name of reducing panics and encouraging better error handling
(#14275).
- Currently though, command and system error handling are completely
disjoint and use different mechanisms.
- Additionally, there's a number of distinct ways to set the
default/fallback/global error handler that have limited value. As far as
I can tell, this will be cfg flagged to toggle between dev and
production builds in 99.9% of cases, with no real value in more granular
settings or helpers.
- Fixes#17272
## Solution
- Standardize error handling on the OnceLock global error mechanisms
ironed out in https://github.com/bevyengine/bevy/pull/17215
- As discussed there, there are serious performance concerns there,
especially for commands
- I also think this is a better fit for the use cases, as it's truly
global
- Move from `SystemErrorContext` to a more general purpose
`ErrorContext`, which can handle observers and commands more clearly
- Cut the superfluous setter methods on `App` and `SubApp`
- Rename the limited (and unhelpful) `fallible_systems` example to
`error_handling`, and add an example of command error handling
## Testing
Ran the `error_handling` example.
## Notes for reviewers
- Do you see a clear way to allow commands to retain &mut World access
in the per-command custom error handlers? IMO that's a key feature here
(allowing the ad-hoc creation of custom commands), but I'm not sure how
to get there without exploding complexity.
- I've removed the feature gate on the default_error_handler: contrary
to @cart's opinion in #17215 I think that virtually all apps will want
to use this. Can you think of a category of app that a) is extremely
performance sensitive b) is fine with shipping to production with the
panic error handler? If so, I can try to gather performance numbers
and/or reintroduce the feature flag. UPDATE: see benches at the end of
this message.
- ~~`OnceLock` is in `std`: @bushrat011899 what should we do here?~~
- Do you have ideas for more automated tests for this collection of
features?
## Benchmarks
I checked the impact of the feature flag introduced: benchmarks might
show regressions. This bears more investigation. I'm still skeptical
that there are users who are well-served by a fast always panicking
approach, but I'm going to re-add the feature flag here to avoid
stalling this out.
---------
Co-authored-by: Zachary Harrold <zac@harrold.com.au>
# Objective
Fixes#18357
## Solution
Generalize `RelationshipInsertHookMode` to `RelationshipHookMode`, wire
it up to on_replace execution, and use it in the
`Relationship::on_replace` hook.
# Objective
FilteredResource::get should return a Result instead of Option
Fixes#17480
---
## Migration Guide
Users will need to handle the different return type on
FilteredResource::get, FilteredResource::get_id,
FilteredResource::get_mut as it is now a Result not an Option.
# Objective
Continuation of #17449.
#17449 implemented the wrapper types around `IndexMap`/`Set` and co.,
however punted on the slice types.
They are needed to support creating `EntitySetIterator`s from their
slices, not just the base maps and sets.
## Solution
Add the wrappers, in the same vein as #17449 and #17589 before.
The `Index`/`IndexMut` implementations take up a lot of space, however
they cannot be merged because we'd then get overlaps.
They are simply named `Slice` to match the `indexmap` naming scheme, but
this means they cannot be differentiated properly until their modules
are made public, which is already a follow-up mentioned in #17954.
# Objective
Now that #13432 has been merged, it's important we update our reflected
types to properly opt into this feature. If we do not, then this could
cause issues for users downstream who want to make use of
reflection-based cloning.
## Solution
This PR is broken into 4 commits:
1. Add `#[reflect(Clone)]` on all types marked `#[reflect(opaque)]` that
are also `Clone`. This is mandatory as these types would otherwise cause
the cloning operation to fail for any type that contains it at any
depth.
2. Update the reflection example to suggest adding `#[reflect(Clone)]`
on opaque types.
3. Add `#[reflect(clone)]` attributes on all fields marked
`#[reflect(ignore)]` that are also `Clone`. This prevents the ignored
field from causing the cloning operation to fail.
Note that some of the types that contain these fields are also `Clone`,
and thus can be marked `#[reflect(Clone)]`. This makes the
`#[reflect(clone)]` attribute redundant. However, I think it's safer to
keep it marked in the case that the `Clone` impl/derive is ever removed.
I'm open to removing them, though, if people disagree.
4. Finally, I added `#[reflect(Clone)]` on all types that are also
`Clone`. While not strictly necessary, it enables us to reduce the
generated output since we can just call `Clone::clone` directly instead
of calling `PartialReflect::reflect_clone` on each variant/field. It
also means we benefit from any optimizations or customizations made in
the `Clone` impl, including directly dereferencing `Copy` values and
increasing reference counters.
Along with that change I also took the liberty of adding any missing
registrations that I saw could be applied to the type as well, such as
`Default`, `PartialEq`, and `Hash`. There were hundreds of these to
edit, though, so it's possible I missed quite a few.
That last commit is **_massive_**. There were nearly 700 types to
update. So it's recommended to review the first three before moving onto
that last one.
Additionally, I can break the last commit off into its own PR or into
smaller PRs, but I figured this would be the easiest way of doing it
(and in a timely manner since I unfortunately don't have as much time as
I used to for code contributions).
## Testing
You can test locally with a `cargo check`:
```
cargo check --workspace --all-features
```
# Objective
Currently experimenting with manually implementing
`Relationship`/`RelationshipTarget` to support associated edge data,
which means I need to replace the default hook implementations provided
by those traits. However, copying them over for editing revealed that
`UnsafeWorldCell::get_raw_command_queue` is `pub(crate)`, and I would
like to not have to clone the source collection, like the default impl.
So instead, I've taken to providing a safe abstraction for being able to
access entities and queue commands simultaneously.
## Solution
Added `World::entities_and_commands` and
`DeferredWorld::entities_and_commands`, which can be used like so:
```rust
let eid: Entity = /* ... */;
let (mut fetcher, mut commands) = world.entities_and_commands();
let emut = fetcher.get_mut(eid).unwrap();
commands.entity(eid).despawn();
```
## Testing
- Added a new test for each of the added functions.
---------
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
# Objective
Continuation to #16547 and #17954.
The `get_many` family are the last methods on `Query`/`QueryState` for
which we're still missing a `unique` version.
## Solution
Offer `get_many_unique`/`get_many_unique_mut` and
`get_many_unique_inner`!
Their implementation is the same as `get_many`, the difference lies in
their guaranteed-to-be unique inputs, meaning we never do any aliasing
checks.
To reduce confusion, we also rename `get_many_readonly` into
`get_many_inner` and the current `get_many_inner` into
`get_many_mut_inner` to clarify their purposes.
## Testing
Doc examples.
## Migration Guide
`get_many_inner` is now called `get_many_mut_inner`.
`get_many_readonly` is now called `get_many_inner`.
Registering dynamic bundles was not possible for the user yet.
It is alone not very useful though as there are no methods to clone,
move or remove components via a `BundleId`. This could be a follow-up
work if this PR is approved and such a third (besides `T: Bundle` and
`ComponentId`(s)) API for structural operation is desired. I certainly
would use a hypothetical `EntityClonerBuilder::allow_by_bundle_id`.
I personally still would like this register method because I have a
`Bundles`-like custom data structure and I would like to not reinvent
the wheel. Then instead of having boxed `ComponentId` slices in my
collection I could look up explicit and required components there.
For reference scroll up to the typed version right above the new one.
# Objective
Installment of the #16547 series.
The vast majority of uses for these types will be the `Entity` case, so
it makes sense for it to be the default.
## Solution
`UniqueEntityVec`, `UniqueEntitySlice`, `UniqueEntityArray` and their
helper iterator aliases now have `Entity` as a default.
Unfortunately, this means the the `T` parameter for `UniqueEntityArray`
now has to be ordered after the `N` constant, which breaks the
consistency to `[T; N]`.
Same with about a dozen iterator aliases that take some `P`/`F`
predicate/function parameter.
This should however be an ergonomic improvement in most cases, so we'll
just have to live with this inconsistency.
## Migration Guide
Switch type parameter order for the relevant wrapper types/aliases.
# 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`
# Objective
While poking at https://github.com/bevyengine/bevy/issues/17272, I
noticed a few small things to clean up.
## Solution
- Improve the docs
- ~~move `SystemErrorContext` out of the `handler.rs` module: it's not
an error handler~~
# Objective
Prevents duplicate implementation between IntoSystemConfigs and
IntoSystemSetConfigs using a generic, adds a NodeType trait for more
config flexibility (opening the door to implement
https://github.com/bevyengine/bevy/issues/14195?).
## Solution
Followed writeup by @ItsDoot:
https://hackmd.io/@doot/rJeefFHc1x
Removes IntoSystemConfigs and IntoSystemSetConfigs, instead using
IntoNodeConfigs with generics.
## Testing
Pending
---
## Showcase
N/A
## Migration Guide
SystemSetConfigs -> NodeConfigs<InternedSystemSet>
SystemConfigs -> NodeConfigs<ScheduleSystem>
IntoSystemSetConfigs -> IntoNodeConfigs<InternedSystemSet, M>
IntoSystemConfigs -> IntoNodeConfigs<ScheduleSystem, M>
---------
Co-authored-by: Christian Hughes <9044780+ItsDoot@users.noreply.github.com>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
# Objective
In updating examples to use the Improved Spawning API I got tripped up
on being able to spawn children with a Vec. I eventually figured out
that I could use `Children::spawn(SpawnIter(my_vec.into_iter()))` but
thought there might be a more ergonomic way to approach it. After
tinkering with it for a while I came up with the implementation here. It
allows authors to use `Children::spawn(my_vec)` as an equivalent
implementation.
## Solution
- Implements `<R: Relationship, B: Bundle SpawnableList<R> for Vec<B>`
- uses `alloc::vec::Vec` for compatibility with `no_std` (`std::Vec`
also inherits implementations against the `alloc::vec::Vec` because std
is a re-export of the alloc struct, thanks @bushrat011899 for the info
on this!)
## Testing
- Did you test these changes? If so, how?
- Opened the examples before and after and verified the same behavior
was observed. I did this on Ubuntu 24.04.2 LTS using `--features
wayland`.
- Are there any parts that need more testing?
- Other OS's and features can't hurt, but this is such a small change it
shouldn't be a problem.
- How can other people (reviewers) test your changes? Is there anything
specific they need to know?
- Run the examples yourself with and without these changes.
- If relevant, what platforms did you test these changes on, and are
there any important ones you can't test?
- see above
## Showcase
n/a
## Migration Guide
- Optional: you may use the new API to spawn `Vec`s of `Bundle` instead
of using the `SpawnIter` approach.
# Objective
- Prevent usage of `println!`, `eprintln!` and the like because they
require `std`
- Fixes#17446
## Solution
- Enable the `print_stdout` and `print_stderr` clippy lints
- Replace all `println!` and `eprintln!` occurrences with `log::*` where
applicable or alternatively ignore the warnings
## Testing
- Run `cargo clippy --workspace` to ensure that there are no warnings
relating to printing to `stdout` or `stderr`
# Objective
Part of the #16547 series.
The entity wrapper types often have some associated types an aliases
with them that cannot be re-exported into an outer module together.
Some helper types are best used with part of their path:
`bevy::ecs::entity::index_set::Slice` as `index_set::Slice`.
This has already been done for `entity::hash_set` and
`entity::hash_map`.
## Solution
Publicize the `index_set`, `index_map`, `unique_vec`, `unique_slice`,
and `unique_array` modules.
## Migration Guide
Any mention or import of types in the affected modules have to add the
respective module name to the import path.
F.e.:
`bevy::ecs::entity::EntityIndexSet` ->
`bevy::ecs::entity::index_set::EntityIndexSet`
# Objective
While redoing #18058 I needed `RelationshipSourceCollection` (henceforth
referred to as the **Trait**) to implement `clear` so I added it.
## Solution
Add the `clear` method to the **Trait**.
Also make `add` and `remove` report if they succeeded.
## Testing
Eyeballs
---
## Showcase
The `RelationshipSourceCollection` trait now reports if adding or
removing an entity from it was successful.
It also not contains the `clear` method so you can easily clear the
collection in generic contexts.
## Changes
EDITED by Alice: We should get this into 0.16, so no migration guide
needed.
The `RelationshipSourceCollection` methods `add` and `remove` now need
to return a boolean indicating if they were successful (adding a entity
to a set that already contains it counts as failure). Additionally the
`clear` method has been added to support clearing the collection in
generic contexts.
# Objective
I was recently exploreing `Entities` and stumbled on something strange.
`Entities::len` (the field) has the comment `Stores the number of free
entities for [`len`](Entities::len)`, refering to the method. But that
method says `The count of currently allocated entities.` Looking at the
code, the field's comment is wrong, and the public `len()` is correct.
Phew!
## Solution
So, I was just going to fix the comment, so it didn't confuse anyone
else, but as it turns out, we can just remove the field entirely. As a
bonus, this saves some book keeping work too. We can just calculate it
on the fly.
Also, add additional length methods and documentation for completeness.
These new length methods might be useful debug tools in the future.
---------
Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com>
# Objective
Make component registration faster. This is a tinny, almost petty PR,
but it led to roughly 10% faster registration, according to my
benchmarks in #17871.
Up to y'all if we do this or not. It is completely unnecessary, but its
such low hanging fruit that I wanted to put it out there.
## Solution
Instead of cloning a `HashSet`, collect it into a `SmallVec`. Since this
is empty for many components, this saves a lot of allocation and hashing
work.
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
# Objective
This is an alternative to #17871 and #17701 for tracking issue #18155.
This thanks to @maniwani for help with this design.
The goal is to enable component ids to be reserved from multiple threads
concurrently and with only `&World`. This contributes to assets as
entities, read-only query and system parameter initialization, etc.
## What's wrong with #17871 ?
In #17871, I used my proposed staging utilities to allow *fully*
registering components from any thread concurrently with only
`&Components`. However, if we want to pursue components as entities
(which is desirable for a great many reasons. See
[here](https://discord.com/channels/691052431525675048/692572690833473578/1346499196655505534)
on discord), this staging isn't going to work. After all, if registering
a component requires spawning an entity, and spawning an entity requires
`&mut World`, it is impossible to register a component fully with only
`&World`.
## Solution
But what if we don't have to register it all the way? What if it's
enough to just know the `ComponentId` it will have once it is registered
and to queue it to be registered at a later time? Spoiler alert: That is
all we need for these features.
Here's the basic design:
Queue a registration:
1. Check if it has already been registered.
2. Check if it has already been queued.
3. Reserve a `ComponentId`.
4. Queue the registration at that id.
Direct (normal) registration:
1. Check if this registration has been queued.
2. If it has, use the queued registration instead.
3. Otherwise, proceed like normal.
Appllying the queue:
1. Pop queued items off one by one.
2. Register them directly.
One other change:
The whole point of this design over #17871 is to facilitate coupling
component registration with the World. To ensure that this would fully
work with that, I went ahead and moved the `ComponentId` generator onto
the world itself. That stemmed a couple of minor organizational changes
(see migration guide). As we do components as entities, we will replace
this generator with `Entities`, which lives on `World` too. Doing this
move early let me verify the design and will reduce migration headaches
in the future. If components as entities is as close as I think it is, I
don't think splitting this up into different PRs is worth it. If it is
not as close as it is, it might make sense to still do #17871 in the
meantime (see the risks section). I'll leave it up to y'all what we end
up doing though.
## Risks and Testing
The biggest downside of this compared to #17871 is that now we have to
deal with correct but invalid `ComponentId`s. They are invalid because
the component still isn't registered, but they are correct because, once
registered, the component will have exactly that id.
However, the only time this becomes a problem is if some code violates
safety rules by queuing a registration and using the returned id as if
it was valid. As this is a new feature though, nothing in Bevy does
this, so no new tests were added for it. When we do use it, I left
detailed docs to help mitigate issues here, and we can test those
usages. Ex: we will want some tests on using queries initialized from
queued registrations.
## Migration Guide
Component registration can now be queued with only `&World`. To
facilitate this, a few APIs needed to be moved around.
The following functions have moved from `Components` to
`ComponentsRegistrator`:
- `register_component`
- `register_component_with_descriptor`
- `register_resource_with_descriptor`
- `register_non_send`
- `register_resource`
- `register_required_components_manual`
Accordingly, functions in `Bundle` and `Component` now take
`ComponentsRegistrator` instead of `Components`.
You can obtain `ComponentsRegistrator` from the new
`World::components_registrator`.
You can obtain `ComponentsQueuedRegistrator` from the new
`World::components_queue`, and use it to stage component registration if
desired.
# Open Question
Can we verify that it is enough to queue registration with `&World`? I
don't think it would be too difficult to package this up into a
`Arc<MyComponentsManager>` type thing if we need to, but keeping this on
`&World` certainly simplifies things. If we do need the `Arc`, we'll
need to look into partitioning `Entities` for components as entities, so
we can keep most of the allocation fast on `World` and only keep a
smaller partition in the `Arc`. I'd love an SME on assets as entities to
shed some light on this.
---------
Co-authored-by: andriyDev <andriydzikh@gmail.com>
# Objective
Fixes#18030
## Solution
When running a one-shot system, requeue the system's command queue onto
the world's command queue, then execute the later.
If running the entire command queue of the world is undesired, I could
add a new method to `RawCommandQueue` to only apply part of it.
## Testing
See the new test.
---
## Showcase
```rust
#[derive(Resource)]
pub struct Test {
id: SystemId,
counter: u32,
}
let mut world = World::new();
let id = world.register_system(|mut commands: Commands, mut test: ResMut<Test>| {
print!("{:?} ", test.counter);
test.counter -= 1;
if test.counter > 0 {
commands.run_system(test.id);
}
});
world.insert_resource(Test { id, counter: 5 });
world.run_system(id).unwrap();
```
```
5 4 3 2 1
```
# Objective
I found a bug while working on #17871. When required components are
registered, ones that are more specific (smaller inheritance depth) are
preferred to others. So, if a ComponentA is already required, but it is
registered as required again, it will be updated if and only if the new
requirement has a smaller inheritance depth (is more specific). However,
this logic was not reflected in merging `RequriedComponents`s together.
Hence, for complicated requirement trees, the wrong initializer could be
used.
## Solution
Re-write merging to work by extending the collection via
`require_dynamic` instead of blindly combining the inner storage.
## Testing
I created this test to ensure this bug doesn't creep back in. This test
fails on main, but passes on this branch.
```rs
#[test]
fn required_components_inheritance_depth_bias() {
#[derive(Component, PartialEq, Eq, Clone, Copy, Debug)]
struct MyRequired(bool);
#[derive(Component, Default)]
#[require(MyRequired(|| MyRequired(false)))]
struct MiddleMan;
#[derive(Component, Default)]
#[require(MiddleMan)]
struct ConflictingRequire;
#[derive(Component, Default)]
#[require(MyRequired(|| MyRequired(true)))]
struct MyComponent;
let mut world = World::new();
let order_a = world
.spawn((ConflictingRequire, MyComponent))
.get::<MyRequired>()
.cloned();
let order_b = world
.spawn((MyComponent, ConflictingRequire))
.get::<MyRequired>()
.cloned();
assert_eq!(order_a, Some(MyRequired(true)));
assert_eq!(order_b, Some(MyRequired(true)));
}
```
Note that when the inheritance depth is 0 (Like if there were no middle
man above), the order of the components in the bundle still matters.
## Migration Guide
`RequiredComponents::register_dynamic` has been changed to
`RequiredComponents::register_dynamic_with`.
Old:
```rust
required_components.register_dynamic(
component_id,
component_constructor.clone(),
requirement_inheritance_depth,
);
```
New:
```rust
required_components.register_dynamic_with(
component_id,
requirement_inheritance_depth,
|| component_constructor.clone(),
);
```
This can prevent unnecessary cloning.
---------
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
Co-authored-by: Joona Aalto <jondolf.dev@gmail.com>
# Objective
Fix unsound lifetimes in `Query::join` and `Query::join_filtered`.
The joined query allowed access from either input query, but it only
took the `'world` lifetime from `self`, not from `other`. This meant
that after the borrow of `other` ended, the joined query would unsoundly
alias `other`.
## Solution
Change the lifetimes on `join` and `join_filtered` to require mutable
borrows of the *same* lifetime for the input queries. This ensures both
input queries are borrowed for the full lifetime of the joined query.
Change `join_inner` to take `other` by value instead of reference so
that the returned query is still usable without needing to borrow from a
local variable.
## Testing
Added a compile-fail test.
