Ensure required components are consistently depth-first #20110

SkiFire13 · 2025-07-13T14:12:26Z

Objective

Fixes RequiredComponent.inheritance_depth docs do not matchComponent derive impl #19863 by removing inheritance_depth
Fixed Inconsistent Behavior with Required Components #19333 by properly using depth-first order priority
Helps with Required Component with higher specificity should "win" regardless of order #19300 (not sure if it could be closed with this PR)
Fixes Component::register_required_components can be implemented in an unsound way #20173
Overall, fix the weird situation that required components are in, where some operations respect a depth-first order priority while other respect a breadth-first order priority.

Solution

Switch everything to a depth-first order priority as @cart originally intended it.
Effectively revert Use register_dynamic for merging #18028, which tried to give priority to components with higher inheritance depth (i.e. with a breadth-first order priority).

Testing

Most existing tests pass, except a couple that were removed due to testing inheritance depth or the breadth-first order priority that this PR will remove.

Some tests were also added, some of which as regression tests and others to add more coverage.

alice-i-cecile · 2025-07-14T19:32:38Z

@SkiFire13 is this ready for review now that the bundle-split PR is merged? This is still in draft mode :)

SkiFire13 · 2025-07-14T19:49:05Z

@SkiFire13 is this ready for review now that the bundle-split PR is merged? This is still in draft mode :)

Ah yes, I was forgetting to change the status! I think it might still need some more tests, but the implementation should be ready.

urben1680 · 2025-07-15T20:47:40Z

I encountered an issue where a required component is not registered at all on main during my work on #19852. This PR seems to fix that too. That problem seems to be not among the issues listed here.

I try to review this the next days and if needed suggest another test if this is missing.

…gistrator

SkiFire13 · 2025-07-19T15:06:29Z

I added a few more tests, cleaned up some parts of the code and added a fix for #20173 (I would have preferred a separate PR but that ended up being requiring too many changes that would be included in this PR anyway)

…ponents

cBournhonesque

First pass, haven't looked in depth at required.rs

crates/bevy_ecs/src/bundle/info.rs

cBournhonesque · 2025-07-23T16:53:52Z

crates/bevy_ecs/src/bundle/info.rs

-        let explicit_components_len = component_ids.len();
-        let mut required_components = RequiredComponents::default();
-        for component_id in component_ids.iter().copied() {
+        let mut depth_first_components = IndexMap::<_, _, FixedHasher>::default();


If i understand correctly there could be 2 meanings to DFS:

if we add two components A and B at the same time in a bundle and they both required C, we want to add the version required by A because it appears first in the bundle list. This is the DFS performed here

if A requires B and C, and both B and C require D, we want to add the version required by B first because it appears first in the required list. This is handled somewhere else, probably at registration time (I have yet to review that)

Yes. A way in which you can unify these two views is seeing a bundle as if it was directly requiring all its components, with the "default" value the one in the actual bundle.

crates/bevy_ecs/src/bundle/info.rs

crates/bevy_ecs/src/component/info.rs

crates/bevy_ecs/src/component/required.rs

chescock

Looks good! The comments were extremely helpful in understanding some of the tricky parts, especially the step-by-step comments in register_required_components! I left some nits on things that tripped me up while I was reading it, but none of them should be blocking.

chescock · 2025-07-24T15:31:37Z

crates/bevy_ecs/src/component/required.rs

-                    Entity,
-                    MaybeLocation,
-                );
+        // If already registered as a direct required component then bail.


Why ignore the new constructor instead of updating the registration? It looks like the old code did an update in this case.

Or, is this an error case and we should panic! or return an Err somehow?

The old code used to decide based on the given inheritance_depth. If we go by the new rules the second occurence of a duplicated direct required components would always "lose" to the first and hence be ignored, though I can see why this is not ideal. I'll try to see if we can update this to return a Result and/or panic (possibly with a try_ variant of the methods).

I changed this to panic for now. I'm not particularly a fan, but it's a bit awkward to return a RequiredComponentsError. Let me know what you (and others) think.

I'm fine with the decision to panic for now: this PR is already very involved.

crates/bevy_ecs/src/bundle/info.rs

crates/bevy_ecs/src/component/required.rs

alice-i-cecile

This is very complex, but extremely well-commented and has a robust set of tests. In the course of reviewing I've been convinced that this is the correct behavior, and I'm very happy to remove inheritance_depth.

Thanks for taking the time to do this well.

@cart

