Add common component conditions #7711

Shatur · 2023-02-16T16:50:07Z

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.

alice-i-cecile

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 · 2023-02-16T16:59:39Z

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 · 2023-02-16T17:03:44Z

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 · 2023-02-16T17:07:18Z

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 · 2023-02-16T17:50:06Z

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().

james7132 · 2023-02-16T19:28:14Z

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


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.

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.

crates/bevy_ecs/src/schedule/condition.rs

Co-authored-by: Ida "Iyes" <[email protected]>

inodentry

Suggesting better wording for the doc comments!

crates/bevy_ecs/src/schedule/condition.rs

Co-authored-by: Ida "Iyes" <[email protected]>

Shatur · 2023-02-20T16:02:49Z

Thanks!

inodentry · 2023-02-20T16:03:01Z

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. :)

crates/bevy_ecs/src/schedule/condition.rs

This reverts commit 86c78a6.

This reverts commit 7fcad94.

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 <[email protected]>

Shatur · 2023-04-18T16:32:23Z

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.

Shatur · 2024-02-17T11:54:29Z

It was decided to not implement it, closing.

Add common component conditions

24302cc

Shatur requested a review from inodentry February 16, 2023 16:50

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 requested changes Feb 16, 2023

View reviewed changes

Use more clear description

749a42b

alice-i-cecile approved these changes Feb 16, 2023

View reviewed changes

Shatur force-pushed the component-conditions branch from 037bc82 to 749a42b Compare February 16, 2023 17:52

Add any_component_removed

e992e67

james7132 requested changes Feb 16, 2023

View reviewed changes

james7132 added the X-Controversial There is active debate or serious implications around merging this PR label Feb 16, 2023

andresovela reviewed Feb 17, 2023

View reviewed changes

crates/bevy_ecs/src/schedule/condition.rs Outdated Show resolved Hide resolved

Fix typo

93ab94a

inodentry reviewed Feb 20, 2023

View reviewed changes

crates/bevy_ecs/src/schedule/condition.rs Outdated Show resolved Hide resolved

Use is_empty()

7fcad94

Co-authored-by: Ida "Iyes" <[email protected]>

inodentry suggested changes Feb 20, 2023

View reviewed changes

crates/bevy_ecs/src/schedule/condition.rs Outdated Show resolved Hide resolved

crates/bevy_ecs/src/schedule/condition.rs Outdated Show resolved Hide resolved

inodentry reviewed Feb 20, 2023

View reviewed changes

crates/bevy_ecs/src/schedule/condition.rs Outdated Show resolved Hide resolved

Shatur and others added 2 commits February 20, 2023 18:01

Apply docs suggestions

6027d4d

Co-authored-by: Ida "Iyes" <[email protected]>

Apply more docs suggestions

9bcad18

Co-authored-by: Ida "Iyes" <[email protected]>

Shatur added 2 commits February 20, 2023 18:15

Merge remote-tracking branch 'origin/main' into component-conditions

74a2c08

Remove extra mut

86c78a6

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 reviewed Feb 20, 2023

View reviewed changes

crates/bevy_ecs/src/schedule/condition.rs Outdated Show resolved Hide resolved

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 added 2 commits February 21, 2023 00:20

Revert "Remove extra mut"

7d33c0d

This reverts commit 86c78a6.

Revert "Use is_empty()"

15df851

This reverts commit 7fcad94.

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 added 2 commits February 22, 2023 00:07

Merge remote-tracking branch 'origin/main' into component-conditions

48ab039

Add comment about iter().count()

ffaea8f

Shatur requested a review from inodentry February 21, 2023 22:10

Shatur mentioned this pull request Apr 7, 2023

Add any_component_removed condition #8326

Merged

Merge remote-tracking branch 'origin/main' into component-conditions

5b4af1b

james7132 mentioned this pull request Sep 14, 2023

Add component change detection conditions #9746

Closed

Shatur closed this Feb 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add common component conditions #7711

Add common component conditions #7711

Shatur commented Feb 16, 2023 •

edited

Loading

alice-i-cecile left a comment

Shatur commented Feb 16, 2023

alice-i-cecile commented Feb 16, 2023

Shatur commented Feb 16, 2023 •

edited

Loading

Shatur commented Feb 16, 2023

james7132 Feb 16, 2023 •

edited

Loading

Shatur Feb 16, 2023 •

edited

Loading

inodentry left a comment

Shatur commented Feb 20, 2023

inodentry commented Feb 20, 2023

Shatur commented Apr 18, 2023 •

edited

Loading

Shatur commented Feb 17, 2024

Add common component conditions #7711

Add common component conditions #7711

Conversation

Shatur commented Feb 16, 2023 • edited Loading

Changelog

Added

alice-i-cecile left a comment

Choose a reason for hiding this comment

Shatur commented Feb 16, 2023

alice-i-cecile commented Feb 16, 2023

Shatur commented Feb 16, 2023 • edited Loading

Shatur commented Feb 16, 2023

james7132 Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

Shatur Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

inodentry left a comment

Choose a reason for hiding this comment

Shatur commented Feb 20, 2023

inodentry commented Feb 20, 2023

Shatur commented Apr 18, 2023 • edited Loading

Shatur commented Feb 17, 2024

Shatur commented Feb 16, 2023 •

edited

Loading

Shatur commented Feb 16, 2023 •

edited

Loading

james7132 Feb 16, 2023 •

edited

Loading

Shatur Feb 16, 2023 •

edited

Loading

Shatur commented Apr 18, 2023 •

edited

Loading