ReadOnlyFetch is not implimented for With<T>

[tom-leys]

Bevy version

bevy_ecs 0.5.0

Operating system & version

Windows 10, GCC

What you did

    {
    // SpaceShip is a component, sector.space() is a &mut World
        (|(mut commands, query) : (Commands, Query<(Entity, With<SpaceShip>)>)|
        {
            for entity in query.iter() {
                commands.entity(entity).despawn();
            }

        }).system().run((), &mut sector.space())
    }

What you expected to happen

All queries which involve states (With, Without, Changed) should be read-only.

What actually happened

Did not compile

--> sg_simulation\src\network\sync\test_sector.rs:704:37
|
704 | for entity in query.iter() {
| ^^^^ the trait ReadOnlyFetch is not implemented for WithFetch<&ship_component::SpaceShip>
|
= note: required because of the requirements on the impl of ReadOnlyFetch for (EntityFetch, WithFetch<&ship_component::SpaceShip>)

Additional information

Non-critical, I can use iter_mut. With #2305 I would not have noticed this issue at all.

Is perhaps similar to #763

Does this bug mean that I can't run two queries using With<SpaceShip> at the same time?

[tom-leys]

Oh, PEBKAC (problem exists between keyboard and chair)

This is what the query filter API was for...

        (|(mut commands, mut query) : (Commands, Query<(Entity), (With<Biped>)>)|
            {
                for entity in query.iter() {
                    commands.entity(entity).despawn();
                }

            }).system().run((), &mut sector.space());

I should have put a comma there - the With<> goes in the filter section.

Some way to make this more obvious to other users? More docs in the With<> class with examples? Discuss use of filters more explictly?

[cart]

We actually probably should implement ReadOnlyFetch for With (and all of the other filters). Filters are just normal Fetch impls that return a boolean (and there are probably legitimate reasons to query for the actual boolean results of With). If you called iter_mut(), your old code should work (although it would return (Entity, bool) instead of just the Entity) ... but you definitely shouldn't need iter_mut() for this scenario.

In terms of docs, the Bevy Book covers this. But the "rust api docs" could definitely use more examples for cases like this.

As an aside, you can simplify Query<(Entity), (With<Biped>)> to Query<Entity, With<Biped>>.

[alice-i-cecile]

We actually probably should implement ReadOnlyFetch for With (and all of the other filters). Filters are just normal Fetch impls that return a boolean (and there are probably legitimate reasons to query for the actual boolean results of With)

Hmm. I worry that this decreases the value of compiler errors in order to support a niche use case. Generally speaking, I would just use Option<T> queries if I wanted to use this for With / Without.

That said, this would be much more useful for Changed and Added; perhaps it's worth including after all.

[Guvante]

My PR #2305 adds ReadOnlyFetch to every existing Fetch that isn't WriteFetch as part of adding WorldQuery::ReadOnlyFetch which has a ReadOnlyFetch constraint (names are hard...) to support iter() working for any possible WorldQuery.

[alice-i-cecile]

From @mvlabat, discussing #2713, point out that you can currently include query filters in the first Query type parameter. They always return true though, which is not the desired or intuitive behavior.

[cart]

Hmm yeah "archetype filters" like With<T> will always return true because Queries won't iterate entities for mismatched archetypes. Entity filters like Changed<T> will return false for entities containing non-changed T. Whether or not a filter is an "archetype filter" or an "entity filter" is a bit of an implementation detail, so I agree that this is still less-than-ideal ux.

[alice-i-cecile]

Closed with the merging of #2305.