Skip to content

Add DefaultQueryFilters #13120

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

Merged
merged 19 commits into from
Jan 20, 2025
Merged

Conversation

NiseVoid
Copy link
Contributor

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 iiYese self-requested a review April 27, 2024 21:18
@iiYese
Copy link
Contributor

iiYese commented Apr 28, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

iiYese commented Apr 28, 2024

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
Copy link
Contributor Author

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.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible X-Controversial There is active debate or serious implications around merging this PR labels Apr 29, 2024
@NiseVoid NiseVoid mentioned this pull request May 3, 2024
@NiseVoid NiseVoid added the S-Needs-SME Decision or review from an SME is required label May 14, 2024
Copy link
Contributor

@Shatur Shatur left a comment

Choose a reason for hiding this comment

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

The code is simple and clear. I would add references to DefaultQueryFilters from Query for discoverability.

This feature would be quite useful for rollback in networking.

Copy link
Contributor

@cBournhonesque cBournhonesque left a comment

Choose a reason for hiding this comment

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

Seems well thought-out, and definitely useful

@alice-i-cecile alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label May 19, 2024
@NiseVoid NiseVoid added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Oct 2, 2024
@BenjaminBrienen BenjaminBrienen added the D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes label Oct 14, 2024
@NthTensor NthTensor added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 8, 2025
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

@maniwani and I have signed off on the core design as SME-ECS (hence the change to X-Blessed a while back). The core challenge here is how to handle disabling entities WRT relations. After discussing this with Sander Martens (of flecs), I'm of the opinion that the simple, intuitive behavior here is the right one to ship with, while making sure that it's easy to "disable/enable recursively".

Before 0.16, I think it's important that we:

  1. Ship a first party Disabled component.
  2. Demonstrate this feature in an example, which showcases the hierarchical disabling pattern.

@NiseVoid has suffered long enough waiting for this to land though, and those are easily moved to follow-up. Merging as soon as CI is green.

@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Jan 20, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 20, 2025
Merged via the queue into bevyengine:main with commit de54867 Jan 20, 2025
28 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Jan 21, 2025
…tyCommands` (#17463)

# Objective

While being able to quickly add / remove components down a tree is
broadly useful (material changing!), it's particularly necessary when
combined with the newly added #13120.

## Solution

Write four methods: covering both adding and removal on both
`EntityWorldMut` and `EntityCommands`.

These methods are generic over the `RelationshipTarget`, thanks to the
freshly merged relations 🎉

## Testing

I've added a simple unit test for these methods.

---------

Co-authored-by: Zachary Harrold <[email protected]>
mrchantey pushed a commit to mrchantey/bevy that referenced this pull request Feb 4, 2025
# 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
mrchantey pushed a commit to mrchantey/bevy that referenced this pull request Feb 4, 2025
…tyCommands` (bevyengine#17463)

# Objective

While being able to quickly add / remove components down a tree is
broadly useful (material changing!), it's particularly necessary when
combined with the newly added bevyengine#13120.

## Solution

Write four methods: covering both adding and removal on both
`EntityWorldMut` and `EntityCommands`.

These methods are generic over the `RelationshipTarget`, thanks to the
freshly merged relations 🎉

## Testing

I've added a simple unit test for these methods.

---------

Co-authored-by: Zachary Harrold <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Feb 9, 2025
# Objective

The entity disabling / default query filter work added in #17514 and
#13120 is neat, but we don't teach users how it works!

We should fix that before 0.16.

## Solution

Write a simple example to teach the basics of entity disabling!

## Testing

`cargo run --example entity_disabling`

## Showcase


![image](https://github.com/user-attachments/assets/9edcc5e1-2bdf-40c5-89b7-5b61c817977a)

---------

Co-authored-by: Zachary Harrold <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Feb 11, 2025
…y filters (#17768)

# 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

1. Change `DefaultQueryFilters` to store a SmallVec of ComponentId,
rather than an Option.
2. Expose methods on `DefaultQueryFilters`, `World` and `App` to
actually configure this.
3. 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!
mrchantey pushed a commit to mrchantey/bevy that referenced this pull request Feb 17, 2025
# Objective

The entity disabling / default query filter work added in bevyengine#17514 and
bevyengine#13120 is neat, but we don't teach users how it works!

We should fix that before 0.16.

## Solution

Write a simple example to teach the basics of entity disabling!

## Testing

`cargo run --example entity_disabling`

## Showcase


![image](https://github.com/user-attachments/assets/9edcc5e1-2bdf-40c5-89b7-5b61c817977a)

---------

Co-authored-by: Zachary Harrold <[email protected]>
@alice-i-cecile
Copy link
Member

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1982 if you'd like to help out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events A-Networking Sending data between clients, servers and machines C-Feature A new feature, making something new possible D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Needs-Release-Note Work that should be called out in the blog due to impact S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers
Projects
None yet
Development

Successfully merging this pull request may close these issues.