Inconsistent empty Children component between hierarchy commands

[oceantume]

Bevy version

Main branch

What you did

Run the following unit test for crates\bevy_hierarchy\src\child_builder.rs:
https://gist.github.com/oceantume/c401731182f59ff07d6002f231fb7109

What went wrong

The test fails because the final state of the Children component for a parent that is left with no more children is different according to how it happens:

• Moving all children from a parent to another one results in the Children component getting removed.
• Removing all children from a parent with remove_children results in a Children component with an empty list of children.

Expected behavior

I do not have a strong preference on what should happen, but I think it should be consistent. I see two simple outcomes with advantages and disadvantages here:

1. The Children component always gets removed.
2. The Children component always remains, containing an empty list of children.

Option 1 has the disadvantage that Changed<Children> queries will not be fulfilled when an entity loses all its children, but that's because it "cleans up" after itself in the case that entities are not longer part of a tree. For example, in option 2, an entity that loses its parent and children still keeps its Children component so it's still being returned in Query<Children> queries.

Using the HierarchyEvent::ChildMoved and HierarchyEvent::ChildRemoved events instead of Changed<Children> could help with option 1, but it's not as intuitive for users.

[alice-i-cecile]

My preference would be for 2; I think that the change detection + lack of archetype change is more important than complete cleanup. Eventually we might want to toss on vec shrinking, but eh.

I do think there should be an API for completely removing the corresponding component, but it should be explicit and opt-in.

[oceantume]

That makes sense. I will probably make a PR to implement option 2, including some more unit tests that will check for this consistency.

As a side note, are you at all worried about the fact that the Parent component is different and is always removed when an entity loses its parent and therefore Children and Parent will be somewhat inconsistent with each other in that regard?

[alice-i-cecile]

Not as much, but it needs to be clearly documented. I think it's much more common in practice for children to end up temporarily empty rather than for an entity to only be temporarily unparented (as parents are generally longer-lived).

[james7132]

Quickly chiming in: I actually suggest consistently removing the Children component, as it ensures Without<Children> consistently returns leaf entities. This does have some issues with change detection, but there are workarounds, whereas there is no workaround if we leave empty Children components around.

[oceantume]

Like I said I see advantages and disadvantages in both solution and they both make sense depending on what use-cases you consider.

I think @james7132 hits a good point here related to consistency though. Since they're one of the main mechanisms to query entities that are part of a hierarchy, I expect Query<Children>, With<Children> and Without<Children> to be consistent in what they return. It's not consistent if what they return depends on whether each leaf had children at some point during execution or not.

[alice-i-cecile]

I see your point @james7132. Consider my vote flipped (and I agree we should have consistency).

Down the line I'd really like a RemovedOrChanged query filter which could be used to catch all those cases. That's a different issue though.