# 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
- 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`
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
## 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
# 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>
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".
# Objective
Enable using exact World lifetimes during read-only access . This is motivated by the new renderer's need to allow read-only world-only queries to outlive the query itself (but still be constrained by the world lifetime).
For example:
115b170d1f/pipelined/bevy_pbr2/src/render/mod.rs (L774)
## Solution
Split out SystemParam state and world lifetimes and pipe those lifetimes up to read-only Query ops (and add into_inner for Res). According to every safety test I've run so far (except one), this is safe (see the temporary safety test commit). Note that changing the mutable variants to the new lifetimes would allow aliased mutable pointers (try doing that to see how it affects the temporary safety tests).
The new state lifetime on SystemParam does make `#[derive(SystemParam)]` more cumbersome (the current impl requires PhantomData if you don't use both lifetimes). We can make this better by detecting whether or not a lifetime is used in the derive and adjusting accordingly, but that should probably be done in its own pr.
## Why is this a draft?
The new lifetimes break QuerySet safety in one very specific case (see the query_set system in system_safety_test). We need to solve this before we can use the lifetimes given.
This is due to the fact that QuerySet is just a wrapper over Query, which now relies on world lifetimes instead of `&self` lifetimes to prevent aliasing (but in systems, each Query has its own implied lifetime, not a centralized world lifetime). I believe the fix is to rewrite QuerySet to have its own World lifetime (and own the internal reference). This will complicate the impl a bit, but I think it is doable. I'm curious if anyone else has better ideas.
Personally, I think these new lifetimes need to happen. We've gotta have a way to directly tie read-only World queries to the World lifetime. The new renderer is the first place this has come up, but I doubt it will be the last. Worst case scenario we can come up with a second `WorldLifetimeQuery<Q, F = ()>` parameter to enable these read-only scenarios, but I'd rather not add another type to the type zoo.
# Objective
- Remove all the `.system()` possible.
- Check for remaining missing cases.
## Solution
- Remove all `.system()`, fix compile errors
- 32 calls to `.system()` remains, mostly internals, the few others should be removed after #2446
This is extracted out of eb8f973646476b4a4926ba644a77e2b3a5772159 and includes some additional changes to remove all references to AppBuilder and fix examples that still used App::build() instead of App::new(). In addition I didn't extract the sub app feature as it isn't ready yet.
You can use `git diff --diff-filter=M eb8f973646476b4a4926ba644a77e2b3a5772159` to find all differences in this PR. The `--diff-filtered=M` filters all files added in the original commit but not in this commit away.
Co-Authored-By: Carter Anderson <mcanders1@gmail.com>
# Objective
- Continue work of #2398 and friends.
- Make `.system()` optional in chaining.
## Solution
- Slight change to `IntoChainSystem` signature and implementation.
- Remove some usages of `.system()` in the chaining example, to verify the implementation.
---
I swear, I'm not splitting these up on purpose, I just legit forgot about most of the things where `System` appears in public API, and my trait usage explorer mingles that with the gajillion internal uses.
In case you're wondering what happened to part 5, #2446 ate it.
# Objective
- Currently `Commands` are quite slow due to the need to allocate for each command and wrap it in a `Box<dyn Command>`.
- For example:
```rust
fn my_system(mut cmds: Commands) {
cmds.spawn().insert(42).insert(3.14);
}
```
will have 3 separate `Box<dyn Command>` that need to be allocated and ran.
## Solution
- Utilize a specialized data structure keyed `CommandQueueInner`.
- The purpose of `CommandQueueInner` is to hold a collection of commands in contiguous memory.
- This allows us to store each `Command` type contiguously in memory and quickly iterate through them and apply the `Command::write` trait function to each element.
In #2034, the `Remove` Command did not get the same treatment as the rest of the commands. There's no discussion saying it shouldn't have public fields, so I am assuming it was an oversight. This fixes that oversight.
# Objective
Beginners semi-regularly appear on the Discord asking for help with using `QuerySet` when they have a system with conflicting data access.
This happens because the Resulting Panic message only mentions `QuerySet` as a solution, even if in most cases `Without<T>` was enough to solve the problem.
## Solution
Mention the usage of `Without<T>` to create disjoint queries as an alternative to `QuerySet`
## Open Questions
- Is `disjoint` a too technical/mathematical word?
- Should `Without<T>` be mentioned before or after `QuerySet`?
- Before: Using `Without<T>` should be preferred and mentioning it first reinforces this for a reader.
- After: The Panics can be very long and a Reader could skip to end and only see the `QuerySet`
Co-authored-by: MinerSebas <66798382+MinerSebas@users.noreply.github.com>
# Objective
- Continue work of #2398 and #2403.
- Make `.system()` syntax optional when using `.config()` API.
## Solution
- Introduce new prelude trait, `ConfigurableSystem`, that shorthands `my_system.system().config(...)` as `my_system.config(...)`.
- Expand `configure_system_local` test to also cover the new syntax.
This can be your 6 months post-christmas present.
# Objective
- Make `.system` optional
- yeet
- It's ugly
- Alternative title: `.system` is dead; long live `.system`
- **yeet**
## Solution
- Use a higher ranked lifetime, and some trait magic.
N.B. This PR does not actually remove any `.system`s, except in a couple of examples. Once this is merged we can do that piecemeal across crates, and decide on syntax for labels.
# Objective
Currently, you can add `Option<Res<T>` or `Option<ResMut<T>` as a SystemParam, if the Resource could potentially not exist, but this functionality doesn't exist for `NonSend` and `NonSendMut`
## Solution
Adds implementations to use `Option<NonSend<T>>` and Option<NonSendMut<T>> as SystemParams.
# Objective
- CI jobs are starting to fail due to `clippy::bool-assert-comparison` and `clippy::single_component_path_imports` being triggered.
## Solution
- Fix all uses where `asset_eq!(<condition>, <bool>)` could be replace by `assert!`
- Move the `#[allow()]` for `single_component_path_imports` to `#![allow()]` at the start of the files.
# Objective
- The `DetectChanges` trait is used for types that detect change on mutable access (such as `ResMut`, `Mut`, etc...)
- `DetectChanges` was not implemented for `NonSendMut`
## Solution
- implement `NonSendMut` in terms of `DetectChanges`
# Objective
Currently, you can't call `is_added` or `is_changed` on a `NonSend` SystemParam, unless the Resource is a Component (implements `Send` and `Sync`).
This defeats the purpose of providing change detection for NonSend Resources.
While fixing this, I also noticed that `NonSend` does not have a bound at all on its struct.
## Solution
Change the bounds of `T` to always be `'static`.
## Problem
- The `Query` struct does not provide an easy way to check if it is empty.
- Specifically, users have to use `.iter().peekable()` or `.iter().next().is_none()` which is not very ergonomic.
- Fixes: #2270
## Solution
- Implement an `is_empty` function for queries to more easily check if the query is empty.
This enables `SystemParams` to be used outside of function systems. Anything can create and store `SystemState`, which enables efficient "param state cached" access to `SystemParams`.
It adds a `ReadOnlySystemParamFetch` trait, which enables safe `SystemState::get` calls without unique world access.
I renamed the old `SystemState` to `SystemMeta` to enable us to mirror the `QueryState` naming convention (but I'm happy to discuss alternative names if people have other ideas). I initially pitched this as `ParamState`, but given that it needs to include full system metadata, that doesn't feel like a particularly accurate name.
```rust
#[derive(Eq, PartialEq, Debug)]
struct A(usize);
#[derive(Eq, PartialEq, Debug)]
struct B(usize);
let mut world = World::default();
world.insert_resource(A(42));
world.spawn().insert(B(7));
// we get nice lifetime elision when declaring the type on the left hand side
let mut system_state: SystemState<(Res<A>, Query<&B>)> = SystemState::new(&mut world);
let (a, query) = system_state.get(&world);
assert_eq!(*a, A(42), "returned resource matches initial value");
assert_eq!(
*query.single().unwrap(),
B(7),
"returned component matches initial value"
);
// mutable system params require unique world access
let mut system_state: SystemState<(ResMut<A>, Query<&mut B>)> = SystemState::new(&mut world);
let (a, query) = system_state.get_mut(&mut world);
// static lifetimes are required when declaring inside of structs
struct SomeContainer {
state: SystemState<(Res<'static, A>, Res<'static, B>)>
}
// this can be shortened using type aliases, which will be useful for complex param tuples
type MyParams<'a> = (Res<'a, A>, Res<'a, B>);
struct SomeContainer {
state: SystemState<MyParams<'static>>
}
// It is the user's responsibility to call SystemState::apply(world) for parameters that queue up work
let mut system_state: SystemState<(Commands, Query<&B>)> = SystemState::new(&mut world);
{
let (mut commands, query) = system_state.get(&world);
commands.insert_resource(3.14);
}
system_state.apply(&mut world);
```
## Future Work
* Actually use SystemState inside FunctionSystem. This would be trivial, but it requires FunctionSystem to wrap SystemState in Option in its current form (which complicates system metadata lookup). I'd prefer to hold off until we adopt something like the later designs linked in #1364, which enable us to contruct Systems using a World reference (and also remove the need for `.system`).
* Consider a "scoped" approach to automatically call SystemState::apply when systems params are no longer being used (either a container type with a Drop impl, or a function that takes a closure for user logic operating on params).
Continuing the work on reducing the safety footguns in the code, I've removed one extra `UnsafeCell` in favour of safe `Cell` usage inisde `ComponentTicks`. That change led to discovery of misbehaving component insert logic, where data wasn't properly dropped when overwritten. Apart from that being fixed, some method names were changed to better convey the "initialize new allocation" and "replace existing allocation" semantic.
Depends on #2221, I will rebase this PR after the dependency is merged. For now, review just the last commit.
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
`ResMut`, `Mut` and `ReflectMut` all share very similar code for change detection.
This PR is a first pass at refactoring these implementation and removing a lot of the duplicated code.
Note, this introduces a new trait `ChangeDetectable`.
Please feel free to comment away and let me know what you think!
This new api stems from this [discord conversation](https://discord.com/channels/691052431525675048/742569353878437978/844057268172357663).
This exposes a public facing `set_changed` method on `ResMut` and `Mut`.
As a side note: `ResMut` and `Mut` have a lot of duplicated code, I have a PR I may put up later that refactors these commonalities into a trait.
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
- simplified code around archetype generations a little bit, as the special case value is not actually needed
- removed unnecessary UnsafeCell around pointer value that is never updated through shared references
- fixed and added a test for correct drop behaviour when removing sparse components through remove_bundle command
While trying to figure out how to implement a `SystemParam`, I spent a
long time looking for a feature that would do exactly what `Config`
does. I ignored it at first because all the examples I could find used
`()` and I couldn't see a way to modify it.
This is documented in other places, but `Config` is a logical place to
include some breadcrumbs. I've added some text that gives a brief
overview of what `Config` is for, and links to the existing docs on
`FunctionSystem::config` for more details.
This would have saved me from embarrassing myself by filing https://github.com/bevyengine/bevy/issues/2178.
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
Related to [discussion on discord](https://discord.com/channels/691052431525675048/742569353878437978/824731187724681289)
With const generics, it is now possible to write generic iterator over multiple entities at once.
This enables patterns of query iterations like
```rust
for [e1, e2, e3] in query.iter_combinations() {
// do something with relation of all three entities
}
```
The compiler is able to infer the correct iterator for given size of array, so either of those work
```rust
for [e1, e2] in query.iter_combinations() { ... }
for [e1, e2, e3] in query.iter_combinations() { ... }
```
This feature can be very useful for systems like collision detection.
When you ask for permutations of size K of N entities:
- if K == N, you get one result of all entities
- if K < N, you get all possible subsets of N with size K, without repetition
- if K > N, the result set is empty (no permutation of size K exist)
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
This can save users from having to type `&*X` all the time at the cost of some complexity in the type signature. For instance, this allows me to accommodate @jakobhellermann's suggestion in #1799 without requiring users to type `&*windows` 99% of the time.
I'm using Bevy ECS in a project of mine and I'd like to do world changes asynchronously.
The current public API for creating entities, `Commands` , has a lifetime that restricts it from being sent across threads. `CommandQueue` on the other hand is a Vec of commands that can be later ran on a World.
So far this is all public, but the commands themselves are private API. I know the intented use is with `Commands`, but that's not possible for my use case as I mentioned, and so I simply copied over the code for the commands I need and it works. Obviously, this isn't a nice solution, so I'd like to ask if it's not out of scope to make the commands public?
Some panic messages for systems include the system name, but there's a few panic messages which do not. This PR adds the system name for the remaining panic messages.
This is a continuation of the work done in #1864.
Related: #1846
