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 source location in change detection #14034

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

Conversation

aevyrie
Copy link
Member

@aevyrie aevyrie commented Jun 26, 2024

Objective

  • Make it possible to know what changed your component or resource.
  • Common need when debugging, when you want to know the last code location that mutated a value in the ECS.
  • This feature would be very useful for the editor alongside system stepping.

Solution

  • Adds the caller location to column data.

Testing

  • The component_change_detection example now shows where the component was mutated:
2024-06-26T06:56:42.980958Z  INFO component_change_detection: Entity { index: 1, generation: 1 } changed: Ref(MyComponent(2.0)) from: "examples/ecs/component_change_detection.rs:34:23"

Changelog

  • Added source locations to ECS change detection.

Migration Guide

  • Added caller field to many internal ECS functions used with change detection. Use Location::caller() to provide the source of the function call.

@aevyrie aevyrie added C-Enhancement A new feature A-ECS Entities, components, systems, and events labels Jun 26, 2024
@aevyrie aevyrie force-pushed the changed-by branch 4 times, most recently from d3242ff to 9ac216d Compare June 26, 2024 08:12
@cart
Copy link
Member

cart commented Jun 26, 2024

Mut is one of the hottest paths in Bevy. I'm concerned about the performance implications of introducing anything (even the cheapest ops) to it. I'd like to either see compelling evidence that this won't meaningfully affect performance, especially given that this is a "debugging tool". I'd feel much more comfortable if this was behind a feature flag.

@alice-i-cecile alice-i-cecile added the X-Contentious There are nontrivial implications that should be thought through label Jun 26, 2024
@alice-i-cecile
Copy link
Member

I really like this feature and think it'll be very useful, but I'll second the request for putting it behind an off-by-default feature flag.

@alice-i-cecile alice-i-cecile added the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Jun 26, 2024
@aevyrie
Copy link
Member Author

aevyrie commented Jun 26, 2024

Yup, I assumed as much. I was hoping to get a read on whether this is even an acceptable approach to take for the feature. Before going down the path of adding feature flags.

@Brezak
Copy link
Contributor

Brezak commented Jun 26, 2024

Is there a reason you're de referencing the static reference you get from Location::caller? Couldn't you just pass the reference around?

@BD103
Copy link
Member

BD103 commented Jun 27, 2024

Is there a reason you're de referencing the static reference you get from Location::caller? Couldn't you just pass the reference around?

Location is two u32s and one &str, making it 16 bytes on 64-bit platforms. Storing a reference instead would bring it down to 8 bytes, which might have some mild performance gains because it would fit in a register.

Furthermore, why do you use core::panic::Location as compared with the std variant?

@aevyrie
Copy link
Member Author

aevyrie commented Jun 28, 2024

  • static reference: I started with this, but was having trouble with the deref making it hard to update the field, so I switched to an owned value to get it working. I agree it should be fixed if this is going to be merged.
  • using core: because it can't hurt? I wasn't sure if bevy_ecs was ever intended to be used in a no_std context, and figured it would be safer to use core for a type so ubiquitous.

The answer to most questions is probably going to be "because this is a proof of concept". 😄
I wasn't sure if this was going to be possible or if it would return any useful information, so I'm mostly just happy to see that it's doable. The useful findings from this are:

  1. It's possible to get line and column level tracing for change detection, which is super useful
  2. Changes from commands completely break this approach because commands are deferred, and the call stack can't be traced to the calling location by the time the command is actually executed.

For 2, it seems like it should be possible to get information from tracing to figure out what system the commands are being executed for. Not quite sure how to do that though, might need to replace Location with a new struct that also contains the Location as well as the current system span.

@BD103
Copy link
Member

BD103 commented Jun 28, 2024

2. Changes from commands completely break this approach because commands are deferred, and the call stack can't be traced to the calling location by the time the command is actually executed.

Is it possible to find the Location in Commands::insert, then pass that forward to the actual command?

@alice-i-cecile
Copy link
Member

using core: because it can't hurt? I wasn't sure if bevy_ecs was ever intended to be used in a no_std context, and figured it would be safer to use core for a type so ubiquitous.

I would like this, one day :) Probably core + alloc, but still! Not a priority, but might as well do it when there's no cost to it.

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-Enhancement A new feature S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants