Skip to content
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

Add Disabled marker #12928

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions crates/bevy_ecs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,9 @@ pub mod prelude {
component::Component,
entity::{Entity, EntityMapper},
event::{Event, EventReader, EventWriter, Events},
query::{Added, AnyOf, Changed, Has, Or, QueryBuilder, QueryState, With, Without},
query::{
Added, AnyOf, Changed, Disabled, Has, Or, QueryBuilder, QueryState, With, Without,
},
removal_detection::RemovedComponents,
schedule::{
apply_deferred, apply_state_transition, common_conditions::*, Condition,
Expand All @@ -69,7 +71,7 @@ mod tests {
change_detection::Ref,
component::{Component, ComponentId},
entity::Entity,
query::{Added, Changed, FilteredAccess, QueryFilter, With, Without},
query::{Added, Changed, Disabled, FilteredAccess, QueryFilter, With, Without},
system::Resource,
world::{EntityRef, Mut, World},
};
Expand Down Expand Up @@ -1379,13 +1381,15 @@ mod tests {
#[test]
fn filtered_query_access() {
let mut world = World::new();
let disabled_id = world.init_component::<Disabled>();
let query = world.query_filtered::<&mut A, Changed<B>>();

let mut expected = FilteredAccess::<ComponentId>::default();
let a_id = world.components.get_id(TypeId::of::<A>()).unwrap();
let b_id = world.components.get_id(TypeId::of::<B>()).unwrap();
expected.add_write(a_id);
expected.add_read(b_id);
expected.and_without(disabled_id);
assert!(
query.component_access.eq(&expected),
"ComponentId access from query fetch and query filter should be combined"
Expand Down
6 changes: 6 additions & 0 deletions crates/bevy_ecs/src/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@ pub use par_iter::*;
pub use state::*;
pub use world_query::*;

use crate as bevy_ecs;
alice-i-cecile marked this conversation as resolved.
Show resolved Hide resolved

/// A marker component that excludes an entity from queries that don't specifically request them.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These docs need to be extended.

I would start with a doc test showing off how this differs from an ordinary component.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// A marker component that excludes an entity from queries that don't specifically request them.
/// A special marker component that excludes an entity from queries that don't specifically request them.

#[derive(bevy_ecs_macros::Component)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type needs to be Reflect, Clone and Copy, and the type should be registered in bevy_core. This is needed for various serialization use cases, namely networking and save games.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The component should probably also use sparse storage to avoid table moves.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. It would slow down the iteration in turn though. I think we stick to the simplest design for now until we can benchmark it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't slow down iteration unless you are accessing the component. Since it's a ZST that will likely be never.

Copy link
Contributor

@james-j-obrien james-j-obrien Apr 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh actually you're correct since most queries now need to filter on this, disregard.

pub struct Disabled;

/// A debug checked version of [`Option::unwrap_unchecked`]. Will panic in
/// debug modes if unwrapping a `None` or `Err` value in debug mode, but is
/// equivalent to `Option::unwrap_unchecked` or `Result::unwrap_unchecked`
Expand Down
72 changes: 69 additions & 3 deletions crates/bevy_ecs/src/query/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use crate::{
archetype::{Archetype, ArchetypeComponentId, ArchetypeGeneration, ArchetypeId},
component::{ComponentId, Tick},
entity::Entity,
prelude::FromWorld,
prelude::{Disabled, FromWorld},
query::{
Access, BatchingStrategy, DebugCheckedUnwrap, FilteredAccess, QueryCombinationIter,
QueryIter, QueryParIter,
Expand Down Expand Up @@ -146,6 +146,17 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
}
}

fn contains_disabled(
component_access: &FilteredAccess<ComponentId>,
disabled_id: ComponentId,
) -> bool {
component_access.access().has_read(disabled_id)
|| component_access.access().has_archetypal(disabled_id)
|| component_access.filter_sets.iter().any(|f| {
f.with.contains(disabled_id.index()) || f.without.contains(disabled_id.index())
})
}

impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
/// Creates a new [`QueryState`] from a given [`World`] and inherits the result of `world.id()`.
pub fn new(world: &mut World) -> Self {
Expand Down Expand Up @@ -190,6 +201,12 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
// properly considered in a global "cross-query" context (both within systems and across systems).
component_access.extend(&filter_component_access);

let disabled_id = world.init_component::<Disabled>();
let allow_disabled = contains_disabled(&component_access, disabled_id);
if !allow_disabled {
component_access.and_without(disabled_id);
}

Self {
world_id: world.id(),
archetype_generation: ArchetypeGeneration::initial(),
Expand All @@ -214,13 +231,20 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
let filter_state = F::init_state(builder.world_mut());
D::set_access(&mut fetch_state, builder.access());

let mut component_access = builder.access().clone();
let disabled_id = builder.world_mut().init_component::<Disabled>();
let allow_disabled = contains_disabled(&component_access, disabled_id);
if !allow_disabled {
component_access.and_without(disabled_id);
}

let mut state = Self {
world_id: builder.world().id(),
archetype_generation: ArchetypeGeneration::initial(),
matched_storage_ids: Vec::new(),
fetch_state,
filter_state,
component_access: builder.access().clone(),
component_access,
matched_tables: Default::default(),
matched_archetypes: Default::default(),
#[cfg(feature = "trace")]
Expand Down Expand Up @@ -1627,7 +1651,7 @@ impl<D: QueryData, F: QueryFilter> From<QueryBuilder<'_, D, F>> for QueryState<D
mod tests {
use crate as bevy_ecs;
use crate::world::FilteredEntityRef;
use crate::{component::Component, prelude::*, query::QueryEntityError};
use crate::{prelude::*, query::QueryEntityError};

#[test]
fn get_many_unchecked_manual_uniqueness() {
Expand Down Expand Up @@ -1999,4 +2023,46 @@ mod tests {
let query_2 = QueryState::<&B, Without<C>>::new(&mut world);
let _: QueryState<Entity, Changed<C>> = query_1.join_filtered(&world, &query_2);
}

#[test]
fn does_exclude_disabled() {
let mut world = World::new();
let entity_a = world.spawn(A(0)).id();
let _ = world.spawn((A(1), Disabled)).id();
let entity_c = world.spawn(A(1)).id();
world.disable(entity_c);

let mut query = QueryState::<Entity>::new(&mut world);
assert_eq!(entity_a, query.single(&world));

let mut query = QueryState::<Entity, Without<Disabled>>::new(&mut world);
assert_eq!(entity_a, query.single(&world));
}

#[test]
fn can_request_disabled() {
let mut world = World::new();
let _ = world.spawn(A(0)).id();
let _ = world.spawn((A(1), Disabled)).id();
let entity_c = world.spawn(A(1)).id();
world.disable(entity_c);

let mut query = QueryState::<Entity, With<Disabled>>::new(&mut world);
assert_eq!(2, query.iter(&world).count());

let mut query = QueryState::<(Entity, &Disabled)>::new(&mut world);
assert_eq!(2, query.iter(&world).count());

let mut query = QueryState::<(Entity, &mut Disabled)>::new(&mut world);
assert_eq!(2, query.iter(&world).count());

let mut query = QueryState::<(Entity, Ref<Disabled>)>::new(&mut world);
assert_eq!(2, query.iter(&world).count());

let mut query = QueryState::<(Entity, Option<&Disabled>)>::new(&mut world);
assert_eq!(3, query.iter(&world).count());

let mut query = QueryState::<(Entity, Has<Disabled>)>::new(&mut world);
assert_eq!(3, query.iter(&world).count());
}
}
18 changes: 18 additions & 0 deletions crates/bevy_ecs/src/system/commands/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -933,6 +933,16 @@ impl EntityCommands<'_> {
self.add(despawn);
}

/// Enable this entity, removing the [`bevy_ecs::prelude::Disabled`] marker if it's present
pub fn enable(&mut self) {
self.add(enable);
}

/// Disable this entity, adding the [`bevy_ecs::prelude::Disabled`] marker to the entity
pub fn disable(&mut self) {
self.add(disable);
}

/// Pushes an [`EntityCommand`] to the queue, which will get executed for the current [`Entity`].
///
/// # Examples
Expand Down Expand Up @@ -1084,6 +1094,14 @@ fn despawn(entity: Entity, world: &mut World) {
world.despawn(entity);
}

fn enable(entity: Entity, world: &mut World) {
world.enable(entity);
}

fn disable(entity: Entity, world: &mut World) {
world.disable(entity);
}

/// An [`EntityCommand`] that adds the components in a [`Bundle`] to an entity.
fn insert<T: Bundle>(bundle: T) -> impl EntityCommand {
move |entity: Entity, world: &mut World| {
Expand Down
35 changes: 33 additions & 2 deletions crates/bevy_ecs/src/world/entity_ref.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::{
change_detection::MutUntyped,
component::{Component, ComponentId, ComponentTicks, Components, StorageType},
entity::{Entities, Entity, EntityLocation},
query::{Access, DebugCheckedUnwrap},
query::{Access, DebugCheckedUnwrap, Disabled},
removal_detection::RemovedComponentEvents,
storage::Storages,
world::{Mut, World},
Expand Down Expand Up @@ -1136,6 +1136,8 @@ impl<'w> EntityWorldMut<'w> {
///
/// See [`EntityCommands::retain`](crate::system::EntityCommands::retain) for more details.
pub fn retain<T: Bundle>(&mut self) -> &mut Self {
let disabled_id = self.world.init_component::<Disabled>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior needs to be documented. It's a nice bit of design though!


let archetypes = &mut self.world.archetypes;
let storages = &mut self.world.storages;
let components = &mut self.world.components;
Expand All @@ -1148,7 +1150,7 @@ impl<'w> EntityWorldMut<'w> {

let to_remove = &old_archetype
.components()
.filter(|c| !retained_bundle_info.components().contains(c))
.filter(|c| !retained_bundle_info.components().contains(c) && *c != disabled_id)
.collect::<Vec<_>>();
let remove_bundle = self.world.bundles.init_dynamic_info(components, to_remove);

Expand Down Expand Up @@ -1269,6 +1271,16 @@ impl<'w> EntityWorldMut<'w> {
self.entity
}

/// Enables the current entity, removing the [`Disabled`] marker.
pub fn enable(&mut self) {
self.remove::<Disabled>();
}

/// Disables the current entity, adding the [`Disabled`] marker.
pub fn disable(&mut self) {
self.insert(Disabled);
}

/// Gets read-only access to the world that the current entity belongs to.
#[inline]
pub fn world(&self) -> &World {
Expand Down Expand Up @@ -2583,6 +2595,25 @@ mod tests {
assert_eq!(world.entity(ent).archetype().components().next(), None);
}

// Test that calling retain retains [`Disabled`].
#[test]
fn retain_disabled() {
#[derive(Component)]
struct Marker<const N: usize>;

let mut world = World::new();
let ent = world
.spawn((Marker::<1>, Marker::<2>, Marker::<3>, Disabled))
.id();

let disabled_id = world.init_component::<Disabled>();
world.entity_mut(ent).retain::<Marker<1>>();
assert!(world.entity(ent).contains_id(disabled_id));

world.entity_mut(ent).retain::<()>();
assert!(world.entity(ent).contains_id(disabled_id));
}

// Test removing some components with `retain`, including components not on the entity.
#[test]
fn retain_some_components() {
Expand Down
26 changes: 26 additions & 0 deletions crates/bevy_ecs/src/world/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -939,6 +939,32 @@ impl World {
}
}

/// Enables the given `entity`, if it exists and was [`crate::prelude::Disabled`].
/// Returns `true` if the `entity` is now enabled and `false` if the `entity` does not exist.
#[inline]
pub fn enable(&mut self, entity: Entity) -> bool {
if let Some(mut entity) = self.get_entity_mut(entity) {
entity.enable();
true
} else {
warn!("error[B0003]: Could not enable entity {:?} because it doesn't exist in this World.", entity);
false
}
}

/// Disabled the given `entity`, if it exists and didn't have [`crate::prelude::Disabled`] yet.
/// Returns `true` if the `entity` is now disabled and `false` if the `entity` does not exist.
#[inline]
pub fn disable(&mut self, entity: Entity) -> bool {
if let Some(mut entity) = self.get_entity_mut(entity) {
entity.disable();
true
} else {
warn!("error[B0003]: Could not disable entity {:?} because it doesn't exist in this World.", entity);
false
}
}

/// Clears the internal component tracker state.
///
/// The world maintains some internal state about changed and removed components. This state
Expand Down
Loading