Split WorldQuery into WorldQueryData and WorldQueryFilter

[wainwrightmark]

Objective

• Fixes Use distinct types for query "data" generics and query "filter" generics #7680
• This is an updated for Split WorldQuery into WorldQueryData and WorldQueryFilter #8899 which had the same objective but fell a long way behind the latest changes

Solution

The traits WorldQueryData : WorldQuery and WorldQueryFilter : WorldQuery have been added and some of the types and functions from WorldQuery has been moved into them.

ReadOnlyWorldQuery has been replaced with ReadOnlyWorldQueryData.

WorldQueryFilter is safe (as long as WorldQuery is implemented safely).

WorldQueryData is unsafe - safely implementing it requires that Self::ReadOnly is a readonly version of Self (this used to be a safety requirement of WorldQuery)

The type parameters Q and F of Query must now implement WorldQueryData and WorldQueryFilter respectively.

This makes it impossible to accidentally use a filter in the data position or vice versa which was something that could lead to bugs. Compile failure tests have been added to check this.

It was previously sometimes useful to use Option<With<T>> in the data position. Use Has<T> instead in these cases.

The WorldQuery derive macro has been split into separate derive macros for WorldQueryData and WorldQueryFilter.

Previously it was possible to derive both WorldQuery for a struct that had a mixture of data and filter items. This would not work correctly in some cases but could be a useful pattern in others. This is no longer possible.

---

Notes

• The changes outside of bevy_ecs are all changing type parameters to the new types, updating the macro use, or replacing Option<With<T>> with Has<T>.

• All WorldQueryData types always returned true for IS_ARCHETYPAL so I moved it to WorldQueryFilter and
  replaced all calls to it with true. That should be the only logic change outside of the macro generation code.

• Changed<T> and Added<T> were being generated by a macro that I have expanded. Happy to revert that if desired.

• The two derive macros share some functions for implementing WorldQuery but the tidiest way I could find to implement them was to give them a ton of arguments and ask clippy to ignore that.

Changelog

Changed

• Split WorldQuery into WorldQueryData and WorldQueryFilter which now have separate derive macros. It is not possible to derive both for the same type.
• Query now requires that the first type argument implements WorldQueryData and the second implements WorldQueryFilter

Migration Guide

• Update derives

// old
#[derive(WorldQuery)]
#[world_query(mutable, derive(Debug))]
struct CustomQuery {
    entity: Entity,
    a: &'static mut ComponentA
}

#[derive(WorldQuery)]
struct QueryFilter {
    _c: With<ComponentC>
}

// new 
#[derive(WorldQueryData)]
#[world_query_data(mutable, derive(Debug))]
struct CustomQuery {
    entity: Entity,
    a: &'static mut ComponentA,
}

#[derive(WorldQueryFilter)]
struct QueryFilter {
    _c: With<ComponentC>
}

• Replace Option<With<T>> with Has<T>

/// old
fn my_system(query: Query<(Entity, Option<With<ComponentA>>)>)
{
  for (entity, has_a_option) in query.iter(){
    let has_a:bool = has_a_option.is_some();
    //todo!()
  }
}

/// new
fn my_system(query: Query<(Entity, Has<ComponentA>)>)
{
  for (entity, has_a) in query.iter(){
    //todo!()
  }
}

• Fix queries which had filters in the data position or vice versa.

// old
fn my_system(query: Query<(Entity, With<ComponentA>)>)
{
  for (entity, _) in query.iter(){
  //todo!()
  }
}

// new
fn my_system(query: Query<Entity, With<ComponentA>>)
{
  for entity in query.iter(){
  //todo!()
  }
}

// old
fn my_system(query: Query<AnyOf<(&ComponentA, With<ComponentB>)>>)
{
  for (entity, _) in query.iter(){
  //todo!()
  }
}

// new
fn my_system(query: Query<Option<&ComponentA>, Or<(With<ComponentA>, With<ComponentB>)>>)
{
  for entity in query.iter(){
  //todo!()
  }
}

[Trashtalk217]

Alright, I'm going to review this and for my own sanity, I'll split it up in parts. Here's a short checklist for what I'm going to do.

• Check all renames outside of bevy_ecs (easy)
• Check the code within bevy_ecs, without macros (normal)
• Check the macro code (very hard)

I'll let you know if I have any questions.

[Trashtalk217]

I'm done looking at the non-macro ecs code. I think the separation of filter and data queries cleans it up a lot.

[Trashtalk217]

What exactly do you mean here?

[Trashtalk217]

