# Objective
Make the schedule graph code more understandable, and replace some
panics with `Result`s.
I found the `check_edges` and `check_hierarchy` functions [a little
confusing](https://github.com/bevyengine/bevy/pull/19352#discussion_r2181486099),
as they combined two concerns: Initializing graph nodes for system sets,
and checking for self-edges on system sets. It was hard to understand
the self-edge checks because it wasn't clear what `NodeId` was being
checked against! So, let's separate those concerns, and move them to
more appropriate locations.
Fix a bug where `schedule.configure_sets((SameSet, SameSet).chain());`
would panic with an unhelpful message: `assertion failed: index_a <
index_b`.
## Solution
Remove the `check_edges` and `check_hierarchy` functions, separating the
initialization behavior and the checking behavior and moving them where
they are easier to understand.
For initializing graph nodes, do this on-demand using the `entry` API by
replacing later `self.system_set_ids[&set]` calls with a
`self.system_sets.get_or_add_set(set)` method. This should avoid the
need for an extra pass over the graph and an extra lookup.
Unfortunately, implementing that method directly on `ScheduleGraph`
leads to borrowing errors as it borrows the entire `struct`. So, split
out the collections managing system sets into a separate `struct`.
For checking self-edges, move this check later so that it can be
reported by returning a `Result` from `Schedule::initialize` instead of
having to panic in `configure_set_inner`. The issue was that `iter_sccs`
does not report self-edges as cycles, since the resulting components
only have one node, but that later code assumes all edges point forward.
So, check for self-edges directly, immediately before calling
`iter_sccs`.
This also ensures we catch *every* way that self-edges can be added. The
previous code missed an edge case where `chain()`ing a set to itself
would create a self-edge and would trigger a `debug_assert`.
## Objective
Fixes#19884.
## Solution
- Add an internal entity command `insert_with`, which takes a function
returning a component and checks if the component would actually be
inserted before invoking the function.
- Add the same check to `insert_from_world`, since it's a similar
situation.
- Update the `or_insert_with`, `or_try_insert_with`, and `or_default`
methods on `EntityEntryCommands` to use the new command.
Since the function/closure returning the component now needs to be sent
into the command (rather than being invoked before the command is
created), the function now has `Send + 'static` bounds. Pretty typical
for command stuff, but I don't know how/if it'll affect existing users.
---------
Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com>
# Objective
#19649 introduced new `*_if_new` and `*_by_bundle_id_*` variations to
`EntityClonerBuilder` filtering functionality, which resulted in
increase in method permutations - there are now 8 allow variants to
support various id types and 2 different insert modes.
## Solution
This PR introduces a new trait `FilterableIds` to unify all id types and
their `IntoIterator` implementations, which is somewhat similar to
`WorldEntityFetch`. It supports `TypeId`, `ComponentId` and `BundleId`,
allowing us to reduce the number of `allow` methods to 4: `allow`,
`allow_if_new`, `allow_by_ids`, `allow_by_ids_if_new`. The function
signature is a bit less readable now, but the docs mention types that
can be passed in.
## Testing
All existing tests pass, performance is unchanged.
---------
Co-authored-by: urben1680 <55257931+urben1680@users.noreply.github.com>
# Objective
- The usage of ComponentId is quite confusing: events are not
components. By newtyping this, we can prevent stupid mistakes, avoid
leaking internal details and make the code clearer for users and engine
devs reading it.
- Adopts https://github.com/bevyengine/bevy/pull/19755
---------
Co-authored-by: oscar-benderstone <oscarbenderstone@gmail.com>
Co-authored-by: Oscar Bender-Stone <88625129+oscar-benderstone@users.noreply.github.com>
# Objective
- I accidentally left a `register_component_hooks` without actually
adding a hook and didnt notice
## Solution
- mark it must_use so it doesnt happen to other people (maybe this is
just skill issue on me though)
# Objective
Allow combinator and pipe systems to delay validation of the second
system, while still allowing the second system to be skipped.
Fixes#18796
Allow fallible systems to be used as one-shot systems, reporting errors
to the error handler when used through commands.
Fixes#19722
Allow fallible systems to be used as run conditions, including when used
with combinators. Alternative to #19580.
Always validate parameters when calling the safe
`run_without_applying_deferred`, `run`, and `run_readonly` methods on a
`System`.
## Solution
Have `System::run_unsafe` return a `Result`.
We want pipe systems to run the first system before validating the
second, since the first system may affect whether the second system has
valid parameters. But if the second system skips then we have no output
value to return! So, pipe systems must return a `Result` that indicates
whether the second system ran.
But if we just make pipe systems have `Out = Result<B::Out>`, then
chaining `a.pipe(b).pipe(c)` becomes difficult. `c` would need to accept
the `Result` from `a.pipe(b)`, which means it would likely need to
return `Result` itself, giving `Result<Result<Out>>`!
Instead, we make *all* systems return a `Result`! We move the handling
of fallible systems from `IntoScheduleConfigs` and `IntoObserverSystem`
to `SystemParamFunction` and `ExclusiveSystemParamFunction`, so that an
infallible system can be wrapped before being passed to a combinator.
As a side effect, this enables fallible systems to be used as run
conditions and one-shot systems.
Now that the safe `run_without_applying_deferred`, `run`, and
`run_readonly` methods return a `Result`, we can have them perform
parameter validation themselves instead of requiring each caller to
remember to call them. `run_unsafe` will continue to not validate
parameters, since it is used in the multi-threaded executor when we want
to validate and run in separate tasks.
Note that this makes type inference a little more brittle. A function
that returns `Result<T>` can be considered either a fallible system
returning `T` or an infallible system returning `Result<T>` (and this is
important to continue supporting `pipe`-based error handling)! So there
are some cases where the output type of a system can no longer be
inferred. It will work fine when directly adding to a schedule, since
then the output type is fixed to `()` (or `bool` for run conditions).
And it will work fine when `pipe`ing to a system with a typed input
parameter.
I used a dedicated `RunSystemError` for the error type instead of plain
`BevyError` so that skipping a system does not box an error or capture a
backtrace.
...which previously used a HashSet, whos iter has no ordering guarantee
fixes#19687
i also discovered that the asserted order in the unit test is reversed,
so i fixed that. I dont know if that reversed order is intentional
Edit: i referenced the wrong issue oops
# Objective
- First step towards #279
## Solution
Makes the necessary internal data structure changes in order to allow
system removal to be added in a future PR: `Vec`s storing systems and
system sets in `ScheduleGraph` have been replaced with `SlotMap`s.
See the included migration guide for the required changes.
## Testing
Internal changes only and no new features *should* mean no new tests are
requried.
# Objective
- `MapEntities` is not implemented for arrays, `HashMap`, `BTreeMap`,
and `IndexMap`.
## Solution
- Implement `MapEntities` for arrays, `HashMap`, `BTreeMap`, `IndexMap`
## Testing
- I didn't add a test for this as the implementations seems pretty
trivial
# Objective
Fixes#19356
Issue: Spawning a batch of entities in relationship with the same target
adds the relationship between the target and only the last entity of the
batch. `spawn_batch` flushes only after having spawned all entities.
This means each spawned entity will have run the `on_insert` hook of its
`Relationship` component. Here is the relevant part of that hook:
```Rust
if let Some(mut relationship_target) =
target_entity_mut.get_mut::<Self::RelationshipTarget>()
{
relationship_target.collection_mut_risky().add(entity);
} else {
let mut target = <Self::RelationshipTarget as RelationshipTarget>::with_capacity(1);
target.collection_mut_risky().add(entity);
world.commands().entity(target_entity).insert(target);
}
```
Given the above snippet and since there's no flush between spawns, each
entity finds the target without a `RelationshipTarget` component and
defers the insertion of that component with the entity's id as the sole
member of its collection. When the commands are finally flushed, each
insertion after the first replaces the one before and in the process
triggers the `on_replace` hook of `RelationshipTarget` which removes the
`Relationship` component from the corresponding entity. That's how we
end up in the invalid state.
## Solution
I see two possible solutions
1. Flush after every spawn
2. Defer the whole code snippet above
I don't know enough about bevy as a whole but 2. seems much more
efficient to me. This is what I'm proposing here. I have a doubt though
because I've started to look at #19348 that 1. would fix as well.
## Testing
I added a test for the issue. I've put it in `relationship/mod.rs` but I
could see it in `world/spawn_batch.rs` or `lib.rs` because the test is
as much about `spawn_batch` as it is about relationships.
# Objective
- bevy_ecs has a expected lint that is both needed and unneeded
## Solution
- Change the logic so that it's always not needed
## Testing
`cargo clippy -p bevy_ecs --no-default-features --no-deps -- -D
warnings`
# Objective
1. Reduce overhead from error handling for ECS commands that
intentionally ignore errors, such as `try_despawn`. These commands
currently allocate error objects and pass them to a no-op handler
(`ignore`), which can impact performance when many operations fail.
2. Fix a hang when removing `ChildOf` components during entity
despawning. Excessive logging of these failures can cause significant
hangs (I'm noticing around 100ms).
- Fixes https://github.com/bevyengine/bevy/issues/19777
- Fixes https://github.com/bevyengine/bevy/issues/19753
<img width="1387" alt="image"
src="https://github.com/user-attachments/assets/5c67ab77-97bb-46e5-b287-2c502bef9358"
/>
## Solution
* Added a `ignore_error` method to the `HandleError` trait to use
instead of `handle_error_with(ignore)`. It swallows errors and does not
create error objects.
* Replaced `remove::<ChildOf>` with `try_remove::<ChildOf>` to suppress
expected (?) errors and reduce log noise.
## Testing
- I ran these changes on a local project.
# Objective
Further tests after #19326 showed that configuring `EntityCloner` with
required components is bug prone and the current design has several
weaknesses in it's API:
- Mixing `EntityClonerBuilder::allow` and `EntityClonerBuilder::deny`
requires extra care how to support that which has an impact on
surrounding code that has to keep edge cases in mind. This is especially
true for attempts to fix the following issues. There is no use-case
known (to me) why someone would mix those.
- A builder with `EntityClonerBuilder::allow_all` configuration tries to
support required components like `EntityClonerBuilder::deny_all` does,
but the meaning of that is conflicting with how you'd expect things to
work:
- If all components should be cloned except component `A`, do you also
want to exclude required components of `A` too? Or are these also valid
without `A` at the target entity?
- If `EntityClonerBuilder::allow_all` should ignore required components
and not add them to be filtered away, which purpose has
`EntityClonerBuilder::without_required_components` for this cloner?
- Other bugs found with the linked PR are:
- Denying `A` also denies required components of `A` even when `A` does
not exist at the source entity
- Allowing `A` also allows required components of `A` even when `A` does
not exist at the source entity
- Adding `allow_if_new` filters to the cloner faces the same issues and
require a common solution to dealing with source-archetype sensitive
cloning
Alternative to #19632 and #19635.
# Solution
`EntityClonerBuilder` is made generic and split into
`EntityClonerBuilder<OptOut>` and `EntityClonerBuilder<OptIn>`
For an overview of the changes, see the migration guide. It is generally
a good idea to start a review of that.
## Algorithm
The generic of `EntityClonerBuilder` contains the filter data that is
needed to build and clone the entity components.
As the filter needs to be borrowed mutably for the duration of the
clone, the borrow checker forced me to separate the filter value and all
other fields in `EntityCloner`. The latter are now in the
`EntityClonerConfig` struct. This caused many changed LOC, sorry.
To make reviewing easier:
1. Check the migration guide
2. Many methods of `EntityCloner` now just call identitcal
`EntityClonerConfig` methods with a mutable borrow of the filter
3. Check `EntityClonerConfig::clone_entity_internal` which changed a bit
regarding the filter usage that is now trait powered (`CloneByFilter`)
to support `OptOut`, `OptIn` and `EntityClonerFilter` (an enum combining
the first two)
4. Check `OptOut` type that no longer tracks required components but has
a `insert_mode` field
5. Check `OptIn` type that has the most logic changes
# Testing
I added a bunch of tests that cover the new logic parts and the fixed
issues.
Benchmarks are in a comment a bit below which shows ~4% to 9%
regressions, but it varied wildly for me. For example at one run the
reflection-based clonings were on-par with main while the other are not,
and redoing that swapped the situation for both.
It would be really cool if I could get some hints how to get better
benchmark results or if you could run them on your machine too.
Just be aware this is not a Performance PR but a Bugfix PR, even if I
smuggled in some more functionalities. So doing changes to
`EntityClonerBuilder` is kind of required here which might make us bite
the bullet.
---------
Co-authored-by: eugineerd <70062110+eugineerd@users.noreply.github.com>
# Objective
- Notice a word duplication typo
- Small quest to fix similar or nearby typos with my faithful companion
`\b(\w+)\s+\1\b`
## Solution
Fix em
# Objective
The objective of this PR is to enable Components to use their
`MapEntities` implementation for `Component::map_entities`.
With the improvements to the entity mapping system, there is definitely
a huge reduction in boilerplate. However, especially since
`(Entity)HashMap<..>` doesn't implement `MapEntities` (I presume because
the lack of specialization in rust makes `HashMap<Entity|X, Entity|X>`
complicated), when somebody has types that contain these hashmaps they
can't use this approach.
More so, we can't even depend on the previous implementation, since
`Component::map_entities` is used instead of
`MapEntities::map_entities`. Outside of implementing `Component `and
`Component::map_entities` on these types directly, the only path forward
is to create a custom type to wrap the hashmaps and implement map
entities on that, or split these components into a wrapper type that
implement `Component`, and an inner type that implements `MapEntities`.
## Current Solution
The solution was to allow adding `#[component(map_entities)]` on the
component. By default this will defer to the `MapEntities`
implementation.
```rust
#[derive(Component)]
#[component(map_entities)]
struct Inventory {
items: HashMap<Entity, usize>
}
impl MapEntities for Inventory {
fn map_entities<M: EntityMapper>(&mut self, entity_mapper: &mut M) {
self.items = self.items
.drain()
.map(|(id, count)|(entity_mapper.get_mapped(id), count))
.collect();
}
}
```
You can use `#[component(map_entities = <function path>)]` instead to
substitute other code in for components. This function can also include
generics, but sso far I haven't been able to find a case where they are
needed.
```rust
#[derive(Component)]
#[component(map_entities = map_the_map)]
// Also works #[component(map_entities = map_the_map::<T,_>)]
struct Inventory<T> {
items: HashMap<Entity, T>
}
fn map_the_map<T, M: EntityMapper>(inv: &mut Inventory<T>, entity_mapper: &mut M) {
inv.items = inv.items
.drain()
.map(|(id, count)|(entity_mapper.get_mapped(id), count))
.collect();
}
```
The idea is that with the previous changes to MapEntities, MapEntities
is implemented more for entity collections than for Components. If you
have a component that makes sense as both, `#[component(map_entities)]`
would work great, while otherwise a component can use
`#[component(map_entities = <function>)]` to change the behavior of
`Component::map_entities` without opening up the component type to be
included in other components.
## (Original Solution if you want to follow the PR)
The solution was to allow adding `#[component(entities)]` on the
component itself to defer to the `MapEntities` implementation
```rust
#[derive(Component)]
#[component(entities)]
struct Inventory {
items: HashMap<Entity, usize>
}
impl MapEntities for Inventory {
fn map_entities<M: EntityMapper>(&mut self, entity_mapper: &mut M) {
self.items = self.items
.drain()
.map(|(id, count)|(entity_mapper.get_mapped(id), count))
.collect();
}
}
```
## Testing
I tested this by patching my local changes into my own bevy project. I
had a system that loads a scene file and executes some logic with a
Component that contains a `HashMap<Entity, UVec2>`, and it panics when
Entity is not found from another query. Since the 0.16 update this
system has reliably panicked upon attempting to the load the scene.
After patching my code in, I added `#[component(entities)]` to this
component, and I was able to successfully load the scene.
Additionally, I wrote a doc test.
## Call-outs
### Relationships
This overrules the default mapping of relationship fields. Anything else
seemed more problematic, as you'd have inconsistent behavior between
`MapEntities` and `Component`.
# Objective
While working on #17607, I found myself confused and frustrated by the
tangled web woven by the various modules inside of our observers code.
Rather than tackle that as part of a big rewrite PR, I've decided to do
the mature (if frustrating) thing where you split out your trivial but
noisy refactoring first.
There are a large number of moving parts, especially in terms of
storage, and these are strewn willy-nilly across the module with no
apparent ordering. To make matters worse, this was almost all just
dumped into a multi-thousand LOC mod.rs at the root.
## Solution
I've reshuffled the modules, attempting to:
- reduce the size of the mod.rs file
- organize structs so that smaller structs are found after the larger
structs that contain them
- group related functionality together
- document why modules exist, and their broad organization
No functional changes have been made here, although I've had to increase
the visibility of a few fields from private to pub(crate) or pub(super)
to keep things compiling.
During these changes, I've opted for the lazy private module, public
re-export strategy, to avoid causing any breakages, both within and
outside of `bevy` itself. I think we can do better, but I want to leave
that for a proper cleanup pass at the end. There's no sense maintaining
migration guides and forcing multiple breaking changes throughout the
cycle.
## Testing
No functional changes; relying on existing test suite and the Rust
compiler.
# Objective
Fix https://github.com/bevyengine/bevy/issues/19617
# Solution
Add newlines before all impl blocks.
I suspect that at least some of these will be objectionable! If there's
a desired Bevy style for this then I'll update the PR. If not then we
can just close it - it's the work of a single find and replace.
# Objective
There is a lot of `world.entities().len()`, especially in tests. In
tests, usually, the assumption is made that empty worlds do not contain
any entities. This is about to change (#19711), and as such all of these
tests are failing for that PR.
## Solution
`num_entities` is a convenience method that returns the number of
entities inside a world. It can later be adapted to exclude 'unexpected'
entities, associated with internal data structures such as Resources,
Queries, Systems. In general I argue for a separation of concepts where
`World` ignores internal entities in methods such as `iter_entities()`
and `clear_entities()`, that discussion is, however, separate from this
PR.
## Testing
I replaced most occurrences of `world.entities().len()` with
`world.num_entities()` and the tests passed.
---------
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
# Objective
Some methods and commands carelessly overwrite `Relationship`
components. This may overwrite additional data stored at them which is
undesired.
Part of #19589
## Solution
A new private method will be used instead of insert:
`modify_or_insert_relation_with_relationship_hook_mode`.
This method behaves different to `insert` if `Relationship` is a larger
type than `Entity` and already contains this component. It will then use
the `modify_component` API and a new `Relationship::set_risky` method to
set the related entity, keeping all other data untouched.
For the `replace_related`(`_with_difference`) methods this also required
a `InsertHookMode` parameter for efficient modifications of multiple
children. The changes here are limited to the non-public methods.
I would appreciate feedback if this is all good.
# Testing
Added tests of all methods that previously could reset `Relationship`
data.
---------
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
# Objective
I've noticed that some methods with `MaybeLocation::caller` don't have
`#[track_caller]` which resulted in wrong locations reported when
`track_location` is enabled.
## Solution
add `#[track_caller]` to them.
Without this dependency, the bevy_ecs tests fail with missing as_string
methods.
# Objective
- Fixes#19734
## Solution
- add bevy_utils with feature = "Debug" to dev-dependencies
## Testing
- Ran `cargo test -p bevy_ecs`
- Ran `taplo fmt --check`
---
# Objective
- Splitted off from #19491
- Make adding generated code to the `Bundle` derive macro easier
- Fix a bug when multiple fields are `#[bundle(ignore)]`
## Solution
- Instead of accumulating the code for each method in a different `Vec`,
accumulate only the names of non-ignored fields and their types, then
use `quote` to generate the code for each of them in the method body.
- To fix the bug, change the code populating the `BundleFieldKind` to
push only one of them per-field (previously each `#[bundle(ignore)]`
resulted in pushing twice, once for the correct
`BundleFieldKind::Ignore` and then again unconditionally for
`BundleFieldKind::Component`)
## Testing
- Added a regression test for the bug that was fixed
