Skip to content
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

As-needed change detection #17629

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

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 MutMarkChanges<T> instead of a Mut<T>. 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 reading type new query term old query term PR
Ptr
MutMarkChangesUntyped ➕︎
MutUntyped
&T &T &T
Ref<T> Ref<T> Ref<T>
MutMarkChanges<T> &mut T ➕︎
Mut<T> Mut<T> &mut T, Mut<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.

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)?

Draft

  • 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
  • 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-Needs-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-Needs-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.

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-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide M-Needs-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