Skip to content

Replace HierarchyEvent with OnParentChange trigger #13925

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

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
189 changes: 84 additions & 105 deletions crates/bevy_hierarchy/src/child_builder.rs
Original file line number Diff line number Diff line change
@@ -1,21 +1,12 @@
use crate::{Children, HierarchyEvent, Parent};
use crate::{Children, OnParentChange, Parent};
use bevy_ecs::{
bundle::Bundle,
entity::Entity,
prelude::Events,
system::{Commands, EntityCommands},
world::{Command, EntityWorldMut, World},
};
use smallvec::{smallvec, SmallVec};

// Do not use `world.send_event_batch` as it prints error message when the Events are not available in the world,
// even though it's a valid use case to execute commands on a world without events. Loading a GLTF file for example
fn push_events(world: &mut World, events: impl IntoIterator<Item = HierarchyEvent>) {
if let Some(mut moved) = world.get_resource_mut::<Events<HierarchyEvent>>() {
moved.extend(events);
}
}

/// Adds `child` to `parent`'s [`Children`], without checking if it is already present there.
///
/// This might cause unexpected results when removing duplicate children.
Expand Down Expand Up @@ -72,18 +63,18 @@ fn update_old_parent(world: &mut World, child: Entity, parent: Entity) {
if previous_parent == parent {
return;
}

remove_from_children(world, previous_parent, child);

push_events(
world,
[HierarchyEvent::ChildMoved {
child,
previous_parent,
new_parent: parent,
}],
world.trigger_targets(
OnParentChange::Moved {
previous: previous_parent,
new: parent,
},
child,
);
} else {
push_events(world, [HierarchyEvent::ChildAdded { child, parent }]);
world.trigger_targets(OnParentChange::Added(parent), child);
}
}

Expand All @@ -96,7 +87,6 @@ fn update_old_parent(world: &mut World, child: Entity, parent: Entity) {
///
/// Sends [`HierarchyEvent`]'s.
fn update_old_parents(world: &mut World, parent: Entity, children: &[Entity]) {
let mut events: SmallVec<[HierarchyEvent; 8]> = SmallVec::with_capacity(children.len());
for &child in children {
if let Some(previous) = update_parent(world, child, parent) {
// Do nothing if the entity already has the correct parent.
Expand All @@ -105,37 +95,32 @@ fn update_old_parents(world: &mut World, parent: Entity, children: &[Entity]) {
}

remove_from_children(world, previous, child);
events.push(HierarchyEvent::ChildMoved {
world.trigger_targets(
OnParentChange::Moved {
previous,
new: parent,
},
child,
previous_parent: previous,
new_parent: parent,
});
);
} else {
events.push(HierarchyEvent::ChildAdded { child, parent });
world.trigger_targets(OnParentChange::Added(parent), child);
}
}
push_events(world, events);
}

/// Removes entities in `children` from `parent`'s [`Children`], removing the component if it ends up empty.
/// Also removes [`Parent`] component from `children`.
fn remove_children(parent: Entity, children: &[Entity], world: &mut World) {
let mut events: SmallVec<[HierarchyEvent; 8]> = SmallVec::new();
if let Some(parent_children) = world.get::<Children>(parent) {
for &child in children {
if parent_children.contains(&child) {
events.push(HierarchyEvent::ChildRemoved { child, parent });
}
}
} else {
let Some(parent_children) = world.get::<Children>(parent).map(|c| c.0.clone()) else {
return;
}
for event in &events {
if let &HierarchyEvent::ChildRemoved { child, .. } = event {
world.entity_mut(child).remove::<Parent>();
};

for child in children {
if parent_children.contains(child) {
world.trigger_targets(OnParentChange::Removed(parent), *child);
world.entity_mut(*child).remove::<Parent>();
}
}
push_events(world, events);

let mut parent = world.entity_mut(parent);
if let Some(mut parent_children) = parent.get_mut::<Children>() {
Expand Down Expand Up @@ -484,13 +469,8 @@ impl<'w> WorldChildBuilder<'w> {
pub fn spawn(&mut self, bundle: impl Bundle) -> EntityWorldMut<'_> {
let entity = self.world.spawn((bundle, Parent(self.parent))).id();
push_child_unchecked(self.world, self.parent, entity);
push_events(
self.world,
[HierarchyEvent::ChildAdded {
child: entity,
parent: self.parent,
}],
);
self.world
.trigger_targets(OnParentChange::Added(self.parent), entity);
self.world.entity_mut(entity)
}

Expand All @@ -499,13 +479,8 @@ impl<'w> WorldChildBuilder<'w> {
pub fn spawn_empty(&mut self) -> EntityWorldMut<'_> {
let entity = self.world.spawn(Parent(self.parent)).id();
push_child_unchecked(self.world, self.parent, entity);
push_events(
self.world,
[HierarchyEvent::ChildAdded {
child: entity,
parent: self.parent,
}],
);
self.world
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

.trigger_targets(OnParentChange::Added(self.parent), entity);
self.world.entity_mut(entity)
}

Expand Down Expand Up @@ -670,7 +645,7 @@ impl<'w> BuildWorldChildren for EntityWorldMut<'w> {
if let Some(parent) = self.take::<Parent>().map(|p| p.get()) {
self.world_scope(|world| {
remove_from_children(world, parent, child);
push_events(world, [HierarchyEvent::ChildRemoved { child, parent }]);
world.trigger_targets(OnParentChange::Removed(parent), child);
});
}
self
Expand All @@ -694,15 +669,15 @@ mod tests {
use super::{BuildChildren, BuildWorldChildren};
use crate::{
components::{Children, Parent},
HierarchyEvent::{self, ChildAdded, ChildMoved, ChildRemoved},
OnParentChange,
};
use smallvec::{smallvec, SmallVec};

use bevy_ecs::{
component::Component,
entity::Entity,
event::Events,
system::Commands,
observer::Trigger,
system::{Commands, ResMut, Resource},
world::{CommandQueue, World},
};

Expand All @@ -716,51 +691,53 @@ mod tests {
assert_eq!(world.get::<Children>(parent).map(|c| &**c), children);
}

#[derive(Resource, Default)]
struct TriggeredEvents(Vec<(OnParentChange, Entity)>);

/// Used to omit a number of events that are not relevant to a particular test.
fn omit_events(world: &mut World, number: usize) {
let mut events_resource = world.resource_mut::<Events<HierarchyEvent>>();
let mut events: Vec<_> = events_resource.drain().collect();
events_resource.extend(events.drain(number..));
let mut events_resource = world.resource_mut::<TriggeredEvents>();
let mut events: Vec<_> = events_resource.0.drain(0..).collect();
events_resource.0.extend(events.drain(number..));
}

fn assert_events(world: &mut World, expected_events: &[HierarchyEvent]) {
fn assert_events(world: &mut World, expected_events: &[(OnParentChange, Entity)]) {
let events: Vec<_> = world
.resource_mut::<Events<HierarchyEvent>>()
.drain()
.resource_mut::<TriggeredEvents>()
.0
.drain(0..)
.collect();
assert_eq!(events, expected_events);
}

#[test]
fn add_child() {
let world = &mut World::new();
world.insert_resource(Events::<HierarchyEvent>::default());

world.init_resource::<TriggeredEvents>();

world.observe(
|trigger: Trigger<OnParentChange>, mut triggered_events: ResMut<TriggeredEvents>| {
triggered_events
.0
.push((trigger.event().clone(), trigger.entity()));
},
);

let [a, b, c, d] = std::array::from_fn(|_| world.spawn_empty().id());

world.entity_mut(a).add_child(b);

assert_parent(world, b, Some(a));
assert_children(world, a, Some(&[b]));
assert_events(
world,
&[ChildAdded {
child: b,
parent: a,
}],
);
assert_events(world, &[(OnParentChange::Added(a), b)]);

world.entity_mut(a).add_child(c);

assert_children(world, a, Some(&[b, c]));
assert_parent(world, c, Some(a));
assert_events(
world,
&[ChildAdded {
child: c,
parent: a,
}],
);
assert_events(world, &[(OnParentChange::Added(a), c)]);

// Children component should be removed when it's empty.
world.entity_mut(d).add_child(b).add_child(c);
assert_children(world, a, None);
Expand All @@ -769,21 +746,24 @@ mod tests {
#[test]
fn set_parent() {
let world = &mut World::new();
world.insert_resource(Events::<HierarchyEvent>::default());

world.init_resource::<TriggeredEvents>();

world.observe(
|trigger: Trigger<OnParentChange>, mut triggered_events: ResMut<TriggeredEvents>| {
triggered_events
.0
.push((trigger.event().clone(), trigger.entity()));
},
);

let [a, b, c] = std::array::from_fn(|_| world.spawn_empty().id());

world.entity_mut(a).set_parent(b);

assert_parent(world, a, Some(b));
assert_children(world, b, Some(&[a]));
assert_events(
world,
&[ChildAdded {
child: a,
parent: b,
}],
);
assert_events(world, &[(OnParentChange::Added(b), a)]);

world.entity_mut(a).set_parent(c);

Expand All @@ -792,11 +772,13 @@ mod tests {
assert_children(world, c, Some(&[a]));
assert_events(
world,
&[ChildMoved {
child: a,
previous_parent: b,
new_parent: c,
}],
&[(
OnParentChange::Moved {
previous: b,
new: c,
},
a,
)],
);
}

Expand All @@ -820,7 +802,16 @@ mod tests {
#[test]
fn remove_parent() {
let world = &mut World::new();
world.insert_resource(Events::<HierarchyEvent>::default());

world.init_resource::<TriggeredEvents>();

world.observe(
|trigger: Trigger<OnParentChange>, mut triggered_events: ResMut<TriggeredEvents>| {
triggered_events
.0
.push((trigger.event().clone(), trigger.entity()));
},
);

let [a, b, c] = std::array::from_fn(|_| world.spawn_empty().id());

Expand All @@ -831,24 +822,12 @@ mod tests {
assert_parent(world, c, Some(a));
assert_children(world, a, Some(&[c]));
omit_events(world, 2); // Omit ChildAdded events.
assert_events(
world,
&[ChildRemoved {
child: b,
parent: a,
}],
);
assert_events(world, &[(OnParentChange::Removed(a), b)]);

world.entity_mut(c).remove_parent();
assert_parent(world, c, None);
assert_children(world, a, None);
assert_events(
world,
&[ChildRemoved {
child: c,
parent: a,
}],
);
assert_events(world, &[(OnParentChange::Removed(a), c)]);
}

#[allow(dead_code)]
Expand Down
40 changes: 14 additions & 26 deletions crates/bevy_hierarchy/src/events.rs
Original file line number Diff line number Diff line change
@@ -1,31 +1,19 @@
use bevy_ecs::{event::Event, prelude::Entity};

/// An [`Event`] that is fired whenever there is a change in the world's hierarchy.
/// A [`Trigger`] emitted whenever there is a change in the world's hierarchy.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

///
/// [`Event`]: bevy_ecs::event::Event
#[derive(Event, Debug, Clone, PartialEq, Eq)]
pub enum HierarchyEvent {
/// Fired whenever an [`Entity`] is added as a child to a parent.
ChildAdded {
/// The child that was added
child: Entity,
/// The parent the child was added to
parent: Entity,
},
/// Fired whenever a child [`Entity`] is removed from its parent.
ChildRemoved {
/// The child that was removed
child: Entity,
/// The parent the child was removed from
parent: Entity,
},
/// Fired whenever a child [`Entity`] is moved to a new parent.
ChildMoved {
/// The child that was moved
child: Entity,
/// The parent the child was removed from
previous_parent: Entity,
/// The parent the child was added to
new_parent: Entity,
/// [`Trigger`]: bevy_ecs::observer::Trigger
#[derive(Event, Clone, Debug, PartialEq)]
pub enum OnParentChange {
/// Emitted whenever the entity is added as a child to a parent.
Added(Entity),
/// Emitted whenever the child entity is removed from its parent.
Removed(Entity),
/// Emitted whenever the child entity is moved to a new parent.
Moved {
/// The parent the child was removed from.
previous: Entity,
/// The parent the child was added to.
new: Entity,
},
}
4 changes: 1 addition & 3 deletions crates/bevy_hierarchy/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,6 @@ pub struct HierarchyPlugin;
#[cfg(feature = "bevy_app")]
impl Plugin for HierarchyPlugin {
fn build(&self, app: &mut App) {
app.register_type::<Children>()
.register_type::<Parent>()
.add_event::<HierarchyEvent>();
app.register_type::<Children>().register_type::<Parent>();
}
}