…0110) # Objective - Fixes bevyengine#19863 by removing `inheritance_depth` - Fixed bevyengine#19333 by properly using depth-first order priority - Helps with bevyengine#19300 (not sure if it could be closed with this PR) - Fixes bevyengine#20173 - Overall, fix the weird situation that required components are in, where some operations respect a depth-first order priority while other respect a breadth-first order priority. ## Solution - Switch everything to a depth-first order priority as @cart originally intended it. - Effectively revert bevyengine#18028, which tried to give priority to components with higher inheritance depth (i.e. with a breadth-first order priority). ## Testing Most existing tests pass, except a couple that were removed due to testing inheritance depth or the breadth-first order priority that this PR will remove. Some tests were also added, some of which as regression tests and others to add more coverage.

SkiFire13 force-pushed the rework-required-components branch 2 times, most recently from 5362a26 to 8b14b9c Compare July 13, 2025 14:48

alice-i-cecile added A-ECS Entities, components, systems, and events S-Blocked This cannot move forward until something else changes C-Bug An unexpected or incorrect behavior X-Contentious There are nontrivial implications that should be thought through labels Jul 14, 2025

alice-i-cecile added this to the 0.17 milestone Jul 14, 2025

alice-i-cecile requested a review from ElliottjPierce July 14, 2025 17:35

alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Blocked This cannot move forward until something else changes labels Jul 14, 2025

SkiFire13 added 7 commits July 14, 2025 20:39

Move recursion_check_stack parameter into ComponentsRegistrator

c182932

Rewrite required components

e335244

Move required components tests

dfb290a

Remove and/or adapt invalid tests

2d40443

Add migration guide

229a897

Add regression test

87bd81b

Fix CI issues

18c4ca4

SkiFire13 force-pushed the rework-required-components branch from 8b14b9c to 18c4ca4 Compare July 14, 2025 18:39

alice-i-cecile requested review from cart and Jondolf July 14, 2025 19:33

SkiFire13 marked this pull request as ready for review July 14, 2025 19:48

urben1680 mentioned this pull request Jul 15, 2025

World::register_required_components updates bundles #19852

Open

Nit: merge two impl blocks

156f46e

SkiFire13 mentioned this pull request Jul 17, 2025

Component::register_required_components can be implemented in an unsound way #20173

Closed

SkiFire13 added 3 commits July 17, 2025 18:56

Fix soundness issue and add more migration docs

a5c3921

Move register methods from RequiredComponents to RequiredComponentsRe…

adb7930

…gistrator

Simplify temporarily taking RequiredComponents from Components

afb123e

Add test for cyclic requirement error

4f7a135

Merge remote-tracking branch 'upstream/main' into rework-required-com…

5b16cc1

…ponents

cBournhonesque reviewed Jul 23, 2025

View reviewed changes

SkiFire13 added 6 commits July 23, 2025 20:21

Mark explicit_components_len as inline

1f00001

Fix and simplify invariant of required_by

9bcb156

Improve docs for maintaining invariants on required_by

55b5511

Improve algorithmic complexity of updating required_by

df83128

Fix typos

bf7c4a6

Rename contributed_components to contributed_component_ids

8d2c8ff

chescock approved these changes Jul 24, 2025

View reviewed changes

SkiFire13 added 8 commits July 25, 2025 20:34

Optimization: take ownership of depth_first_components

79a63f4

Apply suggestion for new_requiree_components

93bd256

Add debug check in required_components_scope

8f4d8a6

Fix old comment

5850929

Apply suggestion for improving comment

88b8996

Change duplicate registration to panic

feea5c2

Fix: changed wrong variable to Vec

8722d58

Fix typo

0a0d6d2

alice-i-cecile added D-Complex Quite challenging from either a design or technical perspective. Ask for help! C-Code-Quality A section of code that is hard to understand or change labels Jul 28, 2025

alice-i-cecile changed the title ~~Rework required components~~ Ensure required components are consistently depth-first Jul 29, 2025

alice-i-cecile approved these changes Jul 29, 2025

View reviewed changes

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-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 29, 2025

alice-i-cecile added this pull request to the merge queue Jul 29, 2025

Merged via the queue into bevyengine:main with commit 45c997c Jul 29, 2025
38 checks passed

cBournhonesque mentioned this pull request Sep 24, 2025

Allow cycles in required components #21196

Open

Uh oh!

Ensure required components are consistently depth-first #20110

Ensure required components are consistently depth-first #20110

Uh oh!

Conversation

SkiFire13 commented Jul 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Testing

Uh oh!

alice-i-cecile commented Jul 14, 2025

Uh oh!

SkiFire13 commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

urben1680 commented Jul 15, 2025

Uh oh!

SkiFire13 commented Jul 19, 2025

Uh oh!

cBournhonesque left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cBournhonesque Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

SkiFire13 Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chescock left a comment

Choose a reason for hiding this comment

Uh oh!

chescock Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

SkiFire13 Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

SkiFire13 Jul 25, 2025

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SkiFire13 commented Jul 13, 2025 •

edited

Loading

SkiFire13 commented Jul 14, 2025 •

edited

Loading