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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Improve debugging tools for change detection #4160

Closed
wants to merge 5 commits into from
Closed

[Merged by Bors] - Improve debugging tools for change detection #4160

wants to merge 5 commits into from

Conversation

alice-i-cecile
Copy link
Member

@alice-i-cecile alice-i-cecile commented Mar 8, 2022 •
edited
Loading

Objective

  1. Previously, the change_tick and last_change_tick fields on SystemChangeTick were pub.
    1. This was actively misleading, as while this can be fetched as a SystemParam, a copy is returned instead
  2. This information could be useful for debugging, but there was no way to investigate when data was changed.
  3. There were no docs!

Solution

  1. Move these to a getter method.
  2. Add last_changed method to the DetectChanges trait to enable inspection of when data was last changed.
  3. Add docs.

Changelog

SystemChangeTick now provides getter methods for the current and previous change tick, rather than public fields.
This can be combined with DetectChanges::last_changed() to debug the timing of changes.

Migration guide

The change_tick and last_change_tick fields on SystemChangeTick are now private, use the corresponding getter method instead.

@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 labels Mar 8, 2022
@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 8, 2022
@alice-i-cecile alice-i-cecile added C-Docs An addition or correction to our documentation and removed S-Needs-Triage This issue needs to be labelled labels Mar 8, 2022
maniwani
maniwani reviewed Mar 9, 2022
View reviewed changes
Copy link
Contributor

@maniwani maniwani left a comment

Choose a reason for hiding this comment

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

LGTM, just some nits on the wording in the docs.

crates/bevy_ecs/src/system/system_param.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/system_param.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/system_param.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/change_detection.rs Outdated Show resolved Hide resolved
@alice-i-cecile
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request Mar 9, 2022
@bors
Try #4160:
5407a6f
@bors
Copy link
Contributor

bors bot commented Mar 9, 2022

try

Build succeeded:

mockersf
mockersf reviewed Mar 9, 2022
View reviewed changes
crates/bevy_ecs/src/system/system_param.rs
Comment on lines +1160 to +1171
/// - `last_change_tick` copies the previous value of `change_tick`
/// - `change_tick` copies the current value of [`World::read_change_tick`]
Copy link
Member

Choose a reason for hiding this comment

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

As you're making those fields private, it may be a little confusing to read in docs.rs where they will be hidden

Copy link
Member Author

Choose a reason for hiding this comment

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

So, my thought was that users should be able to piece together what's going on because we expose methods of the same name.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 9, 2022
@alice-i-cecile alice-i-cecile force-pushed the change-ticks-getters branch from e640022 to ce77cae Compare May 2, 2022 16:02
@alice-i-cecile
Copy link
Member Author

bors r+

bors bot pushed a commit that referenced this pull request May 2, 2022
@alice-i-cecile
Improve debugging tools for change detection (#4160)
b6519af 
# Objective

1. Previously, the `change_tick` and `last_change_tick` fields on `SystemChangeTick` [were `pub`](https://docs.rs/bevy/0.6.1/bevy/ecs/system/struct.SystemChangeTick.html).
   1.  This was actively misleading, as while this can be fetched as a `SystemParam`, a copy is returned instead
2. This information could be useful for debugging, but there was no way to investigate when data was changed.
3. There were no docs!

## Solution

1. Move these to a getter method.
2. Add `last_changed` method to the `DetectChanges` trait to enable inspection of when data was last changed.
3. Add docs.

# Changelog

 `SystemChangeTick` now provides getter methods for the current and previous change tick, rather than public fields.
 This can be combined with `DetectChanges::last_changed()` to debug the timing of changes.

# Migration guide

The `change_tick` and `last_change_tick` fields on `SystemChangeTick` are now private, use the corresponding getter method instead.
@bors
Copy link
Contributor

bors bot commented May 2, 2022

This PR was included in a batch that timed out, it will be automatically retried

bors bot pushed a commit that referenced this pull request May 2, 2022
@alice-i-cecile
Improve debugging tools for change detection (#4160)
3fbe368 
# Objective

1. Previously, the `change_tick` and `last_change_tick` fields on `SystemChangeTick` [were `pub`](https://docs.rs/bevy/0.6.1/bevy/ecs/system/struct.SystemChangeTick.html).
   1.  This was actively misleading, as while this can be fetched as a `SystemParam`, a copy is returned instead
2. This information could be useful for debugging, but there was no way to investigate when data was changed.
3. There were no docs!

## Solution

1. Move these to a getter method.
2. Add `last_changed` method to the `DetectChanges` trait to enable inspection of when data was last changed.
3. Add docs.

# Changelog

 `SystemChangeTick` now provides getter methods for the current and previous change tick, rather than public fields.
 This can be combined with `DetectChanges::last_changed()` to debug the timing of changes.

# Migration guide

The `change_tick` and `last_change_tick` fields on `SystemChangeTick` are now private, use the corresponding getter method instead.
@bors
Copy link
Contributor

bors bot commented May 2, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Improve debugging tools for change detection [Merged by Bors] - Improve debugging tools for change detection May 2, 2022
@bors bors bot closed this May 2, 2022
exjam pushed a commit to exjam/bevy that referenced this pull request May 22, 2022
@alice-i-cecile @exjam
Improve debugging tools for change detection (bevyengine#4160)
25df2ac 
# Objective

1. Previously, the `change_tick` and `last_change_tick` fields on `SystemChangeTick` [were `pub`](https://docs.rs/bevy/0.6.1/bevy/ecs/system/struct.SystemChangeTick.html).
   1.  This was actively misleading, as while this can be fetched as a `SystemParam`, a copy is returned instead
2. This information could be useful for debugging, but there was no way to investigate when data was changed.
3. There were no docs!

## Solution

1. Move these to a getter method.
2. Add `last_changed` method to the `DetectChanges` trait to enable inspection of when data was last changed.
3. Add docs.

# Changelog

 `SystemChangeTick` now provides getter methods for the current and previous change tick, rather than public fields.
 This can be combined with `DetectChanges::last_changed()` to debug the timing of changes.

# Migration guide

The `change_tick` and `last_change_tick` fields on `SystemChangeTick` are now private, use the corresponding getter method instead.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@alice-i-cecile @ItsDoot
Improve debugging tools for change detection (bevyengine#4160)
d3f5d30 
# Objective

1. Previously, the `change_tick` and `last_change_tick` fields on `SystemChangeTick` [were `pub`](https://docs.rs/bevy/0.6.1/bevy/ecs/system/struct.SystemChangeTick.html).
   1.  This was actively misleading, as while this can be fetched as a `SystemParam`, a copy is returned instead
2. This information could be useful for debugging, but there was no way to investigate when data was changed.
3. There were no docs!

## Solution

1. Move these to a getter method.
2. Add `last_changed` method to the `DetectChanges` trait to enable inspection of when data was last changed.
3. Add docs.

# Changelog

 `SystemChangeTick` now provides getter methods for the current and previous change tick, rather than public fields.
 This can be combined with `DetectChanges::last_changed()` to debug the timing of changes.

# Migration guide

The `change_tick` and `last_change_tick` fields on `SystemChangeTick` are now private, use the corresponding getter method instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cart cart cart left review comments

@maniwani maniwani maniwani approved these changes

@mockersf mockersf mockersf approved these changes

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Docs An addition or correction to our documentation C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alice-i-cecile @cart @mockersf @maniwani