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] - Provide public EntityRef::get_change_ticks_by_id that takes ComponentId #6683

Closed
wants to merge 5 commits into from

Conversation

sQu1rr
Copy link
Contributor

@sQu1rr sQu1rr commented Nov 18, 2022

Objective

Fixes #6682

Solution

Add EntityRef::get_change_ticks_by_id
Add EntityMut::get_change_ticks_by_id

@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 Nov 18, 2022
@alice-i-cecile alice-i-cecile added this to the 0.10 milestone Nov 18, 2022
/// use this in cases where the actual component types are not known at
/// compile time.**
#[inline]
pub fn get_change_ticks_by_id(&self, component_id: ComponentId) -> Option<&'w ComponentTicks> {
Copy link
Member

Choose a reason for hiding this comment

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

These impls aren't safe. get_ticks assumes component_id is valid. get_ticks_with_type validates this by looking up the ComponentId using the TypeId (and returning None if there is no valid mapping).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback @cart
What is the preferred course of action in this case?

I assume it is better to add the following validation, just double-checking before making another commit.

if !self.contains_id(component_id) {
    return None;
}

Alternatively, I could make the methods unsafe and delegate the safety validation to the end user.

Copy link
Member

Choose a reason for hiding this comment

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

The other *_by_id APIs are also all unsafe and require user validation. It might be best to just mark it unsafe and document the safety invariants.

Copy link
Member

Choose a reason for hiding this comment

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

Strong preference to keep these safe :) The extra expressiveness isn't useful here, just a footgun.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with the choice to validate the ComponentId and keep the method safe.
Other public by_id methods are sometimes unsafe by necessity (like OwningPtr required to match ComponentId) but limit the safety requirements to a minimum.

Internal unsafe helpers usually require component_id to be valid for performance reasons, and because the surrounding code usually has already checked their validity.

@sQu1rr
Copy link
Contributor Author

sQu1rr commented Nov 20, 2022

Updated the implementation. Now EntityRef::get_change_ticks_by_id and EntityMut::get_change_ticks_by_id
will now validate component_id and return None if it is invalid. This is in line with the get_change_ticks that does the same internally. @james7132 - if you prefer the method name to be different to avoid confusion with unsafe API, I don't mind.

Copy link
Contributor

@jakobhellermann jakobhellermann left a comment

Choose a reason for hiding this comment

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

Approved, with one suggestion to the docs.

/// use this in cases where the actual component types are not known at
/// compile time.**
#[inline]
pub fn get_change_ticks_by_id(&self, component_id: ComponentId) -> Option<&'w ComponentTicks> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with the choice to validate the ComponentId and keep the method safe.
Other public by_id methods are sometimes unsafe by necessity (like OwningPtr required to match ComponentId) but limit the safety requirements to a minimum.

Internal unsafe helpers usually require component_id to be valid for performance reasons, and because the surrounding code usually has already checked their validity.

crates/bevy_ecs/src/world/entity_ref.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/world/entity_ref.rs Outdated Show resolved Hide resolved
@sQu1rr
Copy link
Contributor Author

sQu1rr commented Nov 24, 2022

Thank you for the docs improvement suggestion, committed your adjustment

sQu1rr and others added 4 commits November 24, 2022 16:10
Also adds get_change_ticks_by_id for EntityMut
EntityRef::get_change_ticks_by_id and EntityMut::get_change_ticks_by_id
will now validate component_id and return None if it is invalid.
…ellermann from code review

Co-authored-by: Jakob Hellermann <jakob.hellermann@protonmail.com>
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Nov 24, 2022
Apply suggestions from code review by JoJoJet

Co-authored-by: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
@cart
Copy link
Member

cart commented Dec 2, 2022

bors r+

bors bot pushed a commit that referenced this pull request Dec 2, 2022
…entId` (#6683)

# Objective

Fixes #6682 

## Solution

Add `EntityRef::get_change_ticks_by_id`
Add `EntityMut::get_change_ticks_by_id`


Co-authored-by: Aleksandr Belkin <sQu1rr@users.noreply.github.com>
@bors bors bot changed the title Provide public EntityRef::get_change_ticks_by_id that takes ComponentId [Merged by Bors] - Provide public EntityRef::get_change_ticks_by_id that takes ComponentId Dec 2, 2022
@bors bors bot closed this Dec 2, 2022
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
…entId` (bevyengine#6683)

# Objective

Fixes bevyengine#6682 

## Solution

Add `EntityRef::get_change_ticks_by_id`
Add `EntityMut::get_change_ticks_by_id`


Co-authored-by: Aleksandr Belkin <sQu1rr@users.noreply.github.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…entId` (bevyengine#6683)

# Objective

Fixes bevyengine#6682 

## Solution

Add `EntityRef::get_change_ticks_by_id`
Add `EntityMut::get_change_ticks_by_id`


Co-authored-by: Aleksandr Belkin <sQu1rr@users.noreply.github.com>
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-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide public unsafe EntityRef::get_change_ticks_by_id that takes ComponentId
6 participants