I've looked at the macro code and I'll be honest: I am not qualified to give a good opinion on it. I know this is not the most rigorous but given that it has passed all the CI, I'm assuming that it is probably fine.

[james-j-obrien]

By restricting Q to WorldQueryData we lose the ability to express things like AnyOf<(&A, With<B>)> where we could theoretically want to only match entities with either A or B but only want to access A. This is a bit of an edge case and you can still express this with Option<&A>, Or<(With<A>, With<B>)> so I don't think it's blocking to be losing this behavior but it should be included in the migration guide at least.

[joseph-gio]

Maybe I'm missing something, but I don't really see the utility behind splitting the traits in this way. What kind of bugs would this actually prevent? Why do we want to prevent people from writing Query<Changed<T>>? (For example)

The WorldQueryFilter trait would be useful, as it prevents you from writing Query<&A, &B> when you meant to write Query<(&A, &B)>. However I just don't really see what we gain from the WorldQueryData trait. I think we could reduce the impact of this PR a lot if we cut out WorldQueryData and made WorldQueryFilter essentially a marker trait.

[wainwrightmark]

Maybe I'm missing something, but I don't really see the utility behind splitting the traits in this way. What kind of bugs would this actually prevent? Why do we want to prevent people from writing Query<Changed<T>>? (For example)

The WorldQueryFilter trait would be useful, as it prevents you from writing Query<&A, &B> when you meant to write Query<(&A, &B)>. However I just don't really see what we gain from the WorldQueryData trait. I think we could reduce the impact of this PR a lot if we cut out WorldQueryData and made WorldQueryFilter essentially a marker trait.

Query<Changed<T>> doesn't do what you expect (it returns the same as Query<With<T>>). That's the main class of bugs this prevents. And that's the reason for the separate traits - if we cut WorldQueryData there would be nothing stopping you from putting filters in the data position.

[wainwrightmark]

By restricting Q to WorldQueryData we lose the ability to express things like AnyOf<(&A, With<B>)> where we could theoretically want to only match entities with either A or B but only want to access A. This is a bit of an edge case and you can still express this with Option<&A>, Or<(With<A>, With<B>)> so I don't think it's blocking to be losing this behavior but it should be included in the migration guide at least.

That's a good point - I have added that example to the migration guide.

[joseph-gio]

