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

Pass query change ticks to QueryParIter instead of always using change ticks from World. #8029

Merged
merged 4 commits into from
Mar 13, 2023

Conversation

chescock
Copy link
Contributor

Objective

Make Changed<T> filters work reliably with Query::par_iter_mut().

While updating my game to Bevy 0.10, I had a bug where GlobalTransform was not being updated. The problem turned out to be that I was changing Transform in a system that ran after the TransformSystem::TransformPropagate set. I would have expected that to cause a one frame delay in updates, but instead it was ignoring them completely.

When #4777 introduced QueryParIter, it consolidated code in QueryState that used world.last_change_tick() and code in Query that used self.last_change_tick. The new code always uses world.last_change_tick(). This means that any system using Query::par_iter_mut() uses the last_change_tick from the end of the previous frame, and won't see any changes made by systems that ran after it did on the previous frame.

In particular, sync_simple_transforms never sees any Transform changes made by systems that run after it.

Solution

Pass last_change_tick and change_tick from Query to QueryParIter.

@alice-i-cecile alice-i-cecile added this to the 0.10.1 milestone Mar 10, 2023
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events labels Mar 10, 2023
@chescock
Copy link
Contributor Author

Whoops, these types got changed by #7905 after my last fetch. Let me rebase and try again.

Copy link
Member

@JoJoJet JoJoJet left a comment

Choose a reason for hiding this comment

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

This looks right, but I think a simple regression test would be helpful here.

crates/bevy_ecs/src/query/state.rs Outdated Show resolved Hide resolved
@james7132 james7132 requested a review from JoJoJet March 11, 2023 19:09
Copy link
Member

@JoJoJet JoJoJet left a comment

Choose a reason for hiding this comment

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

Thank you for adding a test!

Comment on lines 763 to 764
let mut propagate_system = IntoSystem::into_system(propagate_system);
propagate_system.initialize(&mut world);
Copy link
Member

Choose a reason for hiding this comment

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

Is there any particular reason that you're manually initializing and calling these systems instead of just adding them to a Schedule?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, I just copied that approach from the previous test in that file. Should I replace it with

        let mut schedule = Schedule::new();
        schedule.add_systems((propagate_system, modify_system).chain());
        schedule.run(&mut world);
        world.clear_trackers();
        schedule.run(&mut world);
        world.clear_trackers();

?

(I thought I could put World::clear_trackers in the schedule, too, but exclusive systems can't actually update last_change_tick, and without that update the test can't catch the bug.)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think that looks better.

@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 Mar 13, 2023
@james7132 james7132 added this pull request to the merge queue Mar 13, 2023
Merged via the queue into bevyengine:main with commit a638819 Mar 13, 2023
Shfty pushed a commit to shfty-rust/bevy that referenced this pull request Mar 19, 2023
…nge ticks from `World`. (bevyengine#8029)

Co-authored-by: Chris Russell <[email protected]>
Co-authored-by: James Liu <[email protected]>
Shfty pushed a commit to shfty-rust/bevy that referenced this pull request Mar 19, 2023
…nge ticks from `World`. (bevyengine#8029)

Co-authored-by: Chris Russell <[email protected]>
Co-authored-by: James Liu <[email protected]>
@KirmesBude
Copy link
Contributor

KirmesBude commented Apr 1, 2023

@alice-i-cecile Not sure you are the right person, but this change has the 0.10.1 milestone label but does not appear to be included in that release.

@alice-i-cecile
Copy link
Member

@cart was responsible for cherrypicking from the milestone this time.

@KirmesBude
Copy link
Contributor

@cart Would this change warrant another minor release?

@mockersf
Copy link
Member

I prepared the patch for the 0.10.1, and this PR depends on #7905 which is breaking so it couldn't be cherry picked. See #8029 (comment)

JoJoJet pushed a commit to JoJoJet/bevy that referenced this pull request Apr 11, 2023
…nge ticks from `World`. (bevyengine#8029)

Co-authored-by: Chris Russell <[email protected]>
Co-authored-by: James Liu <[email protected]>
@JoJoJet
Copy link
Member

JoJoJet commented Apr 11, 2023

I've cherry picked this PR on this branch, in case we are interested in another minor version. It seems like multiple people are affected by this problem so it might be a good idea.

@cart cart removed this from the 0.10.1 milestone Apr 11, 2023
@cart cart added this to the 0.10.2 milestone Apr 11, 2023
@cart
Copy link
Member

cart commented Apr 11, 2023

There are 56 days between today and the tentative Bevy 0.11 release date. That does seem like a reasonably long time for this to be broken. I've created a new 0.10.2 milestone and added this to it. Lets take a couple more days to consider other items for the release and then we can do the cut.

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-Bug An unexpected or incorrect behavior 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
Development

Successfully merging this pull request may close these issues.

7 participants