[Merged by Bors] - Add warning when a hierarchy component is missing by nicopap · Pull Request #5590 · bevyengine/bevy

nicopap · 2022-08-06T09:02:24Z

Objective

A common pitfall since 0.8 is the requirement on ComputedVisibility
being present on all ancestors of an entity that itself has
ComputedVisibility, without which, the entity becomes invisible.

I myself hit the issue and got very confused, and saw a few people hit
it as well, so it makes sense to provide a hint of what to do when such
a situation is encountered.

Solution

We now check that all entities with both a Parent and a
ComputedVisibility component have parents that themselves have a
ComputedVisibility component.

Note that the warning is only printed once.

We also add a similar warning to GlobalTransform.

This only emits a warning. Because sometimes it could be an intended
behavior.

Alternatives:

Do nothing and keep repeating to newcomers how to avoid recurring
pitfalls
Make the transform and visibility propagation tolerant to missing
components (Visibility inheritance should ignore parents without a Visibility component #5616)
Probably archetype invariants, though the current draft would not
allow detecting that kind of errors

Changelog

Add a warning when encountering dubious component hierarchy structure

nicopap · 2022-08-06T09:17:07Z

crates/bevy_hierarchy/src/hierarchy.rs

 }

+/// System to print a warning if at any time we detect an inconsistent hierarchy.
+pub fn hierarchy_healthcheck_system<T: Component>(


I defined it in the bevy_hierarchy crate to reflect #5507

crates/bevy_render/src/view/visibility/mod.rs

ramirezmike · 2022-08-08T16:06:05Z

I like this change but did have one comment about the term "HealthCheck" although please take it with a grain of salt; I'm pretty new at looking at Bevy code!

To me it's not quite clear from "hierarchy_healthcheck_system" or the VisibilitySystems enum variant "HealthCheck" that it's related to verifying that the entities within a hierarchy are consistent with regards to these specific (hierarchy-affecting?) components. My initial reaction is "what is a healthy hierarchy vs unhealthy?"

hierarchy_consistency_check_system and VisibilitySystems::CheckHierarchyConsistency or something like that? Again, it's totally possible HealthCheck is an established pattern that I'm oblivious to though just thought I'd comment and learn

nicopap · 2022-08-08T16:38:14Z

@ramirezmike I actually took the "health check" term from both neovim and my previous job (where we created a tool to check if internal services were online).

I do agree that what the system does is not well reflected in the helthcheck name,
but I don't think that hierarchy_consistency_check is necessarily clearer than hierarchy_healthcheck. Neither of those hint at what it actually being checked, which is that no parents are missing the required component.

Maybe there is a better name yeT?

crates/bevy_hierarchy/src/hierarchy.rs

ramirezmike · 2022-08-09T13:37:34Z

I do agree that what the system does is not well reflected in the helthcheck name, but I don't think that hierarchy_consistency_check is necessarily clearer than hierarchy_healthcheck. Neither of those hint at what it actually being checked, which is that no parents are missing the required component.

Maybe there is a better name yeT?

Perhaps we will discover it!

hierarchy_component_mismatch_check?

crates/bevy_internal/src/default_plugins.rs

mockersf · 2022-08-19T09:19:40Z

This feels like the same kind of diagnostic level as system ordering ambiguities, and with the same kind of consequences in case of an issue (it kinda doesn't work but not in an explicit way).

What do you think of having a resource ReportHierarchyIssue similar to ReportExecutionOrderAmbiguities, and add those systems if the resource is present?

nicopap · 2022-08-19T09:26:05Z

What do you think of having a resource ReportHierarchyIssue similar to ReportExecutionOrderAmbiguities, and add those systems if the resource is present?

I like the idea, but I think it should be on by default. Supposedly, the user only benefits from the warning message if they don't know why their code is not working. But adding the resource requires knowing why their code is not working, making the message moot. The resource could be added on #[cfg(debug_assertions)] though, which I think is still better than adding the plugin on cfg flag.

crates/bevy_hierarchy/src/valid_parent_check_plugin.rs

errors/B0004.md

tim-blackbird

Looking good. Just a couple nits.

errors/B0004.md

crates/bevy_hierarchy/src/valid_parent_check_plugin.rs

nicopap · 2022-09-12T12:39:08Z

Btw I counted three more questions that would have been answered by this diagnostic.

A common pitfall since 0.8 is the requirement on `ComputedVisibility` being present on all ancestors of an entity that itself has `ComputedVisibility`, without which, the entity becomes invisible. I myself hit the issue and got very confused, and saw a few people hit it as well, so it makes sense to provide a hint of what to do when such a situation is encountered. We now check that all entities with both a `Parent` and a `ComputedVisibility` component have parents that themselves have a `ComputedVisibility` component. Note that the warning is only printed once. We also add a similar warning to `GlobalTransform`. This only emits a warning. Because sometimes it could be an intended behavior. Alternatives: - Do nothing and keep repeating to newcomers how to avoid recurring pitfalls - Make the transform and visibility propagations tolerant to missing components

Co-authored-by: ira <JustTheCoolDude@gmail.com>

alice-i-cecile

Annoying that we need this, but this is a very well-crafted diagnostic.

bors r+

# Objective A common pitfall since 0.8 is the requirement on `ComputedVisibility` being present on all ancestors of an entity that itself has `ComputedVisibility`, without which, the entity becomes invisible. I myself hit the issue and got very confused, and saw a few people hit it as well, so it makes sense to provide a hint of what to do when such a situation is encountered. - Fixes #5849 - Closes #5616 - Closes #2277 - Closes #5081 ## Solution We now check that all entities with both a `Parent` and a `ComputedVisibility` component have parents that themselves have a `ComputedVisibility` component. Note that the warning is only printed once. We also add a similar warning to `GlobalTransform`. This only emits a warning. Because sometimes it could be an intended behavior. Alternatives: - Do nothing and keep repeating to newcomers how to avoid recurring pitfalls - Make the transform and visibility propagation tolerant to missing components (#5616) - Probably archetype invariants, though the current draft would not allow detecting that kind of errors --- ## Changelog - Add a warning when encountering dubious component hierarchy structure Co-authored-by: Nicola Papale <nicopap@users.noreply.github.com>

bors · 2022-09-19T16:11:55Z

Build failed (retrying...):

build-and-install-on-iOS

# Objective A common pitfall since 0.8 is the requirement on `ComputedVisibility` being present on all ancestors of an entity that itself has `ComputedVisibility`, without which, the entity becomes invisible. I myself hit the issue and got very confused, and saw a few people hit it as well, so it makes sense to provide a hint of what to do when such a situation is encountered. - Fixes #5849 - Closes #5616 - Closes #2277 - Closes #5081 ## Solution We now check that all entities with both a `Parent` and a `ComputedVisibility` component have parents that themselves have a `ComputedVisibility` component. Note that the warning is only printed once. We also add a similar warning to `GlobalTransform`. This only emits a warning. Because sometimes it could be an intended behavior. Alternatives: - Do nothing and keep repeating to newcomers how to avoid recurring pitfalls - Make the transform and visibility propagation tolerant to missing components (#5616) - Probably archetype invariants, though the current draft would not allow detecting that kind of errors --- ## Changelog - Add a warning when encountering dubious component hierarchy structure Co-authored-by: Nicola Papale <nicopap@users.noreply.github.com>

bors · 2022-09-19T16:33:27Z

Pull request successfully merged into main.

Build succeeded:

# Objective A common pitfall since 0.8 is the requirement on `ComputedVisibility` being present on all ancestors of an entity that itself has `ComputedVisibility`, without which, the entity becomes invisible. I myself hit the issue and got very confused, and saw a few people hit it as well, so it makes sense to provide a hint of what to do when such a situation is encountered. - Fixes bevyengine#5849 - Closes bevyengine#5616 - Closes bevyengine#2277 - Closes bevyengine#5081 ## Solution We now check that all entities with both a `Parent` and a `ComputedVisibility` component have parents that themselves have a `ComputedVisibility` component. Note that the warning is only printed once. We also add a similar warning to `GlobalTransform`. This only emits a warning. Because sometimes it could be an intended behavior. Alternatives: - Do nothing and keep repeating to newcomers how to avoid recurring pitfalls - Make the transform and visibility propagation tolerant to missing components (bevyengine#5616) - Probably archetype invariants, though the current draft would not allow detecting that kind of errors --- ## Changelog - Add a warning when encountering dubious component hierarchy structure Co-authored-by: Nicola Papale <nicopap@users.noreply.github.com>

nicopap added C-Docs An addition or correction to our documentation C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Diagnostics Logging, crash handling, error reporting and performance analysis A-Hierarchy labels Aug 6, 2022

nicopap force-pushed the warn-against-missing-computed-visibility branch from a95bc6a to b021e81 Compare August 6, 2022 09:08

nicopap requested a review from mockersf August 6, 2022 09:12

nicopap commented Aug 6, 2022

View reviewed changes

ramirezmike reviewed Aug 8, 2022

View reviewed changes

crates/bevy_render/src/view/visibility/mod.rs Outdated Show resolved Hide resolved

nicopap mentioned this pull request Aug 8, 2022

Visibility inheritance should ignore parents without a Visibility component #5616

Closed

cart reviewed Aug 8, 2022

View reviewed changes

crates/bevy_hierarchy/src/hierarchy.rs Outdated Show resolved Hide resolved

cart reviewed Aug 8, 2022

View reviewed changes

crates/bevy_hierarchy/src/hierarchy.rs Outdated Show resolved Hide resolved

killercup reviewed Aug 9, 2022

View reviewed changes

crates/bevy_hierarchy/src/hierarchy.rs Outdated Show resolved Hide resolved

nicopap marked this pull request as draft August 10, 2022 16:23

nicopap force-pushed the warn-against-missing-computed-visibility branch from b021e81 to 26b8f08 Compare August 18, 2022 09:38

nicopap marked this pull request as ready for review August 18, 2022 10:04

nicopap requested review from cart, killercup and ramirezmike and removed request for killercup and ramirezmike August 18, 2022 10:04

nicopap commented Aug 18, 2022

View reviewed changes

crates/bevy_internal/src/default_plugins.rs Outdated Show resolved Hide resolved

killercup approved these changes Aug 18, 2022

View reviewed changes

tim-blackbird reviewed Sep 3, 2022

View reviewed changes

crates/bevy_hierarchy/src/valid_parent_check_plugin.rs Outdated Show resolved Hide resolved

errors/B0004.md Outdated Show resolved Hide resolved

errors/B0004.md Outdated Show resolved Hide resolved

errors/B0004.md Outdated Show resolved Hide resolved

nicopap force-pushed the warn-against-missing-computed-visibility branch from 6e46fa9 to e5db0f9 Compare September 5, 2022 11:25

tim-blackbird approved these changes Sep 6, 2022

View reviewed changes

errors/B0004.md Outdated Show resolved Hide resolved

tim-blackbird reviewed Sep 6, 2022

View reviewed changes

crates/bevy_hierarchy/src/valid_parent_check_plugin.rs Outdated Show resolved Hide resolved

nicopap force-pushed the warn-against-missing-computed-visibility branch from c1bbab5 to 4617dbf Compare September 12, 2022 13:55

nicopap and others added 5 commits September 12, 2022 15:55

Make validity check enabled through a resource

daa98a4

Improve B0004 warning description

5c0d896

wording on B0004

7d5bd86

Co-authored-by: ira <JustTheCoolDude@gmail.com>

wording on B0004 (2)

61a0c75

Co-authored-by: ira <JustTheCoolDude@gmail.com>

nicopap force-pushed the warn-against-missing-computed-visibility branch from 4617dbf to 61a0c75 Compare September 12, 2022 13:55

IceSentry approved these changes Sep 12, 2022

View reviewed changes

IceSentry 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 Sep 12, 2022

alice-i-cecile approved these changes Sep 19, 2022

View reviewed changes

bors bot changed the title ~~Add warning when a hierarchy component is missing~~ [Merged by Bors] - Add warning when a hierarchy component is missing Sep 19, 2022

bors bot closed this Sep 19, 2022

nicopap mentioned this pull request Sep 26, 2022

Entities are invisible when spawned as child via add_child() #6105

Closed

tim-blackbird mentioned this pull request Dec 9, 2022

Sprite z-order is wrong when parent entity has no Transform. #1976

Closed

nicopap mentioned this pull request Jan 19, 2023

Required Components #7272

Closed

nicopap deleted the warn-against-missing-computed-visibility branch September 5, 2023 15:46

alice-i-cecile added A-ECS Entities, components, systems, and events and removed A-Hierarchy labels Jan 28, 2025

Uh oh!

Conversation

nicopap commented Aug 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Changelog

Uh oh!

nicopap Aug 6, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ramirezmike commented Aug 8, 2022

Uh oh!

nicopap commented Aug 8, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ramirezmike commented Aug 9, 2022

Uh oh!

Uh oh!

mockersf commented Aug 19, 2022

Uh oh!

nicopap commented Aug 19, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tim-blackbird left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nicopap commented Sep 12, 2022

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

bors bot commented Sep 19, 2022

Uh oh!

bors bot commented Sep 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

nicopap commented Aug 6, 2022 •

edited

Loading