Add Disabled marker #12928

NiseVoid · 2024-04-11T16:12:31Z

Objective

Add the ability to disable entities. See Disable and enable entities #11090

Solution

Implement functionality to disable and enable entities (using the Disabled marker component)
Skip the Disabled component in retain
Add a way to disable entities recursively

Changelog

Add Disabled marker component, which hides an entity queries that don't specifically request them

Migration Guide

Entities can get disabled, causing them to "disappear" from queries without being removed and thus triggering things like RemovedComponents, some systems might need to be adjusted to account for this.

crates/bevy_ecs/src/query/mod.rs

BD103 · 2024-04-11T16:34:06Z

Adding this to "Needs release notes," I thinks its noteworthy.

NiseVoid · 2024-04-11T16:40:28Z

Definitely worth mentioning in release notes, if people aren't aware of the existance of disabled entities it could definitely cause subtle bugs. It could probably even be considered a breaking change 🤔

alice-i-cecile · 2024-04-13T01:50:14Z

I like how relatively simple the implementation is. I know that flecs uses a bit in the Entity identifier for this: is that the right choice for us too? Should we delay that until later?

alice-i-cecile · 2024-04-13T01:50:58Z

crates/bevy_ecs/src/query/mod.rs

@@ -21,6 +21,12 @@ pub use par_iter::*;
 pub use state::*;
 pub use world_query::*;

+use crate as bevy_ecs;
+
+/// A marker component that excludes an entity from queries that don't specifically request them.


These docs need to be extended.

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

alice-i-cecile · 2024-04-13T01:51:11Z

crates/bevy_ecs/src/query/mod.rs

@@ -21,6 +21,12 @@ pub use par_iter::*;
 pub use state::*;
 pub use world_query::*;

+use crate as bevy_ecs;
+
+/// A marker component that excludes an entity from queries that don't specifically request them.


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.

james-j-obrien · 2024-04-13T01:53:18Z

I know that flecs uses a bit in the Entity identifier for this: is that the right choice for us too?

flecs uses a bit in the identifier to mark toggle-able versions of components, not for whether or not an individual entity is disabled. There wouldn't be any way to communicate that to existing ids.

alice-i-cecile · 2024-04-13T01:54:09Z

crates/bevy_ecs/src/world/entity_ref.rs

@@ -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>();


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

alice-i-cecile · 2024-04-13T01:56:17Z

crates/bevy_ecs/src/query/mod.rs

+use crate as bevy_ecs;
+
+/// A marker component that excludes an entity from queries that don't specifically request them.
+#[derive(bevy_ecs_macros::Component)]


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.

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

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.

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

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

alice-i-cecile

Generally quite pleased with the design. I personally think that starting with a marker component approach is substantially similar: we shouldn't move off of it until we have solid benchmarks.

I like that the .enable and .disable APIs exist: nice and clear, and signals to new users that this fancy special API exists! It also makes future migrations easier.

Needs more docs before we can merge this though: in particular, both doc tests for Disabled and a top-level ECS example are essential for helping teach about this important new feature.

NiseVoid · 2024-04-13T17:57:43Z

a top-level ECS example is essential for helping teach about this important new feature.

I think an example is probably best as a follow-up PR, to avoid example bikeshedding and any blockage from existing systems not responding to Disabled nicely. If systems respond only to changed values, disabling the entity could simply make it appear unchanged. I think rendering has a few of such systems, and ideally the example would be somewhat visual.

alice-i-cecile · 2024-04-13T19:57:51Z

crates/bevy_ecs/src/query/mod.rs

+
+/// A special marker component to disable an entity.
+///
+/// Disabled entities do not show up in most queries, unless the query mentions Disabled.


Suggested change

/// Disabled entities do not show up in most queries, unless the query mentions Disabled.

/// Disabled entities do not show up in queries unless the query mentions [`Disabled`] in some way.

alice-i-cecile

I'm happy with this now. We should probably still mention that retain doesn't strip out the Disabled component, (or remove that behavior), but that's a simple follow-up.

SkiFire13

I wonder how existing systems behave when only some entities are disabled (not the whole subtree rooted in them). There may be some that expect some entities to exist but not to be invisible to them. Might be worth adding tests to them for those cases.

SkiFire13 · 2024-04-13T20:13:33Z

crates/bevy_ecs/src/query/mod.rs

+#[cfg_attr(feature = "bevy_reflect", derive(bevy_reflect::Reflect))]
+pub struct Disabled;


