Skip to content

Add common component conditions - #7711

Closed
Shatur wants to merge 14 commits into
bevyengine:mainfrom
simgine:component-conditions
Closed

Add common component conditions#7711
Shatur wants to merge 14 commits into
bevyengine:mainfrom
simgine:component-conditions

Conversation

@Shatur

@Shatur Shatur commented Feb 16, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Add two useful common run conditions for components similar to any_with_component from #7579. Unlike any_with_component these two conditions could be potentially expensive, but super useful if user knows that there is only a few entities with a component.

Changelog

Added

  • Add any_component_added and any_component_changed.

@Shatur
Shatur requested a review from inodentry February 16, 2023 16:50
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Feb 16, 2023
alice-i-cecile
alice-i-cecile requested changes Feb 16, 2023
View reviewed changes

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Otherwise this check could be expensive and hold up the executor preventing it from running any systems during the check.

I think this can be more clearly phrased by replacing your warning with the following:

Run conditions are evaluated on the main thread, blocking any other systems from running.
This run condition is relatively expensive, as it iterates over every entity with this component.
As a result, you likely only want to use this run condition when the number of entitities with the component T is small.

One more request: can we have the same flavor of run condition, but for RemovedComponent<T>?

@Shatur

Shatur commented Feb 16, 2023

Copy link
Copy Markdown
Contributor Author

I think this can be more clearly phrased by replacing your warning with the following:

I like it!

One more request: can we have the same flavor of run condition, but for RemovedComponent?

I think we switched to events for this: #5680

@alice-i-cecile

alice-i-cecile commented Feb 16, 2023

Copy link
Copy Markdown
Member

I think we switched to events for this: #5680

That PR was effectively non-breaking :) The RemovedComponents system param is still around, it's just now double-buffered and no longer quite as bespoke.

@Shatur

Shatur commented Feb 16, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

The RemovedComponents system param is still around, it's just now double-buffered and no longer quite as bespoke.

Right, missed that it was an internal change. Will add the condition.

@Shatur

Shatur commented Feb 16, 2023

Copy link
Copy Markdown
Contributor Author

Right, missed that it was an internal change. Will add the condition.

We probably should add RemovedComponents::is_empty() for it to avoid calling iter().count().

@Shatur
Shatur force-pushed the component-conditions branch from 037bc82 to 749a42b Compare February 16, 2023 17:52
james7132
james7132 requested changes Feb 16, 2023
View reviewed changes
Comment thread crates/bevy_ecs/src/schedule/condition.rs
/// This run condition is relatively expensive, as it iterates over every entity with this component.
/// As a result, you likely only want to use this run condition when the number of entitities with the component `T` is small.
pub fn any_component_added<T: Component>() -> impl FnMut(Query<(), Added<T>>) -> bool {
move |query: Query<(), Added<T>>| !query.is_empty()

@james7132 james7132 Feb 16, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These potentially do a full world scan in the multithreaded executor, which not only runs single threaded but blocks any further system's run conditions from being evaluated and tasks launched. The only early return is if no archetypes match the query, everything else does a full scan, and will scale linearly with the number of those components in the world. This is relatively easy performance footgun, same with the other condition below.

Even with it documented, I still don't think it's a good idea to readily provide this to users without some form of archetype-level change detection optimization.

@Shatur Shatur Feb 16, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yep, this is what I mentioned in the description. I thought it's fine if documented. But your concern is exactly why it wasn't included in #7579.

@james7132 james7132 added the X-Needs-SME This type of work requires an SME to approve it. label Feb 16, 2023
andresovela
andresovela reviewed Feb 17, 2023
View reviewed changes
Comment thread crates/bevy_ecs/src/schedule/condition.rs Outdated
inodentry
inodentry reviewed Feb 20, 2023
View reviewed changes
Comment thread crates/bevy_ecs/src/schedule/condition.rs
@Shatur @inodentry
Use is_empty()
7fcad94 
Co-authored-by: Ida "Iyes" <40234599+inodentry@users.noreply.github.com>
inodentry
inodentry suggested changes Feb 20, 2023
View reviewed changes

@inodentry inodentry left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggesting better wording for the doc comments!

Comment thread crates/bevy_ecs/src/schedule/condition.rs Outdated
Comment thread crates/bevy_ecs/src/schedule/condition.rs Outdated
inodentry
inodentry reviewed Feb 20, 2023
View reviewed changes
Comment thread crates/bevy_ecs/src/schedule/condition.rs Outdated
Shatur and others added 2 commits February 20, 2023 18:01
@Shatur @inodentry
Apply docs suggestions
6027d4d 
Co-authored-by: Ida "Iyes" <40234599+inodentry@users.noreply.github.com>
@Shatur @inodentry
Apply more docs suggestions
9bcad18 
Co-authored-by: Ida "Iyes" <40234599+inodentry@users.noreply.github.com>
@Shatur

Shatur commented Feb 20, 2023

Copy link
Copy Markdown
Contributor Author

Thanks!

@inodentry

inodentry commented Feb 20, 2023

Copy link
Copy Markdown
Contributor

Other than the little things I pointed out above, I approve! Given the warning in the documentation, I think this is worth including in Bevy.

As implemented, if, in the future, change detection gets optimized so that the queries don't have to iterate, this API will just magically become fast, and we can remove the perf warning. :)

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 20, 2023
inodentry
inodentry reviewed Feb 20, 2023
View reviewed changes
Comment thread crates/bevy_ecs/src/schedule/condition.rs Outdated
@alice-i-cecile alice-i-cecile removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 20, 2023
@Shatur
Revert "Remove extra mut"
7d33c0d 
This reverts commit 86c78a6.
@Shatur
Revert "Use is_empty()"
15df851 
This reverts commit 7fcad94.
@Shatur Shatur added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 20, 2023
@Shatur
Shatur requested a review from inodentry February 21, 2023 22:10
@Shatur Shatur mentioned this pull request Apr 7, 2023
Merged
alice-i-cecile pushed a commit that referenced this pull request Apr 18, 2023
@Shatur @mockersf
Add any_component_removed condition (#8326)
315f3ca 
Added helper extracted from #7711. that PR contains some controversy
conditions, but this one should be good to go.

---

## Changelog

### Added

- `any_component_removed` condition.

---------

Co-authored-by: François <mockersf@gmail.com>
@Shatur

Shatur commented Apr 18, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I moved non-controversial any_component_removed to a separate PR (#8326) that was merged. Updated this PR with to the latest master just in case someone will be interested to include these two other conditions.

@james7132 james7132 mentioned this pull request Sep 14, 2023
Closed
@Shatur

Shatur commented Feb 17, 2024

Copy link
Copy Markdown
Contributor Author

It was decided to not implement it, closing.

@Shatur Shatur closed this Feb 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@james7132 james7132 james7132 requested changes
@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes
@inodentry inodentry Awaiting requested review from inodentry
+1 more reviewer
@andresovela andresovela andresovela left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Needs-SME This type of work requires an SME to approve it.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Shatur @alice-i-cecile @inodentry @james7132 @andresovela