-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Minor refactors in change detection #21562
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
Minor refactors in change detection #21562
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are nice improvements! I especially like that you folded in the changed_by: MaybeLocation; it seems like whenever we want to know "when" we'll also want to know "where", and keeping them together will make it harder to miss one if we use them elsewhere in the future. (Hmm, and it's also more obvious now that there's no added_by. I wonder if we'll want that someday.)
Ticks->RefComponentTicksTicksMut->MutComponentTicksTickCells->ComponentTickCells
It feels odd that Ref and Mut are prefixes, but Cells is a suffix. I guess CellComponentTicks wouldn't be right, though. Maybe ComponentRefTicks/ComponentMutTicks/ComponentCellTicks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with this, but this needs a migration guide since we're changing pub types.
|
It looks like your PR is a breaking change, but you didn't provide a migration guide. Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes. |
My goal was to make |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed with @chescock that I would prefer the names to be consistent.
Maybe ComponentRefTicks/ComponentMutTicks/ComponentCellTicks?
or
Maybe ComponentTicksRef/ComponentTicksMut/ComponentTicksCells?
|
I'll approve once the names are consistent. I prefer the second suggestion :) |
## Objective While tinkering with change detection, I've collected some nitpicks: - The word "ticks" is kind of overloaded; ticks are used by components, systems, and the world, but "ticks" is often used to refer to the "added + changed" pair of ticks that only components have (and resources, but they're close enough to components). - `Ticks` is a nice name for a struct, but it's taken by a `pub(crate)` struct that's pretty niche. (I don't have any plans for the `Ticks` name, but it could be useful for something.) - Every use of `Ticks`, `TicksMut`, and `TickCells` is accompanied by a `MaybeLocation` that represents the calling location that last modified the component, so it should just be part of the structs. - `NonSend` seems to be in the wrong file, and doesn't implement methods with the macros that every other change-detecting query parameter uses. ## Solution Renamed the following structs: - `Ticks` -> `RefComponentTicks` - `TicksMut` -> `MutComponentTicks` - `TickCells` -> `ComponentTickCells` Added a `changed_by: MaybeLocation` field to `RefComponentTicks`, `MutComponentTicks`, and `ComponentTickCells` and folded in the loose `MaybeLocation`s. Moved `NonSend` from `system/system_param.rs` to `change_detection.rs` and updated its implementation to match similar query parameters. Removed `ComponentTickCells::read` because it is now unused (and not public).
Objective
While tinkering with change detection, I've collected some nitpicks:
Ticksis a nice name for a struct, but it's taken by apub(crate)struct that's pretty niche. (I don't have any plans for theTicksname, but it could be useful for something.)Ticks,TicksMut, andTickCellsis accompanied by aMaybeLocationthat represents the calling location that last modified the component, so it should just be part of the structs.NonSendseems to be in the wrong file, and doesn't implement methods with the macros that every other change-detecting query parameter uses.Solution
Renamed the following structs:
Ticks->RefComponentTicksTicksMut->MutComponentTicksTickCells->ComponentTickCellsAdded a
changed_by: MaybeLocationfield toRefComponentTicks,MutComponentTicks, andComponentTickCellsand folded in the looseMaybeLocations.Moved
NonSendfromsystem/system_param.rstochange_detection.rsand updated its implementation to match similar query parameters.Removed
ComponentTickCells::readbecause it is now unused (and not public).