This should reflect Component, and maybe also implement and reflect Serialize and Deserialize.

Yes to both.

iiYese · 2024-04-14T17:27:03Z

@mockersf

Disabling a window entity should mean the window will be closed|reduced, not just hidden from queries

I'm failing to understand why this would at all be the sane or expected behavior. There is no precedent for it. This is a data model level operation meaning it should have zero side effects on anything beyond the ECS. That is exactly how it behaves in all of the frameworks mentioned. Expecting otherwise would be like the equivalent of expecting With<X> to add X to entities.

Why would anyone expect it to behave like that?
Why would we make it behave like that?

These two things need to be answered for this hypothetical and any other hypothetical. Otherwise we are doing a very poor job of scrutinizing because we're not asking if the things we're hallucinating are relevant.

mockersf · 2024-04-14T17:29:26Z

I'm failing to understand why this would at all be the sane or expected behavior.

Because words have meaning, and "disabled" doesn't mean "hidden". A disabled window is one I can't interact with.

iiYese · 2024-04-14T17:33:07Z

Because words have meaning, and "disabled" doesn't mean "hidden". A disabled window is one I can't interact with.

"Hidden" would have the same problem then because it'd be synonymous with "minimized". Whatever name is chosen is specific to the context of the ECS. Bringing it out of that and expecting it to have applicable meanings to every kind of entity is taking it out of context. The same way Changed<Window> mightn't necessarily correspond to a user observable change on a window.

"Disabled"/"Hidden" would mean for the ECS & queries because this is an ECS feature.

SanderMertens · 2024-04-14T17:41:55Z

@JoJoJet

I haven't used flecs personally, but from the descriptions I've seen of it, some of the behavior seems overly implicit (such as disabling systems when their primary query is empty)

That's not something that happens in Flecs :) I also prefer explicit > implicit.

I don't think I understand the desire for this feature

It's a feature that's very similar to what other game engines like Unity provide, reinterpreted for ECS. Whether or not you solve it with a marker component, the ability to disable an entity is useful.

Consider a laser turret with a beam entity. I want to prevent the laser beam from rendering if the turret is not active. I can't modify the query of the renderer to filter on an additional tag. That's where an engine/query-wide disable feature comes in handy. It's a way to let other systems know to ignore the entity.

The semantics of Disabled are straightforward: the game should act as if the entity doesn't exist. You're correct that in some cases just excluding it from queries isn't enough, like for a window entity. In those cases you can create an observer for the Disabled tag, and run the additional logic required. Creating the observer is the responsibility of a plugin.

JoJoJet · 2024-04-14T17:47:47Z

I haven't used flecs personally, but from the descriptions I've seen of it, some of the behavior seems overly implicit (such as disabling systems when their primary query is empty)

That's not something that happens in Flecs :) I also prefer explicit > implicit.

I must have misunderstood, I was referring to this comment from Joy https://discord.com/channels/691052431525675048/749335865876021248/1198882444254920704

UkoeHB · 2024-04-14T17:49:42Z

It's really just adding a Without at the time the QueryState gets constructed, so the performance is nearly identical to adding Without manually everywhere (but that would obviously be a huge footgun, because people would forget).

This is a comparison between a world with manual Disabled and a world with built-in Disabled. What about the perf between a world with no Disabled and a world with built-in Disabled? That's what interests me.

SanderMertens · 2024-04-14T18:01:21Z

I must have misunderstood, I was referring to this comment from Joy https://discord.com/channels/691052431525675048/749335865876021248/1198882444254920704

That comment is incorrect. It's the other way around, systems are only added to a schedule once they've started matching entities for the first time. This prevents adding 20 systems from a plugin to the scheduler, if the app actually only uses 2.

This is not implemented with the Disabled tag either. Enabling/disabling an entity is always an explicit action by the user.

NiseVoid · 2024-04-14T18:03:13Z

This is a comparison between a world with manual Disabled and a world with built-in Disabled. What about the perf between a world with no Disabled and a world with built-in Disabled? That's what interests me.

Would be pretty hard to get any conclusive numbers on it, since it would heavily depends on the number of queries and archetypes. Filtering archetypes is a overal pretty cheap however, think of it like adding Without<UnusedComponent> to every query in your app. If your app doesn't have archetype explosions it would probably be impossible to measure, possibly the more problematic thing is disabling entities, since I'm not sure how efficient ZST components currently are in bevy.

maniwani · 2024-04-14T18:09:11Z

