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.

Sign up for GitHub

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

Replace HierarchyEvent with OnParentChange trigger #13925

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
810f584
Replace `HierarchyEvent` with `OnParentChange` trigger
Jondolf Jun 19, 2024
3f6cbab
Remove unnecessary event initialization
Jondolf Jun 19, 2024
90152d7
Revert function rename
Jondolf Jun 19, 2024
a9b4f3c
Remove `trigger_events`
Jondolf Jun 20, 2024
41cdaab
Merge branch 'main' into hierarchy-observer
Jondolf Dec 5, 2024
6a8aff0
Improve docs for `OnParentChange`
Jondolf Dec 5, 2024
fb99eef
Fix links to `HierarchyEvent`
Jondolf Dec 5, 2024
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
194 changes: 86 additions & 108 deletions crates/bevy_hierarchy/src/child_builder.rs
View file
Open in desktop
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 @@ -64,26 +55,26 @@ fn remove_from_children(world: &mut World, parent: Entity, child: Entity) {
///
/// Does nothing if `child` was already a child of `parent`.
///
/// Sends [`HierarchyEvent`]'s.
/// Triggers [`OnParentChange`].
fn update_old_parent(world: &mut World, child: Entity, parent: Entity) {
let previous = update_parent(world, child, parent);
if let Some(previous_parent) = previous {
// Do nothing if the child was already parented to this 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 @@ -94,9 +85,8 @@ fn update_old_parent(world: &mut World, child: Entity, parent: Entity) {
///
/// Does nothing for a child if it was already a child of `parent`.
///
/// Sends [`HierarchyEvent`]'s.
/// Triggers [`OnParentChange`].
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 @@ -546,26 +531,16 @@ impl ChildBuild for WorldChildBuilder<'_> {
fn spawn(&mut self, bundle: impl Bundle) -> EntityWorldMut {
let entity = self.world.spawn((bundle, Parent(self.parent))).id();
add_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)
}

fn spawn_empty(&mut self) -> EntityWorldMut {
let entity = self.world.spawn(Parent(self.parent)).id();
add_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);
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?

self.world.entity_mut(entity)
}

Expand Down Expand Up @@ -682,7 +657,7 @@ impl BuildChildren for EntityWorldMut<'_> {
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 @@ -706,15 +681,15 @@ mod tests {
use super::{BuildChildren, ChildBuild};
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 @@ -728,6 +703,9 @@ mod tests {
assert_eq!(world.get::<Children>(parent).map(|c| &**c), children);
}

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

/// Assert the number of children in the parent's [`Children`] component if it exists.
fn assert_num_children(world: &World, parent: Entity, num_children: usize) {
assert_eq!(
Expand All @@ -738,49 +716,48 @@ mod tests {

/// 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.add_observer(
|trigger: Trigger<OnParentChange>, mut triggered_events: ResMut<TriggeredEvents>| {
triggered_events
.0
.push((trigger.event().clone(), trigger.entity()));
},
);

let [a, b, c, d] = core::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 @@ -789,21 +766,24 @@ mod tests {
#[test]
fn set_parent() {
let world = &mut World::new();
world.insert_resource(Events::<HierarchyEvent>::default());

world.init_resource::<TriggeredEvents>();

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

let [a, b, c] = core::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 @@ -812,11 +792,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 @@ -840,7 +822,16 @@ mod tests {
#[test]
fn remove_parent() {
let world = &mut World::new();
world.insert_resource(Events::<HierarchyEvent>::default());

world.init_resource::<TriggeredEvents>();

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

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

Expand All @@ -851,24 +842,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 Expand Up @@ -1289,7 +1268,6 @@ mod tests {
#[test]
fn with_child() {
let world = &mut World::new();
world.insert_resource(Events::<HierarchyEvent>::default());

let a = world.spawn_empty().id();
let b = ();
Expand Down
Loading
Loading