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

Closed
wants to merge 5 commits into from
Closed

[Merged by Bors] - Provide public EntityRef::get_change_ticks_by_id that takes ComponentId #6683

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 simple 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
cart
cart reviewed Nov 18, 2022
View reviewed changes
crates/bevy_ecs/src/world/entity_ref.rs Outdated
/// 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 •
edited
Loading

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.

jakobhellermann
jakobhellermann approved these changes Nov 24, 2022
View reviewed changes
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.

crates/bevy_ecs/src/world/entity_ref.rs Outdated
/// 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
@sQu1rr
Adds get_change_ticks_by_id for EntityRef
0c81961 
Also adds get_change_ticks_by_id for EntityMut
@sQu1rr
EntityRef::get_change_ticks_by_id safety
e8752b4 
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.
@sQu1rr @jakobhellermann
Improve EntityRef::get_change_ticks_by_id docs as suggested by jakobh…
22d4652 
…ellermann from code review

Co-authored-by: Jakob Hellermann <jakob.hellermann@protonmail.com>
@sQu1rr
Updated EntityRef::get_change_ticks_by_id branch with upstream
85e1088
@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
@sQu1rr @JoJoJet
EntityRef:ge_change_ticks_by_id docs improvement
3853a31 
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
@sQu1rr
Provide public EntityRef::get_change_ticks_by_id that takes `Compon…
9b72780 
…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
Copy link
Contributor

bors bot commented Dec 2, 2022

Pull request successfully merged into main.

Build succeeded:

@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
@sQu1rr @alradish
Provide public EntityRef::get_change_ticks_by_id that takes `Compon…
1148e4b 
…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
@sQu1rr @ItsDoot
Provide public EntityRef::get_change_ticks_by_id that takes `Compon…
56a6b3e 
…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
Reviewers

@james7132 james7132 james7132 left review comments

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

@JoJoJet JoJoJet JoJoJet approved these changes

@jakobhellermann jakobhellermann jakobhellermann approved these changes

@maniwani maniwani Awaiting requested review from maniwani

@cart cart Awaiting requested review from cart

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use
Projects
None yet
Milestone
0.10
Development

Successfully merging this pull request may close these issues.

Provide public unsafe EntityRef::get_change_ticks_by_id that takes ComponentId
6 participants
@sQu1rr @cart @james7132 @alice-i-cecile @JoJoJet @jakobhellermann