I must have misunderstood, I was referring to this comment from Joy discord.com/channels/691052431525675048/749335865876021248/1198882444254920704

That comment is incorrect. It's the other way around, systems are only added to a schedule once they've started matching entities for the first time. This prevents adding 20 systems from a plugin to the scheduler, if the app actually only uses 2.

This is not implemented with the Disabled tag either. Enabling/disabling an entity is always an explicit action by the user.

Ah, my bad. For some reason, I thought flecs queries (and so systems) were excluded in some way any time their matched tables (archetypes) were all empty. I also assumed it was done using the Disabled tag. Thanks for clarifying.

JoJoJet · 2024-04-14T18:17:31Z

You're correct that in some cases just excluding it from queries isn't enough, like for a window entity. In those cases you can create an observer for the Disabled tag, and run the additional logic required. Creating the observer is the responsibility of a plugin.

This is exactly the kind of added complexity I'd like to avoid. "Disabling" is too vague of a term to be generally meaningful, and it will require workarounds like this to make it behave the way people would expect in the general case. This kind of behavior would be better served by specifically meaningful operations

SanderMertens · 2024-04-14T18:22:39Z

@JoJoJet

How would you address the laser turret scenario? Delete/recreate the laser beam entity?

JoJoJet · 2024-04-14T18:25:10Z

@JoJoJet

How would you address the laser turret scenario? Delete/recreate the laser beam entity?

That specific scenario? I would set it to Visibility::Hidden

NiseVoid · 2024-04-14T18:26:23Z

This is exactly the kind of added complexity I'd like to avoid. "Disabling" is too vague of a term to be generally meaningful, and it will require workarounds like this to make it behave the way people would expect in the general case. This kind of behavior would be better served by specifically meaningful operations

Besides some edge cases (like Window entities), in most cases the workarounds would be necessary to have the desired behavior. If I spawn a player and disable it, I'd expect it to be functionally identical to despawning it, except I retain the ability to write systems that re-enable it later.

The only way to meaningfully split it up would involve specifying the purpose of each and every query, that way you could have more specific markers like "Unsimulated", but that would be more work than just supporting the more silly cases like Window entities. And there's also the very simple option of adding some marker (or having archetype invariants) to prevent you from disabling entities like Window entities.

NiseVoid · 2024-04-14T18:27:36Z

That specific scenario? I would set it to Visibility::Hidden

But now the sensor collider still reports collision events and it keeps damaging players.

JoJoJet · 2024-04-14T18:52:46Z

That specific scenario? I would set it to Visibility::Hidden

But now the sensor collider still reports collision events and it keeps damaging players.

You could add a specific marker component that prevents the entity from interacting with the rest of the game -- or even better, you could remove a marker component that allows it to interact with the game. I don't think it's necessarily productive to focus on this one example; my point is that this type of functionality would be better served with operations tailor-made for each purpose

EmileGr · 2024-04-14T19:41:09Z

@JoJoJet Well, hang on now. When you brought up your example of a window being disabled you didn't say it wasn't productive to focus on one example, but now that a counter-example is introduced that would indeed be simpler to implement under the proposed PR, you do say it's not productive to focus on one example. Both yours and Sander's are equally valid scenarios and worthy of proper consideration. There's no need to be dismissive.

Personally, as a veteran developer who recently staring playing around with Bevy, this is going to be a very common feature request from people new to Bevy, if it isn't already. I don't see how asking developers to introduce dozens of different marker components in their projects is going to be any less complicated than one generalized marker that can invoke additional logic via observers. In your example of a window being disabled, querying for the presence or absence of a specific marker to minimize or destroy a window is no less complicated than having an observer on a generalized marker that minimizes or destroys said window. It's just two different ways of approaching the problem. Perhaps it would be best to wait until observers land before looking at this again...

NiseVoid · 2024-04-14T19:47:24Z

You could add a specific marker component that prevents the entity from interacting with the rest of the game -- or even better, you could remove a marker component that allows it to interact with the game. I don't think it's necessarily productive to focus on this one example, but my point is that this type of functionality would be better served with operations tailor-made for each purpose

Tailor-made operations can be useful but that's an entirely separate usecase. If a player gets hit it might make sense to disable their hitbox until their invulnerability ends. But a game isn't just one hitbox, you have lots of separate systems, which would then all need their own tailor-made components. Even if hypothetically every plugin did add those, creating a "disable this entity" behavior would be stupidly complex and might as well be impossible. You would need to register all of these components, track if they existed before, insert them when you try to disable the entity, and remove the ones that weren't already present when you re-enable the entity. You might now also get conflicts with systems that disable specific behaviors, so now every one of these PluginXDisabled components needs to be replaced with a lock counter.

