Allow users to register their own disabling components / default query filters #17768

alice-i-cecile · 2025-02-10T00:50:22Z

Objective

Currently, default query filters, as added in #13120 / #17514 are hardcoded to only use a single query filter.

This is limiting, as multiple distinct disabling components can serve important distinct roles. I ran into this limitation when experimenting with a workflow for prefabs, which don't represent the same state as "an entity which is temporarily nonfunctional".

Solution

Change DefaultQueryFilters to store a SmallVec of ComponentId, rather than an Option.
Expose methods on DefaultQueryFilters, World and App to actually configure this.
While we're here, improve the docs, write some tests, make use of FromWorld and make some method names more descriptive.

Follow-up

I'm not convinced that supporting sparse set disabling components is useful, given the hit to iteration performance and runtime checks incurred. That's disjoint from this PR though, so I'm not doing it here. The existing warnings are fine for now.

Testing

I've added both a doc test and an mid-level unit test to verify that this works!

cBournhonesque · 2025-02-10T04:37:35Z

I also need this, but wasn't there concerns in the previous PR that forces us to abandon letting users define their own disabled components?

For example using a sparse component as a disabling component was incorrect, or something to that effect

MrGVSV · 2025-02-10T15:26:36Z

Does this automatically work with scenes?

alice-i-cecile · 2025-02-10T17:43:39Z

Does this automatically work with scenes?

Can you say more about what you mean? Scenes aren't hooked up to any prefab system yet for example.

alice-i-cecile · 2025-02-10T18:00:01Z

On sparse set filters: #13120 (comment) and #13120 (comment). However, the soundness issues appear to be resolved: #13120 (comment)

That said, I'm happy to just forbid them: I think they're a serious performance footgun.

From @NiseVoid on Discord:

This brings back the reason that blocked DQF at the end, it encourages people to create multiple components for Disabled and you won't be able to see them again unless you disable all filters, which has other annoying side effects (picking up prefabs as well for example)
The description also contradicts the implementation, it says "prefab" doesn't "disable", yet as far as the code is concerned it totally does disable
The naming of methods and the use of a HashSet rather than a Vec further suggest that adding mountains of disabling components is the desired use of the feature 🤔

A Vec is likely faster / more sensible, or even a SmallVec. I don't expect more than a handful of these to exist.

I'll also clean up the description.

We can discuss the ecosystem concerns further: I share some of them, but I also think they're broadly hypothetical. Crates can already effectively have "private entities" by making their component types public or nesting them in a subworld or something: that hasn't been a serious problem AFAIK.

NiseVoid · 2025-02-10T18:08:45Z

I think the problem is largely a documentation and method naming issue fwiw.

There are cases where the concerns would apply, like if someone creates dozens of alternatives to Disabled, users and other crate will struggle to find that entity when they want to. But cases like Prefab or something like an IsResource filter would have no overlap with Disabled, and thus don't create this problem.

MrGVSV · 2025-02-10T18:55:26Z

Does this automatically work with scenes?

Can you say more about what you mean? Scenes aren't hooked up to any prefab system yet for example.

Iirc the original PR to add Disabled auto filtered it out of saving to scenes. For custom disabling components, users may also want that behavior (or a way to opt out of it).

alice-i-cecile · 2025-02-11T01:19:33Z

Iirc the original PR to add Disabled auto filtered it out of saving to scenes. For custom disabling components, users may also want that behavior (or a way to opt out of it).

Looking at #17514, this PR actually opts DefaultQueryFilters out of being saved to scenes as a resource. Which is sensible enough but doesn't pose any new problems for this API :) See https://github.com/bevyengine/bevy/pull/17514/files#diff-978e33bfbd643635947d2a46e239e7cc222d75541cd32fd8de6cd4f7fe1ab19dR354

alice-i-cecile · 2025-02-11T01:35:10Z

Alright, I've added more documentation, cleaned up the PR description, swapped to a SmallVec and made a nice little Default -> FromWorld change. I think this is in a good spot to merge, but y'all are the reviewers.

@SkiFire13, could I get your review here as well? You had concerns about the original DQF, and I want to be sure that you're okay with the tradeoffs we're making here.

crates/bevy_ecs/src/entity_disabling.rs

Carter0 · 2025-02-11T03:36:04Z

crates/bevy_ecs/src/entity_disabling.rs

+/// Semantically, this component is used to mark entities that are temporarily disabled (typically for gameplay reasons),
+/// but will likely be re-enabled at some point.
+///
+/// Like all disabling components, this only disables the entity itself,


For all lot of these comments here, I think you can just reference what you wrote at the top of the module.

Might be good to build the expectation for users that they should look to the top of the module for most of the comments. That's where most of the info is likely to be I suspect, so that way users can get all the info rather than bits and pieces. Also it prevents us from having to maintain all these comments if they go out of date.

I also think this would be a nice thing to get new contributors to do too. We have a lot of docs and centralizing them could be a good way to learn about the engine. IDK food for thought

I think that's a good strategy in general! In this particular case, there are enough concerns that I want to go overkill on the warnings. I'd really like to avoid having to take this toy away in the future 😅

Carter0

I like it!

SkiFire13

I'm a bit out of loop and recently got a new job so I don't have lot of time to fully review the code. I gave a quick glance and left a couple of comments, from what I can see nothing much has changed in terms of tradeoffs, so I think I have the same concerns as before, but if you're ok with them you could give this a try.

SkiFire13 · 2025-02-11T07:57:16Z

crates/bevy_ecs/src/entity_disabling.rs

+//! For example, `Query<&Position>` will not include entities with the [`Disabled`] component,
+//! even if they have a `Position` component,
+//! but `Query<&Position, With<Disabled>>` or `Query<(&Position, Has<Disabled>)>` will see them.


IMO this should include an example of a Query that matches entities that are both enabled and disabled. The ones shown match only entities that are disabled.

Has matches both!

Ah right. It still feels kinda hacky though

Yeah, it's awkward to have an extra bool that isn't used, and it will count the component as archetypal access, which is somewhat misleading.

Would it make sense to have a QueryFilter with the meaning "include entities whether or not they have T, even though it's normally excluded by default query filters"? (That wouldn't be part of this PR, of course.)

SkiFire13 · 2025-02-11T07:59:27Z

crates/bevy_ecs/src/entity_disabling.rs

-//! Currently only queries for which the cache is built after enabling a filter will have entities
+//! Currently, only queries for which the cache is built after enabling a default query filter will have entities
 //! with those components filtered. As a result, they should generally only be modified before the
 //! app starts.


My concern with this still stands, but I don't see an easy way to solve it. Maybe there could be a warning in the console if someone attempts this? Or maybe this could be exposed mainly on App.

We can add a check for this and an internal bool. Lemme toss that behind a debug asserts flag...

Ugh, I'm not happy with the design I had in mind there. Doing this will be really fragile due to how plugin initialization works, and I think the false positive rate will be too high.

urben1680 · 2025-02-11T08:13:21Z

I would like this feature, crate owners can disable entities "for their own reasons" that cannot be accidentally enabled by other parts of the code.

cBournhonesque

Great docs!

chescock · 2025-02-11T14:43:26Z

crates/bevy_ecs/src/entity_disabling.rs

 #[cfg_attr(feature = "bevy_reflect", derive(bevy_reflect::Reflect))]
 pub struct DefaultQueryFilters {
-    disabled: Option<ComponentId>,
+    // We only expect a few components per application to act as disabling components, so we use a SmallVec here
+    // to avoid heap allocation in most cases.


As there is only one DefaultQueryFilters value per world, it seems like overkill to worry about one heap allocation, but it's harmless at worst. :)

Won't iterating over the vector be slower than if we use a smallvec? I'm less concerned about the initial allocation than I am about maximizing iteration speed at tiny n.

Won't iterating over the vector be slower than if we use a smallvec?

Oh, I don't know! I think they both iterate by Derefing to a slice, so I'd expect it to be similar. I think smallvec has an extra branch to check whether it's inline or heap-allocated, but on the other hand the data is closer by so there might be better cache locality. I don't expect it to be a big deal either way.

crates/bevy_ecs/src/entity_disabling.rs

alice-i-cecile · 2025-02-11T18:12:30Z

Thanks for the last minute nits @chescock, and thanks for the reviews to everyone!

alice-i-cecile added 9 commits February 9, 2025 15:38

Rewrite entity disabling module docs

58a62ce

Derive Clone and Debug for Disabled

f3029ee

Rename set_disabled

37fd363

Use a hashset rather than a single Option to store DQF

0bb3165

Rename DefaultQueryFilters::apply

9904e4c

Improve docs for DefaultQueryFilters

3dc884d

More docs for Disabled

b6f93b1

Add public API for registering disabling components

997e548

Add test for multiple disabling components

014ba3d

alice-i-cecile requested a review from NiseVoid February 10, 2025 00:50

alice-i-cecile added this to the 0.16 milestone Feb 10, 2025

alice-i-cecile mentioned this pull request Feb 10, 2025

Add an example demonstrating how to use default query filtering and entity cloning to create a prefab system #17769

Draft

6 tasks

alice-i-cecile added 5 commits February 10, 2025 12:16

Interop warnings

8a0dc86

Use a SmallVec instead of a HashSet to optimize for very low N

00f8657

Fix up docs for disabling_ids

1bbd764

Make register_disalbling_component idempotent

37f32f6

Use FromWorld to make DQF initialization cleaner

7cbe1d7

alice-i-cecile added 2 commits February 10, 2025 17:21

Clippy note on new_without_default

92806af

Try moving the expect?

b879c3f

alice-i-cecile requested review from chescock, Aceeri and cBournhonesque February 11, 2025 01:28

Even more docs, focusing on insert_recursive

32383b5

alice-i-cecile commented Feb 11, 2025

View reviewed changes

crates/bevy_ecs/src/entity_disabling.rs Show resolved Hide resolved

alice-i-cecile added 2 commits February 10, 2025 17:49

Now redundant doc link

bf1cf7d

Remember to enable the smallvec feature in bevy_reflect

3cb3d8d

alice-i-cecile added the X-Contentious There are nontrivial implications that should be thought through label Feb 11, 2025

Carter0 reviewed Feb 11, 2025

View reviewed changes

Carter0 approved these changes Feb 11, 2025

View reviewed changes

SkiFire13 reviewed Feb 11, 2025

View reviewed changes

urben1680 approved these changes Feb 11, 2025

View reviewed changes

cBournhonesque approved these changes Feb 11, 2025

View reviewed changes

alice-i-cecile added this pull request to the merge queue Feb 11, 2025

chescock approved these changes Feb 11, 2025

View reviewed changes

alice-i-cecile mentioned this pull request Feb 11, 2025

Add a Includes query filter for working with disabling components #17803

Open

alice-i-cecile removed this pull request from the merge queue due to a manual request Feb 11, 2025

alice-i-cecile added 2 commits February 11, 2025 10:10

Return a copied iterator

ac17cc7

new -> empty

db954f5

alice-i-cecile enabled auto-merge February 11, 2025 18:12

alice-i-cecile added this pull request to the merge queue Feb 11, 2025

Merged via the queue into bevyengine:main with commit fcc77fe Feb 11, 2025
33 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow users to register their own disabling components / default query filters #17768

Allow users to register their own disabling components / default query filters #17768

alice-i-cecile commented Feb 10, 2025 •

edited

Loading

cBournhonesque commented Feb 10, 2025 •

edited

Loading

MrGVSV commented Feb 10, 2025

alice-i-cecile commented Feb 10, 2025 •

edited

Loading

alice-i-cecile commented Feb 10, 2025

NiseVoid commented Feb 10, 2025

MrGVSV commented Feb 10, 2025

alice-i-cecile commented Feb 11, 2025

alice-i-cecile commented Feb 11, 2025

Carter0 Feb 11, 2025 •

edited

Loading

alice-i-cecile Feb 11, 2025

Carter0 left a comment

SkiFire13 left a comment

SkiFire13 Feb 11, 2025

alice-i-cecile Feb 11, 2025

SkiFire13 Feb 11, 2025

chescock Feb 11, 2025

SkiFire13 Feb 11, 2025

alice-i-cecile Feb 11, 2025

alice-i-cecile Feb 11, 2025

urben1680 commented Feb 11, 2025 •

edited

Loading

cBournhonesque left a comment

chescock Feb 11, 2025

alice-i-cecile Feb 11, 2025

chescock Feb 11, 2025

alice-i-cecile commented Feb 11, 2025

Allow users to register their own disabling components / default query filters #17768

Allow users to register their own disabling components / default query filters #17768

Conversation

alice-i-cecile commented Feb 10, 2025 • edited Loading

Objective

Solution

Follow-up

Testing

cBournhonesque commented Feb 10, 2025 • edited Loading

MrGVSV commented Feb 10, 2025

alice-i-cecile commented Feb 10, 2025 • edited Loading

alice-i-cecile commented Feb 10, 2025

NiseVoid commented Feb 10, 2025

MrGVSV commented Feb 10, 2025

alice-i-cecile commented Feb 11, 2025

alice-i-cecile commented Feb 11, 2025

Carter0 Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Carter0 left a comment

Choose a reason for hiding this comment

SkiFire13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

urben1680 commented Feb 11, 2025 • edited Loading

cBournhonesque left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alice-i-cecile commented Feb 11, 2025

alice-i-cecile commented Feb 10, 2025 •

edited

Loading

cBournhonesque commented Feb 10, 2025 •

edited

Loading

alice-i-cecile commented Feb 10, 2025 •

edited

Loading

Carter0 Feb 11, 2025 •

edited

Loading

urben1680 commented Feb 11, 2025 •

edited

Loading