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

added any_match_filter common condition #17327

Merged
merged 3 commits into from
Jan 14, 2025
Merged

Conversation

Jaso333
Copy link
Contributor

@Jaso333 Jaso333 commented Jan 12, 2025

Objective

resolves #17326.

Solution

Simply added the suggested run condition.

Testing

A self-explanatory run condition. Fully verified by the operation of QueryFilter in a system.

Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@BenjaminBrienen BenjaminBrienen added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 12, 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.

I like this but the name is quite unclear to me :) How about any_match_filter?

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Uncontroversial This work is generally agreed upon and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 12, 2025
@Jaso333
Copy link
Contributor Author

Jaso333 commented Jan 12, 2025

@alice-i-cecile agreed, it was more of a placeholder name initially, forgot to mention the fact. The documentation above the method even describes it according to your suggestion... 😅

/// A [`Condition`]-satisfying system that returns `true`
/// if there are any entities that match the given [`QueryFilter`].

I have updated the code accordingly.

@benfrankel
Copy link
Contributor

benfrankel commented Jan 12, 2025

Grammatically it should be any_matches_filter.

EDIT: I guess "any" can be either singular or plural. So I'll amend that to "IMO any_matches_filter reads better".

@Jaso333
Copy link
Contributor Author

Jaso333 commented Jan 12, 2025

Grammatically it should be any_matches_filter.

EDIT: I guess "any" can be either singular or plural. So I'll amend that to "IMO any_matches_filter reads better".

I think I prefer any_match_filter as its more natural, in my opinion, to think of the "any" as a plural in the context of a set of entities. This was probably due to me thinking the same way with any_with_component, but that also works both ways too!

@alice-i-cecile alice-i-cecile changed the title added any_for_filter common condition added any_match_filter common condition Jan 13, 2025
@chescock
Copy link
Contributor

Do we still need this now that we have Populated? The motivating example in the linked issue seems like it would be better served by replacing Query with Populated, since that avoids having to duplicate the query filters.

@Jaso333
Copy link
Contributor Author

Jaso333 commented Jan 13, 2025

Do we still need this now that we have Populated? The motivating example in the linked issue seems like it would be better served by replacing Query with Populated, since that avoids having to duplicate the query filters.

There's a few reasons I can think of:

  • Populated fails validation and emits a warning when there are no entities. This can be turned off, but it's an extra step that goes against the engine's default behaviour.
  • There is another example in my game where I want to run a system if a component combination exists, but I don't want to do anything with those entities. Without this, I would have to include an unused parameter on my system.
  • any_with_component already exists, and has the same purpose as this condition, only more restrictive.

@alice-i-cecile alice-i-cecile 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-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jan 13, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 14, 2025
Merged via the queue into bevyengine:main with commit 9ef1964 Jan 14, 2025
29 checks passed
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 C-Feature A new feature, making something new possible D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add any_for_filter to built-in run conditions
5 participants