Add a limited `Not` query filter #11116

CatThingy · 2023-12-28T04:44:33Z

Objective

Resolve #7265.

Solution

Create a Not query filter, inverting primitive query filters.

Implementation note and future work

This implementation only allows for inversions of Added<T>, Changed<T>, With<T> and Without<T>. The implementation without the added InvertibleFilter trait produces the correct result for tuples (and Or tuples) of only archetypal filters and only non-archetypal filters, but not tuples that mix the two. I was unable to figure out how to distinguish these cases, and chose to omit tuples altogether for correctness since it does not affect the possible filters that can be expressed.

Changelog

Add a Not query filter, inverting the condition of primitive query filters.

crates/bevy_ecs/src/query/filter.rs

alice-i-cecile

Very nice stuff. I'm pleased to see this work out, and I'm fine with the limitation.

Requests:

Link to de Morgan's laws.
Make InvertibleFilter pub, with appropriate docs.
Add a doc test to Changed filter, showing how to reproduce the "changed but not added" behavior that Mutated used to provide.

crates/bevy_ecs/src/query/filter.rs

SkiFire13 · 2023-12-29T13:31:22Z

crates/bevy_ecs/src/query/filter.rs

+        if T::IS_ARCHETYPAL {
+            !T::matches_component_set(state, set_contains_id)
+        } else {
+            T::matches_component_set(state, set_contains_id)


I find this a bit questionable since it means that Or<(Added<A>, Not<Added<A>>) won't match every entity, but only those that have A. This is also the fundamental problem you face when filter being negated is a tuple mixing With/Without and Added/Changed. In one case you want to negate the archetypes being matched, in the other you want to negate the filter_fetch, but in case of tuples you need a mix of them.

I don't think that it makes sense for Not<Changed<A>> to also match entities that do not have A at all. It's not the correct logical negation, but is probably the expected result when filtering on Not<Changed<A>>.

While I agree that the expectation is different, it also means that the operation that Not is doing is different for different types, and this can break all sorts of expectations in the code.

Here's yet another test case that fails:

#[test] fn query_filter_not_broken() { use crate::system::Query; fn system( q1: Query<(Entity, &mut A), Not<Changed<B>>>, q2: Query<(Entity, &mut A), With<B>>, ) { for (e1, _) in &q1 { for (e2, _) in &q2 { assert_ne!(e1, e2); } } } let mut world = World::default(); world.spawn((A(0), B(0))); let system_id = world.register_system(system); _ = world.run_system(system_id); _ = world.run_system(system_id); }

The problem is that you always invert the component access, but then when setting which component sets are matched by Not<Changed<B>> you don't invert them, hence declaring that Not<Changed<B>> accesses only archetypes without B (making it disjoint from the second query) but in the end accessing only those that have B (making it conflicting with the second query).

If I had to generalize the operations Not actually does:

there is an "applicable components set" on which a filter can be applied on. Every archetype not matching it is always excluded by it. For With/Without this set contains every possible component set, but for Added<T>/Changed<T> this contains only component sets containing Ts. This is not inverted by Not, and should be kept as is.

there is a "filtered components set" which is a subset of the applicable components set that the filter can reduce to. For example With<T>/Without<T> can reduce the applicable components set to only those containing T or not. Not inverts this filter.

there is a "fetch filter" that filters individual entities matching the filtered components set. For example Added<T>/Changed<T> apply filters at this point. Not also inverts this filter.

The problem with this approach is that it adds yet another level of component access/filtering, complicating WorldQuery even more. But without it I don't think you can reasonably support inverting both With/Without and Changed/Added without bugs.

I think Not<Changed<T>> should include entities which do not contain a T. I think the problem here is the wording between Not Changed T and an Unchanged T. Unchanged T implies the existence of T, whereas Not Changed T is strictly the inverse of Changed T.

A resolution to this problem could be to define:

pub type Unchanged<T> = (With<T>, Not<Changed<T>>); pub type Existing<T> = (With<T>, Not<Added<T>>);

With some documentation explaining why it's not as simple as Not<Changed<T>>. For something as fundamental as a Not operation, it should be identical to the laws of boolean algebra.

SkiFire13

I'm still not convinced this is correct, although I don't think it can show misbehaviour with the current implementation (I think it would if Not<(F1, F2, F3,)> was allowed).

SkiFire13 · 2023-12-29T23:56:29Z

crates/bevy_ecs/src/query/filter.rs

+        for i in 0..intermediate.filter_sets.len() {
+            for j in i..intermediate.filter_sets.len() {
+                let filter_a = &intermediate.filter_sets[i];
+                let filter_b = &intermediate.filter_sets[j];


When I said cartesian product I meant between all the filter sets in intermediate, not between each pair of them. That is, if the formula that the access expresses is (A & B) | (C & D) | (E & F) and you want to negate it, you need one atom for each conjuction to be false, so:

(!A & !C & !E) | (!A & !C & !F) | (!A & !D & !E) | (!A & !D & !F) | (!B & !C & !E) | (!B & !C & !F) | (!B & !D & !E) | (!B & !D & !F)

while I expect your code to produce something like this (not tested though):

(!A & !A) | (!A & !B) | (!A & !C) | (!A & !D) | (!B & !A) | (!B & !B) | (!B & !C) | (!B & !D) | (!A & !E) | (!A & !F) | (!B & !E) | (!B & !F) | (!C & !C) | (!C & !D) | (!C & !E) | (!C & !F) | (!D & !C) | (!D & !D) | (!D & !E) | (!D & !F) | (!E & !E) | (!E & !F) | (!F & !E) | (!F & !F)

In a more generic way, you need to compute access & !intermediate. Expanding them we get something in the following shape ((A1 & ...) | (A2 & ...) | ...) & !((I1 & ...) | (I2 & ...) | ...). We can expand the ! in front of the intermediate part to get ((A1 & ...) | (A2 & ...) | ...) & !(I1 & ...) & !(I2 & ...) & !..., which means it is a repeated AND with the negation of each filter set in intermediate, which in turn can be computed by observing that !(I1 & ...) is equal to !I1 | !....

SkiFire13 · 2023-12-29T23:58:22Z

crates/bevy_ecs/src/query/filter.rs

+                        let mut inverted = FilteredAccess::<ComponentId>::default();
+                        inverted.and_without(ComponentId::new(a));
+                        inverted.and_without(ComponentId::new(b));
+                        access.append_or(&inverted);


access.append_or(&inverted) is essentially equal to access = access OR inverted, but you need to compute an AND instead because we're adding restrictions, not alternatives to the existing restrictions. I would use access.extend(&inverted) instead.

SkiFire13 · 2023-12-30T00:10:12Z

crates/bevy_ecs/src/query/filter.rs

+        for i in 0..intermediate.filter_sets.len() {
+            for j in i..intermediate.filter_sets.len() {
+                let filter_a = &intermediate.filter_sets[i];
+                let filter_b = &intermediate.filter_sets[j];
+
+                for a in filter_a.with.ones() {
+                    for b in filter_b.with.ones() {
+                        let mut inverted = FilteredAccess::<ComponentId>::default();
+                        inverted.and_without(ComponentId::new(a));
+                        inverted.and_without(ComponentId::new(b));
+                        access.append_or(&inverted);
+                    }
+                    for b in filter_b.without.ones() {
+                        // with & without the same component is always false, so don't add it
+                        if a == b {
+                            continue;
+                        }
+                        let mut inverted = FilteredAccess::<ComponentId>::default();
+                        inverted.and_without(ComponentId::new(a));
+                        inverted.and_with(ComponentId::new(b));
+                        access.append_or(&inverted);
+                    }
+                }
+
+                for a in filter_a.without.ones() {
+                    for b in filter_b.with.ones() {
+                        // with & without the same component is always false, so don't add it
+                        if a == b {
+                            continue;
+                        }
+                        let mut inverted = FilteredAccess::<ComponentId>::default();
+                        inverted.and_with(ComponentId::new(a));
+                        inverted.and_without(ComponentId::new(b));
+                        access.append_or(&inverted);
+                    }
+                    for b in filter_b.without.ones() {
+                        let mut inverted = FilteredAccess::<ComponentId>::default();
+                        inverted.and_with(ComponentId::new(a));
+                        inverted.and_with(ComponentId::new(b));
+                        access.append_or(&inverted);
+                    }
+                }
+            }


I would also move all of this into a method on FilteredAccessSet

hymm · 2024-01-03T06:37:05Z

Without being able to use Not on Or and tuples is it worth taking on this complexity over just having a NotChanged and NotAdded Filter types?

SkiFire13

Also see my comment here on why this is still unsound.

SkiFire13 · 2024-01-03T11:25:59Z

crates/bevy_ecs/src/query/filter.rs

+        archetype: &Archetype,
+        access: &mut Access<ArchetypeComponentId>,
+    ) {
+        T::update_archetype_component_access(state, archetype, access);


I didn't find time to look for a failing example, but this feels wrong, it should be inverted (but again only for archetypal filters...)

SkiFire13 · 2024-01-03T11:37:17Z

crates/bevy_ecs/src/query/access.rs

+    pub fn invert_filters(&mut self) {
+        let filter_sets = self.filter_sets.drain(..);
+
+        #[derive(Debug, Copy, Clone)]
+        enum AccessType {
+            With(usize),
+            Without(usize),
+        }
+
+        // Gather and unify each filter set from separate bitsets for With and Without into
+        // a single collection to simplify logic
+        let mut accesses = vec![];
+        for filter in filter_sets {
+            let mut filters = vec![];
+            filters.extend(filter.with.ones().map(AccessType::With).collect::<Vec<_>>());
+            filters.extend(
+                filter
+                    .without
+                    .ones()
+                    .map(AccessType::Without)
+                    .collect::<Vec<_>>(),
+            );
+            accesses.push(filters);
+        }
+
+        let mut indices = vec![0; accesses.len()];
+        let lengths = accesses.iter().map(|v| v.len()).collect::<Vec<_>>();
+
+        loop {
+            let mut new_filter = AccessFilters::default();
+            for (el, index) in indices.iter().enumerate() {
+                match accesses[el][*index] {
+                    AccessType::With(id) => {
+                        new_filter.without.grow(id + 1);
+                        new_filter.without.insert(id);
+                    }
+                    AccessType::Without(id) => {
+                        new_filter.with.grow(id + 1);
+                        new_filter.with.insert(id);
+                    }
+                }
+            }
+            self.filter_sets.push(new_filter);
+
+            let mut update_index = indices.len() - 1;
+            loop {
+                // update while rolling over indices
+                indices[update_index] = (indices[update_index] + 1) % lengths[update_index];
+
+                // if index did not roll over, or we are at the first element, stop looping
+                if indices[update_index] != 0 || update_index == 0 {
+                    break;
+                }
+
+                update_index -= 1;
+            }
+
+            // if we have rolled over the first element, we have gone through all combinations
+            if update_index == 0 && indices[0] == 0 {
+                break;
+            }
+        }
+    }


This feels way more complicated than what I had imagined. I was thinking of something like this:

pub fn invert_filters(&mut self) { let mut acc = FilteredAccess::default(); for filter_set in self.filter_sets.drain(..) { let mut new = FilteredAccess::default(); for with in filter_set.with.ones() { let mut tmp = FilteredAccess::default(); tmp.and_without(with); new.append_or(&tmp); } for without in filter_set.with.ones() { let mut tmp = FilteredAccess::default(); tmp.and_with(without); new.append_or(&tmp); } acc.extend(&new); } *self = acc; }

The idea is that !((... & ...) | (... & ...) | ...) = !(... & ...) & !(... & ...) & ... = (!... | !...) & (!... | !...) & ..., so you just need to invert the singlular terms in each conjunction (using and_without for the existing with and and_with for the existing without), combine them in a disjunction (using append_or, but could be done more efficiently) and then combine everything together in a conjunction (using extend).

I'm also not sure what should be done with self.accesses (not to be confused with your accessed local variable)

CatThingy added 5 commits December 27, 2023 20:32

initial attempt

aa74d25

Improve tests

40be6e9

Reduce scope, add docs

8dd8147

formatting

c52c273

Improve docs

b605cac

MrGVSV added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events labels Dec 28, 2023

fix ci

20875bd

alice-i-cecile reviewed Dec 28, 2023

View reviewed changes

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

alice-i-cecile reviewed Dec 28, 2023

View reviewed changes

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

alice-i-cecile requested changes Dec 28, 2023

View reviewed changes

alice-i-cecile added this to the 0.13 milestone Dec 28, 2023

Address review feedback

b09e519

SkiFire13 suggested changes Dec 29, 2023

View reviewed changes

alice-i-cecile removed this from the 0.13 milestone Dec 29, 2023

alice-i-cecile added the D-Complex Quite challenging from either a design or technical perspective. Ask for help! label Dec 29, 2023

CatThingy added 2 commits December 29, 2023 13:42

Make inversion of filters more robust

a2dd8c0

fix ci

16589c5

SkiFire13 suggested changes Dec 30, 2023

View reviewed changes

CatThingy added 3 commits December 30, 2023 03:35

Correctly test robustness of Not filter

23d7cf6

Initial implementation of correct FilteredAccess inversion

3242d59

Move filter inversion into FilteredAccess

17c8af1

SkiFire13 reviewed Jan 3, 2024

View reviewed changes

CatThingy closed this Jun 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Add a limited `Not` query filter #11116

Add a limited `Not` query filter #11116

Uh oh!

CatThingy commented Dec 28, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

alice-i-cecile left a comment

Uh oh!

Uh oh!

SkiFire13 Dec 29, 2023

Uh oh!

CatThingy Dec 29, 2023

Uh oh!

SkiFire13 Jan 3, 2024

Uh oh!

bushrat011899 Feb 8, 2024

Uh oh!

SkiFire13 left a comment

Uh oh!

SkiFire13 Dec 29, 2023

Uh oh!

SkiFire13 Dec 29, 2023

Uh oh!

SkiFire13 Dec 30, 2023

Uh oh!

hymm commented Jan 3, 2024

Uh oh!

SkiFire13 left a comment

Uh oh!

SkiFire13 Jan 3, 2024

Uh oh!

SkiFire13 Jan 3, 2024

Uh oh!

Uh oh!

Uh oh!

Add a limited Not query filter #11116

Add a limited Not query filter #11116

Uh oh!

Conversation

CatThingy commented Dec 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Implementation note and future work

Changelog

Uh oh!

Uh oh!

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SkiFire13 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hymm commented Jan 3, 2024

Uh oh!

SkiFire13 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Add a limited `Not` query filter #11116

Add a limited `Not` query filter #11116

CatThingy commented Dec 28, 2023 •

edited

Loading