Additionally if apps handle the logic to disable entities correctly, they probably also handle disabling components correctly. Which means we could introduce that feature later and get the "tailor-made" components even if the developer did not implement them.

I don't see how forcing the entire ecosystem to either:
a. not build a large subset of features that need disabled entities
b. implement all of these markers (or lock counters)
would be lower in complexity than providing standard ways to disable things and the tooling to do so in a reasonable way.

JoJoJet · 2024-04-14T19:50:04Z

Apologies if I came across as dismissive but that's not my intent -- I'm just trying not to get lost in the details of one specific example when the problem it's trying to solve can be better solved with a simpler approach. I believe that windows are a valid counterexample, as they're a straightforward, simple example of an abstract, non-object entity that we are all familiar with. Windows are not the only cases of abstract entities -- at my work I'm developing an application that is nearly production ready and we are using custom non-object entities for things like user assets that can be imported into a scene. What would it mean to disable an entity like that -- does it get hidden from the user's library but remain a valid asset? Does it become illegal to import it? Does it appear grayed out? None of these seem like an obvious meaning for being disabled so we'd just have to pick something arbitrarily, which is why we have specific marker components for each of these possibilities.

NiseVoid · 2024-04-14T20:07:31Z

custom non-object entities for things like user assets

This really does sound like more of an archetype invariant problem. If it doesn't make sense to disable a Window, or disable an Asset, or disable a SystemId, then you shouldn't be able to insert that component on that entity. I doubt people would actually expect Window or Asset to respect Disabled in any meaningful way. We could of course add some way to mark things to stop adding Disabled to them, which could later be replaced by archetype invariants.

iiYese · 2024-04-14T20:10:23Z

we are using custom non-object entities for things like user assets that can be imported into a scene. What would it mean to disable an entity like that --

The meaning is why you're disabling an entity. Disabling doesn't happen in a vacuum. From there you have to ask is disabling the right tool for something? If the case is "these entities being matched in queries should be the exception rather than the rule temporarily or otherwise" then yes. If however you just want them excluded from specific queries in specific systems then you want markers & filters. Which is why they're more appropriate for your scenario:

does it get hidden from the user's library but remain a valid asset? Does it become illegal to import it? Does it appear grayed out? None of these seem like an obvious meaning for being disabled so we'd just have to pick something arbitrarily, which is why we have specific marker components for each of these possibilities.

People have also been discussing misuses of Disabled where they would actually want archetype level change detection, observers or another feature. I think people are seeing too many nails because of this hammer.

SanderMertens · 2024-04-14T20:16:35Z

None of these seem like an obvious meaning for being disabled so we'd just have to pick something arbitrarily

Just because a feature doesn't map to a specific application/use case doesn't mean it's not useful. There are many ECS applications where I'm not using entity disabling & scenarios where the semantics aren't 100% fleshed out. That by itself shouldn't (IMO) block development of a new feature- especially if there is well documented prior art (not just Flecs, but Unity, Godot, Unreal), and there are use cases where it has a clear benefit.

FWIW I don't think the Window use case is a counter example. If you interpret the semantics of Disabled as a "the app should run as if the entity doesn't exist", disabling a window should hide the window.

mockersf · 2024-04-14T20:26:31Z

Given the discussion, this would benefit from a RFC (that will be based on this PR and reference it).

From what I could collect, it needs to clarify:

how it works with hierarchy
how it works with change detection
how it could interact with a retained rendering world
maybe present a few uses cases

This kind of back and forth is hard to follow and track in a GitHub PR. @NiseVoid do you feel up to write the RFC or would you prefer help or to leave that to someone else?

NiseVoid · 2024-04-14T20:55:48Z

I could try to write something up but I'd need some help in making it clearer and filling in some of the details (especially the hierarchy and rendering ones, since those are some of the parts I'm least familiar with). Some more examples would also be useful, so far people here have only brought up loading, rollback and @SanderMertens's laser turret, people probably have more examples.

alice-i-cecile · 2024-04-14T22:17:53Z

More motivating examples:

State-tied entity disabling to solve Enable World Swapping #12860's needs.
A prefab "zoo" of entities, which can be quickly cloned and spawned.

NiseVoid · 2024-05-03T14:55:50Z

