World query filter RFC.

[Monnoroch]

Disallow query filter types that don't make sense as filters, e.g. &MyComponent.

Rendered.

[alice-i-cecile]

I'm in favor of this direction. I think it would add quite a bit of conceptual clarity to the end user, and result in better error messages and more foolproof APIs.

The fact that you can use fetch data in filters and vice versa is surprising, and as far as I've seen, is never idiomatically used.

[Monnoroch]

Thanks Alice! This is probably the fastest RFC review I have ever had, too.

[alice-i-cecile]

Can we rule out the reverse mistake too? I've seen users struggle with putting query filters in their fetch argument, only to receive a useless piece of data.

My instinct is that this needs two disjoint traits for each argument, each of which relies on a common data-access-defining subtrait.

[Monnoroch]

Yes we can and my proposal discusses that as a future possibility. But I would prefer to design it separately as there are multiple ways to do it and I don't have a prototype implementation yet, just a couple of ideas. I'll happily do that work if/when this first step is merged.

[alice-i-cecile]

Interesting. My instinct is that it would be less painful to reconceptualize and rewrite this code once to account for both changes.

That said, your proposed implementation is extremely easy, and my suggestion would not be.

[Monnoroch]

I had exactly the same instinct, but then I thought of the described trick.

The problem with "reconceptualize and rewrite" into "two disjoint traits for each argument, each of which relies on a common data-access-defining subtrait" is that we don't just have the one trait. We have all those helper traits (Fetch, FetchState, WorldQueryGats, etc) that are used all over the place. So "reconceptualize and rewrite" becomes not just more complicated, but literally much more work to update lots of call sites across the codebase. I am currently in search of a trick that would allow me to avoid all that. I'm thinking of something like renaming WorldQuery into WorldQueryImpl and adding a new marker trait WorldQuery: WorldQueryImpl {}. Again, it's a lot of renaming, but at least I don't need a bunch of new helper traits. But as of now it's just an idea, I didn't manage to make it compile yet.

[DJMcNab]

Why is this an RFC?

This being allowed is clearly a regression, which isn't something which needs an RFC. It appears that this regressed in TheRawMeatball/bevy#48 by @BoxyUwU, which landed as part of bevyengine/bevy#3001.

(Fwiw, I don't think we should do the inverse without a replacement (e.g. an EvaluateFilter WorldQuery). It's clear that there are cases where we need to know the result of a Filter dynamically. You can use ChangeTrackers, but the asymmetry there does not spark joy)

[Monnoroch]

This is an RFC because it was not clear to me that this is a regression. In fact, to a newby it appears to be a design choice as the Query type has the same bounds for both parts. Having explicitly the same bounds conveys intent.

Independently of it being a regression, I do believe that codifying it is a good idea. An RFC that documents this part of the ECS API feels necessary, given that there was no RFC and as a result the cited author has broken the back-then-verbal contract.

I don't think we should do the inverse without a replacement

We should, just not for change detection. For example, With and Without should not appear in the query part. I do agree that querying for Changed and Added is useful.

[DJMcNab]

This is a regression since the latest release 0.7.0 - there were extra bounds.

I would have thought a regression test would have sufficed in the PR fixing this.

As far as I know, this wasn't an intentional break of this contract, just an unconsidered consequence which was snuck in as part of the unreviewable bevyengine/bevy#3001. Therefore, even if this was codified as an RFC before then, it would still probably have landed - that is, this being an RFC is not a good way to achieve that stated goal.

I think I agree that With and Without probably have no reason to exist in the query 'item'. In theory With could be useful in some cases, although it would be mostly equivalent to Option<&T>.

[BoxyUwU]

With have reason to exist in the data part i.e. Option<With<T>>

[BoxyUwU]

if we do this the "correct" impl is definitely trait FilterWorldQuery: WorldQuery {} not an entirely duplicated trait hierarchy. Though im not convinced we really want to do this as it is nice to not need to mark a derive(WorldQuery) as a filter. Infact in an ideal world I think it would be great if we could make Query<(), &T> work the same as Query<(), With<T>> (and i think this is probably possible).

[Monnoroch]

it would be great if we could make Query<(), &T> work the same as Query<(), With<T>>

Hmm, personally I would rather read code that explicitly states what it does.

[BoxyUwU]

Query<(), &T> is just as clear as Query<(), With<T>> to me if we understand filters as just being "the same" as the data generic except the data doesnt get returned

[Monnoroch]

In fact, another idea I has was to not split the query and filter at all, like so:

Query<(Fetch<Entity>, Fetch<&ComponentA>, With<ComponentB>, Changed<ComponentC>)>

Only the Fetch commands would result in a value in the returned tuple.

Although I'm not sure it's a good idea. But right now seeing those fetch.set_table(...); filter.set_table(...) is just weird.

[alice-i-cecile]

Basically implemented in bevyengine/bevy#9918!