Add DefaultQueryFilters

[NiseVoid]

Objective

Some usecases in the ecosystems are blocked by the inability to stop bevy internals and third party plugins from touching their entities. However the specifics of a general purpose entity disabling system are controversial and further complicated by hierarchies. We can partially unblock these usecases with an opt-in approach: default query filters.

Solution

• Introduce DefaultQueryFilters, these filters are automatically applied to queries that don't otherwise mention the filtered component.
• End users and third party plugins can register default filters and are responsible for handling entities they have hidden this way.
• Extra features can be left for after user feedback
• The default value could later include official ways to hide entities

---

Changelog

• Add DefaultQueryFilters

[iiYese]

Two things you should probably do:

• Add a way to bypass default filters.
• Change .retain. Make it so it doesn't remove components that are filtered as Without & add another variant that bypasses that.

[NiseVoid]

Add a way to bypass default filters.

This seems reasonable, yet it might also be problematic for things that get added to the list in the future. It's hard to anticipiate all the uses for this before people have a chance to use it. It's hard to say if a universal bypass makes sense or if we'd be better off with some alternative approach like assigning a group to each filter, and allowing groups to be bypassed. Or perhaps bypassing default filters won't even be needed in practice. It might be better to leave it until people report running into issues, or until we start including default features and know we need a way to bypass them collectively.

Change .retain. Make it so it doesn't remove components that are filtered as Without & add another variant that bypasses that.

This behavior feels a bit implicit, but I think creating a list of components retain shouldn't touch could make a lot of sense, it would also be a lot easier to explain and reason about for users. There might even be uses that do want to bypass retain but not be filtered out, or the other way around. But it should probably be in a separate PR. The bypass variant might also have the same issues as the point above.

[iiYese]

This behavior feels a bit implicit, but I think creating a list of components retain shouldn't touch could make a lot of sense

At the very least it should be the same list. It shouldn't be a different one. I still think it would be a good idea to include this now anyway even if you don't want to add bypasses yet because introducing it later will be a breaking change & using it without default filters applied will surprise users.

[NiseVoid]

At the very least it should be the same list.

I'm thinking more along the lines: Most users wouldn't manually add default filters, we'll probably want built-in abstractions on top in a future release. Meaning they'd probably be configured by plugins (something like a DisabledPlugin<T> or a HidePlugin<T>), and at this point the list of plugins serves as the list, which this list and a "retain filters" list could then be derived from.

Tho I've never actually used retain so it's a bit hard for me to comment on how it should behave.

[re0312]

The code and documentation are very clean and impressive ,but current impl is unsound. The following code will panic. To make it work correctly, we should only allow setting table component filter.

    let mut world = World::new();
    world.spawn((Table(1), Sparse(1)));
    world.spawn(Table(2));

    world.set_default_with_filter::<Sparse>();

    let mut query = QueryState::<&Table>::new(&mut world);
    // panic!
    assert_eq!(1, query.iter(&world).count());

IMO ,global query configuration will greatly increase the coupling between different parts of the code. If we decide to land it, we should mark the relevant APIs as Unsafe .

Additionally, considering that it will likely be widely used by third-party libraries in the foreseeable future, the configuration of one plugin may easily disrupt the usage of another plugin, even if they appear unrelated.This will undoubtedly increase the maintenance cost for third parts author as they must consider compatibility with other unofficial plugins.

I personally lean towards implementing it after query as entities landed. it will unlock query-level custom settings, which could have more granular control, However, considering the strong demand for this feature within the community and the current implementation's adequacy,I'll defer my opinion to those with a more comprehensive understanding to decide.

[NiseVoid]

but current impl is unsound

Looking at the behavior it doesn't look like it's unsound in the usual Rust way, but this is definitely a potential footgun since it just straight up ignores the added filter without even giving you a warning.

To make it work correctly, we should only allow setting table component filter.

This does seem like the right call, in theory we could fix the filtering for sparse components, but setting a default filter for a sparse component would be very expensive and doesn't seem like something we should allow.

