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

Child's Transform component ignored when parent does not have the GlobalTransform/Transform components #2277

Closed
allsey87 opened this issue May 29, 2021 · 11 comments
Labels
A-ECS Entities, components, systems, and events A-Transform Translations, rotations and scales C-Docs An addition or correction to our documentation C-Usability A targeted quality-of-life change that makes Bevy easier to use

Comments

@allsey87
Copy link
Contributor

allsey87 commented May 29, 2021

Bevy version

9f94f7e

Operating system & version

Arch Linux

What you did

  1. Create an entity using just spawn, i.e., without Transform and GlobalTransform components.
  2. Add a child using spawn_bundle and PbrBundle, setting the mesh, material, and transform fields (use ..Default::default() for the other fields)

What you expected to happen

Since the parent entity didn't have either a Transform or a GlobalTransform component, I would expect that the transform component in the child entity would be relative to the reference frame. If this had been the case, the transform_propagate_system should have set the GlobalTransform component according to the Transform component and the mesh should have been rendered at the offset provided in the PbrBundle.

What actually happened

The mesh renders at the origin (i.e., the transform field from the PbrBundle appears to have been ignored).

Additional information

From the query in the transform_propagate_system, it appears that this system does not handle the case where the parent entity doesn't have a GlobalTransform/Transform component, but it's children do. Is this behavior intentional?

@allsey87 allsey87 added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels May 29, 2021
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets and removed S-Needs-Triage This issue needs to be labelled labels May 29, 2021
@allsey87
Copy link
Contributor Author

allsey87 commented May 29, 2021

@alice-i-cecile I think this issue is more about bevy's transform and its associated systems. Perhaps ecs and usability are the most suitable tags here.

@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets labels May 29, 2021
@alice-i-cecile
Copy link
Member

alice-i-cecile commented May 29, 2021

Yeah; this is... kind of at the intersection of a number of concerns, due to how heavily Transforms are used. @NathanSWard what would you classify this as under the proposed new labelling scheme?

@NathanSWard
Copy link
Contributor

NathanSWard commented May 29, 2021

what would you classify this as under the proposed new labelling scheme

I wouldn't say ecs since the problem isn't due to ECS, but rather the transform systems that are implemented using ECS.
So
T: bug
S: needs investigation
C: ???

it doesn't really fit nicely into a single category, so does it deserve it's own? I mean it already has it's own crate?

C: Transform

@alice-i-cecile
Copy link
Member

Yeah, I think that if we were to use content labels based on crates it would work well.

@djeedai
Copy link
Contributor

djeedai commented Oct 2, 2021

As of 0.5 release, my observation is that the children collapse at origin not if the parent is missing Transform or GlobalTransform, but if it's missing Transform and GlobalTransform (both). That is, you can add one of those two only to the parent, and the bug still repro. Only when you add both Transform and GlobalTransform then the children are positioned correctly relative to the parent. I suppose this makes sense given how I imagine things work internally, but this is extremely confusing for new users like me.

@alice-i-cecile alice-i-cecile added the A-Transform Translations, rotations and scales label Dec 12, 2021
@maniwani
Copy link
Contributor

maniwani commented Mar 8, 2022

So is this just missing documentation?

Since the parent entity didn't have Transform or GlobalTransform components, I expected the Transform component in the child entity to be treated as relative to the [world?] reference frame.

This seems like a misunderstanding. Even "empty" objects in other game engines and tools like Blender have transforms.

If the Parent and Children components are intended for use with the transform hierarchy alone, it's internally consistent that Transform and GlobalTransform are required on every entity in the hierarchy for it work properly.

@alice-i-cecile alice-i-cecile added C-Docs An addition or correction to our documentation and removed C-Bug An unexpected or incorrect behavior C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Mar 8, 2022
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Mar 8, 2022

I agree, IMO this is primarily a docs issue.

@maniwani
Copy link
Contributor

maniwani commented Mar 8, 2022

There was a suggestion to parameterize Parent and Children into Parent<T> and Children<T> that came up in #2789. That might be worth pursuing if those component names are popular for describing other, non-transform hierarchies.

@djeedai
Copy link
Contributor

djeedai commented Mar 8, 2022

This seems like a misunderstanding. Even "empty" objects in other game engines and tools like Blender have transforms.

In Unity, yes. A design decision they came to regret since then, having people make hierarchies of objects just for grouping / scene organization in editor, and ruining performance at runtime as Unity needs to update all of those transforms for nothing. UE4 avoids that with UActorComponent which does not have any transform. So I'd argue there's room for various expectations.

If the Parent and Children components are intended for use with the transform hierarchy alone, it's internally consistent that Transform and GlobalTransform are required on every entity in the hierarchy for it work properly.

In this context the expectation of @allsey87 seems perfectly legitimate to me. There is nothing in the docs indicating that Transform will be ignored on a child if the parent itself doesn't have a Transform and GlobalTransform (at best the docs suggest the parent will not be displayed, and that's all). In fact I'm still not sure what the right behavior is, because it depends on the scope of Parent and Children and this is not documented, so we're only assuming here based on actual usage. So yes there's at least a doc issue. And if the current implementation is to be the behavior, then adding children to a transform-less parent should probably raise a warning.

@maniwani
Copy link
Contributor

maniwani commented Mar 9, 2022

In Unity, yes. A design decision they came to regret since then, having people make hierarchies of objects just for grouping / scene organization in editor, and ruining performance at runtime as Unity needs to update all of those transforms for nothing.

Hmm, I see your point. Bevy does have change detection helping, but definitely want to avoid pointless work.

UE4 avoids that with UActorComponent which does not have any transform. So I'd argue there's room for various expectations.

So does Unreal behave like the reporter expected? Where if an actor's parent in the scene hierarchy has no Transform, its transform is interpreted as being in world space?

In fact I'm still not sure what the right behavior is, because it depends on the scope of Parent and Children and this is not documented, so we're only assuming here based on actual usage.

All valid points. I assumed from the fact that these components are defined in bevy_transform (which I guess doesn't matter since they're re-exported in the prelude) that their sole purpose was propagating the transform hierarchy.

And if the current implementation is to be the behavior, then adding children to a transform-less parent should probably raise a warning.

I think enforcing that Parent must be accompanied by a Transform and GlobalTransform requires "archetype invariants", but that mechanism hasn't been implemented yet. Aside from that, we can't really specialize the insert and remove commands, so our only other option would be to scan for entities in violation.

@alice-i-cecile alice-i-cecile added the C-Usability A targeted quality-of-life change that makes Bevy easier to use label Mar 9, 2022
@nicopap
Copy link
Contributor

nicopap commented Jul 28, 2022

IMO Bevy could really do with an optional system for debugging stuff like non-constant hierarchies. For example by printing messages when it detects

  • an entity with a Transform but a parent without a GlobalTransform
  • an entity with a Visibility but a parent without a ComputedVisibility

I don't think it's exclusively a doc issue, as it's fairly easy to end up in such a situation accidentally, and no amount of docs can completely prevent programming errors.

User-defined hierarchies would be what fixes this, and I don't think we have a short term plan for this (it would be covered by relations, but that's a fairly long term plan)

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-ECS Entities, components, systems, and events A-Transform Translations, rotations and scales 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.

6 participants