Skip to content

As-needed change detection#17629

Closed
SpecificProtagonist wants to merge 10 commits intobevyengine:mainfrom
SpecificProtagonist:as-needed-change-detection
Closed

As-needed change detection#17629
SpecificProtagonist wants to merge 10 commits intobevyengine:mainfrom
SpecificProtagonist:as-needed-change-detection

Conversation

@SpecificProtagonist
Copy link
Contributor

@SpecificProtagonist SpecificProtagonist commented Feb 1, 2025

Objective

Make change detection optional in a runtime-configurable way.
See #4882.
For prior art, also see #6659, which statically determined whether query items would be change-detection-enabled based on the type of the component.

Solution

Querying for &mut T now gives you a Mut<T> (newly introduced type) instead of a RefMut<T> (previously called Mut). This still writes change ticks, but doesn't let you read them. Resources are unchanged.

By default, change detection is disabled for all components. It can be manually enabled via World::enable_change_detection::<T>(). Using a query with a Ref<T>, Mut<T>, Added<T> or Changed<T> term also enables change detection for T if not yet enabled, but prints a warning if entities with this component exist (as their change ticks were missed).

Updating change ticks via MutMarkChanges does not branch: When change detection for T is disabled, they're instead written to a dummy sink.

This PR doesn't turn change ticks into components of their own, which can happen in a followup if desired.

typed mutation tick read old type new type old query term new query term
Ptr Ptr
MutUntyped
MutUntyped RefMutUntyped
&T &T &T &T
Ref<T> Ref<T> Ref<T> Ref<T>
Mut<T> &mut T
Mut<T> RefMut<T> &mut T, Mut<T> RefMut<T>

Many methods that previously worked with Mut/MutUntyped now have both a version with and without read access to change ticks (this is responsible for much of the line count; if you review this PR, going commit by commit helps). There's likely some opportunity for deduplication there, but I wanted to go with the simple solution for the initial PR.

This design should be easy to expand to coarse-grained (per-table) ticks, but doesn't make multi-level or equality based change detection easier.

Status

This is in draft. I haven't pushed quite all the changes described above, but because of several ecs changes that have been merged since, it will be easier to recreate the changes than to rebase them.

Most of the diff (and growth in line count) is because many _mut methods have been split into a _mut and _mut_with_ticks variant. Can and should we prevent this duplication by making them generic over this – e.g. instead of get_mut<T: Component<Mutability = Mutable>>(… and get_mut_with_ticks<T: Component<Mutability = Mutable>>(… we would have get_mut<T: Component<Mutability = Mutable>, const TICKS: bool>(?

Besides this, there is more important work to do:

  • Performance
    • Benchmark this!
    • Does this allow autovectorization? Should the dummy sink be turned into an array of 4 or 8 or so change ticks to help with that?
    • Work on parallel iteration so multiple threads aren't fighting over ownership of the same cache line
  • Is this the user interface we want – are there footguns (e.g. with SystemRegistry), bad edge cases, or unwieldiness?
  • Add change tick access for dynamic queries & ReflectComponent
  • Consider splitting enabling of added and changed ticks
  • Consider pointer type which optionally lets you read change ticks, if present. Use this to reduce API duplication.
  • Adjust APIs that expect change tracking to always be present, write new tests, write documentation incl. migration guide

Testing

(todo)

Showcase

let id = world.register_component::<Foo>();
assert!(!world.components().get_info(id).unwrap().change_detection());

Migration Guide

(todo)

@SpecificProtagonist SpecificProtagonist added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide X-Contentious There are nontrivial implications that should be thought through D-Unsafe Touches with unsafe code in some way labels Feb 1, 2025
@alice-i-cecile alice-i-cecile added the M-Release-Note Work that should be called out in the blog due to impact label Feb 2, 2025
@alice-i-cecile
Copy link
Member

I'd also be happy to hear about better name suggestions – e.g. should Mut be renamed to something like MutWithTicks and MutMarkChanges to Mut (and which one would be easier for migration)?

I would prefer this flavor of design: the "nice" name should be the one returned by default.

@SpecificProtagonist
Copy link
Contributor Author

I'll need to do some experimenting on how to avoid API duplication and then split this into multiple PRs. Closing for now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible C-Performance A change motivated by improving speed, memory usage or compile times D-Unsafe Touches with unsafe code in some way M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide M-Release-Note Work that should be called out in the blog due to impact X-Contentious There are nontrivial implications that should be thought through

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants