Replace HierarchyEvent with OnParentChange trigger

[Jondolf]

Objective

Currently, the HierarchyEvent is sent whenever an entity is added as a child, its parent is removed, or it is moved from one parent to another.

For these kinds of hierarchy changes however, an observer-based approach can often be more valuable than a batched event type. It could potentially help unblock new transform propagation patterns, for example.

In my use case, I need to mark ancestors of entities that have a given component with a marker component to speed up transform propagation and skip traversing unnecessary trees. To add and clean up these markers, I am using observers, but for a robust and efficient solution, I need to detect hierarchy changes.

Solution

Remove the HierarchyEvent in favor of an OnParentChange trigger. The target is the child entity.

Testing

There were existing tests to make sure the correct events get sent. I modified these tests to use observers and the OnParentChange trigger.

Design Questions

• The new trigger targets the child, but it could also be flipped to target the parent. Which one is more valuable?
• Are the variant names Added, Removed, and Moved fine, or are there better/clearer options?

---

Migration Guide

The HierarchyEvent has been replaced by an OnParentChange trigger. Instead of using an EventReader<HierarchyEvent>, you should use an observer to listen for the event.

// Old: Use `EventReader`.
fn listen_to_hierarchy_events(mut event_reader: EventReader<HierarchyEvent>) {
    for event in event.read() {
        match event {
            HierarchyEvent::ChildAdded { child, parent } => todo!(),
            _ => todo!("other match arms")
        }
    }
}

// New: Use an observer. You could also target a specific entity.
app.observe(|trigger: Trigger<OnParentChange>| {
    let child = trigger.entity();
    match trigger.event() {
        OnParentChange::Added(parent) => todo!(),
        _ => todo!("other match arms")
    }
});

[alice-i-cecile]

I do think we should do this, but it's a bit more than uncontroversial :)

[komadori]

I assume the SmallVec was originally used here to amortise the cost of get_resource_mut inside push_events. Events are implicitly deferred until this system finishes and the reader system runs, so it can't have been to conceal the intermediate state of the world.

Could you just call world.trigger_targets() directly now without building the events buffer? trigger_events doesn't do anything more than call it in a loop.

OTOH, observers are called immediately and if you do need to defer execution until after you've finished processing for some reason then you can use world.commands().trigger_targets() and make use of the existing command queue. This might be more performant and avoid the allocation that currently happens every frame if the stack allocated capacity of the SmallVec is exceeded.

[Jondolf]

Yeah I don't think we need deferred execution here, so I just removed the event buffers and trigger_events in favor of simply calling world.trigger_targets(...) directly.

For remove_children I had to clone the SmallVec contained in Children to get around issues with simultaneous mutable and immutable World access. I guess maybe there could be some unsafe way to avoid cloning (or some other approach I'm missing), but then you'd need to ensure that parent != child.

[komadori]

Regarding eliminating the clone() in remove_children, I have two ideas:

i) You could move the call to world.get() inside the loop. This means that &Children is only borrowed from world until the call to parent_children.contain() finishes, and then you're free to mutably borrow world in order to trigger the observer, etc. The downside is that when removing multiple children you have to pay the overhead of looking up the entity and component multiple times rather than once.

for child in children {
    let Some(parent_children) = world.get::<Children>(parent) else {
        return;
    };
    if parent_children.contains(child) {
        // ...

ii) You could temporarily remove the vector of parent's children from the world and put it back afterwards. This can be efficiently accomplished by swapping the Children component with an empty one. If the vector is large enough to have required the SmallVec to allocate then this is just a pointer swap. The downside is that the parent's Children component will appear empty to any observers examining the world during the remove operation.

This approach also lets you combine processing the child entities with removing elements from the parent_children vector, and hence halves the number of calls to contains().

fn remove_children(parent: Entity, children: &[Entity], world: &mut World) {
    let Some(mut borrowed_parent_children) = world.get_mut::<Children>(parent) else {
        return;
    };

    // Temporarily take the vector of parent's children out of the world.
    let mut parent_children = Children::from_entities(&[]);
    mem::swap(&mut parent_children, borrowed_parent_children.as_mut());

    // Remove children and trigger observers
    parent_children.0.retain(|child| {
        let remove = children.contains(child);
        if remove {
            world.trigger_targets(OnParentChange::Removed(parent), *child);
            world.entity_mut(*child).remove::<Parent>();
        }
        !remove
    });

    // Return the parent's children to the world if there are any left.
    let mut parent = world.entity_mut(parent);
    if parent_children.is_empty() {
        parent.remove::<Children>();
    } else {
        if let Some(mut borrowed_parent_children) = parent.get_mut::<Children>() {
            mem::swap(&mut parent_children, borrowed_parent_children.as_mut());
        }
    }
}

Since this code involves triggering an observer in the loop, I don't think there any is way you could rely on unsafe primitives to help as an observer could break any invariants.

Anyway, I'm really just nitpicking :-). I think this is a good use case for observers.

[komadori]

This is an example of somewhere the observer will be called much sooner that an EventReader<HierarchyEvent> could have responded. It will see the Entity just as it has been spawned, before any components could be added. Open Question: Is this desirable?

[komadori]

It's worth thinking about what the World looks like when you trigger an observer in the middle of an operation as this isn't something that mattered with the old event. Overall though, I think observers are a much better fit because they can be targeted at the entities involved and this is a good PR for ergonomics.

Edit: Since GitHub seems to want to collapse it as resolved, I'm going to drop in a link to the comment I just added in the review about the efficiency of remove_children: #13925 (comment)

[alice-i-cecile]

I'd like to see more clarity in these docs about which entity is triggered. I believe it's the Parent, but that should be explicit.

A doc test would also be nice here.

[Jondolf]

Sorry for the long wait! I tried to clarify this a bit now. It's actually triggered for the child entity that has the Parent component, hence the name OnParentChange.

If the event was instead triggered for parents, it would need to be triggered for both the old and new parent for the OnParentChange::Moved case, which might have a bit more overhead. It could also complicate some logic, since you would need to take into account that a single hierarchy change could trigger the same observer twice (for different entities), and make sure things don't conflict.

[Jondolf]

Also added a simple doc example. Not sure if it should use World directly, or have the general App setup (bevy_app is an optional dependency).

Couldn't make it into an actual doc test nicely, since the observer makes it a bit awkward to test things. I did test it manually though, and the actual tests should cover it.

[Jondolf]

I think the removal of bevy_hierarchy and HierarchyEvent in #17398 makes this PR pretty redundant and very outdated, so I'll close this. I'm not sure how you're intended to respond to hierarchy changes now though (e.g. moving a child from one parent to another, with access to both Entity IDs), since there doesn't seem to be any equivalent to HierarchyEvent anymore, unless I've just missed it.

[alice-i-cecile]

You can use observers on the add/removal of ChildOf, since that's an immutable component.

[Jondolf]

How do I access the old parent's Entity ID when I move a child from one parent to another?

Edit: I suppose OnReplace has the old value. You just don't have access to the new and old parent at the same time, unlike with the old HierarchyEvent.