`Query<Changed> doesn't do what you expect (it returns the same as Query<With>). That's the main class of bugs this prevents.

IIRC Query<Changed<T>> returns a Boolean indicating whether or not T has changed. I don't think that's a bug, or something that needs to be prevented.

[joseph-gio]

Just for illustration, I made a quick and dirty implementation of my suggestion on this branch: main...JoJoJet:bevy:query-filter

I prefer this approach since it a simpler change, is far less breaking for users, retains flexibility, and still fixes the major footguns.

[Trashtalk217]

IIRC Query<Changed<T>> returns a Boolean indicating whether or not T has changed. I don't think that's a bug, or something that needs to be prevented.

I double-checked what Query<Changed<T>> does and this is correct. I also quickly looked at @JoJoJet's implementation which looks correct (it's a lot shorter, which is nice). I do have one question, what are the use cases for something like Query<Changed<T>>? I wasn't able to think of any.

EDIT: Never mind, I thought of one. It can be used as a workaround for inverse filters (see also #7265)

fn system(query: Query<Changed<Foo>>, query_two: Query<Foo>) {
    let has_changed = query.to_vec();
    let foos = query_two.to_vec();
    for i in 0..has_changed.size() {
        if !has_changed[i] {
            // do something to the unchanged foos[i]
        }
    }
}

[joseph-gio]

what are the use cases for something like Query<Changed<T>>

It's not particularly useful since you can just use Ref<T>::is_changed to detect changes -- however since it's not something we need to forbid, I think we can save on complexity by allowing it.

[wainwrightmark]

Sorry for being slow replying - having a very busy weekend.

I wasn't aware that Changed<T> did that when used in the data position. But I still think it's a likely source of bugs in user code (not least because it was my suffering at the hands of such a bug that led me to writing this PR).

Just to explain, my bug looked something like this. I think it's fairly unlikely that I would notice it to be incorrect if I read it in someone else's code:

fn my_function(query: Query<Changed<ComponentA>>){
    if !query.is_empty()
    {
        // do expensive computation that's only necessary when at least one `ComponentA` has changed
    }
}

I find it particularly unintuitive that With<T> works exactly the same in either position whilst Changed<T> has a completely different behaviour depending on which side of the query it is on. This also doesn't seem to be documented anywhere. All the documentation seems to imply that one should only use filters on the right hand side of the query and it seems to me more logical to enforce this with the type system rather than to write extensive documentation about how the different filters behave when used incorrectly.

I will certainly admit that bugs caused by this are likely to be rare (to assume otherwise based on my having been affected by one would be to succumb to selection bias) and the changes in this PR will likely break a lot of users who happen to be using filters "incorrectly" but whose program works perfectly well. I am not well placed to make a judgement about which concern is the more pressing but I did want to at least make clear the advantages of my approach.

Just for illustration, I made a quick and dirty implementation of my suggestion on this branch: main...JoJoJet:bevy:query-filter

I prefer this approach since it a simpler change, is far less breaking for users, retains flexibility, and still fixes the major footguns.

I've just read it casually but this all looks very sensible. I particularly like how you've done the macros.

[Trashtalk217]

I've thought about it some more and I think that disallowing filters in the data slot is the right choice. The possible use you could get out of a vector of booleans without components to give it context is minimal. As the documentation states, you could also check for Added and Changed by using Ref<T>.

I think that there is potential for filters in the data slot, e.g.

Query<(Added<&ComponentA>, &ComponentB)>> == Query<(&ComponentA, &ComponentB), Added<ComponentA>>

This would add a nice bit of syntactic sugar to queries. This should probably not be implemented for Or, With, or Without, as it doesn't make much sense for those.

But for now, I think it's fine to remove the possibility for it outright as the current state is more confusing than helpful. This feature is also - as far as I've checked - completely undocumented (admittedly, that's an easy fix).

[hymm]

FYI, Query<With<T>> is a bit nonsensical since it returns the empty tuple. not a bool.

[alice-i-cecile]

@wainwrightmark I'm doing a review pass to try and get this into 0.12 (ETA Saturday): can you resolve merge conflicts for us?

[alice-i-cecile]

Waiting on another review from an SME-ECS.

[james7132]

Overall, I'm in favor of moving in this direction. It makes the Query DSL much more well-formed, without allowing for odd edge case interactions. The code seems to work, and there aren't any glaring unsoundness issues. Most of my comments here are focused on code quality and style. LGTM otherwise.

[james7132]

Is there any way to avoid the sheer number of arguments here and below?

[wainwrightmark]

Quite possibly. I will have a go at factoring all the arguments into a small number of structs instead.

[wainwrightmark]

IMO doing a merge commit is safer: it's much easier to revert :)

I have done this. It's a bit scary because it looks like there are 400 files changed but the only files I had to touch were:

examples\ecs\custom_query_param.rs slight conflict with imports

crates\bevy_render\src\batching\mod.rs The QueryFilter associated type had been added to GetBatchData. I changed the type from ReadOnlyWorldQuery to WorldQueryFilter

crates\bevy_pbr\src\render\mesh.rs and crates\bevy_animation\src\lib.rs someone else had done the migration to Has. I kept their changes.

crates\bevy_render\src\extract_instances.rs
A new trait ExtractInstance had been added. I changed its associated types to use ReadOnlyWorldQueryData and WorldQueryFilter

crates\bevy_ecs\src\query\filter.rs

There were a couple of conflicts around world.unsafe_world().storages() which had been changed to world.storages() and also with removing redundant explicit links in documentation.

[wainwrightmark]

Will try to do the larger changes suggested by @james7132 this afternoon (GMT)

[wainwrightmark]

Will try to do the larger changes suggested by @james7132 this afternoon (GMT)

I have moved split back onto WorldQuery. Simplifying the macros is ironically quite complex I'm afraid. I can try to do it tomorrow morning.

[wainwrightmark]

I have had a go at tidying the macros (2098790) - it working and expanding to the same code as before but I'm not 100% happy with it yet and realistically I think it would be better to do it in a separate PR.

[alice-i-cecile]

@wainwrightmark this is in a good state; can you please resolve conflicts and we can merge?

[Nilirad]

Added breaking change label since it requires adapting WorldQuery derives.

[wainwrightmark]

@wainwrightmark this is in a good state; can you please resolve conflicts and we can merge?

Sorry for taking a few days to get round to this. I merged main back and resolved the conflicts.

They were mostly trivial, just tweaking the use statements in
crates/bevy_ecs/macros/src/lib.rs
examples/ecs/custom_query_param.rs
crates/bevy_ecs/src/world/mod.rs

And there were some comments added to crates/bevy_ecs/src/query/filter.rs in 48af029 which I copied across.

[StrikeForceZero]

For future people clicking on the Split WorldQuery into WorldQueryData and WorldQueryFilter link in the blog post and not realizing these were renamed: #10776