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

Increase type safety and clarity for change detection #7905

Merged
merged 22 commits into from
Mar 9, 2023
Merged

Increase type safety and clarity for change detection #7905

merged 22 commits into from
Mar 9, 2023

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Mar 5, 2023 •
edited
Loading

Objective

  • We have the type Tick for dealing with change ticks, but most of the engine still just uses u32.
  • The names last_change_tick and change_tick often appear together, however the meaning of these names is unclear.

Solution

  • Use the Tick type throughout the engine.
  • Rename last_change_tick and change_tick to last_run and this_run, respectively.

Changelog

  • Change detection now uses the strongly-typed Tick instead of u32.

Migration Guide

The engine now uses the type Tick for dealing with change ticks, instead of u32. Any code that interfaced with engine internals will need to be updated, including:

  • Manual implementers of the traits SystemParam, WorldQuery, DetectChanges, and DetectChangesMut.
  • The methods World::change_tick and read_change_tick.
  • System::set_last_change_tick and get_last_change_tick. Also, these methods have been renamed to set_last_run and get_last_run, respectively.
  • The methods SystemChangeTick::change_tick and last_change_tick. These methods have been renamed to this_run and last_run, respectively.
  • The method Tick::set_changed, which has been renamed to just set.

@JoJoJet JoJoJet added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Mar 5, 2023
@james7132 james7132 requested a review from maniwani March 5, 2023 15:02
@james7132 james7132 added this to the 0.11 milestone Mar 5, 2023
alice-i-cecile
alice-i-cecile approved these changes Mar 5, 2023
View reviewed changes
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.

Rename and refactor appears to be done correctly. I'm strongly in favoring of improving type safety here.

I have some suggestions for how we can further improve encapsulation here now, but they're non-blocking.

@JoJoJet
Copy link
Member Author

JoJoJet commented Mar 5, 2023

I've encapsulated the field on Tick, which I think makes this even cleaner. I want to do a bit of file re-organization, but I'll leave it for a follow-up so the diffs aren't horrible.

@james7132 james7132 self-requested a review March 6, 2023 12:10
Carter0
Carter0 approved these changes Mar 7, 2023
View reviewed changes
Copy link
Contributor

@Carter0 Carter0 left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! I like the new names. I recall looking over this stuff earlier and being fairly confused so much appreciated.

@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 7, 2023
@JoJoJet
Copy link
Member Author

JoJoJet commented Mar 9, 2023

I have no idea why CI is failing now.

@alice-i-cecile
Copy link
Member

CI failure looks like #8002.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 9, 2023
Merged via the queue into bevyengine:main with commit 2e7b915 Mar 9, 2023
@JoJoJet JoJoJet deleted the newtype-ticks branch March 9, 2023 17:46
@chescock chescock mentioned this pull request Mar 10, 2023
Merged
Shfty pushed a commit to shfty-rust/bevy that referenced this pull request Mar 19, 2023
@JoJoJet @Shfty
Increase type safety and clarity for change detection (bevyengine#7905)
2a631dc
Shfty pushed a commit to shfty-rust/bevy that referenced this pull request Mar 19, 2023
@JoJoJet @Shfty
Increase type safety and clarity for change detection (bevyengine#7905)
596ce46
@JoJoJet JoJoJet mentioned this pull request Apr 3, 2023
Merged
james7132 pushed a commit that referenced this pull request Apr 3, 2023
@JoJoJet
Update increment_change_tick to return a strongly-typed Tick (#8295)
086db6d 
# Objective

While migrating the engine to use the `Tick` type in #7905, I forgot to
update `UnsafeWorldCell::increment_change_tick`.

## Solution

Update the function.

---

## Changelog

- The function `UnsafeWorldCell::increment_change_tick` is now
strongly-typed, returning a value of type `Tick` instead of a raw `u32`.

## Migration Guide

The function `UnsafeWorldCell::increment_change_tick` is now
strongly-typed, returning a value of type `Tick` instead of a raw `u32`.
@Shatur Shatur mentioned this pull request May 7, 2023
Closed
2 tasks
@JoJoJet JoJoJet mentioned this pull request May 28, 2023
Closed
@alice-i-cecile alice-i-cecile mentioned this pull request Nov 29, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrisjuchem chrisjuchem chrisjuchem left review comments

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@Carter0 Carter0 Carter0 approved these changes

@maniwani maniwani Awaiting requested review from maniwani

@james7132 james7132 Awaiting requested review from james7132

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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
0.11
Development

Successfully merging this pull request may close these issues.
5 participants
@JoJoJet @alice-i-cecile @chrisjuchem @Carter0 @james7132