Column change detection for immutable components #18788
Open
+371
−14
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
alice-i-cecile
added
C-Feature
|
It looks like your PR has been selected for a highlight in the next release blog post, but you didn't provide a release note. Please review the instructions for writing release notes, then expand or revise the content in the release notes directory to showcase your changes. |
|
|
|
Benchmark for |
notmd
changed the title
kristoff3r
added
S-Needs-Review
| @@ -20,6 +20,7 @@ pub struct ThinColumn { | |||
| pub(super) added_ticks: ThinArrayPtr<UnsafeCell<Tick>>, | |||
| pub(super) changed_ticks: ThinArrayPtr<UnsafeCell<Tick>>, | |||
| pub(super) changed_by: MaybeLocation<ThinArrayPtr<UnsafeCell<&'static Location<'static>>>>, | |||
| change_tick: Tick, | |||
There was a problem hiding this comment.
Maybe we can add a comment here? This is the change_tick associated with the most recent change in the table (i.e. of any row in the table)
| @@ -170,18 +173,19 @@ impl ThinColumn { | |||
| &mut self, | |||
| row: TableRow, | |||
| data: OwningPtr<'_>, | |||
| change_tick: Tick, | |||
| tick: Tick, | |||
There was a problem hiding this comment.
I think keeping this as change_tick was clearer
| @@ -227,6 +232,7 @@ impl ThinColumn { | |||
| self_changed_by.initialize_unchecked(dst_row.as_usize(), changed_by); | |||
| }, | |||
| ); | |||
| self.change_tick = tick; | |||
There was a problem hiding this comment.
The other values (added_ticks/change_ticks) were taken from the other ThinColumn, should we do the same for the Table-wide ChangeTick?
Or is this required for the correctness of archetype_filter_fetch?
There was a problem hiding this comment.
I feel like it might make the most sense to make both columns take the most recent of their change ticks. But IDK
There was a problem hiding this comment.
Wdym table-wide ChangeTick? Currently, change tick only updated when an element is inserted or replaced. This is to reduce false positive of archetype_filter_fetch for Added and Changed. It's can be updated in more cases but not needed for now.
| @@ -105,6 +105,18 @@ pub unsafe trait QueryFilter: WorldQuery { | |||
| entity: Entity, | |||
| table_row: TableRow, | |||
| ) -> bool; | |||
|
|
|||
| /// # Safety | |||
There was a problem hiding this comment.
I think we need extra comments to explain what this does.
Basically this is a way to filter/skip an entire table/archetype instead of an individual Entity or TableRow.
I would add something similar to the comment of filter_fetch and explain that currently this is mostly used as an optimization for non-archetypal filters such as Changed/Added
There was a problem hiding this comment.
I agree another comment would be good here. But this is brilliant btw.
|
The benchmarks you linked in the PR description are only for immutable components, right? There's an extra function call in every query-iteration now |
alice-i-cecile
added
X-Controversial
I tried to run benchmark but it very noisy. The only overhead is size of |
ElliottjPierce
approved these changes
May 9, 2025
| @@ -227,6 +232,7 @@ impl ThinColumn { | |||
| self_changed_by.initialize_unchecked(dst_row.as_usize(), changed_by); | |||
| }, | |||
| ); | |||
| self.change_tick = tick; | |||
There was a problem hiding this comment.
I feel like it might make the most sense to make both columns take the most recent of their change ticks. But IDK
| @@ -105,6 +105,18 @@ pub unsafe trait QueryFilter: WorldQuery { | |||
| entity: Entity, | |||
| table_row: TableRow, | |||
| ) -> bool; | |||
|
|
|||
| /// # Safety | |||
There was a problem hiding this comment.
I agree another comment would be good here. But this is brilliant btw.
|
Here is the benchmark. The result is kinda weird. Click me |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
Query<(), Changed<SomeImmutableComponent>>Solution
Column/ThinColumn.Change tick will be updated when inserting, replacing or removing elements. Updating when removing seems unnecessary because change detection doesn't detect removal. I am still updating it because I want to use this somehow to help implement. Change tick will be update when inserting or replacing elements.query.is_changed(). We may consider introducing a separate tick for removal in the future.Testing
archetype_filter_fetchQuery<Entity, Changed<T>>. You can reproduce this by runningcargo bench -p benches --bench ecs -- --save-baseline current none_changed_detectionandcritcmp benches current -g '^(none_changed_detection/\d+\w+::\w+::[A-Z][a-z]+)'Future work
changed_ticksfor immutable component. Because it can't be mutated, useadded_ticksfor is enough.