Closing this PR, ideally we'd unblock the usecase for now with #13120, that would allow for experimentation on the user and third-party plugin side and help designing a user friendly API for disabling entities.

Add Disabled marker

e0509bd

BD103 reviewed Apr 11, 2024

View reviewed changes

crates/bevy_ecs/src/query/mod.rs Show resolved Hide resolved

BD103 added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events M-Needs-Release-Note Work that should be called out in the blog due to impact labels Apr 11, 2024

Fix clippy warning

eccd940

alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Apr 11, 2024

Fix Has<Disabled>

374f7b4

NiseVoid marked this pull request as ready for review April 13, 2024 01:07

alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Apr 13, 2024

alice-i-cecile requested review from james-j-obrien and maniwani April 13, 2024 01:49

alice-i-cecile reviewed Apr 13, 2024

View reviewed changes

Impove docs. Register Disabled. Add some derives

8eb8b02

alice-i-cecile added this to the 0.14 milestone Apr 13, 2024

alice-i-cecile reviewed Apr 13, 2024

View reviewed changes

alice-i-cecile approved these changes Apr 13, 2024

View reviewed changes

alice-i-cecile requested review from james7132 and JoJoJet April 13, 2024 20:00

james-j-obrien approved these changes Apr 13, 2024

View reviewed changes

SkiFire13 reviewed Apr 13, 2024

View reviewed changes

NiseVoid marked this pull request as draft April 14, 2024 23:11

NiseVoid mentioned this pull request Apr 14, 2024

Disabled entities bevyengine/rfcs#81

Draft

NiseVoid closed this May 3, 2024

iiYese mentioned this pull request Jul 9, 2024

Add DefaultQueryFilters #13120

Open

Jondolf mentioned this pull request Sep 22, 2024

Make disabling joints possible. Jondolf/avian#519

Merged

	/// 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.

	/// Disabled entities do not show up in most queries, unless the query mentions Disabled.
	/// Disabled entities do not show up in queries unless the query mentions [`Disabled`] in some way.

		#[cfg_attr(feature = "bevy_reflect", derive(bevy_reflect::Reflect))]
		pub struct Disabled;

Add Disabled marker #12928

Add Disabled marker #12928

Conversation

NiseVoid commented Apr 11, 2024

Objective

Solution

Changelog

Migration Guide

BD103 commented Apr 11, 2024

NiseVoid commented Apr 11, 2024

alice-i-cecile commented Apr 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-j-obrien commented Apr 13, 2024 • edited Loading

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

james-j-obrien Apr 13, 2024 • edited Loading

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

NiseVoid commented Apr 13, 2024

alice-i-cecile Apr 13, 2024 • edited Loading

Choose a reason for hiding this comment

alice-i-cecile 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

iiYese commented Apr 14, 2024

mockersf commented Apr 14, 2024

iiYese commented Apr 14, 2024 • edited Loading

SanderMertens commented Apr 14, 2024

JoJoJet commented Apr 14, 2024

UkoeHB commented Apr 14, 2024

SanderMertens commented Apr 14, 2024

NiseVoid commented Apr 14, 2024

maniwani commented Apr 14, 2024

JoJoJet commented Apr 14, 2024

SanderMertens commented Apr 14, 2024 • edited Loading

JoJoJet commented Apr 14, 2024

NiseVoid commented Apr 14, 2024

NiseVoid commented Apr 14, 2024

JoJoJet commented Apr 14, 2024 • edited Loading

EmileGr commented Apr 14, 2024

NiseVoid commented Apr 14, 2024

JoJoJet commented Apr 14, 2024 • edited Loading

NiseVoid commented Apr 14, 2024

iiYese commented Apr 14, 2024 • edited Loading

SanderMertens commented Apr 14, 2024

mockersf commented Apr 14, 2024

NiseVoid commented Apr 14, 2024

alice-i-cecile commented Apr 14, 2024

NiseVoid commented May 3, 2024

james-j-obrien commented Apr 13, 2024 •

edited

Loading

james-j-obrien Apr 13, 2024 •

edited

Loading

alice-i-cecile Apr 13, 2024 •

edited

Loading

iiYese commented Apr 14, 2024 •

edited

Loading

SanderMertens commented Apr 14, 2024 •

edited

Loading

JoJoJet commented Apr 14, 2024 •

edited

Loading

JoJoJet commented Apr 14, 2024 •

edited

Loading

iiYese commented Apr 14, 2024 •

edited

Loading