Lifecycle event observers for queries
- Dominant language
- Rust
- Stars
- 48.2k
- Forks
- 4.8k
- Avg merge
- 3d 16h
- Merged PRs (30d)
- 171
Description
## What problem does this solve or what need does it fill?
Lifecycle event observers are hard to use for anything but the simplest cases.
Let's consider some cases where they should be useful:
### Maintaining an index
Suppose we want to maintain an index that lets us look up the set of entities with a given component value. Not everything needs to be in the index, so we also define a marker component that should exclude entities from being indexed. Our first attempt will be a system that rebuilds the index:
```rust
#[derive(Component, Eq, PartialEq, Hash, Copy, Clone)]
#[component(immutable)]
struct Value(usize);
#[derive(Component)]
struct ExcludeFromIndex;
#[derive(Resource, Default)]
struct Index(HashMap);
fn rebuild_index(
mut index: ResMut,
query: Query<(Entity, &Value), Without>,
) {
index.0.clear();
for (entity, value) in query {
index.0.entry(*value).or_default().insert(entity);
}
}
```
### Computed values
`bevy_feathers` wants to calculate the background color for a button based on the values and presence of other components. The [actual code](https://github.com/bevyengine/bevy/blob/cbf989c9dab5ce8152ab9407e0d4cb42b0138003/crates/bevy_feathers/src/controls/button.rs#L93) has two `Has` inputs and two `&C` inputs, but to make a cleaner example I'm using one of each plus an `Option<&C>`:
```rust
fn update_color(query: Query<(Option<&Hovered>, Has, &ButtonVariant, &mut BackgroundColor)>) {
for (hovered, disabled, pressed, variant, mut color) in query {
*color = calculate_color(hovered, pressed, variant);
}
}
```
### We need observers!
The problem with using ordinary systems here is that the code both runs *too often* but also *not often enough*! It runs too often because it has to evaluate every entity, even if they don't change. And it runs not often enough because the index or background color will be out-of date between when changes were made and when the system runs.
## What alternative(s) have you considered?
Lifecycle events are intended to solve exactly this use case. Let's try to use them! First, maintaining the index:
```rust
fn add_to_index_when_value_is_added(
event: On,
mut index: ResMut,
query: Query<(Entity, &Value), Without>,
) {
if let Ok((entity, value)) = query.get(event.target()) {
index.0.entry(*value).or_default().insert(entity);
}
}
fn add_to_index_when_exclude_is_removed(
event: On,
mut index: ResMut,
query: Query<(Entity, &Value)>,
value_component_id: ComponentIdFor,
) {
if !event.components().contains(&value_component_id) // Wait, what?
&& let Ok((entity, value)) = query.get(event.target())
{
index.0.entry(*value).or_default().insert(entity);
}
}
fn remove_from_index_when_value_is_removed(
event: On,
mut index: ResMut,
query: Query<(Entity, &Value), Without>,
) {
if let Ok((entity, value)) = query.get(event.target()) {
index.0.entry(*value).or_default().remove(&entity);
}
}
fn remove_from_index_when_exclude_is_added(
event: On,
mut index: ResMut,
query: Query<(Entity, &Value)>,
) {
if let Ok((entity, value)) = query.get(event.target()) {
index.0.entry(*value).or_default().remove(&entity);
}
}
```
That's a lot more code! We have separate observers to add to the index and remove from it, which is reasonable. But we also need separate observers to handle adding to the index because `Value` was added and adding to the index because `ExcludeFromIndex` was removed.
Worse, they need slightly different signatures: `add_to_index_when_value_is_added` *needs* the `Without` clause in order to respect `ExcludeFromIndex`, but `add_to_index_when_exclude_is_removed` *can't* have that clause, since it runs while `ExcludeFromIndex` is still present!
And it's especially hard to correctly handle the case where both components are removed at once, which notably includes despawning an entity that has both. The problem is that the observer for removing `ExcludeFromIndex` runs before the archetype change, so it sees that `Value` exists and thinks we need to add the value to the index. The only way I could think of to correctly handle it is to check `event.components()` to see whether `Value` is being removed at the same time. (And if #17634 were done then I don't think it would be possible at all.)
Well, what about the computed values case? What we *want* is something like
```rust
fn update_color(
event: On,
mut query: Query<(Option<&Hovered>, Has, &ButtonVariant, &mut BackgroundColor)>,
) {
if let Ok((hovered, pressed, variant, mut color)) = query.get_mut(event.target()) {
*color = calculate_color(hovered, pressed, variant);
}
}
```
But we need to update the value if `Hovered` or `Pressed` are *removed*, and the `Remove` and `Replace` triggers fire *before* the archetype change, so `Option<&Hovered>` and `Has` would return the *old* value. I think it's possible to handle that correctly, but it would be a lot of code so I'm not going to try.
### `Before` and `After` lifecycle events
The issues so far could be fixed by adding `Before` and `After` variants of each lifecycle event, as described in #20729. Let's pretend we have those, and try again. First, maintaining the index:
```rust
fn add_to_index_when_value_is_added(
event: On, Value>,
mut index: ResMut,
query: Query<(Entity, &Value), Without>,
) {
if let Ok((entity, value)) = query.get(event.target()) {
index.0.entry(*value).or_default().insert(entity);
}
}
fn add_to_index_when_exclude_is_removed(
event: On, ExcludeFromIndex>,
mut index: ResMut,
query: Query<(Entity, &Value), Without>,
) {
if let Ok((entity, value)) = query.get(event.target()) {
index.0.entry(*value).or_default().insert(entity);
}
}
fn remove_from_index_when_value_is_removed(
event: On, Value>,
mut index: ResMut,
query: Query<(Entity, &Value), Without>,
) {
if let Ok((entity, value)) = query.get(event.target()) {
index.0.entry(*value).or_default().remove(&entity);
}
}
fn remove_from_index_when_exclude_is_added(
event: On, ExcludeFromIndex>,
mut index: ResMut,
query: Query<(Entity, &Value), Without>,
) {
if let Ok((entity, value)) = query.get(event.target()) {
index.0.entry(*value).or_default().remove(&entity);
}
}
```
That's nicer! We still need two observers, but they have the same signature and implementation so we can at worst just copy-paste them.
Now, for computed values, we *can* write the `update_color` observer as above. We still need multiple copies for the different trigger types, but they are:
```rust
event: On, (Hovered, ButtonVariant, BackgroundColor)>,
event: On, Pressed>,
event: On, (Hovered, Pressed)>,
```
The `Insert` events ensure we run when the query starts matching an entity, or when the value of `Hovered` or `ButtonVariant` is replaced. (And when `BackgroundColor` is replaced, since we need to repopulate it!)
We use `Add` instead of `Insert` for `Pressed`, since replacing the value wouldn't change the result of `Has`.
And we need `Remove` events for `Hovered` and `Pressed` since they change the result of `Option` and `Has`.
### More problems
There are still some problems with this solution:
#### Default query filters
One major problem is that it doesn't handle default query filters like `Disabled` correctly. If we add a `Disabled` component to an entity in our index, it won't be removed. Worse, if we later change or remove the value, the entity won't match the queries we use to remove it from the index! So if we disable an entity and then despawn it without re-enabling it, it will remain in the index forever.
If the observer author remembers the `Disabled` component, it's possible to handle it explicitly the way we did with our `ExcludeFromIndex` components. But other code may add other default query filters, and there is no way to know about all of them.
One option here is to fire `Remove` and `Replace` triggers for *all* components on an entity when `Disabled` is added, and `Add` and `Insert` when it's removed. That would fix the problem at the cost of some complexity, although we'd need some *other* solution to handle cases that use `Allow` filters.
#### Separate observers for each event
We had to write two copies of each function for the index code, and three for the computed value. There are ways to manage that, either by extracting the body of the observer to a separate function, or by making the function generic in the event type and doing `world.add_observer(update_color::, Pressed>)`.
I believe this is the problem that #14649 describes, and there are some possible solutions there.
#### Observers are invoked unnecessarily
Many of the invocations of these observers will do nothing because the query doesn't match. The `add_to_index_when_value_is_added` observer still gets invoked for entities with the `ExcludeFromIndex` component. The `update_color` observer gets invoked every time `Hovered` changes, even on entities that aren't buttons. It's not a huge expense to to `query.get` and return, but it's still an unavoidable overhead.
#### Hard to figure out exactly what events to observe
With the introduction of `Before` and `After`, there are *eight* different lifecycle events we could potentially observe. And the `Add`/`Insert` and `Replace`/`Remove` pairs are similar enough that it can be hard to remember which is which.
And once we get it right, it's still a maintenance problem. If we modify the query in `update_color` to consider a new value, we need to make sure to add it to the appropriate lists of observed components. It would be easy to forget to do that, and it can be hard to detect bugs where an observer *sometimes* fails to run when expected. And conversely, if we modify the query to stop considering a value, it's easy to forget to remove it from the lists of observed components, resulting in wasted calculation that's hard to discover.
## What solution would you like?
We tried using the current lifecycle events, but had to add several new features, and still had something that was hard to get right. Instead, let's forget everything we know about them and start from scratch!
We started with systems that ran *queries*, so let's try to write something that looks like that. Instead of taking a `Query` and iterating over it, we'll take an `On>` with a single value. And, of course, the index will need to remove entities one-by-one instead of doing `clear()`.
Here is what I want to write:
```rust
fn add_to_index(
event: On>>,
mut index: ResMut,
) {
let (entity, value) = event.data();
index.0.entry(*value).or_default().insert(entity);
}
fn remove_from_index(
event: On>>,
mut index: ResMut,
) {
let (entity, value) = event.data();
index.0.entry(*value).or_default().remove(&entity);
}
fn update_color(
event: On, Has, &ButtonVariant, &mut BackgroundColor)>>,
) {
let (hovered, pressed, variant, mut color) = event.data();
*color = calculate_color(hovered, pressed, variant);
}
```
Those observers were much easier to write, and much more clear in what they intend to do. So, if that's what we want to write, what do the semantics of `DataAdded` and `DataRemoving` need to be to get the behavior we want?
They need to trigger when the *data* being returned by the query changes. Note that data changes aren't just when the set of entities changes! When `Value` changes for an existing entity, we need to remove it at the old value and add it at the new value. And when a button has a `Pressed` component added we need to recalculate its background color.
So we trigger `DataAdded` after an insert, remove, or spawn of an entity that now matches the query, where either it didn't match before the change, or where a component it has read access to was replaced or removed, or a component it has archetypal (`Has`) access to was added or removed.
And we trigger `DataRemoving` before an insert, remove, or despawn of an entity that currently matches the query, where either it won't match after the change, or where a component it has read access to will be replaced or removed, or a component it has archetypal (`Has`) access to will be added or removed.
### How do we implement that?
The most straightforward option is to keep the infrastructure for the existing lifecycle events and translate the `DataAdded` or `DataRemoving` observers to those events. The translation can be done relatively simply using the query access, and it's no worse than what users have to do manually today.
But giving the engine more information opens up the possibility of reducing the number of triggers. We're no longer required to trigger observers when `Value` is inserted on an entity with `ExcludeFromIndex`, or when `Hovered` changes on a non-button. This opens up some options for different performance tradeoffs.
For example, we could cache the set of queries that match each archetype. That's no more space than the current cache of archetypes for each query. It may be faster to find the query observers to trigger by starting with ones that match the archetype instead of ones that match the changed components, particularly for `Disabled`, which will trigger almost every observer.
We could even go farther and cache the set of triggers for each *edge* in the archetype graph. That would increase memory usage and would be tricky to update when observers were spawned or despawned, but it would let the per-entity cost be a simple `Vec` iteration, which could be even faster than the current implementation in the hot path.
### What about the existing lifecycle events?
We won't need them anymore! `DataAdded` and `DataRemoving` can handle all the cases that the existing lifecycle events handle:
* `Add, C` is `DataAdded<(), With>`
* `Insert, C` is `DataAdded<&C>`
* `Replace, C` is `DataRemoving<&C>`
* `Remove, C` is `DataRemoving<(), With>`
Multi-component observers don't have exact matches, but could be replaced by multiple single-component observers. In practice, I expect that users of multi-component observers actually want behavior closer to `DataAdded<(), (With, With)>` or `DataAdded<(&A, &B)>` and would be simpler in this model.
So we could remove the existing lifecycle events, at least from the public API. Instead of eight different subtly-different lifecycle event types, there would just be two!
Contributor guide
Research direction
Start with the proposed DataAdded and DataRemoving observer shapes and the current lifecycle-event discussion in this issue. Read crates/bevy_feathers/src/controls/button.rs, then compare the related designs in issues #20729, #14649, and #17634. Done should mean an agreed observer design that handles query filters, component changes, and entity removal without the duplicated lifecycle-specific observers described here.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- game-dev
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100