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

Warn about broken hierarchies #5849

Closed
asafigan opened this issue Sep 1, 2022 · 2 comments
Closed

Warn about broken hierarchies #5849

asafigan opened this issue Sep 1, 2022 · 2 comments
Labels
A-Diagnostics Logging, crash handling, error reporting and performance analysis A-Hierarchy Parent-child entity hierarchies C-Docs An addition or correction to our documentation C-Usability A targeted quality-of-life change that makes Bevy easier to use

Comments

@asafigan
Copy link
Contributor

asafigan commented Sep 1, 2022

What problem does this solve or what need does it fill?

Currently things like rendering require that every ancestor in a hierarchy have a set of components in order to work. Adding a child to a parent which doesn't have these components silently fails. For example, adding a entity with a PbrBundle to a parent that doesn't have a SpatialBundle causes the child not to be rendered. It can be confusing what the cause of the problem is.

What solution would you like?

On debug builds, warn that a child has SpatialBundle components but it's parent doesn't.

What alternative(s) have you considered?

Writing my own system to do this for me.

Additional context

A missing SpatialBundle wasted me an hour of time on Bevy Jam 2. A simple warning would have pointed out the issue to me immediately and saved me all that time and energy.

There are other issues that show this is a common foot gun: #2277, #5081

@asafigan asafigan added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Sep 1, 2022
@tim-blackbird
Copy link
Contributor

There's an open PR for this: #5590.

@nicopap 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 Parent-child entity hierarchies and removed C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Sep 1, 2022
@asafigan
Copy link
Contributor Author

asafigan commented Sep 1, 2022

Great!

bors bot pushed a commit that referenced this issue Sep 19, 2022
# 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 <[email protected]>
@bors bors bot closed this as completed in 6c5403c Sep 19, 2022
james7132 pushed a commit to james7132/bevy that referenced this issue Oct 19, 2022
# 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 <[email protected]>
james7132 pushed a commit to james7132/bevy that referenced this issue Oct 28, 2022
# 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 <[email protected]>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
# 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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Diagnostics Logging, crash handling, error reporting and performance analysis A-Hierarchy Parent-child entity hierarchies C-Docs An addition or correction to our documentation C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants