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

[Merged by Bors] - Add get_change_ticks method to EntityRef and EntityMut #2539

Closed

Conversation

TheRawMeatball
Copy link
Member

Direct access to the change ticks is useful for integrating the reliable change detection with external stuff.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jul 25, 2021
@TheRawMeatball TheRawMeatball added A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible and removed S-Needs-Triage This issue needs to be labelled labels Jul 25, 2021
@mockersf
Copy link
Member

As this is a little advanced, some doc comments would be nice 😃

pub fn get_change_ticks<T: Component>(&self) -> Option<&'w ComponentTicks> {
// SAFE: entity location is valid and returned component is of type T
unsafe {
get_component_and_ticks_with_type(
Copy link
Member

Choose a reason for hiding this comment

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

I know you're trying to reuse code here, but we don't need to retrieve the component for this operation. This seems like the sort of small inefficiency that we'd merge and then forget about. A theoretical get_ticks and the existing get_component and get_component_with_ticks methods probably have some refactoring potential to share code. They all need to "get the entity's offset in each storage". This seems like a relatively trivial refactor for Table storage, but Sparse Set storage would probably need some new get_with_dense_index_unchecked() apis.

@TheRawMeatball
Copy link
Member Author

Closing as I no longer need this, might reopen if I do.

@TheRawMeatball TheRawMeatball reopened this Jan 2, 2022
@TheRawMeatball
Copy link
Member Author

Reopening because I need this

component_id: ComponentId,
entity: Entity,
location: EntityLocation,
) -> Option<*mut ComponentTicks> {
Copy link
Member

Choose a reason for hiding this comment

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

This should use the new pointer types that you added in #3001 now.

@@ -183,10 +183,10 @@ impl ComponentSparseSet {
}

#[inline]
pub fn get_ticks(&self, entity: Entity) -> Option<&ComponentTicks> {
pub fn get_ticks_ptr(&self, entity: Entity) -> Option<*mut ComponentTicks> {
Copy link
Member

Choose a reason for hiding this comment

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

This should use the new pointer types that you added in #3001 now.

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'm having a hard time seeing what users would do with this, but I'm open to being convinced.

In any case, now that #3001 is merged, we can use lifetimed pointers!

@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request May 2, 2022
Direct access to the change ticks is useful for integrating the reliable change detection with external stuff.
@bors bors bot changed the title Add get_change_ticks method to EntityRef and EntityMut [Merged by Bors] - Add get_change_ticks method to EntityRef and EntityMut 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
Direct access to the change ticks is useful for integrating the reliable change detection with external stuff.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
Direct access to the change ticks is useful for integrating the reliable change detection with external stuff.
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-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants