-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
Allow users to register their own disabling components / default query filters #17768
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
58a62ce
f3029ee
37fd363
0bb3165
9904e4c
3dc884d
b6f93b1
997e548
014ba3d
8a0dc86
00f8657
1bbd764
37f32f6
7cbe1d7
92806af
b879c3f
32383b5
bf1cf7d
3cb3d8d
ac17cc7
db954f5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,21 +1,67 @@ | ||
| //! Types for entity disabling. | ||
| //! | ||
| //! Disabled entities do not show up in queries unless the query explicitly mentions them. | ||
| //! | ||
| //! If for example we have `Disabled` as an entity disabling component, when you add `Disabled` | ||
| //! to an entity, the entity will only be visible to queries with a filter like | ||
| //! [`With`]`<Disabled>` or query data like [`Has`]`<Disabled>`. | ||
| //! Entities which are disabled in this way are not removed from the [`World`], | ||
| //! and their relationships remain intact. | ||
| //! In many cases, you may want to disable entire trees of entities at once, | ||
| //! using [`EntityCommands::insert_recursive`](crate::prelude::EntityCommands::insert_recursive). | ||
| //! | ||
| //! While Bevy ships with a built-in [`Disabled`] component, you can also create your own | ||
| //! disabling components, which will operate in the same way but can have distinct semantics. | ||
| //! | ||
| //! ``` | ||
| //! use bevy_ecs::prelude::*; | ||
| //! | ||
| //! // Our custom disabling component! | ||
| //! #[derive(Component, Clone)] | ||
| //! struct Prefab; | ||
| //! | ||
| //! #[derive(Component)] | ||
| //! struct A; | ||
| //! | ||
| //! let mut world = World::new(); | ||
| //! world.register_disabling_component::<Prefab>(); | ||
| //! world.spawn((A, Prefab)); | ||
| //! world.spawn((A,)); | ||
| //! world.spawn((A,)); | ||
| //! | ||
| //! let mut normal_query = world.query::<&A>(); | ||
| //! assert_eq!(2, normal_query.iter(&world).count()); | ||
| //! | ||
| //! let mut prefab_query = world.query_filtered::<&A, With<Prefab>>(); | ||
| //! assert_eq!(1, prefab_query.iter(&world).count()); | ||
| //! | ||
| //! let mut maybe_prefab_query = world.query::<(&A, Has<Prefab>)>(); | ||
| //! assert_eq!(3, maybe_prefab_query.iter(&world).count()); | ||
| //! ``` | ||
| //! | ||
| //! ## Default query filters | ||
| //! | ||
| //! In Bevy, entity disabling is implemented through the construction of a global "default query filter". | ||
| //! Queries which do not explicitly mention the disabled component will not include entities with that component. | ||
| //! If an entity has multiple disabling components, it will only be included in queries that mention all of them. | ||
| //! | ||
| //! 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. | ||
| //! | ||
| //! Entities with disabling components are still present in the [`World`] and can be accessed directly, | ||
| //! using methods on [`World`] or [`Commands`](crate::prelude::Commands). | ||
| //! | ||
| //! ### Note | ||
| //! ### Warnings | ||
| //! | ||
| //! 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. | ||
|
Comment on lines
-11
to
54
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can add a check for this and an internal bool. Lemme toss that behind a debug asserts flag...
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| //! | ||
| //! Because filters are applied to all queries they can have performance implication for | ||
| //! the enire [`World`], especially when they cause queries to mix sparse and table components. | ||
| //! See [`Query` performance] for more info. | ||
| //! | ||
| //! Custom disabling components can cause significant interoperability issues within the ecosystem, | ||
| //! as users must be aware of each disabling component in use. | ||
| //! Libraries should think carefully about whether they need to use a new disabling component, | ||
| //! and clearly communicate their presence to their users to avoid the new for library compatibility flags. | ||
| //! | ||
| //! [`With`]: crate::prelude::With | ||
| //! [`Has`]: crate::prelude::Has | ||
| //! [`World`]: crate::prelude::World | ||
|
|
@@ -24,52 +70,126 @@ | |
| use crate::{ | ||
| component::{ComponentId, Components, StorageType}, | ||
| query::FilteredAccess, | ||
| world::{FromWorld, World}, | ||
| }; | ||
| use bevy_ecs_macros::{Component, Resource}; | ||
| use smallvec::SmallVec; | ||
|
|
||
| #[cfg(feature = "bevy_reflect")] | ||
| use {crate::reflect::ReflectComponent, bevy_reflect::Reflect}; | ||
|
|
||
| /// A marker component for disabled entities. See [the module docs] for more info. | ||
| /// A marker component for disabled entities. | ||
| /// | ||
| /// 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, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 😅 |
||
| /// not its children or other entities that reference it. | ||
| /// To disable an entire tree of entities, use [`EntityCommands::insert_recursive`](crate::prelude::EntityCommands::insert_recursive). | ||
| /// | ||
| /// Every [`World`] has a default query filter that excludes entities with this component, | ||
| /// registered in the [`DefaultQueryFilters`] resource. | ||
| /// See [the module docs] for more info. | ||
| /// | ||
| /// [the module docs]: crate::entity_disabling | ||
| #[derive(Component)] | ||
| #[cfg_attr(feature = "bevy_reflect", derive(Reflect), reflect(Component))] | ||
| #[derive(Component, Clone, Debug)] | ||
| #[cfg_attr( | ||
| feature = "bevy_reflect", | ||
| derive(Reflect), | ||
| reflect(Component), | ||
| reflect(Debug) | ||
| )] | ||
| // This component is registered as a disabling component during World::bootstrap | ||
| pub struct Disabled; | ||
|
|
||
| /// The default filters for all queries, these are used to globally exclude entities from queries. | ||
| /// Default query filters work by excluding entities with certain components from most queries. | ||
| /// | ||
| /// If a query does not explicitly mention a given disabling component, it will not include entities with that component. | ||
| /// To be more precise, this checks if the query's [`FilteredAccess`] contains the component, | ||
| /// and if it does not, adds a [`Without`](crate::prelude::Without) filter for that component to the query. | ||
| /// | ||
| /// This resource is initialized in the [`World`] whenever a new world is created, | ||
| /// with the [`Disabled`] component as a disabling component. | ||
| /// | ||
| /// Note that you can remove default query filters by overwriting the [`DefaultQueryFilters`] resource. | ||
| /// This can be useful as a last resort escape hatch, but is liable to break compatibility with other libraries. | ||
| /// | ||
| /// See the [module docs](crate::entity_disabling) for more info. | ||
| #[derive(Resource, Default, Debug)] | ||
| /// | ||
| /// | ||
| /// # Warning | ||
| /// | ||
| /// Default query filters are a global setting that affects all queries in the [`World`], | ||
| /// and incur a small performance cost for each query. | ||
| /// | ||
| /// They can cause significant interoperability issues within the ecosystem, | ||
| /// as users must be aware of each disabling component in use. | ||
| /// | ||
| /// Think carefully about whether you need to use a new disabling component, | ||
| /// and clearly communicate their presence in any libraries you publish. | ||
| #[derive(Resource, Debug)] | ||
| #[cfg_attr(feature = "bevy_reflect", derive(bevy_reflect::Reflect))] | ||
alice-i-cecile marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As there is only one
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Oh, I don't know! I think they both iterate by |
||
| disabling: SmallVec<[ComponentId; 4]>, | ||
| } | ||
|
|
||
| impl FromWorld for DefaultQueryFilters { | ||
| fn from_world(world: &mut World) -> Self { | ||
| let mut filters = DefaultQueryFilters::empty(); | ||
| let disabled_component_id = world.register_component::<Disabled>(); | ||
| filters.register_disabling_component(disabled_component_id); | ||
| filters | ||
| } | ||
| } | ||
|
|
||
| impl DefaultQueryFilters { | ||
| /// Set the [`ComponentId`] for the entity disabling marker | ||
| pub(crate) fn set_disabled(&mut self, component_id: ComponentId) -> Option<()> { | ||
| if self.disabled.is_some() { | ||
| return None; | ||
| /// Creates a new, completely empty [`DefaultQueryFilters`]. | ||
| /// | ||
| /// This is provided as an escape hatch; in most cases you should initialize this using [`FromWorld`], | ||
| /// which is automatically called when creating a new [`World`]. | ||
| #[must_use] | ||
| pub fn empty() -> Self { | ||
| DefaultQueryFilters { | ||
| disabling: SmallVec::new(), | ||
| } | ||
| } | ||
|
|
||
| /// Adds this [`ComponentId`] to the set of [`DefaultQueryFilters`], | ||
| /// causing entities with this component to be excluded from queries. | ||
| /// | ||
| /// This method is idempotent, and will not add the same component multiple times. | ||
| /// | ||
| /// # Warning | ||
| /// | ||
| /// This method should only be called before the app starts, as it will not affect queries | ||
| /// initialized before it is called. | ||
| /// | ||
| /// As discussed in the [module docs](crate::entity_disabling), this can have performance implications, | ||
| /// as well as create interoperability issues, and should be used with caution. | ||
| pub fn register_disabling_component(&mut self, component_id: ComponentId) { | ||
| if !self.disabling.contains(&component_id) { | ||
| self.disabling.push(component_id); | ||
| } | ||
| self.disabled = Some(component_id); | ||
| Some(()) | ||
| } | ||
|
|
||
| /// Get an iterator over all currently enabled filter components | ||
| pub fn ids(&self) -> impl Iterator<Item = ComponentId> { | ||
| [self.disabled].into_iter().flatten() | ||
| /// Get an iterator over all of the components which disable entities when present. | ||
| pub fn disabling_ids(&self) -> impl Iterator<Item = ComponentId> + use<'_> { | ||
| self.disabling.iter().copied() | ||
| } | ||
|
|
||
| pub(super) fn apply(&self, component_access: &mut FilteredAccess<ComponentId>) { | ||
| for component_id in self.ids() { | ||
| /// Modifies the provided [`FilteredAccess`] to include the filters from this [`DefaultQueryFilters`]. | ||
| pub(super) fn modify_access(&self, component_access: &mut FilteredAccess<ComponentId>) { | ||
| for component_id in self.disabling_ids() { | ||
| if !component_access.contains(component_id) { | ||
| component_access.and_without(component_id); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| pub(super) fn is_dense(&self, components: &Components) -> bool { | ||
| self.ids().all(|component_id| { | ||
| self.disabling_ids().all(|component_id| { | ||
| components | ||
| .get_info(component_id) | ||
| .is_some_and(|info| info.storage_type() == StorageType::Table) | ||
|
|
@@ -81,24 +201,16 @@ impl DefaultQueryFilters { | |
| mod tests { | ||
|
|
||
| use super::*; | ||
| use crate::{ | ||
| prelude::World, | ||
| query::{Has, With}, | ||
| }; | ||
| use alloc::{vec, vec::Vec}; | ||
|
|
||
| #[test] | ||
| fn test_set_filters() { | ||
| let mut filters = DefaultQueryFilters::default(); | ||
| assert_eq!(0, filters.ids().count()); | ||
|
|
||
| assert!(filters.set_disabled(ComponentId::new(1)).is_some()); | ||
| assert!(filters.set_disabled(ComponentId::new(3)).is_none()); | ||
|
|
||
| assert_eq!(1, filters.ids().count()); | ||
| assert_eq!(Some(ComponentId::new(1)), filters.ids().next()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_apply_filters() { | ||
| let mut filters = DefaultQueryFilters::default(); | ||
| filters.set_disabled(ComponentId::new(1)); | ||
| fn filters_modify_access() { | ||
| let mut filters = DefaultQueryFilters::empty(); | ||
| filters.register_disabling_component(ComponentId::new(1)); | ||
|
|
||
| // A component access with an unrelated component | ||
| let mut component_access = FilteredAccess::<ComponentId>::default(); | ||
|
|
@@ -107,7 +219,7 @@ mod tests { | |
| .add_component_read(ComponentId::new(2)); | ||
|
|
||
| let mut applied_access = component_access.clone(); | ||
| filters.apply(&mut applied_access); | ||
| filters.modify_access(&mut applied_access); | ||
| assert_eq!(0, applied_access.with_filters().count()); | ||
| assert_eq!( | ||
| vec![ComponentId::new(1)], | ||
|
|
@@ -118,7 +230,7 @@ mod tests { | |
| component_access.and_with(ComponentId::new(4)); | ||
|
|
||
| let mut applied_access = component_access.clone(); | ||
| filters.apply(&mut applied_access); | ||
| filters.modify_access(&mut applied_access); | ||
| assert_eq!( | ||
| vec![ComponentId::new(4)], | ||
| applied_access.with_filters().collect::<Vec<_>>() | ||
|
|
@@ -133,7 +245,7 @@ mod tests { | |
| component_access.and_with(ComponentId::new(1)); | ||
|
|
||
| let mut applied_access = component_access.clone(); | ||
| filters.apply(&mut applied_access); | ||
| filters.modify_access(&mut applied_access); | ||
| assert_eq!( | ||
| vec![ComponentId::new(1), ComponentId::new(4)], | ||
| applied_access.with_filters().collect::<Vec<_>>() | ||
|
|
@@ -147,11 +259,46 @@ mod tests { | |
| .add_archetypal(ComponentId::new(1)); | ||
|
|
||
| let mut applied_access = component_access.clone(); | ||
| filters.apply(&mut applied_access); | ||
| filters.modify_access(&mut applied_access); | ||
| assert_eq!( | ||
| vec![ComponentId::new(4)], | ||
| applied_access.with_filters().collect::<Vec<_>>() | ||
| ); | ||
| assert_eq!(0, applied_access.without_filters().count()); | ||
| } | ||
|
|
||
| #[derive(Component)] | ||
| struct CustomDisabled; | ||
|
|
||
| #[test] | ||
| fn multiple_disabling_components() { | ||
| let mut world = World::new(); | ||
| world.register_disabling_component::<CustomDisabled>(); | ||
|
|
||
| world.spawn_empty(); | ||
| world.spawn(Disabled); | ||
| world.spawn(CustomDisabled); | ||
| world.spawn((Disabled, CustomDisabled)); | ||
|
|
||
| let mut query = world.query::<()>(); | ||
| assert_eq!(1, query.iter(&world).count()); | ||
|
|
||
| let mut query = world.query_filtered::<(), With<Disabled>>(); | ||
| assert_eq!(1, query.iter(&world).count()); | ||
|
|
||
| let mut query = world.query::<Has<Disabled>>(); | ||
| assert_eq!(2, query.iter(&world).count()); | ||
|
|
||
| let mut query = world.query_filtered::<(), With<CustomDisabled>>(); | ||
| assert_eq!(1, query.iter(&world).count()); | ||
|
|
||
| let mut query = world.query::<Has<CustomDisabled>>(); | ||
| assert_eq!(2, query.iter(&world).count()); | ||
|
|
||
| let mut query = world.query_filtered::<(), (With<Disabled>, With<CustomDisabled>)>(); | ||
| assert_eq!(1, query.iter(&world).count()); | ||
|
|
||
| let mut query = world.query::<(Has<Disabled>, Has<CustomDisabled>)>(); | ||
| assert_eq!(4, query.iter(&world).count()); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this should include an example of a
Querythat matches entities that are both enabled and disabled. The ones shown match only entities that are disabled.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has matches both!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right. It still feels kinda hacky though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's awkward to have an extra
boolthat isn't used, and it will count the component as archetypal access, which is somewhat misleading.Would it make sense to have a
QueryFilterwith the meaning "include entities whether or not they haveT, even though it's normally excluded by default query filters"? (That wouldn't be part of this PR, of course.)