# Objective
There is currently a 1-to-1 mapping between components and real rust types. This means that it is impossible for multiple components to be represented by the same rust type or for a component to not have a rust type at all. This means that component types can't be defined in languages other than rust like necessary for scripting or sandboxed (wasm?) plugins.
## Solution
Refactor `ComponentDescriptor` and `Bundle` to remove `TypeInfo`. `Bundle` now uses `ComponentId` instead. `ComponentDescriptor` is now always created from a rust type instead of through the `TypeInfo` indirection. A future PR may make it possible to construct a `ComponentDescriptor` from it's fields without a rust type being involved.
# 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
# Objective
While looking at the code of `World`, I noticed two basic functions (`get` and `get_mut`) that are probably called a lot and with simple code that are not `inline`
## Solution
- Add benchmark to check impact
- Add `#[inline]`
```
group this pr main
----- ---- ----
world_entity/50000_entities 1.00 115.9±11.90µs ? ?/sec 1.71 198.5±29.54µs ? ?/sec
world_get/50000_entities_SparseSet 1.00 409.9±46.96µs ? ?/sec 1.18 483.5±36.41µs ? ?/sec
world_get/50000_entities_Table 1.00 391.3±29.83µs ? ?/sec 1.16 455.6±57.85µs ? ?/sec
world_query_for_each/50000_entities_SparseSet 1.02 121.3±18.36µs ? ?/sec 1.00 119.4±13.88µs ? ?/sec
world_query_for_each/50000_entities_Table 1.03 13.8±0.96µs ? ?/sec 1.00 13.3±0.54µs ? ?/sec
world_query_get/50000_entities_SparseSet 1.00 666.9±54.36µs ? ?/sec 1.03 687.1±57.77µs ? ?/sec
world_query_get/50000_entities_Table 1.01 584.4±55.12µs ? ?/sec 1.00 576.3±36.13µs ? ?/sec
world_query_iter/50000_entities_SparseSet 1.01 169.7±19.50µs ? ?/sec 1.00 168.6±32.56µs ? ?/sec
world_query_iter/50000_entities_Table 1.00 26.2±1.38µs ? ?/sec 1.91 50.0±4.40µs ? ?/sec
```
I didn't add benchmarks for the mutable path but I don't see how it could hurt to make it inline too...
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>
This logic was in both `remove_bundle` and ` remove_bundle_intersection` but only differed by whether we call `.._forget_missing_..` or `.._drop_missing_..`
* bevy_pbr2: Add support for most of the StandardMaterial textures
Normal maps are not included here as they require tangents in a vertex attribute.
* bevy_pbr2: Ensure RenderCommandQueue is ready for PbrShaders init
* texture_pipelined: Add a light to the scene so we can see stuff
* WIP bevy_pbr2: back to front sorting hack
* bevy_pbr2: Uniform control flow for texture sampling in pbr.frag
From 'fintelia' on the Bevy Render Rework Round 2 discussion:
"My understanding is that GPUs these days never use the "execute both branches
and select the result" strategy. Rather, what they do is evaluate the branch
condition on all threads of a warp, and jump over it if all of them evaluate to
false. If even a single thread needs to execute the if statement body, however,
then the remaining threads are paused until that is completed."
* bevy_pbr2: Simplify texture and sampler names
The StandardMaterial_ prefix is no longer needed
* bevy_pbr2: Match default 'AmbientColor' of current bevy_pbr for now
* bevy_pbr2: Convert from non-linear to linear sRGB for the color uniform
* bevy_pbr2: Add pbr_pipelined example
* Fix view vector in pbr frag to work in ortho
* bevy_pbr2: Use a 90 degree y fov and light range projection for lights
* bevy_pbr2: Add AmbientLight resource
* bevy_pbr2: Convert PointLight color to linear sRGB for use in fragment shader
* bevy_pbr2: pbr.frag: Rename PointLight.projection to view_projection
The uniform contains the view_projection matrix so this was incorrect.
* bevy_pbr2: PointLight is an OmniLight as it has a radius
* bevy_pbr2: Factoring out duplicated code
* bevy_pbr2: Implement RenderAsset for StandardMaterial
* Remove unnecessary texture and sampler clones
* fix comment formatting
* remove redundant Buffer:from
* Don't extract meshes when their material textures aren't ready
* make missing textures in the queue step an error
Co-authored-by: Aevyrie <aevyrie@gmail.com>
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
- Continue work of #2398 and friends.
- Make `.system()` optional in run criteria APIs.
## Solution
- Slight change to `RunCriteriaDescriptorCoercion` signature and implementors.
- Implement `IntoRunCriteria` for `IntoSystem` rather than `System`.
- Remove some usages of `.system()` with run criteria in tests of `stage.rs`, to verify the implementation.
# 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.
# Objective
- Add inline documentation for `StorageType`.
- Currently the README in `bevy_ecs` provides docs for `StorageType`, however, adding addition inline docs makes it simpler for users who are actively reading the source code.
## Solution
- Add inline docs.
# Objective
- Extend work done in #2398.
- Make `.system()` syntax optional when using system descriptor API.
## Solution
- Slight change to `ParallelSystemDescriptorCoercion` signature and implementors.
---
I haven't touched exclusive systems, because it looks like the only two other solutions are going back to doubling our system insertion methods, or starting to lean into stageless. The latter will invalidate the former, so I think exclusive systems should remian pariahs until stageless.
I can grep & nuke `.system()` thorughout the codebase now, which might take a while, or we can do that in subsequent PR(s).
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).
When dropping the data, we originally only checked the size of an individual item instead of the size of the allocation. However with a capacity of 0, we attempt to deallocate a pointer which was not the result of allocation. That is, an item of `Layout { size_: 8, align_: 8 }` produces an array of `Layout { size_: 0, align_: 8 }` when `capacity = 0`.
Fixes#2294
## Objective
- Fixes: #2275
- `Assets` were being flagged as 'changed' each frame regardless of if the assets were actually being updated.
## Solution
- Only have `Assets` change detection be triggered when the collection is actually modified.
- This includes utilizing `ResMut` further down the stack instead of a `&mut Assets` directly.
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!
I've noticed that we are overusing interior mutability of the Table data, where in many cases we already own a unique reference to it. That prompted a slight refactor aiming to reduce number of safety constraints that must be manually upheld. Now the majority of those are just about avoiding bound checking, which is relatively easy to prove right.
Another aspect is reducing the complexity of Table struct. Notably, we don't ever use archetypes stored there, so this whole thing goes away. Capacity and grow amount were mostly superficial, as we are already using Vecs inside anyway, so I've got rid of those too. Now the overall table capacity is being driven by the internal entity Vec capacity. This has a side effect of automatically implementing exponential growth pattern for BitVecs reallocations inside Table, which to my measurements slightly improves performance in tests that are heavy on inserts. YMMV, but I hope that those tests were at least remotely correct.
The previous implementation of `Events::extend` iterated through each event and manually `sent` it via `Events:;send`.
However, this could be a minor performance hit since calling `Vec::push` in a loop is not optimal.
This refactors the code to use `Vec::extend`.
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>
During PR #2046 @cart suggested that the `(): ()` notation is less legible than `_input: ()`. The first notation still managed to slip in though. This PR applies the second writing.
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.
`ParallelSystemContainer`'s `system` pointer was extracted from box, but it was never deallocated. This change adds missing drop implementation that cleans up that memory.
The first commit monomorphizes `add_system_inner` which I think was intended to be monomorphized anyway. The second commit moves the type argument of `GraphNode` to an associated type.
In response to #2023, here is a draft for a PR.
Fixes#2023
I've added an example to show how to use `WithBundle`, and also to test it out.
Right now there is a bug: If a bundle and a query are "the same", then it doesn't filter out
what it needs to filter out.
Example:
```
Print component initated from bundle.
[examples/ecs/query_bundle.rs:57] x = Dummy( <========= This should not get printed
111,
)
[examples/ecs/query_bundle.rs:57] x = Dummy(
222,
)
Show all components
[examples/ecs/query_bundle.rs:50] x = Dummy(
111,
)
[examples/ecs/query_bundle.rs:50] x = Dummy(
222,
)
```
However, it behaves the right way, if I add one more component to the bundle,
so the query and the bundle doesn't look the same:
```
Print component initated from bundle.
[examples/ecs/query_bundle.rs:57] x = Dummy(
222,
)
Show all components
[examples/ecs/query_bundle.rs:50] x = Dummy(
111,
)
[examples/ecs/query_bundle.rs:50] x = Dummy(
222,
)
```
I hope this helps. I'm definitely up for tinkering with this, and adding anything that I'm asked to add
or change.
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
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?
The documentation for `ShouldRun` doesn't completely explain what each of the variants you can return does. For instance, it isn't very clear that looping systems aren't executed again until after all the systems in a stage have had a chance to run.
This PR adds to the documentation for `ShouldRun`, and hopefully clarifies what is happening during a stage's execution when run criteria are checked and systems are being executed.
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
This shrinks breakout from 316k to 310k when using `--feature dynamic`.
I haven't run the ecs benchmark to test performance as my laptop is too noisy for reliable benchmarking.
We discussed with @alice-i-cecile privately on iterators and agreed that making a custom ordered iterator over query makes no sense since materialization is required anyway and it's better to reuse existing components or code. Therefore, just adding an example to the documentation as requested.
Fixes#1470.
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
This includes a lot of single line comments where either saying more wasn't helpful or due to me not knowing enough about things yet to be able to go more indepth. Proofreading is very much welcome.
Fixes#1846
Got scared of the other "Requested resource does not exist" error at line 395 in `system_param.rs`, under `impl<'a, T: Component> SystemParamFetch<'a> for ResMutState<T> {`. Someone with better knowledge of the code might be able to go in and improve that one.
Fixes#1809. It makes it also possible to use `derive` for `SystemParam` inside ECS and avoid manual implementation. An alternative solution to macro changes is to use `use crate as bevy_ecs;` in `event.rs`.
fixes#1772
1st commit: the limit was at 11 as the macro was not using a range including the upper end. I changed that as it feels the purpose of the macro is clearer that way.
2nd commit: as suggested in the `// TODO`, I added a `Config` trait to go to 16 elements tuples. This means that if someone has a custom system parameter with a config that is not a tuple or an `Option`, they will have to implement `Config` for it instead of the standard `Default`.
I think [collection, thing_removed_from_collection] is a more natural order than [thing_removed_from_collection, collection]. Just a small tweak that I think we should include in 0.5.
Fixes#1753.
The problem was introduced while reworking the logic around stages' own criteria. Before #1675 they used to be stored and processed inline with the systems' criteria, and systems without criteria used that of their stage. After, criteria-less systems think they should run, always. This PR more or less restores previous behavior; a less cludge solution can wait until after 0.5 - ideally, until stageless.
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
This is intended to help protect users against #1671. It doesn't resolve the issue, but I think its a good stop-gap solution for 0.5. A "full" fix would be very involved (and maybe not worth the added complexity).
Removing the checks on this line https://github.com/bevyengine/bevy/blob/main/crates/bevy_sprite/src/frustum_culling.rs#L64 and running the "many_sprites" example revealed two corner case bugs in bevy_ecs. The first, a simple and honest missed line introduced in #1471. The other, an insidious monster that has been there since the ECS v2 rewrite, just waiting for the time to strike:
1. #1471 accidentally removed the "insert" line for sparse set components with the "mutated" bundle state. Re-adding it fixes the problem. I did a slight refactor here to make the implementation simpler and remove a branch.
2. The other issue is nastier. ECS v2 added an "archetype graph". When determining what components were added/mutated during an archetype change, we read the FromBundle edge (which encodes this state) on the "new" archetype. The problem is that unlike "add edges" which are guaranteed to be unique for a given ("graph node", "bundle id") pair, FromBundle edges are not necessarily unique:
```rust
// OLD_ARCHETYPE -> NEW_ARCHETYPE
// [] -> [usize]
e.insert(2usize);
// [usize] -> [usize, i32]
e.insert(1i32);
// [usize, i32] -> [usize, i32]
e.insert(1i32);
// [usize, i32] -> [usize]
e.remove::<i32>();
// [usize] -> [usize, i32]
e.insert(1i32);
```
Note that the second `e.insert(1i32)` command has a different "archetype graph edge" than the first, but they both lead to the same "new archetype".
The fix here is simple: just remove FromBundle edges because they are broken and store the information in the "add edges", which are guaranteed to be unique.
FromBundle edges were added to cut down on the number of archetype accesses / make the archetype access patterns nicer. But benching this change resulted in no significant perf changes and the addition of get_2_mut() for archetypes resolves the access pattern issue.
In the current impl, next clears out the entire stack and replaces it with a new state. This PR moves this functionality into a replace method, and changes the behavior of next to only change the top state.
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
I'm opening this prematurely; consider this an RFC that predates RFCs and therefore not super-RFC-like.
This PR does two "big" things: decouple run criteria from system sets, reimagine system sets as weapons of mass system description.
### What it lets us do:
* Reuse run criteria within a stage.
* Pipe output of one run criteria as input to another.
* Assign labels, dependencies, run criteria, and ambiguity sets to many systems at the same time.
### Things already done:
* Decoupled run criteria from system sets.
* Mass system description superpowers to `SystemSet`.
* Implemented `RunCriteriaDescriptor`.
* Removed `VirtualSystemSet`.
* Centralized all run criteria of `SystemStage`.
* Extended system descriptors with per-system run criteria.
* `.before()` and `.after()` for run criteria.
* Explicit order between state driver and related run criteria. Fixes#1672.
* Opt-in run criteria deduplication; default behavior is to panic.
* Labels (not exposed) for state run criteria; state run criteria are deduplicated.
### API issues that need discussion:
* [`FixedTimestep::step(1.0).label("my label")`](eaccf857cd/crates/bevy_ecs/src/schedule/run_criteria.rs (L120-L122)) and [`FixedTimestep::step(1.0).with_label("my label")`](eaccf857cd/crates/bevy_core/src/time/fixed_timestep.rs (L86-L89)) are both valid but do very different things.
---
I will try to maintain this post up-to-date as things change. Do check the diffs in "edited" thingy from time to time.
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
Resolves#1253#1562
This makes the Commands apis consistent with World apis. This moves to a "type state" pattern (like World) where the "current entity" is stored in an `EntityCommands` builder.
In general this tends to cuts down on indentation and line count. It comes at the cost of needing to type `commands` more and adding more semicolons to terminate expressions.
I also added `spawn_bundle` to Commands because this is a common enough operation that I think its worth providing a shorthand.
Fixes#1692
Alternative to #1696
This ensures that the capacity actually grows in increments of grow_amount, and also ensures that Table capacity is always <= column and entity vec capacity.
Debug logs that describe the new logic (running the example in #1692)
[out.txt](https://github.com/bevyengine/bevy/files/6173808/out.txt)
# Problem Definition
The current change tracking (via flags for both components and resources) fails to detect changes made by systems that are scheduled to run earlier in the frame than they are.
This issue is discussed at length in [#68](https://github.com/bevyengine/bevy/issues/68) and [#54](https://github.com/bevyengine/bevy/issues/54).
This is very much a draft PR, and contributions are welcome and needed.
# Criteria
1. Each change is detected at least once, no matter the ordering.
2. Each change is detected at most once, no matter the ordering.
3. Changes should be detected the same frame that they are made.
4. Competitive ergonomics. Ideally does not require opting-in.
5. Low CPU overhead of computation.
6. Memory efficient. This must not increase over time, except where the number of entities / resources does.
7. Changes should not be lost for systems that don't run.
8. A frame needs to act as a pure function. Given the same set of entities / components it needs to produce the same end state without side-effects.
**Exact** change-tracking proposals satisfy criteria 1 and 2.
**Conservative** change-tracking proposals satisfy criteria 1 but not 2.
**Flaky** change tracking proposals satisfy criteria 2 but not 1.
# Code Base Navigation
There are three types of flags:
- `Added`: A piece of data was added to an entity / `Resources`.
- `Mutated`: A piece of data was able to be modified, because its `DerefMut` was accessed
- `Changed`: The bitwise OR of `Added` and `Changed`
The special behavior of `ChangedRes`, with respect to the scheduler is being removed in [#1313](https://github.com/bevyengine/bevy/pull/1313) and does not need to be reproduced.
`ChangedRes` and friends can be found in "bevy_ecs/core/resources/resource_query.rs".
The `Flags` trait for Components can be found in "bevy_ecs/core/query.rs".
`ComponentFlags` are stored in "bevy_ecs/core/archetypes.rs", defined on line 446.
# Proposals
**Proposal 5 was selected for implementation.**
## Proposal 0: No Change Detection
The baseline, where computations are performed on everything regardless of whether it changed.
**Type:** Conservative
**Pros:**
- already implemented
- will never miss events
- no overhead
**Cons:**
- tons of repeated work
- doesn't allow users to avoid repeating work (or monitoring for other changes)
## Proposal 1: Earlier-This-Tick Change Detection
The current approach as of Bevy 0.4. Flags are set, and then flushed at the end of each frame.
**Type:** Flaky
**Pros:**
- already implemented
- simple to understand
- low memory overhead (2 bits per component)
- low time overhead (clear every flag once per frame)
**Cons:**
- misses systems based on ordering
- systems that don't run every frame miss changes
- duplicates detection when looping
- can lead to unresolvable circular dependencies
## Proposal 2: Two-Tick Change Detection
Flags persist for two frames, using a double-buffer system identical to that used in events.
A change is observed if it is found in either the current frame's list of changes or the previous frame's.
**Type:** Conservative
**Pros:**
- easy to understand
- easy to implement
- low memory overhead (4 bits per component)
- low time overhead (bit mask and shift every flag once per frame)
**Cons:**
- can result in a great deal of duplicated work
- systems that don't run every frame miss changes
- duplicates detection when looping
## Proposal 3: Last-Tick Change Detection
Flags persist for two frames, using a double-buffer system identical to that used in events.
A change is observed if it is found in the previous frame's list of changes.
**Type:** Exact
**Pros:**
- exact
- easy to understand
- easy to implement
- low memory overhead (4 bits per component)
- low time overhead (bit mask and shift every flag once per frame)
**Cons:**
- change detection is always delayed, possibly causing painful chained delays
- systems that don't run every frame miss changes
- duplicates detection when looping
## Proposal 4: Flag-Doubling Change Detection
Combine Proposal 2 and Proposal 3. Differentiate between `JustChanged` (current behavior) and `Changed` (Proposal 3).
Pack this data into the flags according to [this implementation proposal](https://github.com/bevyengine/bevy/issues/68#issuecomment-769174804).
**Type:** Flaky + Exact
**Pros:**
- allows users to acc
- easy to implement
- low memory overhead (4 bits per component)
- low time overhead (bit mask and shift every flag once per frame)
**Cons:**
- users must specify the type of change detection required
- still quite fragile to system ordering effects when using the flaky `JustChanged` form
- cannot get immediate + exact results
- systems that don't run every frame miss changes
- duplicates detection when looping
## [SELECTED] Proposal 5: Generation-Counter Change Detection
A global counter is increased after each system is run. Each component saves the time of last mutation, and each system saves the time of last execution. Mutation is detected when the component's counter is greater than the system's counter. Discussed [here](https://github.com/bevyengine/bevy/issues/68#issuecomment-769174804). How to handle addition detection is unsolved; the current proposal is to use the highest bit of the counter as in proposal 1.
**Type:** Exact (for mutations), flaky (for additions)
**Pros:**
- low time overhead (set component counter on access, set system counter after execution)
- robust to systems that don't run every frame
- robust to systems that loop
**Cons:**
- moderately complex implementation
- must be modified as systems are inserted dynamically
- medium memory overhead (4 bytes per component + system)
- unsolved addition detection
## Proposal 6: System-Data Change Detection
For each system, track which system's changes it has seen. This approach is only worth fully designing and implementing if Proposal 5 fails in some way.
**Type:** Exact
**Pros:**
- exact
- conceptually simple
**Cons:**
- requires storing data on each system
- implementation is complex
- must be modified as systems are inserted dynamically
## Proposal 7: Total-Order Change Detection
Discussed [here](https://github.com/bevyengine/bevy/issues/68#issuecomment-754326523). This proposal is somewhat complicated by the new scheduler, but I believe it should still be conceptually feasible. This approach is only worth fully designing and implementing if Proposal 5 fails in some way.
**Type:** Exact
**Pros:**
- exact
- efficient data storage relative to other exact proposals
**Cons:**
- requires access to the scheduler
- complex implementation and difficulty grokking
- must be modified as systems are inserted dynamically
# Tests
- We will need to verify properties 1, 2, 3, 7 and 8. Priority: 1 > 2 = 3 > 8 > 7
- Ideally we can use identical user-facing syntax for all proposals, allowing us to re-use the same syntax for each.
- When writing tests, we need to carefully specify order using explicit dependencies.
- These tests will need to be duplicated for both components and resources.
- We need to be sure to handle cases where ambiguous system orders exist.
`changing_system` is always the system that makes the changes, and `detecting_system` always detects the changes.
The component / resource changed will be simple boolean wrapper structs.
## Basic Added / Mutated / Changed
2 x 3 design:
- Resources vs. Components
- Added vs. Changed vs. Mutated
- `changing_system` runs before `detecting_system`
- verify at the end of tick 2
## At Least Once
2 x 3 design:
- Resources vs. Components
- Added vs. Changed vs. Mutated
- `changing_system` runs after `detecting_system`
- verify at the end of tick 2
## At Most Once
2 x 3 design:
- Resources vs. Components
- Added vs. Changed vs. Mutated
- `changing_system` runs once before `detecting_system`
- increment a counter based on the number of changes detected
- verify at the end of tick 2
## Fast Detection
2 x 3 design:
- Resources vs. Components
- Added vs. Changed vs. Mutated
- `changing_system` runs before `detecting_system`
- verify at the end of tick 1
## Ambiguous System Ordering Robustness
2 x 3 x 2 design:
- Resources vs. Components
- Added vs. Changed vs. Mutated
- `changing_system` runs [before/after] `detecting_system` in tick 1
- `changing_system` runs [after/before] `detecting_system` in tick 2
## System Pausing
2 x 3 design:
- Resources vs. Components
- Added vs. Changed vs. Mutated
- `changing_system` runs in tick 1, then is disabled by run criteria
- `detecting_system` is disabled by run criteria until it is run once during tick 3
- verify at the end of tick 3
## Addition Causes Mutation
2 design:
- Resources vs. Components
- `adding_system_1` adds a component / resource
- `adding system_2` adds the same component / resource
- verify the `Mutated` flag at the end of the tick
- verify the `Added` flag at the end of the tick
First check tests for: https://github.com/bevyengine/bevy/issues/333
Second check tests for: https://github.com/bevyengine/bevy/issues/1443
## Changes Made By Commands
- `adding_system` runs in Update in tick 1, and sends a command to add a component
- `detecting_system` runs in Update in tick 1 and 2, after `adding_system`
- We can't detect the changes in tick 1, since they haven't been processed yet
- If we were to track these changes as being emitted by `adding_system`, we can't detect the changes in tick 2 either, since `detecting_system` has already run once after `adding_system` :(
# Benchmarks
See: [general advice](https://github.com/bevyengine/bevy/blob/master/docs/profiling.md), [Criterion crate](https://github.com/bheisler/criterion.rs)
There are several critical parameters to vary:
1. entity count (1 to 10^9)
2. fraction of entities that are changed (0% to 100%)
3. cost to perform work on changed entities, i.e. workload (1 ns to 1s)
1 and 2 should be varied between benchmark runs. 3 can be added on computationally.
We want to measure:
- memory cost
- run time
We should collect these measurements across several frames (100?) to reduce bootup effects and accurately measure the mean, variance and drift.
Entity-component change detection is much more important to benchmark than resource change detection, due to the orders of magnitude higher number of pieces of data.
No change detection at all should be included in benchmarks as a second control for cases where missing changes is unacceptable.
## Graphs
1. y: performance, x: log_10(entity count), color: proposal, facet: performance metric. Set cost to perform work to 0.
2. y: run time, x: cost to perform work, color: proposal, facet: fraction changed. Set number of entities to 10^6
3. y: memory, x: frames, color: proposal
# Conclusions
1. Is the theoretical categorization of the proposals correct according to our tests?
2. How does the performance of the proposals compare without any load?
3. How does the performance of the proposals compare with realistic loads?
4. At what workload does more exact change tracking become worth the (presumably) higher overhead?
5. When does adding change-detection to save on work become worthwhile?
6. Is there enough divergence in performance between the best solutions in each class to ship more than one change-tracking solution?
# Implementation Plan
1. Write a test suite.
2. Verify that tests fail for existing approach.
3. Write a benchmark suite.
4. Get performance numbers for existing approach.
5. Implement, test and benchmark various solutions using a Git branch per proposal.
6. Create a draft PR with all solutions and present results to team.
7. Select a solution and replace existing change detection.
Co-authored-by: Brice DAVIER <bricedavier@gmail.com>
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
