-
Notifications
You must be signed in to change notification settings - Fork 66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Removed query filter #44
base: main
Are you sure you want to change the base?
Conversation
Not<With> != Without due to archetype filtering
Idea credit to @TheRawMeatball
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks like a very useful addition considering the cheat book had to add a dedicated page just for this.
But the PR looks stale; could you please update @alice-i-cecile to take into account recent changes like the mandatory #[derive(Component)]
? Thanks!
|
||
## Drawbacks | ||
|
||
1. Users must opt-in to removal detection. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still true now that all components require #[derive(Component)]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes: the design here is that the performance cost is too high to enable by default.
It was actually written with |
|
||
Removal detection is a powerful tool for ensuring the validity of game state, but the existing `RemovedComponents` system parameter is unwieldy (see the motivating [bevy #2148](https://github.com/bevyengine/bevy/issues/2148)). | ||
|
||
It cannot be easily composed with other queries, has an unexpected API, and only persists for two frames, unlike reliable change detection. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should note that it doesn't actually persist for two frames which makes it more unwieldy.
One more question this RFC should address is, "What happens to Also, I'm thinking there should be a way to rig this internally so that the options are:
Also if the strategy stays as described, I think "no removal detection" should be default if you can't auto-detect it from queries, since |
# Objective Removal events are unwieldy and require some knowledge of when to put systems that need to catch events for them, it is very easy to end up missing one and end up with memory leak-ish issues where you don't clean up after yourself. ## Solution Consolidate removals with the benefits of `Events<...>` (such as double buffering and per system ticks for reading the events) and reduce the special casing of it, ideally I was hoping to move the removals to a `Resource` in the world, but that seems a bit more rough to implement/maintain because of double mutable borrowing issues. This doesn't go the full length of change detection esque removal detection a la bevyengine/rfcs#44. Just tries to make the current workflow a bit more user friendly so detecting removals isn't such a scheduling nightmare. --- ## Changelog - RemovedComponents<T> is now backed by an `Events<Entity>` for the benefits of double buffering. ## Migration Guide - Add a `mut` for `removed: RemovedComponents<T>` since we are now modifying an event reader internally. - Iterating over removed components now requires `&mut removed_components` or `removed_components.iter()` instead of `&removed_components`.
# Objective Removal events are unwieldy and require some knowledge of when to put systems that need to catch events for them, it is very easy to end up missing one and end up with memory leak-ish issues where you don't clean up after yourself. ## Solution Consolidate removals with the benefits of `Events<...>` (such as double buffering and per system ticks for reading the events) and reduce the special casing of it, ideally I was hoping to move the removals to a `Resource` in the world, but that seems a bit more rough to implement/maintain because of double mutable borrowing issues. This doesn't go the full length of change detection esque removal detection a la bevyengine/rfcs#44. Just tries to make the current workflow a bit more user friendly so detecting removals isn't such a scheduling nightmare. --- ## Changelog - RemovedComponents<T> is now backed by an `Events<Entity>` for the benefits of double buffering. ## Migration Guide - Add a `mut` for `removed: RemovedComponents<T>` since we are now modifying an event reader internally. - Iterating over removed components now requires `&mut removed_components` or `removed_components.iter()` instead of `&removed_components`.
RENDERED
Addresses bevyengine/bevy#2148 by proposing a
Removed<C>
query filter.This works exactly as you might hope, using reliable change detection semantics and mechanisms.
Full credit to @maniwani (Joy) for the core idea of just storing another component with the change tick.
Some notes:
RemovedComponents
, since this doesn't work for despawned entities, since they no longer exist to be queried for.#[derive(Component)]
for component types #27, to avoid pointless performance overhead.