[Merged by Bors] - Implement iter() for mutable Queries #2305

Guvante · 2021-06-04T02:49:38Z

A sample implementation of how to have iter() work on mutable queries without breaking aliasing rules.

Objective

Fixes Allow immutable iter on Queries which contain mutable references #753

Solution

Added a ReadOnlyFetch to WorldQuery that is the &T version of &mut T that is used to specify the return type for read only operations like iter().
~~As the comment suggests specifying the bound doesn't work due to restrictions on defining recursive implementations (like Or). However bounds on the functions are fine~~ Never mind I misread how Or was constructed, bounds now exist.
Note that the only mutable one has a new Fetch for readonly as the State has to be the same for any of this to work

alice-i-cecile · 2021-06-04T02:51:31Z

bors try

Guvante · 2021-06-04T02:51:32Z

Note that I believe #1301 is calling for iter() to work in place of iter_mut() which is different than the way this PR handles things.

bors · 2021-06-04T02:52:56Z

try

Build failed:

build (stable, ubuntu-latest)

Guvante · 2021-06-04T03:42:21Z

Thought this was going to be a normal PR since GitHub was being odd with the button...

Left as draft due to:

Not liking QF (QueryFetch) as a name where F (Filter) is in scope and a different name.
Unsure how to handle the private member functions that are now generic. In the interest of minimizing changes I swapped them to Q::Fetch or Q::ReadOnlyFetch depending on whether ReadOnlyFetch was a constraint on Q::Fetch.
~~I left ReadOnlyFetch which seems unnecessary.~~
Most importantly I didn't touch anything other than what was necessary to relax the Q::Fetch: ReadOnlyFetch constraint on iter() and I don't think that is the only thing that could use a change like this if it is useful.

mockersf · 2021-06-04T07:26:14Z

I'm not sure this is something we want, as it would encourage using a mutable query when an immutable would work, and mutable have a heavier cost for system parallelisation. Having to use iter_mut works a second level of "are you sure?" when you don't want to mutate something

NathanSWard · 2021-06-05T05:42:54Z

crates/bevy_ecs/src/system/query.rs

-        // TODO: This code can be replaced with `self.iter().next().is_none()` if/when
-        // we sort out how to convert "write" queries to "read" queries.
-        self.state
-            .is_empty(self.world, self.last_change_tick, self.change_tick)
+        self.iter().next().is_none()


If this PR lands, I'm curious if we actually want to replace the implementation of is_empty with this.
Now that I think of it, having a specialized is_empty functionality actually gives us a small performance win (even if it is at the cost of extra code duplication). Since in the implementation we never have to touch self.fetch so we bypass that extra unnecessary code path.

Where as in here, we'll have to follow the normal code path and do a bit of extra work.

I refactored this to use a NopFetch to get the performance benefit without the code duplication. How does that look?

crates/bevy_ecs/src/query/fetch.rs

Guvante · 2021-06-11T06:13:28Z

Went and applied the refactor to all of the existing locations. Now the only reference to ReadOnlyFetch the trait is in WorldQuery and the many impls.

I tried to transform the non-pub "_manual" methods to take generics so the API isn't impacted beyond the names of types that now need to know which of the two WorldQuery::Fetch you are using.

Guvante · 2021-06-11T06:16:40Z

crates/bevy_ecs/src/query/iter.rs

    filter: F::Fetch,
    current_len: usize,
    current_index: usize,
    is_dense: bool,
+    phantom: PhantomData<&'w Q>,


I believe that QF: Fetch<'s> and dropping the Q and 'w would work for this, as I believe the lifetime parameter on Fetch is "lasts at least as long" but I wasn't 100% sure so went with a more obviously "correct" solution of including them with a PhantomData.

alice-i-cecile · 2021-06-16T08:30:47Z

bors try

bors · 2021-06-16T08:49:52Z

try

Build succeeded:

Guvante · 2021-07-23T03:29:01Z

Let me know if it needs to be repeated on the other issue but:

I license past and future contributions under the dual MIT/Apache-2.0 license, allowing licensees to choose either at their option.

alice-i-cecile · 2021-08-16T15:28:08Z

Let me know if it needs to be repeated on the other issue but:

I license past and future contributions under the dual MIT/Apache-2.0 license, allowing licensees to choose either at their option.

@DJMcNab IIRC we wanted this all in one thread? This should suffice, but I'll leave it up to your judgement there :)

DJMcNab · 2021-08-16T15:30:01Z

@cart was relatively clear that we should have them in #2373 as a policy

That being said, if we can't reach this user again then this would be fine.

alice-i-cecile

