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

Track callsite for observers & hooks #15607

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

SpecificProtagonist
Copy link
Contributor

@SpecificProtagonist SpecificProtagonist commented Oct 3, 2024

Objective

Fixes #14708

Also fixes try_insert_if_new & dynamic component insertions per command not updating tracked location

Solution

ObserverTrigger has a new caller field with the track_change_detection feature;
hooks take an additional caller parameter (which is Some(…) or None depending on the feature).

Testing

See the new tests in src/observer/mod.rs


Showcase

Observers now know from where they were triggered (if track_change_detection is enabled):

world.observe(move |trigger: Trigger<OnAdd, Foo>| {
    println!("Added Foo from {}", trigger.trigger.caller);
});

Migration

  • hooks now take an additional Option<&'static Location> argument

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 3, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this strategy, but I really want to see benchmarks before we remove the feature flag on BundleInserter.

@alice-i-cecile alice-i-cecile added the S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help label Oct 3, 2024
@SpecificProtagonist SpecificProtagonist changed the title Track callsite for observers Track callsite for observers & hooks Oct 3, 2024
@SpecificProtagonist
Copy link
Contributor Author

SpecificProtagonist commented Oct 3, 2024

I tried running the various add_remove & insert_simple benches, but I need to figure out how to turn of frequency scaling for my laptop first (can't get consistent enough results).

Should this ends up having to sit behind a feature flag, would this be a new feature flag or the (possibly renamed) track_change_detection? And should hooks then take an Option<&Location>?

@BenjaminBrienen BenjaminBrienen added the D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes label Oct 13, 2024
@BenjaminBrienen BenjaminBrienen added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 31, 2024
@NthTensor NthTensor mentioned this pull request Dec 11, 2024
6 tasks
@NthTensor
Copy link
Contributor

Personally, I'd rename track_change_detection and gate all tracking behind it.

For hooks, that's a bit harder. Ideally an optional Option<&Location> parameter, so that people don't have to litter their code with cfg attributes.

@SpecificProtagonist
Copy link
Contributor Author

Personally, I'd rename track_change_detection and gate all tracking behind it.

For hooks, that's a bit harder. Ideally an optional Option<&Location> parameter, so that people don't have to litter their code with cfg attributes.

I think that would be the best, will do. Can you add the rename to your meta issue?

@NthTensor NthTensor self-requested a review December 12, 2024 16:54
@NthTensor NthTensor added this to the 0.16 milestone Dec 12, 2024
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-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Track callsites of where observers are initially triggered.
4 participants