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] - Replace World::read_change_ticks with World::change_ticks within bevy_ecs crate #6816

Closed
wants to merge 5 commits into from
Closed

[Merged by Bors] - Replace World::read_change_ticks with World::change_ticks within bevy_ecs crate #6816

wants to merge 5 commits into from

Conversation

mnmaita
Copy link
Member

@mnmaita mnmaita commented Dec 1, 2022

Objective

Solution

  • Replaced World::read_change_ticks with World::change_ticks within bevy_ecs crate in places where World references were mutable.

@mnmaita
Copy link
Member Author

mnmaita commented Dec 1, 2022 •
edited
Loading

Two questions remain:

  1. Should I move the cached World::change_ticks values outside of unsafe blocks?
  2. Should I change get_unchecked_mut signature to also apply this change on said method? See this comment for reference.

@james7132
Copy link
Member

james7132 commented Dec 1, 2022 •
edited
Loading

  1. Should I move the cached World::change_ticks values outside of unsafe blocks?

This is up to you. It doesn't need unsafe to operate, and minimizing the size of those blocks is good practice, but it's not necessary.

  1. Should I change get_unchecked_mut signature to also apply this change on said method? See this comment for reference.

Get get_unchecked_mut's public signature should not change. That one needs to use a non-mut World reference. That'd also be a breaking change.

@james7132 james7132 added A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times labels Dec 1, 2022
@mnmaita mnmaita force-pushed the mnmaita/replace-read-change-ticks branch from b373c73 to 5409fe3 Compare December 1, 2022 22:57
mnmaita
mnmaita commented Dec 1, 2022
View reviewed changes
crates/bevy_ecs/src/query/state.rs
@@ -526,9 +516,10 @@ impl<Q: WorldQuery, F: ReadOnlyWorldQuery> QueryState<Q, F> {
#[inline]
pub fn iter_mut<'w, 's>(&'s mut self, world: &'w mut World) -> QueryIter<'w, 's, Q, F> {
// SAFETY: query has unique world access
let change_tick = world.change_tick();
unsafe {
self.update_archetypes(world);
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not familiar with the code at all but I noticed this method isn't unsafe. Is there a reason to call it inside the unsafe block then?

Copy link
Member

Choose a reason for hiding this comment

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

It's not unsafe and it could be moved out of the unsafe block. Probably not something to be concerned about for this PR though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds sensible. Should I open a new issue to address this?

Copy link
Member

Choose a reason for hiding this comment

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

No, it's not required, and the code quality benefit isn't too big.

@mnmaita mnmaita force-pushed the mnmaita/replace-read-change-ticks branch from 5409fe3 to 7d54c34 Compare December 1, 2022 23:01
@mnmaita
Copy link
Member Author

mnmaita commented Dec 1, 2022

@james7132 I just moved the variables out of the unsafe blocks and left a question in a comment. LMK if it still looks good! Thanks.

Carter0
Carter0 approved these changes Dec 3, 2022
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.

LGTM

@james7132 james7132 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 Dec 3, 2022
@james7132 james7132 added this to the 0.10 milestone Dec 4, 2022
@cart
Copy link
Member

cart commented Dec 5, 2022

bors r+

bors bot pushed a commit that referenced this pull request Dec 5, 2022
@mnmaita
Replace World::read_change_ticks with World::change_ticks within …
eff632d 
…`bevy_ecs` crate (#6816)

# Objective

- Fixes #6812.

## Solution

- Replaced `World::read_change_ticks` with `World::change_ticks` within `bevy_ecs` crate in places where `World` references were mutable.

---
@bors
Copy link
Contributor

bors bot commented Dec 5, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Replace World::read_change_ticks with World::change_ticks within bevy_ecs crate [Merged by Bors] - Replace World::read_change_ticks with World::change_ticks within bevy_ecs crate Dec 5, 2022
@bors bors bot closed this Dec 5, 2022
ickshonpe pushed a commit to ickshonpe/bevy that referenced this pull request Dec 6, 2022
@mnmaita @ickshonpe
Replace World::read_change_ticks with World::change_ticks within …
be4a908 
…`bevy_ecs` crate (bevyengine#6816)

# Objective

- Fixes bevyengine#6812.

## Solution

- Replaced `World::read_change_ticks` with `World::change_ticks` within `bevy_ecs` crate in places where `World` references were mutable.

---
@mnmaita mnmaita deleted the mnmaita/replace-read-change-ticks branch December 11, 2022 15:32
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
@mnmaita @alradish
Replace World::read_change_ticks with World::change_ticks within …
0eb5d15 
…`bevy_ecs` crate (bevyengine#6816)

# Objective

- Fixes bevyengine#6812.

## Solution

- Replaced `World::read_change_ticks` with `World::change_ticks` within `bevy_ecs` crate in places where `World` references were mutable.

---
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@mnmaita @ItsDoot
Replace World::read_change_ticks with World::change_ticks within …
97e6dbb 
…`bevy_ecs` crate (bevyengine#6816)

# Objective

- Fixes bevyengine#6812.

## Solution

- Replaced `World::read_change_ticks` with `World::change_ticks` within `bevy_ecs` crate in places where `World` references were mutable.

---
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@james7132 james7132 james7132 approved these changes

@Carter0 Carter0 Carter0 approved these changes

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times 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.10
Development

Successfully merging this pull request may close these issues.

Unnecessary internal use of World::read_change_ticks
4 participants
@mnmaita @james7132 @cart @Carter0