This looks fantastic; that's a nice batch of changes! I've done a thorough review of the code, but I could not spot any particular nits to pick.

It may be nice to have a test to verify that this new functionality continues to work, but I'll leave that to your judgement.

Guvante · 2021-08-16T15:41:30Z

Commented on linked issue as well. I can throw together a unit test that verified iter() is a view on the object returned by iter_mut(). If you want to double check that no pub things expose QF that would be great as while I could see exposing that functionality I did not want to do that here.

crates/bevy_ecs/src/query/iter.rs

alice-i-cecile · 2021-08-16T15:54:11Z

If you want to double check that no pub things expose QF that would be great as while I could see exposing that functionality I did not want to do that here.

Looks like it's exposed in QueryIter and QueryCombinationIter. I don't immediately see a way to avoid that; why would you want to keep this fully private?

Guvante · 2021-08-16T16:20:35Z

I am worried about a user constructing with a custom QF, I don't care that users can know something is generic over a QF. Both the QueryIter and QueryCombinationIter have a pub(crate) constructor (new function? not sure the terminology there).

I haven't done the mental work to decide what it means to have a user supply a custom QF to something. Mostly because I would need to figure out what if anything now needs to be marked unsafe. In theory the need for a matching QueryState as well as ReadOnlyFetch being unsafe eliminates most problems but I didn't want to expand the public API options without verifying that is enough.

alice-i-cecile

I've given this another look over and it looks ready to merge. Very nice usability / correctness win, and the docs and tests are solid.

One minor nit, but feel free to merge without it being addressed.

alice-i-cecile · 2021-10-04T01:14:26Z

crates/bevy_ecs/src/system/query.rs

@@ -973,8 +962,6 @@ where
    /// ```
    #[inline]
    pub fn is_empty(&self) -> bool {
-        // TODO: This code can be replaced with `self.iter().next().is_none()` if/when
-        // we sort out how to convert "write" queries to "read" queries.
        self.state


We should replace this with the code in the comment now that we've figured out that conversion. I expect it will be both cleaner and marginally faster.

QueryState::is_empty existed and got updated so I left this as a pass through to it to avoid changing any pub fn

Also the implementation isn't quite what is described in the comment as the actual implementation uses a custom Fetch to avoid touching any of the data much like the custom is_empty did (NopFetch)

Sounds good to me.

deontologician · 2021-10-16T19:25:14Z

Any ETA on merging this?

alice-i-cecile · 2021-10-16T19:28:04Z

Any ETA on merging this?

We could use another review or two; if you're comfortable evaluating it feedback would be helpful @deontologician.

Otherwise, it should be one of the top priorities to be merged once Cart peels away from the focused rendering work.

deontologician · 2021-10-18T22:38:27Z

So, I don't have an informed opinion on whether this PR is the right fix for bevy architecturally etc, but I did manage to compile this PR of fixes for bevy_rapier by @pcwalton:
dimforge/bevy_rapier#98
against this branch and it seems to work.

I had to add some things to deal with the new Component requirement since that popped up since the original bevy_rapier fix branch was made, but I can say that it seems like this PR fixes the issues bevy_rapier had compiling against bevy/main

The biggest change here is that doing the same query twice, one mutably and one immutably, is no longer needed, once bevyengine/bevy#2305 lands. Before that lands, however, this upgrade is actually impossible to do safely, since the `q0`/`q0_mut` distinction is gone and therefore it's impossible to implement the Rapier component traits on Bevy queries. (This is filed upstream as bevyengine/bevy#2744.) Therefore, this upgrade has to wait until that Bevy PR lands. Other minor changes: * `Light` has been renamed to `PointLight`. * `App::build()` is now `App::new()`, and `AppBuilder` is now `App`. * `glam` has been upgraded upstream.

- Requires that all queries specify an immutable Fetch with identical State - Added a unit test to verify that calling iter vs iter_mut returns the same things - Added a second unit test to verify that calling iter_mut borrows correctly

Guvante

Noticed a conflict which was easy to resolve (if monotonous) however upon building the new ExtractComponent is super unhappy with how I broke QueryItem<T>...

I really wish there was a way to guarentee that when Query: ReadOnlyQuery then the two types are the same but I am almost certain that is an impossible ask.

I can totally ensure that their Item values are the same (as I did with State just weirder bounds) but that doesn't work due to &T vs Mut<T> for WriteQuery...

crates/bevy_ecs/src/query/fetch.rs

pipelined/bevy_render2/src/render_component.rs

cart

This generally looks good to me. Impressive first PR! The extra QF type complexity in our internal apis is a bit painful, but I can't think of alternatives that don't involve code duplication. And QF does give us additional flexibility, which has already yielded fruit (ex: the NopFetch impl). We might even find more creative uses of this pattern in the future. I've resolved a couple of my comments already, which I'll push after leaving this review.

crates/bevy_ecs/src/query/filter.rs

crates/bevy_ecs/src/query/state.rs

crates/bevy_ecs/src/query/fetch.rs

crates/bevy_ecs/src/system/mod.rs

cart · 2021-12-01T23:24:50Z

bors r+

A sample implementation of how to have `iter()` work on mutable queries without breaking aliasing rules. # Objective - Fixes #753 ## Solution - Added a ReadOnlyFetch to WorldQuery that is the `&T` version of `&mut T` that is used to specify the return type for read only operations like `iter()`. - ~~As the comment suggests specifying the bound doesn't work due to restrictions on defining recursive implementations (like `Or`). However bounds on the functions are fine~~ Never mind I misread how `Or` was constructed, bounds now exist. - Note that the only mutable one has a new `Fetch` for readonly as the `State` has to be the same for any of this to work Co-authored-by: Carter Anderson <mcanders1@gmail.com>

bors · 2021-12-01T23:45:44Z

Pull request successfully merged into main.

Build succeeded:

The biggest change here is that doing the same query twice, one mutably and one immutably, is no longer needed, once bevyengine/bevy#2305 lands. Before that lands, however, this upgrade is actually impossible to do safely, since the `q0`/`q0_mut` distinction is gone and therefore it's impossible to implement the Rapier component traits on Bevy queries. (This is filed upstream as bevyengine/bevy#2744.) Therefore, this upgrade has to wait until that Bevy PR lands. Other minor changes: * `Light` has been renamed to `PointLight`. * `App::build()` is now `App::new()`, and `AppBuilder` is now `App`. * `glam` has been upgraded upstream.

bors bot added a commit that referenced this pull request Jun 4, 2021

Try #2305:

5020258

NathanSWard added A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible labels Jun 4, 2021

NathanSWard reviewed Jun 5, 2021

View reviewed changes

Guvante commented Jun 7, 2021

View reviewed changes

crates/bevy_ecs/src/query/fetch.rs Outdated Show resolved Hide resolved

Guvante marked this pull request as ready for review June 11, 2021 06:10

Guvante commented Jun 11, 2021

View reviewed changes

bors bot added a commit that referenced this pull request Jun 16, 2021

Try #2305:

18db650

alice-i-cecile approved these changes Jun 16, 2021

View reviewed changes

mockersf added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 16, 2021

Guvante force-pushed the iter_for_mutable_query branch 2 times, most recently from d7845da to 857cb9b Compare August 16, 2021 00:12

alice-i-cecile approved these changes Aug 16, 2021

View reviewed changes

alice-i-cecile reviewed Aug 16, 2021

View reviewed changes

crates/bevy_ecs/src/query/iter.rs Show resolved Hide resolved

alice-i-cecile reviewed Aug 16, 2021

View reviewed changes

crates/bevy_ecs/src/query/iter.rs Show resolved Hide resolved

tom-leys mentioned this pull request Aug 17, 2021

ReadOnlyFetch is not implimented for With<T> #2665

Closed

Guvante force-pushed the iter_for_mutable_query branch 2 times, most recently from 1a59210 to fe78224 Compare October 4, 2021 00:02

alice-i-cecile approved these changes Oct 4, 2021

View reviewed changes

alice-i-cecile reviewed Oct 4, 2021

View reviewed changes

Guvante force-pushed the iter_for_mutable_query branch from fe78224 to b38bf90 Compare October 4, 2021 01:36

deontologician mentioned this pull request Oct 18, 2021

Add bevy-components feature dimforge/parry#57

Closed

deontologician mentioned this pull request Oct 18, 2021

Fixes for Bevy 0.6 dimforge/bevy_rapier#91

Closed

Implement iter() for mutable Queries

65e1885

- Requires that all queries specify an immutable Fetch with identical State - Added a unit test to verify that calling iter vs iter_mut returns the same things - Added a second unit test to verify that calling iter_mut borrows correctly

Guvante force-pushed the iter_for_mutable_query branch from b38bf90 to 65e1885 Compare November 28, 2021 08:25

Guvante commented Nov 28, 2021

View reviewed changes

crates/bevy_ecs/src/query/fetch.rs Outdated Show resolved Hide resolved

pipelined/bevy_render2/src/render_component.rs Outdated Show resolved Hide resolved

Guvante and others added 4 commits November 28, 2021 00:38

rustfmt

e737eda

Missed a reference...

543db2f

remove ReadOnlyQueryItem / use iter_mut in ExtractComponent

5369c3c

remove unused fields from ReadOnlyWriteFetch

3302f8e

cart reviewed Dec 1, 2021

View reviewed changes

cart added 2 commits November 30, 2021 19:29

Fix Or ReadOnlyFetch impls

2380475

remove old compile fail tests and add new trybuild test

a12454f

bors bot changed the title ~~Implement iter() for mutable Queries~~ [Merged by Bors] - Implement iter() for mutable Queries Dec 1, 2021

bors bot closed this Dec 1, 2021

Guvante deleted the iter_for_mutable_query branch December 2, 2021 02:21

Mathspy mentioned this pull request Dec 14, 2021

Clone and IntoIter support for queries #1631

Closed

TheRawMeatball mentioned this pull request Jan 12, 2022

Select a random element (or set of elements) from a query #3652

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Implement iter() for mutable Queries #2305

[Merged by Bors] - Implement iter() for mutable Queries #2305

Guvante commented Jun 4, 2021 •

edited

Loading

alice-i-cecile commented Jun 4, 2021

Guvante commented Jun 4, 2021

bors bot commented Jun 4, 2021

Guvante commented Jun 4, 2021 •

edited

Loading

mockersf commented Jun 4, 2021

NathanSWard Jun 5, 2021

Guvante Jun 7, 2021

Guvante commented Jun 11, 2021 •

edited

Loading

Guvante Jun 11, 2021

alice-i-cecile commented Jun 16, 2021

bors bot commented Jun 16, 2021

Guvante commented Jul 23, 2021

alice-i-cecile commented Aug 16, 2021

DJMcNab commented Aug 16, 2021

alice-i-cecile left a comment

Guvante commented Aug 16, 2021

alice-i-cecile commented Aug 16, 2021

Guvante commented Aug 16, 2021 •

edited

Loading

alice-i-cecile left a comment

alice-i-cecile Oct 4, 2021

Guvante Oct 4, 2021

alice-i-cecile Oct 4, 2021

deontologician commented Oct 16, 2021

alice-i-cecile commented Oct 16, 2021

deontologician commented Oct 18, 2021 •

edited

Loading

Guvante left a comment

cart left a comment •

edited

Loading

cart commented Dec 1, 2021

bors bot commented Dec 1, 2021

[Merged by Bors] - Implement iter() for mutable Queries #2305

[Merged by Bors] - Implement iter() for mutable Queries #2305

Conversation

Guvante commented Jun 4, 2021 • edited Loading

Objective

Solution

alice-i-cecile commented Jun 4, 2021

Guvante commented Jun 4, 2021

bors bot commented Jun 4, 2021

try

Guvante commented Jun 4, 2021 • edited Loading

mockersf commented Jun 4, 2021

NathanSWard Jun 5, 2021

Choose a reason for hiding this comment

Guvante Jun 7, 2021

Choose a reason for hiding this comment

Guvante commented Jun 11, 2021 • edited Loading

Guvante Jun 11, 2021

Choose a reason for hiding this comment

alice-i-cecile commented Jun 16, 2021

bors bot commented Jun 16, 2021

try

Guvante commented Jul 23, 2021

alice-i-cecile commented Aug 16, 2021

DJMcNab commented Aug 16, 2021

alice-i-cecile left a comment

Choose a reason for hiding this comment

Guvante commented Aug 16, 2021

alice-i-cecile commented Aug 16, 2021

Guvante commented Aug 16, 2021 • edited Loading

alice-i-cecile left a comment

Choose a reason for hiding this comment

alice-i-cecile Oct 4, 2021

Choose a reason for hiding this comment

Guvante Oct 4, 2021

Choose a reason for hiding this comment

alice-i-cecile Oct 4, 2021

Choose a reason for hiding this comment

deontologician commented Oct 16, 2021

alice-i-cecile commented Oct 16, 2021

deontologician commented Oct 18, 2021 • edited Loading

Guvante left a comment

Choose a reason for hiding this comment

cart left a comment • edited Loading

Choose a reason for hiding this comment

cart commented Dec 1, 2021

bors bot commented Dec 1, 2021

Guvante commented Jun 4, 2021 •

edited

Loading

Guvante commented Jun 4, 2021 •

edited

Loading

Guvante commented Jun 11, 2021 •

edited

Loading

Guvante commented Aug 16, 2021 •

edited

Loading

deontologician commented Oct 18, 2021 •

edited

Loading

cart left a comment •

edited

Loading