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

Visibility inheritance should ignore parents without a Visibility component #5616

Closed
Alainx277 opened this issue Aug 8, 2022 · 2 comments
Closed
Labels
A-Hierarchy Parent-child entity hierarchies A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide
Milestone

Comments

@Alainx277
Copy link

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

When creating a hierarchy of objects, where some of them have meshes but the root does not, the children will not render. This is due to the fact that the parent requires a VisibilityBundle so the children are visible_in_hierarchy.

What solution would you like?

If a parent entity doesn't specify any visibility, the direct descendants should automatically have visible_in_hierarchy set to true (currently false).

What alternative(s) have you considered?

Rely on the user to know every object in the hierarchy needs a VisibilityBundle so child objects are also visible. This means it has to be clearly documented and shown to beginners (similar to GlobalTransform).

As a personal anecdote: I've wasted quite some time debugging because I didn't know about this (as it is new in 0.8). I imagine many new-ish Bevy users will experience this problem.

@Alainx277 Alainx277 added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Aug 8, 2022
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Hierarchy Parent-child entity hierarchies D-Trivial Nice and easy! A great choice to get started with Bevy and removed C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Aug 8, 2022
@nicopap
Copy link
Contributor

nicopap commented Aug 8, 2022

I'm in favor of this change, but if for whatever reason it is refused, please see #5590

@alice-i-cecile alice-i-cecile added this to the Bevy 0.9 milestone Aug 8, 2022
@alice-i-cecile alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Aug 8, 2022
@mockersf
Copy link
Member

mockersf commented Aug 8, 2022

I think this is an issue mostly for existing users who are not used to the new requirement, or using older tutorials. New users following up to date docs shouldn't (hopefully) have the issue.

This would need to completely rework how inheritance currently works as it uses the component as a filter.

@alice-i-cecile alice-i-cecile removed the D-Trivial Nice and easy! A great choice to get started with Bevy label Aug 8, 2022
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 <nicopap@users.noreply.github.com>
@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 <nicopap@users.noreply.github.com>
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 <nicopap@users.noreply.github.com>
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 <nicopap@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Hierarchy Parent-child entity hierarchies A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants