# Objective
This PR aims to improve the soundness of `CommandQueue`. In particular it aims to:
- make it sound to store commands that contain padding or uninitialized bytes;
- avoid uses of commands after moving them in the queue's buffer (`std::mem::forget` is technically a use of its argument);
- remove useless checks: `self.bytes.as_mut_ptr().is_null()` is always `false` because even `Vec`s that haven't allocated use a dangling pointer. Moreover the same pointer was used to write the command, so it ought to be valid for reads if it was for writes.
## Solution
- To soundly store padding or uninitialized bytes `CommandQueue` was changed to contain a `Vec<MaybeUninit<u8>>` instead of `Vec<u8>`;
- To avoid uses of the command through `std::mem::forget`, `ManuallyDrop` was used.
## Other observations
While writing this PR I noticed that `CommandQueue` doesn't seem to drop the commands that weren't applied. While this is a pretty niche case (you would have to be manually using `CommandQueue`/`std::mem::swap`ping one), I wonder if it should be documented anyway.
# Objective
Don't allocate memory for Component types known at compile-time. Save a bit of memory.
## Solution
Change `ComponentDescriptor::name` from `String` to `Cow<'static, str>` to use the `&'static str` returned by `std::any::type_name`.
# Objective
`debug_assert!` macros must still compile properly in release mode due to how they're implemented. This is causing release builds to fail.
## Solution
Change them to `assert!` macros inside `#[cfg(debug_assertions)]` blocks.
# Objective
- Higher order system could not be created by users.
- However, a simple change to `SystemParamFunction` allows this.
- Higher order systems in this case mean functions which return systems created using other systems, such as `chain` (which is basically equivalent to map)
## Solution
- Change `SystemParamFunction` to be a safe abstraction over `FnMut([In<In>,] ...params)->Out`.
- Note that I believe `SystemParamFunction` should not have been counted as part of our public api before this PR.
- This is because its only use was an unsafe function without an actionable safety comment.
- The safety comment was basically 'call this within bevy code'.
- I also believe that there are no external users in its current form.
- A quick search on Google and in the discord confirmed this.
## See also
- https://github.com/bevyengine/bevy/pull/4666, which uses this and subsumes the example here
---
## Changelog
### Added
- `SystemParamFunction`, which can be used to create higher order systems.
# Objective
Use less memory to store SparseSet components.
## Solution
Change `ComponentSparseSet` to only use `Entity::id` in it's key internally, and change the usize value in it's SparseArray to use u32 instead, as it cannot have more than u32::MAX live entities stored at once.
This should reduce the overhead of storing components in sparse set storage by 50%.
# Objective
Fixes#3183. Requiring a `&TaskPool` parameter is sort of meaningless if the only correct one is to use the one provided by `Res<ComputeTaskPool>` all the time.
## Solution
Have `QueryState` save a clone of the `ComputeTaskPool` which is used for all `par_for_each` functions.
~~Adds a small overhead of the internal `Arc` clone as a part of the startup, but the ergonomics win should be well worth this hardly-noticable overhead.~~
Updated the docs to note that it will panic the task pool is not present as a resource.
# Future Work
If https://github.com/bevyengine/rfcs/pull/54 is approved, we can replace these resource lookups with a static function call instead to get the `ComputeTaskPool`.
---
## Changelog
Removed: The `task_pool` parameter of `Query(State)::par_for_each(_mut)`. These calls will use the `World`'s `ComputeTaskPool` resource instead.
## Migration Guide
The `task_pool` parameter for `Query(State)::par_for_each(_mut)` has been removed. Remove these parameters from all calls to these functions.
Before:
```rust
fn parallel_system(
task_pool: Res<ComputeTaskPool>,
query: Query<&MyComponent>,
) {
query.par_for_each(&task_pool, 32, |comp| {
...
});
}
```
After:
```rust
fn parallel_system(query: Query<&MyComponent>) {
query.par_for_each(32, |comp| {
...
});
}
```
If using `Query(State)` outside of a system run by the scheduler, you may need to manually configure and initialize a `ComputeTaskPool` as a resource in the `World`.
# Objective
The `ComponentId` in `Column` is redundant as it's stored in parallel in the surrounding `SparseSet` all the time.
## Solution
Remove it. Add `SparseSet::iter(_mut)` to parallel `HashMap::iter(_mut)` to allow iterating pairs of columns and their IDs.
---
## Changelog
Added: `SparseSet::iter` and `SparseSet::iter_mut`.
# Objective
- Rebase of #3159.
- Fixes https://github.com/bevyengine/bevy/issues/3156
- add #[inline] to single related functions so that they matches with other function defs
## Solution
* added functions to QueryState
* get_single_unchecked_manual
* get_single_unchecked
* get_single
* get_single_mut
* single
* single_mut
* make Query::get_single use QueryState::get_single_unchecked_manual
* added #[inline]
---
## Changelog
### Added
Functions `QueryState::single`, `QueryState::get_single`, `QueryState::single_mut`, `QueryState::get_single_mut`, `QueryState::get_single_unchecked`, `QueryState::get_single_unchecked_manual`.
### Changed
`QuerySingleError` is now in the `state` module.
## Migration Guide
Change `query::QuerySingleError` to `state::QuerySingleError`
Co-authored-by: 2ne1ugly <chattermin@gmail.com>
Co-authored-by: 2ne1ugly <47616772+2ne1ugly@users.noreply.github.com>
# Objective
the code in these fns are always identical so stop having two functions
## Solution
make them the same function
---
## Changelog
change `matches_archetype` and `matches_table` to `fn matches_component_set(&self, &SparseArray<ComponentId, usize>) -> bool` then do extremely boring updating of all `FetchState` impls
## Migration Guide
- move logic of `matches_archetype` and `matches_table` into `matches_component_set` in any manual `FetchState` impls
# Objective
Even if bevy itself does not provide any builtin scripting or modding APIs, it should have the foundations for building them yourself.
For that it should be enough to have APIs that are not tied to the actual rust types with generics, but rather accept `ComponentId`s and `bevy_ptr` ptrs.
## Solution
Add the following APIs to bevy
```rust
fn EntityRef::get_by_id(ComponentId) -> Option<Ptr<'w>>;
fn EntityMut::get_by_id(ComponentId) -> Option<Ptr<'_>>;
fn EntityMut::get_mut_by_id(ComponentId) -> Option<MutUntyped<'_>>;
fn World::get_resource_by_id(ComponentId) -> Option<Ptr<'_>>;
fn World::get_resource_mut_by_id(ComponentId) -> Option<MutUntyped<'_>>;
// Safety: `value` must point to a valid value of the component
unsafe fn World::insert_resource_by_id(ComponentId, value: OwningPtr);
fn ComponentDescriptor::new_with_layout(..) -> Self;
fn World::init_component_with_descriptor(ComponentDescriptor) -> ComponentId;
```
~~This PR would definitely benefit from #3001 (lifetime'd pointers) to make sure that the lifetimes of the pointers are valid and the my-move pointer in `insert_resource_by_id` could be an `OwningPtr`, but that can be adapter later if/when #3001 is merged.~~
### Not in this PR
- inserting components on entities (this is very tied to types with bundles and the `BundleInserter`)
- an untyped version of a query (needs good API design, has a large implementation complexity, can be done in a third-party crate)
Co-authored-by: Jakob Hellermann <hellermann@sipgate.de>
# Objective
Fixes#4657
Example code that wasnt panic'ing before this PR (and so was unsound):
```rust
#[test]
#[should_panic = "error[B0001]"]
fn option_has_no_filter_with() {
fn sys(_1: Query<(Option<&A>, &mut B)>, _2: Query<&mut B, Without<A>>) {}
let mut world = World::default();
run_system(&mut world, sys);
}
#[test]
#[should_panic = "error[B0001]"]
fn any_of_has_no_filter_with() {
fn sys(_1: Query<(AnyOf<(&A, ())>, &mut B)>, _2: Query<&mut B, Without<A>>) {}
let mut world = World::default();
run_system(&mut world, sys);
}
#[test]
#[should_panic = "error[B0001]"]
fn or_has_no_filter_with() {
fn sys(_1: Query<&mut B, Or<(With<A>, With<B>)>>, _2: Query<&mut B, Without<A>>) {}
let mut world = World::default();
run_system(&mut world, sys);
}
```
## Solution
- Only add the intersection of `with`/`without` accesses of all the elements in `Or/AnyOf` to the world query's `FilteredAccess<ComponentId>` instead of the union.
- `Option`'s fix can be thought of the same way since its basically `AnyOf<T, ()>` but its impl is just simpler as `()` has no `with`/`without` accesses
---
## Changelog
- `Or`/`AnyOf`/`Option` will now report more query conflicts in order to fix unsoundness
## Migration Guide
- If you are now getting query conflicts from `Or`/`AnyOf`/`Option` rip to you and ur welcome for it now being caught
# Objective
We have duplicated code between `QueryIter` and `QueryIterationCursor`. Reuse that code.
## Solution
- Reuse `QueryIterationCursor` inside `QueryIter`.
- Slim down `QueryIter` by removing the `&'w World`. It was only being used by the `size_hint` and `ExactSizeIterator` impls, which can use the QueryState and &Archetypes in the type already.
- Benchmark to make sure there is no significant regression.
Relevant benchmark results seem to show that there is no tangible difference between the two. Everything seems to be either identical or within a workable margin of error here.
```
group embed-cursor main
----- ------------ ----
fragmented_iter/base 1.00 387.4±19.70ns ? ?/sec 1.07 413.1±27.95ns ? ?/sec
many_maps_iter 1.00 27.3±0.22ms ? ?/sec 1.00 27.4±0.10ms ? ?/sec
simple_iter/base 1.00 13.8±0.07µs ? ?/sec 1.00 13.7±0.17µs ? ?/sec
simple_iter/sparse 1.00 61.9±0.37µs ? ?/sec 1.00 62.2±0.64µs ? ?/sec
simple_iter/system 1.00 13.7±0.34µs ? ?/sec 1.00 13.7±0.10µs ? ?/sec
sparse_fragmented_iter/base 1.00 11.0±0.54ns ? ?/sec 1.03 11.3±0.48ns ? ?/sec
world_query_iter/50000_entities_sparse 1.08 105.0±2.68µs ? ?/sec 1.00 97.5±2.18µs ? ?/sec
world_query_iter/50000_entities_table 1.00 27.3±0.13µs ? ?/sec 1.00 27.3±0.37µs ? ?/sec
```
# Objective
- We do a lot of function pointer calls in a hot loop (clearing entities in render). This is slow, since calling function pointers cannot be optimised out. We can avoid that in the cases where the function call is a no-op.
- Alternative to https://github.com/bevyengine/bevy/pull/2897
- On my machine, in `many_cubes`, this reduces dropping time from ~150μs to ~80μs.
## Solution
- Make `drop` in `BlobVec` an `Option`, recording whether the given drop impl is required or not.
- Note that this does add branching in some cases - we could consider splitting this into two fields, i.e. unconditionally call the `drop` fn pointer.
- My intuition of how often types stored in `World` should have non-trivial drops makes me think that would be slower, however.
N.B. Even once this lands, we should still test having a 'drop_multiple' variant - for types with a real `Drop` impl, the current implementation is definitely optimal.
# Objective
`bevy_ecs` assumes that `u32 as usize` is a lossless operation and in a few cases relies on this for soundness and correctness. The only platforms that Rust compiles to where this invariant is broken are 16-bit systems.
A very clear example of this behavior is in the SparseSetIndex impl for Entity, where it converts a u32 into a usize to act as an index. If usize is 16-bit, the conversion will overflow and provide the caller with the wrong index. This can easily result in previously unforseen aliased mutable borrows (i.e. Query::get_many_mut).
## Solution
Explicitly fail compilation on 16-bit platforms instead of introducing UB.
Properly supporting 16-bit systems will likely need a workable use case first.
---
## Changelog
Removed: Ability to compile `bevy_ecs` on 16-bit platforms.
## Migration Guide
`bevy_ecs` will now explicitly fail to compile on 16-bit platforms. If this is required, there is currently no alternative. Please file an issue (https://github.com/bevyengine/bevy/issues) to help detail your use case.
# Objective
- It's pretty common to want to check if an EventReader has received one or multiple events while also needing to consume the iterator to "clear" the EventReader.
- The current approach is to do something like `events.iter().count() > 0` or `events.iter().last().is_some()`. It's not immediately obvious that the purpose of that is to consume the events and check if there were any events. My solution doesn't really solve that part, but it encapsulates the pattern.
## Solution
- Add a `.clear()` method that consumes the iterator.
- It takes the EventReader by value to make sure it isn't used again after it has been called.
---
## Migration Guide
Not a breaking change, but if you ever found yourself in a situation where you needed to consume the EventReader and check if there was any events you can now use
```rust
fn system(events: EventReader<MyEvent>) {
if !events.is_empty {
events.clear();
// Process the fact that one or more event was received
}
}
```
Co-authored-by: Charles <IceSentry@users.noreply.github.com>
# Objective
`Query::par_for_each` and it's variants do not show up when profiling using `tracy` or other profilers. Failing to show the impact of changing batch size, the overhead of scheduling tasks, overall thread utilization, etc. other than the effect on the surrounding system.
## Solution
Add a child span that is entered on every spawned task.
Example view of the results in `tracy` using a modified `parallel_query`:
---
## Changelog
Added: `tracing` spans for `Query::par_for_each` and its variants. Spans should now be visible for all
# Objective
- The code in `events.rs` was a bit messy. There was lots of duplication between `EventReader` and `ManualEventReader`, and the state management code is not needed.
## Solution
- Clean it up.
## Future work
Should we remove the type parameter from `ManualEventReader`?
It doesn't have any meaning outside of its source `Events`. But there's no real reason why it needs to have a type parameter - it's just plain data. I didn't remove it yet to keep the type safety in some of the users of it (primarily related to `&mut World` usage)
Required for https://github.com/bevyengine/bevy/pull/4402.
# Objective
- derived `SystemParam` implementations were never `ReadOnlySystemParamFetch`
- We want them to be, e.g. for `EventReader`
## Solution
- If possible, 'forward' the impl of `ReadOnlySystemParamFetch`.
# Objective
- (Eventually) reduce noise in reporting access conflicts between unordered systems.
- `SystemStage` only looks at unfiltered `ComponentId` access, any conflicts reported are potentially `false`.
- the systems could still be accessing disjoint archetypes
- Comparing systems' filtered access sets can maybe avoid that (for statically known component types).
- #4204
## Solution
- Modify `SparseSetIndex` trait to require `PartialEq`, `Eq`, and `Hash` (all internal types except `BundleId` already did).
- Add `is_compatible` and `get_conflicts` methods to `FilteredAccessSet<T>`
- (existing method renamed to `get_conflicts_single`)
- Add docs for those and all the other methods while I'm at it.
## Objective
- ~~Make absurdly long-lived changes stay detectable for even longer (without leveling up to `u64`).~~
- Give all changes a consistent maximum lifespan.
- Improve code clarity.
## Solution
- ~~Increase the frequency of `check_tick` scans to increase the oldest reliably-detectable change.~~
(Deferred until we can benchmark the cost of a scan.)
- Ignore changes older than the maximum reliably-detectable age.
- General refactoring—name the constants, use them everywhere, and update the docs.
- Update test cases to check for the specified behavior.
## Related
This PR addresses (at least partially) the concerns raised in:
- #3071
- #3082 (and associated PR #3084)
## Background
- #1471
Given the minimum interval between `check_ticks` scans, `N`, the oldest reliably-detectable change is `u32::MAX - (2 * N - 1)` (or `MAX_CHANGE_AGE`). Reducing `N` from ~530 million (current value) to something like ~2 million would extend the lifetime of changes by a billion.
| minimum `check_ticks` interval | oldest reliably-detectable change | usable % of `u32::MAX` |
| --- | --- | --- |
| `u32::MAX / 8` (536,870,911) | `(u32::MAX / 4) * 3` | 75.0% |
| `2_000_000` | `u32::MAX - 3_999_999` | 99.9% |
Similarly, changes are still allowed to be between `MAX_CHANGE_AGE`-old and `u32::MAX`-old in the interim between `check_tick` scans. While we prevent their age from overflowing, the test to detect changes still compares raw values. This makes failure ultimately unreliable, since when ancient changes stop being detected varies depending on when the next scan occurs.
## Open Question
Currently, systems and system states are incorrectly initialized with their `last_change_tick` set to `0`, which doesn't handle wraparound correctly.
For consistent behavior, they should either be initialized to the world's `last_change_tick` (and detect no changes) or to `MAX_CHANGE_AGE` behind the world's current `change_tick` (and detect everything as a change). I've currently gone with the latter since that was closer to the existing behavior.
## Follow-up Work
(Edited: entire section)
We haven't actually profiled how long a `check_ticks` scan takes on a "large" `World` , so we don't know if it's safe to increase their frequency. However, we are currently relying on play sessions not lasting long enough to trigger a scan and apps not having enough entities/archetypes for it to be "expensive" (our assumption). That isn't a real solution. (Either scanning never costs enough to impact frame times or we provide an option to use `u64` change ticks. Nobody will accept random hiccups.)
To further extend the lifetime of changes, we actually only need to increment the world tick if a system has `Fetch: !ReadOnlySystemParamFetch`. The behavior will be identical because all writes are sequenced, but I'm not sure how to implement that in a way that the compiler can optimize the branch out.
Also, since having no false positives depends on a `check_ticks` scan running at least every `2 * N - 1` ticks, a `last_check_tick` should also be stored in the `World` so that any lull in system execution (like a command flush) could trigger a scan if needed. To be completely robust, all the systems initialized on the world should be scanned, not just those in the current stage.
# Objective
- Remove `Resource` binding on events, introduce a new `Event` trait
- Ensure event iterators are `ExactSizeIterator`
## Solution
- Builds on #2382 and #2969
## Changelog
- Events<T>, EventWriter<T>, EventReader<T> and so on now require that the underlying type is Event, rather than Resource. Both of these are trivial supertraits of Send + Sync + 'static with universal blanket implementations: this change is currently purely cosmetic.
- Event reader iterators now implement ExactSizeIterator
1. change `PtrMut::as_ptr(self)` and `OwnedPtr::as_ptr(self)` to take `&self`, otherwise printing the pointer will prevent doing anything else afterwards
2. make all `as_ptr` methods safe. There's nothing unsafe about obtaining a pointer, these kinds of methods are safe in std as well [str::as_ptr](https://doc.rust-lang.org/stable/std/primitive.str.html#method.as_ptr), [Rc::as_ptr](https://doc.rust-lang.org/stable/std/rc/struct.Rc.html#method.as_ptr)
3. rename `offset`/`add` to `byte_offset`/`byte_add`. The unprefixed methods in std add in increments of `std::mem::size_of::<T>`, not in bytes. There's a PR for rust to add these byte_ methods https://github.com/rust-lang/rust/pull/95643 and at the call site it makes it much more clear that you need to do `.byte_add(i * layout_size)` instead of `.add(i)`
# Objective
The pointer types introduced in #3001 are useful not just in `bevy_ecs`, but also in crates like `bevy_reflect` (#4475) or even outside of bevy.
## Solution
Extract `Ptr<'a>`, `PtrMut<'a>`, `OwnedPtr<'a>`, `ThinSlicePtr<'a, T>` and `UnsafeCellDeref` from `bevy_ecs::ptr` into `bevy_ptr`.
**Note:** `bevy_ecs` still reexports the `bevy_ptr` as `bevy_ecs::ptr` so that crates like `bevy_transform` can use the `Bundle` derive without needing to depend on `bevy_ptr` themselves.
# Objective
- `RunOnce` was a manual `System` implementation.
- Adding run criteria to stages was yet to be systemyoten
## Solution
- Make it a normal function
- yeet
## Changelog
- Replaced `RunOnce` with `ShouldRun::once`
## Migration guide
The run criterion `RunOnce`, which would make the controlled systems run only once, has been replaced with a new run criterion function `ShouldRun::once`. Replace all instances of `RunOnce` with `ShouldRun::once`.
# Objective
The `Ptr` types gives free access to the underlying `NonNull<u8>`, which adds more publicly visible pointer wrangling than there needs to be. There are also a few edge cases where Ptr types could be more readily utilized for properly validating the soundness of ECS operations.
## Solution
- Replace `*Ptr(Mut)::inner` with `cast` which requires a concrete type to give the pointer. This function could also have a `debug_assert` with an alignment check to ensure that the pointer is aligned properly, but is currently not included.
- Use `OwningPtr::read` in ECS macros over casting the inner pointer around.
# Objective
1. Previously, the `change_tick` and `last_change_tick` fields on `SystemChangeTick` [were `pub`](https://docs.rs/bevy/0.6.1/bevy/ecs/system/struct.SystemChangeTick.html).
1. This was actively misleading, as while this can be fetched as a `SystemParam`, a copy is returned instead
2. This information could be useful for debugging, but there was no way to investigate when data was changed.
3. There were no docs!
## Solution
1. Move these to a getter method.
2. Add `last_changed` method to the `DetectChanges` trait to enable inspection of when data was last changed.
3. Add docs.
# Changelog
`SystemChangeTick` now provides getter methods for the current and previous change tick, rather than public fields.
This can be combined with `DetectChanges::last_changed()` to debug the timing of changes.
# Migration guide
The `change_tick` and `last_change_tick` fields on `SystemChangeTick` are now private, use the corresponding getter method instead.
# Objective
- Manually running systems is a somewhat obscure process: systems must be initialized before they are run
- The unwrap is rather hard to debug.
## Solution
- Replace unwraps in `FunctionSystem` methods with expects (progress towards #3892).
- Briefly document this requirement.
The only tests we had for `derive(WorldQuery)` checked that the derive doesnt panic/emit a `compiler_error!`. This PR adds tests that actually assert the returned values of a query using the derived `WorldQuery` impl. Also adds a compile fail test to check that we correctly error on read only world queries containing mutable world queries.
# Objective
`bevy_ecs` has large amounts of unsafe code which is hard to get right and makes it difficult to audit for soundness.
## Solution
Introduce lifetimed, type-erased pointers: `Ptr<'a>` `PtrMut<'a>` `OwningPtr<'a>'` and `ThinSlicePtr<'a, T>` which are newtypes around a raw pointer with a lifetime and conceptually representing strong invariants about the pointee and validity of the pointer.
The process of converting bevy_ecs to use these has already caught multiple cases of unsound behavior.
## Changelog
TL;DR for release notes: `bevy_ecs` now uses lifetimed, type-erased pointers internally, significantly improving safety and legibility without sacrificing performance. This should have approximately no end user impact, unless you were meddling with the (unfortunately public) internals of `bevy_ecs`.
- `Fetch`, `FilterFetch` and `ReadOnlyFetch` trait no longer have a `'state` lifetime
- this was unneeded
- `ReadOnly/Fetch` associated types on `WorldQuery` are now on a new `WorldQueryGats<'world>` trait
- was required to work around lack of Generic Associated Types (we wish to express `type Fetch<'a>: Fetch<'a>`)
- `derive(WorldQuery)` no longer requires `'w` lifetime on struct
- this was unneeded, and improves the end user experience
- `EntityMut::get_unchecked_mut` returns `&'_ mut T` not `&'w mut T`
- allows easier use of unsafe API with less footguns, and can be worked around via lifetime transmutery as a user
- `Bundle::from_components` now takes a `ctx` parameter to pass to the `FnMut` closure
- required because closure return types can't borrow from captures
- `Fetch::init` takes `&'world World`, `Fetch::set_archetype` takes `&'world Archetype` and `&'world Tables`, `Fetch::set_table` takes `&'world Table`
- allows types implementing `Fetch` to store borrows into world
- `WorldQuery` trait now has a `shrink` fn to shorten the lifetime in `Fetch::<'a>::Item`
- this works around lack of subtyping of assoc types, rust doesnt allow you to turn `<T as Fetch<'static>>::Item'` into `<T as Fetch<'a>>::Item'`
- `QueryCombinationsIter` requires this
- Most types implementing `Fetch` now have a lifetime `'w`
- allows the fetches to store borrows of world data instead of using raw pointers
## Migration guide
- `EntityMut::get_unchecked_mut` returns a more restricted lifetime, there is no general way to migrate this as it depends on your code
- `Bundle::from_components` implementations must pass the `ctx` arg to `func`
- `Bundle::from_components` callers have to use a fn arg instead of closure captures for borrowing from world
- Remove lifetime args on `derive(WorldQuery)` structs as it is nonsensical
- `<Q as WorldQuery>::ReadOnly/Fetch` should be changed to either `RO/QueryFetch<'world>` or `<Q as WorldQueryGats<'world>>::ReadOnly/Fetch`
- `<F as Fetch<'w, 's>>` should be changed to `<F as Fetch<'w>>`
- Change the fn sigs of `Fetch::init/set_archetype/set_table` to match respective trait fn sigs
- Implement the required `fn shrink` on any `WorldQuery` implementations
- Move assoc types `Fetch` and `ReadOnlyFetch` on `WorldQuery` impls to `WorldQueryGats` impls
- Pass an appropriate `'world` lifetime to whatever fetch struct you are for some reason using
### Type inference regression
in some cases rustc may give spurrious errors when attempting to infer the `F` parameter on a query/querystate this can be fixed by manually specifying the type, i.e. `QueryState:🆕:<_, ()>(world)`. The error is rather confusing:
```rust=
error[E0271]: type mismatch resolving `<() as Fetch<'_>>::Item == bool`
--> crates/bevy_pbr/src/render/light.rs:1413:30
|
1413 | main_view_query: QueryState::new(world),
| ^^^^^^^^^^^^^^^ expected `bool`, found `()`
|
= note: required because of the requirements on the impl of `for<'x> FilterFetch<'x>` for `<() as WorldQueryGats<'x>>::Fetch`
note: required by a bound in `bevy_ecs::query::QueryState::<Q, F>::new`
--> crates/bevy_ecs/src/query/state.rs:49:32
|
49 | for<'x> QueryFetch<'x, F>: FilterFetch<'x>,
| ^^^^^^^^^^^^^^^ required by this bound in `bevy_ecs::query::QueryState::<Q, F>::new`
```
---
Made with help from @BoxyUwU and @alice-i-cecile
Co-authored-by: Boxy <supbscripter@gmail.com>
# Objective
Reduce from scratch build time.
## Solution
Reduce the size of the critical path by removing dependencies between crates where not necessary. For `cargo check --no-default-features` this reduced build time from ~51s to ~45s. For some commits I am not completely sure if the tradeoff between build time reduction and convenience caused by the commit is acceptable. If not, I can drop them.
## Objective
This fixes#1686.
`size_hint` can be useful even if a little niche. For example,
`collect::<Vec<_>>()` uses the `size_hint` of Iterator it collects from
to pre-allocate a memory slice large enough to not require re-allocating
when pushing all the elements of the iterator.
## Solution
To this effect I made the following changes:
* Add a `IS_ARCHETYPAL` associated constant to the `Fetch` trait,
this constant tells us when it is safe to assume that the `Fetch`
relies exclusively on archetypes to filter queried entities
* Add `IS_ARCHETYPAL` to all the implementations of `Fetch`
* Use that constant in `QueryIter::size_hint` to provide a more useful
## Migration guide
The new associated constant is an API breaking change. For the user,
if they implemented a custom `Fetch`, it means they have to add this
associated constant to their implementation. Either `true` if it doesn't limit
the number of entities returned in a query beyond that of archetypes, or
`false` for when it does.
# Objective
- `EntityRef` and `EntityMut` are surpisingly important public types when working directly with the `World`.
- They're undocumented.
## Solution
- Just add docs!
# Objective
`AsSystemLabel` has been introduced on system descriptors to make ordering systems more convenient, but `SystemSet::before` and `SystemSet::after` still take `SystemLabels` directly:
use bevy::ecs::system::AsSystemLabel;
/*…*/ SystemSet::new().before(foo.as_system_label()) /*…*/
is currently necessary instead of
/*…*/ SystemSet::new().before(foo) /*…*/
## Solution
Use `AsSystemLabel` for `SystemSet`
The only way to soundly use this API is already encapsulated within `EntityMut::get`, so this api is removed.
# Migration guide
Replace calls to `EntityMut::get_unchecked` with calls to `EntityMut::get`.
# Objective
- The current API docs of `Commands` is very short and is very opaque to newcomers.
## Solution
- Try to explain what it is without requiring knowledge of other parts of `bevy_ecs` like `World` or `SystemParam`.
Co-authored-by: Charles <IceSentry@users.noreply.github.com>
Free at last!
# Objective
- Using `.system()` is no longer needed anywhere, and anyone using it will have already gotten a deprecation warning.
- https://github.com/bevyengine/bevy/pull/3302 was a super special case for `.system()`, since it was so prevelant. However, that's no reason.
- Despite it being deprecated, another couple of uses of it have already landed, including in the deprecating PR.
- These have all been because of doc examples having warnings not breaking CI - 🎟️?
## Solution
- Remove it.
- It's gone
---
## Changelog
- You can no longer use `.system()`
## Migration Guide
- You can no longer use `.system()`. It was deprecated in 0.7.0, and you should have followed the deprecation warning then. You can just remove the method call.
- Thanks to the @TheRawMeatball for producing
# Objective
- Provide more information when despawning an entity
## Solution
- Add a debug log when despawning an entity
- Add spans to the recursive ways of despawning an entity
```sh
RUST_LOG=debug cargo run --example panic --features trace
# RUST_LOG=debug needed to show debug logs from bevy_ecs
# --features trace needed to have the extra spans
...
DEBUG bevy_app:frame:stage{name=Update}:system_commands{name="panic::despawn_parent"}:command{name="DespawnRecursive" entity=0v0}: bevy_ecs::world: Despawning entity 1v0
DEBUG bevy_app:frame:stage{name=Update}:system_commands{name="panic::despawn_parent"}:command{name="DespawnRecursive" entity=0v0}: bevy_ecs::world: Despawning entity 0v0
```
# Objective
- Previously, `iter_combinations()` does not work on queries that have filters.
- Fixes#3651
## Solution
- Derived Copy on all `*Fetch<T>` structs, and manually implemented `Clone` to allow the test to pass (`.count()` does not work on `QueryCombinationIter` when `Clone` is derived)
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
# Objective
- Make it possible to use `System`s outside of the scheduler/executor without having to define logic to track new archetypes and call `System::add_archetype()` for each.
## Solution
- Replace `System::add_archetype(&Archetype)` with `System::update_archetypes(&World)`, making systems responsible for tracking their own most recent archetype generation the way that `SystemState` already does.
This has minimal (or simplifying) effect on most of the code with the exception of `FunctionSystem`, which must now track the latest `ArchetypeGeneration` it saw instead of relying on the executor to do it.
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
Remove the 'chaining' api, as it's peculiar
~~Implement the label traits for `Box<dyn ThatTrait>` (n.b. I'm not confident about this change, but it was the quickest path to not regressing)~~
Remove the need for '`.system`' when using run criteria piping
# Objective
Make `FromWorld` more useful for abstractions with a form similar to
```rs
trait FancyAbstraction {
type PreInitializedData: FromWorld;
}
```
## Solution
Add a `FromWorld` implementation for `SystemState` as well as a way to group together multiple `FromWorld` implementing types as one.
Note: I plan to follow up this PR with another to add `Local` support to exclusive systems, which should get a fair amount of use from the `FromWorld` implementation on `SystemState`.
Fixes#3408#3001 also solves this but I dont see it getting merged any time soon so...
# Objective
make bevy ecs a lil bit less unsound
## Solution
make `EntityMut::get_component_mut` return borrows from self instead of `'w`
# Objective
- std's new APIs do the same thing as `Query::get_multiple_mut`, but are called `get_many`: https://github.com/rust-lang/rust/pull/83608
## Solution
- Find and replace `get_multiple` with `get_many`
# Objective
- Clarify `RemovedComponents` are flushed in `CoreStage::Last` and systems relying on that should run before that stage
## Solution
- Update `RemovedComponents` doc comment
# Objective
make bevy ecs a lil bit less unsound
## Solution
make unsound API unsafe so that there is an unsafe block to blame:
```rust
use bevy_ecs::prelude::*;
#[derive(Debug, Component)]
struct Foo(u8);
fn main() {
let mut world = World::new();
let e1 = world.spawn().id();
let e2 = world.spawn().insert(Foo(2)).id();
world.entities_mut().meta[0] = world.entities_mut().meta[1].clone();
let foo = world.entity(e1).get::<Foo>().unwrap();
// whoo i love having components i dont have
dbg!(foo);
}
```
This is not _strictly_ speaking UB, however:
- `Query::get_multiple` cannot work if this is allowed
- bevy_ecs is a pile of unsafe code whose soundness generally depends on the world being in a "correct" state with "no funny business" so it seems best to disallow this
- it is trivial to get bevy to panic inside of functions with safety invariants that have been violated (the entity location is not valid)
- it seems to violate what the safety invariant on `Entities::flush` is trying to ensure
# Objective
- The inability to have multiple active mutable borrows into a query is a common source of borrow-checker pain for users.
- This is a pointless restriction if and only if we can guarantee that the entities they are accessing are unique.
- This could already by bypassed with get_unchecked, but that is an extremely unsafe API.
- Closes https://github.com/bevyengine/bevy/issues/2042.
## Solution
- Add `get_multiple`, `get_multiple_mut` and their unchecked equivalents (`multiple` and `multiple_mut`) to `Query` and `QueryState`.
- Improve the `QueryEntityError` type to provide more useful error information.
## Changelog
- Added `get_multiple`, `get_multiple_mut` and their unchecked equivalents (`multiple` and `multiple_mut`) to Query and QueryState.
## Migration Guide
- The `QueryEntityError` enum now has a `AliasedMutability variant, and returns the offending entity.
## Context
This is a fresh attempt at #3333; rebasing was behaving very badly and it was important to rebase on top of the recent query soundness fixes. Many thanks to all the reviewers in that thread, especially @BoxyUwU for the help with lifetimes.
## To-do
- [x] Add compile fail tests
- [x] Successfully deduplicate code
- [x] Decide what to do about failing doc tests
- [x] Get some reviews for lifetime soundness
# Objective
Add a system parameter `ParamSet` to be used as container for conflicting parameters.
## Solution
Added two methods to the SystemParamState trait, which gives the access used by the parameter. Did the implementation. Added some convenience methods to FilteredAccessSet. Changed `get_conflicts` to return every conflicting component instead of breaking on the first conflicting `FilteredAccess`.
Co-authored-by: bilsen <40690317+bilsen@users.noreply.github.com>
# Objective
Fixes#1529
Run bevy_ecs in miri
## Solution
- Don't set thread names when running in miri rust-lang/miri/issues/1717
- Update `event-listener` to `2.5.2` as previous versions have UB that is detected by miri: [event-listener commit](1fa31c553e)
- Ignore memory leaks when running in miri as they are impossible to track down rust-lang/miri/issues/1481
- Make `table_add_remove_many` test less "many" because miri is really quite slow :)
- Make CI run `RUSTFLAGS="-Zrandomize-layout" MIRIFLAGS="-Zmiri-ignore-leaks -Zmiri-tag-raw-pointers -Zmiri-disable-isolation" cargo +nightly miri test -p bevy_ecs`
This adds the concept of "default labels" for systems (currently scoped to "parallel systems", but this could just as easily be implemented for "exclusive systems"). Function systems now include their function's `SystemTypeIdLabel` by default.
This enables the following patterns:
```rust
// ordering two systems without manually defining labels
app
.add_system(update_velocity)
.add_system(movement.after(update_velocity))
// ordering sets of systems without manually defining labels
app
.add_system(foo)
.add_system_set(
SystemSet::new()
.after(foo)
.with_system(bar)
.with_system(baz)
)
```
Fixes: #4219
Related to: #4220
Credit to @aevyrie @alice-i-cecile @DJMcNab (and probably others) for proposing (and supporting) this idea about a year ago. I was a big dummy that both shut down this (very good) idea and then forgot I did that. Sorry. You all were right!
# Objective
make bevy ecs a lil bit less unsound
## Solution
yeet unsound API `World::components_mut`:
```rust
use bevy_ecs::prelude::*;
#[derive(Component)]
struct Foo(u8);
#[derive(Debug, Component)]
struct Bar([u8; 100]);
fn main() {
let mut world = World::new();
let e = world.spawn().insert(Foo(0)).id();
*world.components_mut() = Default::default();
let bar = world.entity_mut(e).remove::<Bar>().unwrap();
// oopsies reading memory copied from outside allocation
dbg!(bar);
}
```
Tracing added support for "inline span entering", which cuts down on a lot of complexity:
```rust
let span = info_span!("my_span").entered();
```
This adapts our code to use this pattern where possible, and updates our docs to recommend it.
This produces equivalent tracing behavior. Here is a side by side profile of "before" and "after" these changes.
# Objective
- Fixes#3300
- `RunSystem` is messy
## Solution
- Adds the trick theorised in https://github.com/bevyengine/bevy/issues/3300#issuecomment-991791234
P.S. I also want this for an experimental refactoring of `Assets`, to remove the duplication of `Events<AssetEvent<T>>`
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
# Objective
- In the large majority of cases, users were calling `.unwrap()` immediately after `.get_resource`.
- Attempting to add more helpful error messages here resulted in endless manual boilerplate (see #3899 and the linked PRs).
## Solution
- Add an infallible variant named `.resource` and so on.
- Use these infallible variants over `.get_resource().unwrap()` across the code base.
## Notes
I did not provide equivalent methods on `WorldCell`, in favor of removing it entirely in #3939.
## Migration Guide
Infallible variants of `.get_resource` have been added that implicitly panic, rather than needing to be unwrapped.
Replace `world.get_resource::<Foo>().unwrap()` with `world.resource::<Foo>()`.
## Impact
- `.unwrap` search results before: 1084
- `.unwrap` search results after: 942
- internal `unwrap_or_else` calls added: 4
- trivial unwrap calls removed from tests and code: 146
- uses of the new `try_get_resource` API: 11
- percentage of the time the unwrapping API was used internally: 93%
# Objective
Continuation of #2663 due to git problems - better documentation for Query::par_for_each and par_for_each_mut
## Solution
Going into more detail about the function parameters
# Objective
- Fix the ugliness of the `config` api.
- Supercedes #2440, #2463, #2491
## Solution
- Since #2398, capturing closure systems have worked.
- Use those instead where we needed config before
- Remove the rest of the config api.
- Related: #2777
# Objective
- Closes#786
- Closes#2252
- Closes#2588
This PR implements a derive macro that allows users to define their queries as structs with named fields.
## Example
```rust
#[derive(WorldQuery)]
#[world_query(derive(Debug))]
struct NumQuery<'w, T: Component, P: Component> {
entity: Entity,
u: UNumQuery<'w>,
generic: GenericQuery<'w, T, P>,
}
#[derive(WorldQuery)]
#[world_query(derive(Debug))]
struct UNumQuery<'w> {
u_16: &'w u16,
u_32_opt: Option<&'w u32>,
}
#[derive(WorldQuery)]
#[world_query(derive(Debug))]
struct GenericQuery<'w, T: Component, P: Component> {
generic: (&'w T, &'w P),
}
#[derive(WorldQuery)]
#[world_query(filter)]
struct NumQueryFilter<T: Component, P: Component> {
_u_16: With<u16>,
_u_32: With<u32>,
_or: Or<(With<i16>, Changed<u16>, Added<u32>)>,
_generic_tuple: (With<T>, With<P>),
_without: Without<Option<u16>>,
_tp: PhantomData<(T, P)>,
}
fn print_nums_readonly(query: Query<NumQuery<u64, i64>, NumQueryFilter<u64, i64>>) {
for num in query.iter() {
println!("{:#?}", num);
}
}
#[derive(WorldQuery)]
#[world_query(mutable, derive(Debug))]
struct MutNumQuery<'w, T: Component, P: Component> {
i_16: &'w mut i16,
i_32_opt: Option<&'w mut i32>,
}
fn print_nums(mut query: Query<MutNumQuery, NumQueryFilter<u64, i64>>) {
for num in query.iter_mut() {
println!("{:#?}", num);
}
}
```
## TODOs:
- [x] Add support for `&T` and `&mut T`
- [x] Test
- [x] Add support for optional types
- [x] Test
- [x] Add support for `Entity`
- [x] Test
- [x] Add support for nested `WorldQuery`
- [x] Test
- [x] Add support for tuples
- [x] Test
- [x] Add support for generics
- [x] Test
- [x] Add support for query filters
- [x] Test
- [x] Add support for `PhantomData`
- [x] Test
- [x] Refactor `read_world_query_field_type_info`
- [x] Properly document `readonly` attribute for nested queries and the static assertions that guarantee safety
- [x] Test that we never implement `ReadOnlyFetch` for types that need mutable access
- [x] Test that we insert static assertions for nested `WorldQuery` that a user marked as readonly
For some keys, it is too expensive to hash them on every lookup. Historically in Bevy, we have regrettably done the "wrong" thing in these cases (pre-computing hashes, then re-hashing them) because Rust's built in hashed collections don't give us the tools we need to do otherwise. Doing this is "wrong" because two different values can result in the same hash. Hashed collections generally get around this by falling back to equality checks on hash collisions. You can't do that if the key _is_ the hash. Additionally, re-hashing a hash increase the odds of collision!
#3959 needs pre-hashing to be viable, so I decided to finally properly solve the problem. The solution involves two different changes:
1. A new generalized "pre-hashing" solution in bevy_utils: `Hashed<T>` types, which store a value alongside a pre-computed hash. And `PreHashMap<K, V>` (which uses `Hashed<T>` internally) . `PreHashMap` is just an alias for a normal HashMap that uses `Hashed<T>` as the key and a new `PassHash` implementation as the Hasher.
2. Replacing the `std::collections` re-exports in `bevy_utils` with equivalent `hashbrown` impls. Avoiding re-hashes requires the `raw_entry_mut` api, which isn't stabilized yet (and may never be ... `entry_ref` has favor now, but also isn't available yet). If std's HashMap ever provides the tools we need, we can move back to that. The latest version of `hashbrown` adds support for the `entity_ref` api, so we can move to that in preparation for an std migration, if thats the direction they seem to be going in. Note that adding hashbrown doesn't increase our dependency count because it was already in our tree.
In addition to providing these core tools, I also ported the "table identity hashing" in `bevy_ecs` to `raw_entry_mut`, which was a particularly egregious case.
The biggest outstanding case is `AssetPathId`, which stores a pre-hash. We need AssetPathId to be cheaply clone-able (and ideally Copy), but `Hashed<AssetPath>` requires ownership of the AssetPath, which makes cloning ids way more expensive. We could consider doing `Hashed<Arc<AssetPath>>`, but cloning an arc is still a non-trivial expensive that needs to be considered. I would like to handle this in a separate PR. And given that we will be re-evaluating the Bevy Assets implementation in the very near future, I'd prefer to hold off until after that conversation is concluded.
# Objective
- `SystemStates` rock for dealing with exclusive world access, but are hard to figure out how to use.
- Fixes#3341.
## Solution
- Clearly document how to use `SystemState`, and why they're useful as an end-user.
What is says on the tin.
This has got more to do with making `clippy` slightly more *quiet* than it does with changing anything that might greatly impact readability or performance.
that said, deriving `Default` for a couple of structs is a nice easy win
# Objective
- Fixes#3078
- Fixes#1397
## Solution
- Implement Commands::init_resource.
- Also implement for World, for consistency and to simplify internal structure.
- While we're here, clean up some of the docs for Command and World resource modification.
# Objective
- `serde_json` assumes that numbers being deserialized are either u64 or i64.
- `Entity` serializes and deserializes as a u32.
- Deserializing an `Entity` with `serde_json` fails with: `Error("invalid type: integer 10947, expected expected Entity"`
## Solution
- Implemented a visitor for u64 that allows an `Entity` to be deserialized in this case.
- While I was here, also fixed the redundant "expected expected Entity" in the error message
- Tested the change in a local project which now correctly deserializes `Entity` structs with `serde_json` when it couldn't before
Implements a new Queryable called AnyOf, which will return an item as long as at least one of it's requested Queryables returns something. For example, a `Query<AnyOf<(&A, &B, &C)>>` will return items with type `(Option<&A>, Option<&B>, Option<&C>)`, and will guarantee that for every element at least one of the option s is Some. This is a shorthand for queries like `Query<(Option<&A>, Option<&B>, Option<&C>), Or<(With<A>, With<B>, With&C>)>>`.
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
# Objective
- Provide impls for mutable types to relevant immutable types.
- Closes#2005
## Solution
- impl From<ResMut> for Res
- impl From<NonSendMut> for NonSend
- Mut to &/&mut already impl'd in change_detection_impl! macro
# Objective
It would be useful to be able to restart a state (such as if an operation fails and needs to be retried from `on_enter`). Currently, it seems the way to restart a state is to transition to a dummy state and then transition back.
## Solution
The solution is to add a `restart` method on `State<T>` that allows for transitioning to the already-active state.
## Context
Based on [this](https://discord.com/channels/691052431525675048/742884593551802431/920335041756815441) question from the Discord.
Closes#2385
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
# Objective
When using empty events, it can feel redundant to have to specify the type of the event when sending it.
## Solution
Add a new `fire()` function that sends the default value of the event. This requires that the event derives Default.
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
# Objective
Make it possible to use `&World` as a system parameter
## Solution
It seems like all the pieces were already in place, very simple impl
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
# Objective
Provide a non-consuming method of checking if there are events in an `EventReader`.
Fixes#2967
## Solution
Implements the `len` and `is_empty` functions for `EventReader` and `ManualEventReader`, giving users the ability to check for the presence of new events without consuming any.
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
# Objective
Calling forget would invalidate the data pointer before it is used.
## Solution
Use `ManuallyDrop` to prevent the value from being dropped without moving it.
This is my first contribution to this exciting project! Thanks so much for your wonderful work. If there is anything that I can improve about this PR, please let me know :)
# Objective
- Fixes#2899
- If a simple one-off command is needed to be added within a System, this simplifies that process so that we can simply do `commands.add(|world: &mut World| { /* code here */ })` instead of defining a custom type implementing `Command`.
## Solution
- This is achieved by `impl Command for F where F: FnOnce(&mut World) + Send + Sync + 'static` as just calling the function.
I am not sure if the bounds can be further relaxed but needed the whole `Send`, `Sync`, and `'static` to get it to compile.
# Objective
A user on Discord couldn't derive SystemParam for this Struct:
```rs
#[derive(SystemParam)]
pub struct SpatialQuery<'w, 's, Q: WorldQuery + Send + Sync + 'static, F: WorldQuery + Send + Sync + 'static = ()>
where
F::Fetch: FilterFetch,
{
query: Query<'w, 's, (C, &'static Transform), F>,
}
```
## Solution
1. The `where`-clause is now also copied to the `SystemParamFetch` impl Block.
2. The `SystemParamState` impl Block no longer gets any defaults for generics
Co-authored-by: MinerSebas <66798382+MinerSebas@users.noreply.github.com>
# Objective
Fixes#3566
## Solution
- [x] Fix broken links in private docs.
- [x] Add the `--document-private-items` flag to the CI.
## Note
The following was said by @killercup in #3566:
> I don't have time to confirm this but I assume that linking to private items throws an error/warning when just running cargo doc, and --document-private-item might actually hide that warning. So to test this, you'd have to run it twice.
I tested this and this is thankfully not the case. If you are linking to a private item you will get a warning no matter if you run `cargo doc` or `cargo doc --document-private-items`.
### Example
I added `struct Test;` to `bevy_core/src/name.rs` and linked to it inside of a doc comment using ``[`Test`]``. After that I ran `cargo doc -p bevy_core --document-private-items` using `RUSTDOCFLAGS="-D warnings"` and got the following output (note the last sentence):
```rust
error: public documentation for `Name` links to private item `Test`
--> crates/bevy_core/src/name.rs:11:82
|
11 | /// Component used to identify an entity. Stores a hash for faster comparisons [`Test`]
| ^^^^ this item is private
|
= note: `-D rustdoc::private-intra-doc-links` implied by `-D warnings`
= note: this link resolves only because you passed `--document-private-items`, but will break without
```
# Objective
Currently, simply calling `iter` on an event reader will mark all of it's events as read, even if the returned iterator is never used
## Solution
With this, the cursor will simply move to the last unread, but available event when iter is called, and incremented by one per `next` call.
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
# Objective
- Calling .id() has no purpose unless you use the Entity returned
- This is an easy source of confusion for beginners.
- This is easily missed during refactors.
## Solution
- Mark the appropriate methods as #[must_use]
# Objective
- Fixes#3616
## Solution
- As described in the issue, documentation for `iter_manual` was copied from `iter_combinations` and did not reflect the behavior of the method. I've pulled some information from #2351 to create a more accurate description.
# Objective
- Removes warning about accidently inserting bundles with `EntityCommands::insert`, but since a component now needs to implement `Component` it is unnecessary.
# Objective
This PR extends the `Events` documentation by:
- informing user about the possible race condition
- explicitly explaining the unusual double buffer implementation
Fixes#3305
Co-authored-by: MiniaczQ <jakub.motyka.2000@gmail.com>
Co-authored-by: MiniaczQ <MiniaczQ@gmail.com>
#3457 adds the `doc_markdown` clippy lint, which checks doc comments to make sure code identifiers are escaped with backticks. This causes a lot of lint errors, so this is one of a number of PR's that will fix those lint errors one crate at a time.
This PR fixes lints in the `bevy_ecs` crate.
# Objective
- Fixes#1920.
- Users often want to know how to get the values of removed components (#1655).
- Stand-alone `bevy_ecs` behavior is very unintuitive, as `World::clear_trackers()` must be manually called.
- Fixes#2999 by extending the existing test (thanks @hymm for pointing me to it) to be clearer and check for component removal as well.
## Solution
- Better docs!
- Better tests!
# Objective
Remove the `StorageType` parameter from `ComponentDescriptor::new_resource` as discussed in #3361.
- fixes#3361
## Solution
- Parameter removed.
- Basic docs added.
## Note
Left a [comment](https://github.com/bevyengine/bevy/issues/3361#issuecomment-996433346) about `SparseStorage` being the more reasonable choice.
Co-authored-by: r4gus <david@thesugar.de>
Dynamic types (`DynamicStruct`, `DynamicTupleStruct`, `DynamicTuple`, `DynamicList` and `DynamicMap`) are used when deserializing scenes, but currently they can only be applied to existing concrete types. This leads to issues when trying to spawn non trivial deserialized scene.
For components, the issue is avoided by requiring that reflected components implement ~~`FromResources`~~ `FromWorld` (or `Default`). When spawning, a new concrete type is created that way, and the dynamic type is applied to it. Unfortunately, some components don't have any valid implementation of these traits.
In addition, any `Vec` or `HashMap` inside a component will panic when a dynamic type is pushed into it (for instance, `Text` panics when adding a text section).
To solve this issue, this PR adds the `FromReflect` trait that creates a concrete type from a dynamic type that represent it, derives the trait alongside the `Reflect` trait, drops the ~~`FromResources`~~ `FromWorld` requirement on reflected components, ~~and enables reflection for UI and Text bundles~~. It also adds the requirement that fields ignored with `#[reflect(ignore)]` implement `Default`, since we need to initialize them somehow.
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
# Objective
- While reading code, found some queries that are `mut` and not used as such
## Solution
- Remove `mut` when possible
Co-authored-by: François <8672791+mockersf@users.noreply.github.com>
# Objective
- Storages are used to store the ECS data.
- They're undocumented.
## Solution
- Add some very basic docs.
## Notes
- Some of this was hard to immediately understand when reading the code, so suggestions on improvements / things to add are particularly welcome.
Fixes#2566Fixes#3005
There are only READMEs in the 4 crates here (with the exception of bevy itself).
Those 4 crates are ecs, reflect, tasks, and transform.
These should each now include their respective README files.
Co-authored-by: Hoidigan <57080125+Hoidigan@users.noreply.github.com>
Co-authored-by: Daniel Nelsen <57080125+Hoidigan@users.noreply.github.com>
# Objective
I thought I'd have a go a trying to fix#2597.
Hopefully fixes#2597.
## Solution
I reused the memory pointed to by the value parameter, that is already required by `insert` to not be dropped, to contain the extracted value while dropping it.
Fills in some gaps we had in our Bevy ECS tracing spans:
* Exclusive systems
* System Commands (for `apply_buffers = true` cases)
* System archetype updates
* Parallel system execution prep
# Objective
- New clippy lints with rust 1.57 are failing
## Solution
- Fixed clippy lints following suggestions
- I ignored clippy in old renderer because there was many and it will be removed soon
A sample implementation of how to have `iter()` work on mutable queries without breaking aliasing rules.
# Objective
- Fixes#753
## Solution
- Added a ReadOnlyFetch to WorldQuery that is the `&T` version of `&mut T` that is used to specify the return type for read only operations like `iter()`.
- ~~As the comment suggests specifying the bound doesn't work due to restrictions on defining recursive implementations (like `Or`). However bounds on the functions are fine~~ Never mind I misread how `Or` was constructed, bounds now exist.
- Note that the only mutable one has a new `Fetch` for readonly as the `State` has to be the same for any of this to work
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
This is a squash-and-rebase of @Ku95's documentation of the new renderer onto the latest `pipelined-rendering` branch.
Original PR is #2884.
Co-authored-by: dataphract <dataphract@gmail.com>
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
# Objective
bevy_ecs has several compile_fail tests that assert lifetime safety. In the past, these tests have been green for the wrong reasons (see e.g. #2984). This PR makes sure, that they will fail if the compiler error changes.
## Solution
Use [trybuild](https://crates.io/crates/trybuild) to assert the compiler errors.
The UI tests are in a separate crate that is not part of the Bevy workspace. This is to ensure that they do not break Bevy's crater builds. The tests get executed by the CI workflow on the stable toolchain.
# Objective
- `bevy_ecs` exposes as an optional feature `bevy_reflect`. Disabling it doesn't compile.
- `bevy_asset` exposes as an optional feature `filesystem_watcher`. Disabling it doesn't compile. It is also not possible to disable this feature from Bevy
## Solution
- Fix compilation errors when disabling the default features. Make it possible to disable the feature `filesystem_watcher` from Bevy
# Objective
- Improve error descriptions and help understand how to fix them
- I noticed one today that could be expanded, it seemed like a good starting point
## Solution
- Start something like https://github.com/rust-lang/rust/tree/master/compiler/rustc_error_codes/src/error_codes
- Remove sentence about Rust mutability rules which is not very helpful in the error message
I decided to start the error code with B for Bevy so that they're not confused with error code from rust (which starts with E)
Longer term, there are a few more evolutions that can continue this:
- the code samples should be compiled check, and even executed for some of them to check they have the correct error code in a panic
- the error could be build on a page in the website like https://doc.rust-lang.org/error-index.html
- most panic should have their own error code
# Objective
- Bevy has several `compile_fail` test
- #2254 added `#[derive(Component)]`
- Those tests now fail for a different reason.
- This was not caught as these test still "successfully" failed to compile.
## Solution
- Add `#[derive(Component)]` to the doctest
- Also changed their cfg attribute from `doc` to `doctest`, so that these tests don't appear when running `cargo doc` with `--document-private-items`
#2605 changed the lifetime annotations on `get_component` introducing unsoundness as you could keep the returned borrow even after using the query.
Example unsoundness:
```rust
use bevy::prelude::*;
fn main() {
App::new()
.add_startup_system(startup)
.add_system(unsound)
.run();
}
#[derive(Debug, Component, PartialEq, Eq)]
struct Foo(Vec<u32>);
fn startup(mut c: Commands) {
let e = c.spawn().insert(Foo(vec![10])).id();
c.insert_resource(e);
}
fn unsound(mut q: Query<&mut Foo>, res: Res<Entity>) {
let foo = q.get_component::<Foo>(*res).unwrap();
let mut foo2 = q.iter_mut().next().unwrap();
let first_elem = &foo.0[0];
for _ in 0..16 {
foo2.0.push(12);
}
dbg!(*first_elem);
}
```
output:
`[src/main.rs:26] *first_elem = 0`
Add the entity ID and generation to the expect() message of two
world accessors, to make it easier to debug use-after-free issues.
Coupled with e.g. bevy-inspector-egui which also displays the entity ID,
this makes it much easier to identify what entity is being misused.
# Objective
Make it easier to identity an entity being accessed after being deleted.
## Solution
Augment the error message of some `expect()` call with the entity ID and
generation. Combined with some external tool like `bevy-inspector-egui`, which
also displays the entity ID, this increases the chances to be able to identify
the entity, and therefore find the error that led to a use-after-despawn.
# Objective
- Fixes#2904 (see for context)
## Solution
- Simply hoist span creation out of the threaded task
- Confirmed to solve the issue locally
Now all events have the full span parent tree up through `bevy_ecs::schedule::stage` all the way to `bevy_app::app::bevy_app` (and its parents in bevy-consumer code, if any).
# Objective
- Avoid usages of `format!` that ~immediately get passed to another `format!`. This avoids a temporary allocation and is just generally cleaner.
## Solution
- `bevy_derive::shader_defs` does a `format!("{}", val.to_string())`, which is better written as just `format!("{}", val)`
- `bevy_diagnostic::log_diagnostics_plugin` does a `format!("{:>}", format!(...))`, which is better written as `format!("{:>}", format_args!(...))`
- `bevy_ecs::schedule` does `tracing::info!(..., name = &*format!("{:?}", val))`, which is better written with the tracing shorthand `tracing::info!(..., name = ?val)`
- `bevy_reflect::reflect` does `f.write_str(&format!(...))`, which is better written as `write!(f, ...)` (this could also be written using `f.debug_tuple`, but I opted to maintain alt debug behavior)
- `bevy_reflect::serde::{ser, de}` do `serde::Error::custom(format!(...))`, which is better written as `Error::custom(format_args!(...))`, as `Error::custom` takes `impl Display` and just immediately calls `format!` again
This implements the most minimal variant of #1843 - a derive for marker trait. This is a prerequisite to more complicated features like statically defined storage type or opt-out component reflection.
In order to make component struct's purpose explicit and avoid misuse, it must be annotated with `#[derive(Component)]` (manual impl is discouraged for compatibility). Right now this is just a marker trait, but in the future it might be expanded. Making this change early allows us to make further changes later without breaking backward compatibility for derive macro users.
This already prevents a lot of issues, like using bundles in `insert` calls. Primitive types are no longer valid components as well. This can be easily worked around by adding newtype wrappers and deriving `Component` for them.
One funny example of prevented bad code (from our own tests) is when an newtype struct or enum variant is used. Previously, it was possible to write `insert(Newtype)` instead of `insert(Newtype(value))`. That code compiled, because function pointers (in this case newtype struct constructor) implement `Send + Sync + 'static`, so we allowed them to be used as components. This is no longer the case and such invalid code will trigger a compile error.
Co-authored-by: = <=>
Co-authored-by: TheRawMeatball <therawmeatball@gmail.com>
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
# Objective
Fixes these issues:
- `WorldId`s currently aren't necessarily unique
- I want to guarantee that they're unique to safeguard my librarified version of https://github.com/bevyengine/bevy/discussions/2805
- There probably hasn't been a collision yet, but they could technically collide
- `SystemId` isn't used for anything
- It's no longer used now that `Locals` are stored within the `System`.
- `bevy_ecs` depends on rand
## Solution
- Instead of randomly generating `WorldId`s, just use an incrementing atomic counter, panicing on overflow.
- Remove `SystemId`
- We do need to allow Locals for exclusive systems at some point, but exclusive systems couldn't access their own `SystemId` anyway.
- Now that these don't depend on rand, move it to a dev-dependency
## Todo
Determine if `WorldId` should be `u32` based instead
Changed out unwraps to use if let syntax instead. Returning false when None.
Also modified an existing test to encompass these methods
This PR fixes#2828
This changes how render logic is composed to make it much more modular. Previously, all extraction logic was centralized for a given "type" of rendered thing. For example, we extracted meshes into a vector of ExtractedMesh, which contained the mesh and material asset handles, the transform, etc. We looked up bindings for "drawn things" using their index in the `Vec<ExtractedMesh>`. This worked fine for built in rendering, but made it hard to reuse logic for "custom" rendering. It also prevented us from reusing things like "extracted transforms" across contexts.
To make rendering more modular, I made a number of changes:
* Entities now drive rendering:
* We extract "render components" from "app components" and store them _on_ entities. No more centralized uber lists! We now have true "ECS-driven rendering"
* To make this perform well, I implemented #2673 in upstream Bevy for fast batch insertions into specific entities. This was merged into the `pipelined-rendering` branch here: #2815
* Reworked the `Draw` abstraction:
* Generic `PhaseItems`: each draw phase can define its own type of "rendered thing", which can define its own "sort key"
* Ported the 2d, 3d, and shadow phases to the new PhaseItem impl (currently Transparent2d, Transparent3d, and Shadow PhaseItems)
* `Draw` trait and and `DrawFunctions` are now generic on PhaseItem
* Modular / Ergonomic `DrawFunctions` via `RenderCommands`
* RenderCommand is a trait that runs an ECS query and produces one or more RenderPass calls. Types implementing this trait can be composed to create a final DrawFunction. For example the DrawPbr DrawFunction is created from the following DrawCommand tuple. Const generics are used to set specific bind group locations:
```rust
pub type DrawPbr = (
SetPbrPipeline,
SetMeshViewBindGroup<0>,
SetStandardMaterialBindGroup<1>,
SetTransformBindGroup<2>,
DrawMesh,
);
```
* The new `custom_shader_pipelined` example illustrates how the commands above can be reused to create a custom draw function:
```rust
type DrawCustom = (
SetCustomMaterialPipeline,
SetMeshViewBindGroup<0>,
SetTransformBindGroup<2>,
DrawMesh,
);
```
* ExtractComponentPlugin and UniformComponentPlugin:
* Simple, standardized ways to easily extract individual components and write them to GPU buffers
* Ported PBR and Sprite rendering to the new primitives above.
* Removed staging buffer from UniformVec in favor of direct Queue usage
* Makes UniformVec much easier to use and more ergonomic. Completely removes the need for custom render graph nodes in these contexts (see the PbrNode and view Node removals and the much simpler call patterns in the relevant Prepare systems).
* Added a many_cubes_pipelined example to benchmark baseline 3d rendering performance and ensure there were no major regressions during this port. Avoiding regressions was challenging given that the old approach of extracting into centralized vectors is basically the "optimal" approach. However thanks to a various ECS optimizations and render logic rephrasing, we pretty much break even on this benchmark!
* Lifetimeless SystemParams: this will be a bit divisive, but as we continue to embrace "trait driven systems" (ex: ExtractComponentPlugin, UniformComponentPlugin, DrawCommand), the ergonomics of `(Query<'static, 'static, (&'static A, &'static B, &'static)>, Res<'static, C>)` were getting very hard to bear. As a compromise, I added "static type aliases" for the relevant SystemParams. The previous example can now be expressed like this: `(SQuery<(Read<A>, Read<B>)>, SRes<C>)`. If anyone has better ideas / conflicting opinions, please let me know!
* RunSystem trait: a way to define Systems via a trait with a SystemParam associated type. This is used to implement the various plugins mentioned above. I also added SystemParamItem and QueryItem type aliases to make "trait stye" ecs interactions nicer on the eyes (and fingers).
* RenderAsset retrying: ensures that render assets are only created when they are "ready" and allows us to create bind groups directly inside render assets (which significantly simplified the StandardMaterial code). I think ultimately we should swap this out on "asset dependency" events to wait for dependencies to load, but this will require significant asset system changes.
* Updated some built in shaders to account for missing MeshUniform fields
## Objective
The upcoming Bevy Book makes many references to the API documentation of bevy.
Most references belong to the first two chapters of the Bevy Book:
- bevyengine/bevy-website#176
- bevyengine/bevy-website#182
This PR attempts to improve the documentation of `bevy_ecs` and `bevy_app` in order to help readers of the Book who want to delve deeper into technical details.
## Solution
- Add crate and level module documentation
- Document the most important items (basically those included in the preludes), with the following style, where applicable:
- **Summary.** Short description of the item.
- **Second paragraph.** Detailed description of the item, without going too much in the implementation.
- **Code example(s).**
- **Safety or panic notes.**
## Collaboration
Any kind of collaboration is welcome, especially corrections, wording, new ideas and guidelines on where the focus should be put in.
---
### Related issues
- Fixes#2246
This updates the `pipelined-rendering` branch to use the latest `bevy_ecs` from `main`. This accomplishes a couple of goals:
1. prepares for upcoming `custom-shaders` branch changes, which were what drove many of the recent bevy_ecs changes on `main`
2. prepares for the soon-to-happen merge of `pipelined-rendering` into `main`. By including bevy_ecs changes now, we make that merge simpler / easier to review.
I split this up into 3 commits:
1. **add upstream bevy_ecs**: please don't bother reviewing this content. it has already received thorough review on `main` and is a literal copy/paste of the relevant folders (the old folders were deleted so the directories are literally exactly the same as `main`).
2. **support manual buffer application in stages**: this is used to enable the Extract step. we've already reviewed this once on the `pipelined-rendering` branch, but its worth looking at one more time in the new context of (1).
3. **support manual archetype updates in QueryState**: same situation as (2).
# Objective
The vast majority of `.single()` usage I've seen is immediately followed by a `.unwrap()`. Since it seems most people use it without handling the error, I think making it easier to just get what you want fast while also having a more verbose alternative when you want to handle the error could help.
## Solution
Instead of having a lot of `.unwrap()` everywhere, this PR introduces a `try_single()` variant that behaves like the current `.single()` and make the new `.single()` panic on error.
# Objective
Sometimes, the unwraps in `entity_mut` could fail here, if the entity was despawned *before* this command was applied.
The simplest case involves two command buffers:
```rust
use bevy::prelude::*;
fn b(mut commands1: Commands, mut commands2: Commands) {
let id = commands2.spawn().insert_bundle(()).id();
commands1.entity(id).despawn();
}
fn main() {
App::build().add_system(b.system()).run();
}
```
However, a more complicated version arises in the case of ambiguity:
```rust
use std::time::Duration;
use bevy::{app::ScheduleRunnerPlugin, prelude::*};
use rand::Rng;
fn cleanup(mut e: ResMut<Option<Entity>>) {
*e = None;
}
fn sleep_randomly() {
let mut rng = rand::thread_rng();
std:🧵:sleep(Duration::from_millis(rng.gen_range(0..50)));
}
fn spawn(mut commands: Commands, mut e: ResMut<Option<Entity>>) {
*e = Some(commands.spawn().insert_bundle(()).id());
}
fn despawn(mut commands: Commands, e: Res<Option<Entity>>) {
let mut rng = rand::thread_rng();
std:🧵:sleep(Duration::from_millis(rng.gen_range(0..50)));
if let Some(e) = *e {
commands.entity(e).despawn();
}
}
fn main() {
App::build()
.add_system(cleanup.system().label("cleanup"))
.add_system(sleep_randomly.system().label("before_despawn"))
.add_system(despawn.system().after("cleanup").after("before_despawn"))
.add_system(sleep_randomly.system().label("before_spawn"))
.add_system(spawn.system().after("cleanup").after("before_spawn"))
.insert_resource(None::<Entity>)
.add_plugin(ScheduleRunnerPlugin::default())
.run();
}
```
In the cases where this example crashes, it's because `despawn` was ordered before `spawn` in the topological ordering of systems (which determines when buffers are applied). However, `despawn` actually ran *after* `spawn`, because these systems are ambiguous, so the jiggles in the sleeping time triggered a case where this works.
## Solution
- Give a better error message
# Objective
Fix `Option<NonSend<T>>` to work when T isn't `Send`
Fix `Option<NonSendMut<T>>` to work when T isnt in the world.
## Solution
Simple two row fix, properly initialize T in `OptionNonSendState` and remove `T: Component` bound for `Option<NonSendMut<T>>`
also added a rudimentary test
Co-authored-by: Ïvar Källström <ivar.kallstrom@gmail.com>
# Objective
- QueryState is lacking documentation.
Fixes#2090
## Solution
- Provide documentation that mirrors Query (as suggested in #2090) and modify as needed.
Co-authored-by: James Leflang <59455417+jleflang@users.noreply.github.com>
This upstreams the code changes used by the new renderer to enable cross-app Entity reuse:
* Spawning at specific entities
* get_or_spawn: spawns an entity if it doesn't already exist and returns an EntityMut
* insert_or_spawn_batch: the batched equivalent to `world.get_or_spawn(entity).insert_bundle(bundle)`
* Clearing entities and storages
* Allocating Entities with "invalid" archetypes. These entities cannot be queried / are treated as "non existent". They serve as "reserved" entities that won't show up when calling `spawn()`. They must be "specifically spawned at" using apis like `get_or_spawn(entity)`.
In combination, these changes enable the "render world" to clear entities / storages each frame and reserve all "app world entities". These can then be spawned during the "render extract step".
This refactors "spawn" and "insert" code in a way that I think is a massive improvement to legibility and re-usability. It also yields marginal performance wins by reducing some duplicate lookups (less than a percentage point improvement on insertion benchmarks). There is also some potential for future unsafe reduction (by making BatchSpawner and BatchInserter generic). But for now I want to cut down generic usage to a minimum to encourage smaller binaries and faster compiles.
This is currently a draft because it needs more tests (although this code has already had some real-world testing on my custom-shaders branch).
I also fixed the benchmarks (which currently don't compile!) / added new ones to illustrate batching wins.
After these changes, Bevy ECS is basically ready to accommodate the new renderer. I think the biggest missing piece at this point is "sub apps".
This is a rather simple but wide change, and it involves adding a new `bevy_app_macros` crate. Let me know if there is a better way to do any of this!
---
# Objective
- Allow adding and accessing sub-apps by using a label instead of an index
## Solution
- Migrate the bevy label implementation and derive code to the `bevy_utils` and `bevy_macro_utils` crates and then add a new `SubAppLabel` trait to the `bevy_app` crate that is used when adding or getting a sub-app from an app.