@alice-i-cecile what is your opinion on this? And if we do block it, how should we let the user know, call a warn! and then ignore the filter?

IMO ,global query configuration will greatly increase the coupling between different parts of the code. If we decide to land it, we should mark the relevant APIs as Unsafe .

I don't think this matches what should be considered unsafe in Rust. As long as no memory safety invariants are introduced the code should be safe, even if you can use it to create logic errors.

Additionally, considering that it will likely be widely used by third-party libraries in the foreseeable future, the configuration of one plugin may easily disrupt the usage of another plugin, even if they appear unrelated.This will undoubtedly increase the maintenance cost for third parts author as they must consider compatibility with other unofficial plugins.

This shouldn't be a major problem as long as you don't use any libraries that abuse this feature, you would specifically need to have two systems adding a filtered component at the same time (between command queue flushes) to see any issues. The chance of such issues happening would of course be greatly reduced once we build a higher level disabling API on top of this feature.

[SkiFire13]

it just straight up ignores the added filter without even giving you a warning.

However those filters were used to ensure that the query doesn't conflict with other system parameters, ignoring them would mean that now it can conflict with them.

I haven't actually tested this, but consider a situation like this:

• a default filter with With<Sparse>
• a query Query<&mut Dense>, which gets the default filter;
  • access-wise this is equivalent to Query<&mut Dense, With<Sparse>>
  • in practice it actually fetches the same entities as Query<&mut Dense>
• another query Query<&mut Dense, Without<Sparse>>
  • in theory this does not conflict with the previous query because the two filters do no overlap
  • in practice however the previous query also fetches the &mut Dense fetched by this query, so they do overlap, hence this is unsound

[NiseVoid]

Oh, yes, that does indeed make it unsound. Either way we definitely need to fix it by either 1. preventing sparse filters from being added to the list; or 2. actually filtering them. Even if you can't create UB with it, the current behavior would be extremely confusing :thinking:

[re0312]

This does seem like the right call, in theory we could fix the filtering for sparse components, but setting a default filter for a sparse component would be very expensive and doesn't seem like something we should allow.

It's not easy to fix sparse components without a performance drop. IIRC to address it to support sparse compoent safely, we must introduce a branch in the hot path, which could significantly impact our performance.

I don't think this matches what should be considered unsafe in Rust. As long as no memory safety invariants are introduced the code should be safe, even if you can use it to create logic errors.

It may not be a memory issue, but the impact is too extensive and could greatly increase the vulnerability of the code. I strongly recommend marking it as unsafe so that people who use it should fully understand the implications of their action.

Additionally, set_default_with_filter only affects queries created before it's set. As we widely use cached queries in the system, it does not affect cached-queries after schedule initialization, even if we set it later. This means if we set it before schedule initialization, the influence will be global and cannot be canceled at runtime. If we set it at runtime to build a query, why not use QueryBuilder to create what we want instead?

systemA(world:&mut World){
    world.set_default_with_filter::<A>();   
}

SystemB(query:Query<&B>){

    // The system query will not be affected by set_default_with_filter 
    // because its query state was created during schedule initialization.
    --------------------
}

App.add_systems(Update ,(SystemA,SystemB).chain());

[SkiFire13]

Note that while restricting filters to dense components would fix this issue, it would prevent future optimizations to sparse only queries (e.g. Query<&Sparse> could be optimized to iterate over only the relative sparse set instead of iterating the various archetypes first). This is a tradeoff between what you can statically assume at compile time for optimizations and what you want to make dynamic.

[iiYese]

From exploring this as the only viable alternative to entity disabling we've discovered:

• Unsoundness from the limitations of current ECS internals
• Flaws with the approach before in #ecs-dev

I think this should be enough conclusive motivation to push forward with the original entity disabling PR #12928 which was simpler and didn't have these problems. Most of the issues brought up with entity disabling were to do with poorly understood/conveyed semantics which is mostly down to naming, a problem that can/should be solved with docs rather than a completely different feature.

@NiseVoid @alice-i-cecile @JoJoJet

[NiseVoid]

If we limit the components you can register to ones with table storage, the unsoundness would be solved, and the only extra issue this approach introduces is the ordering (adding filters after queries are built), which can be solved once we have some way to access query caches and rebuild them (like Queries as Entities). The other mentioned issues (spare optimization and sander's comment on discord) already exist on the Disabled PR.

I think comparing the PRs as different solutions is weird, the Disabled PR relies on this behavior, it's just hardcoded into QueryState there, this PR pulls the behavior out into a reusable form. The goal being to allow third-party crates to experiment rather than trying to get first-party entity disabling merged entirely based off speculation.

[SkiFire13]

If we limit the components you can register to ones with table storage, the unsoundness would be solved

Note that this fix still leaves the possibility for future unsoundness if any new optimizations are done on WorldQuery. This essentially ties the unsafe internal optimizations of WorldQuery with default query filters.

and the only extra issue this approach introduces is the ordering (adding filters after queries are built), which can be solved once we have some way to access query caches and rebuild them (like Queries as Entities)

Will this work with manual QueryState/SystemState? For example if are stored in a Local?

[NiseVoid]

Note that this fix still leaves the possibility for future unsoundness if any new optimizations are done on WorldQuery. This essentially ties the unsafe internal optimizations of WorldQuery with default query filters.

I think those optimizations would all touch QueryState, and these filters are also applied there. This unsound behavior could also probably be caused trough other means, since it's ultimately caused by some odd behavior in QueryState and the coupling of filters and the rules used to avoid conflicting queries.

Will this work with manual QueryState/SystemState? For example if are stored in a Local?

I think that ultimately depends on the implementation, but considering updating caches is necessary for a few prerequisites of relations I'd assume that will work just fine, unless it's a non-cached query, in which case we could just have it always fetch the current filters

[JeanMertz]

I think this should be enough conclusive motivation to push forward with the original entity disabling PR #12928 which was simpler and didn't have these problems.

I would note that PR triggered the creation of a (draft) RFC at https://github.com/bevyengine/rfcs/pull/81. Given the back-and-forth discussion and open questions by @mockersf in https://github.com/bevyengine/bevy/pull/12928#issuecomment-2054175843, it does seem desirable to push that RFC forward first, get it signed off, and then get #12928 into a mergeable state.

[NiseVoid]

Originally my plan too was to push the RFC forward and create entity disabling as a complete PR at once. However, the RFC never got any feedback, and this is a pattern all RFCs share. Additionally creating an RFC almost blind here is nearly impossible considering the complexity introduced by the issues surrounding hierarchies, and the controversiality of copying existing designs of things like despawn_recursive which people already want to get rid of.

We have since introduced working groups, which seems like the better route for entity disabling. But to move forward with a working group it needs to be possible to experiment with some designs. That's why this PR exist, to unblock the ability to experiment with designs, rather than require everyone to use some weird bevy fork that makes it extremely hard to try any design in real projects.

[NiseVoid]

I think with @SkiFire13's fix (#14615) it should be possible to fix the sparse-related problems. I have updated the PR and added a test for it and it seems to work as one would expect. I have somehow never been able to reproduce @re0312's panic, but it also completely ignored sparse filters previously, which caused unsound behavior as queries that got detected as not overlapping could alias mutable references.

[alice-i-cecile]

After discussion with @maniwani, we have approval from two SME-ECS: them and myself.

As a follow-up, we should add a first-party Disabled marker component for ecosystem coordination purposes, and include a default default query filter.

[iiYese]

I'm going to raise a concern I've already mentioned on discord. If we have a Disabled marker it is easy to define what is considered erroneous for that. Eg. mutating an entity while it's disabled. It's also easy for authors to handle the addition & removal of Disabled via hooks & observers. This is not the case for default filters. Default filters will have completely different semantics depending on the author & there is no way for authors to handle the addition & removal of all filters from other authors. It would create users needing to request plugin authors for compatibility hooks for other plugins.