Skip to content

Commit d8fa57b

Browse files
authored
Switch ChildOf back to tuple struct (#18672)
# Objective In #17905 we swapped to a named field on `ChildOf` to help resolve variable naming ambiguity of child vs parent (ex: `child_of.parent` clearly reads as "I am accessing the parent of the child_of relationship", whereas `child_of.0` is less clear). Unfortunately this has the side effect of making initialization less ideal. `ChildOf { parent }` reads just as well as `ChildOf(parent)`, but `ChildOf { parent: root }` doesn't read nearly as well as `ChildOf(root)`. ## Solution Move back to `ChildOf(pub Entity)` but add a `child_of.parent()` function and use it for all accesses. The downside here is that users are no longer "forced" to access the parent field with `parent` nomenclature, but I think this strikes the right balance. Take a look at the diff. I think the results provide strong evidence for this change. Initialization has the benefit of reading much better _and_ of taking up significantly less space, as many lines go from 3 to 1, and we're cutting out a bunch of syntax in some cases. Sadly I do think this should land in 0.16 as the cost of doing this _after_ the relationships migration is high.
1 parent 34f1159 commit d8fa57b

File tree

22 files changed

+90
-118
lines changed

22 files changed

+90
-118
lines changed

benches/benches/bevy_ecs/entity_cloning.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,7 @@ fn bench_clone_hierarchy<B: Bundle + Default + GetTypeRegistration>(
155155

156156
for parent in current_hierarchy_level {
157157
for _ in 0..children {
158-
let child_id = world.spawn((B::default(), ChildOf { parent })).id();
158+
let child_id = world.spawn((B::default(), ChildOf(parent))).id();
159159
hierarchy_level.push(child_id);
160160
}
161161
}

crates/bevy_ecs/src/entity/clone_entities.rs

+3-3
Original file line numberDiff line numberDiff line change
@@ -1337,9 +1337,9 @@ mod tests {
13371337
fn recursive_clone() {
13381338
let mut world = World::new();
13391339
let root = world.spawn_empty().id();
1340-
let child1 = world.spawn(ChildOf { parent: root }).id();
1341-
let grandchild = world.spawn(ChildOf { parent: child1 }).id();
1342-
let child2 = world.spawn(ChildOf { parent: root }).id();
1340+
let child1 = world.spawn(ChildOf(root)).id();
1341+
let grandchild = world.spawn(ChildOf(child1)).id();
1342+
let child2 = world.spawn(ChildOf(root)).id();
13431343

13441344
let clone_root = world.spawn_empty().id();
13451345
EntityCloner::build(&mut world)

crates/bevy_ecs/src/hierarchy.rs

+50-47
Original file line numberDiff line numberDiff line change
@@ -54,9 +54,9 @@ use log::warn;
5454
/// # use bevy_ecs::prelude::*;
5555
/// # let mut world = World::new();
5656
/// let root = world.spawn_empty().id();
57-
/// let child1 = world.spawn(ChildOf { parent: root }).id();
58-
/// let child2 = world.spawn(ChildOf { parent: root }).id();
59-
/// let grandchild = world.spawn(ChildOf { parent: child1 }).id();
57+
/// let child1 = world.spawn(ChildOf(root)).id();
58+
/// let child2 = world.spawn(ChildOf(root)).id();
59+
/// let grandchild = world.spawn(ChildOf(child1)).id();
6060
///
6161
/// assert_eq!(&**world.entity(root).get::<Children>().unwrap(), &[child1, child2]);
6262
/// assert_eq!(&**world.entity(child1).get::<Children>().unwrap(), &[grandchild]);
@@ -96,9 +96,21 @@ use log::warn;
9696
)]
9797
#[relationship(relationship_target = Children)]
9898
#[doc(alias = "IsChild", alias = "Parent")]
99-
pub struct ChildOf {
99+
pub struct ChildOf(pub Entity);
100+
101+
impl ChildOf {
102+
/// The parent entity of this child entity.
103+
#[inline]
104+
pub fn parent(&self) -> Entity {
105+
self.0
106+
}
107+
100108
/// The parent entity of this child entity.
101-
pub parent: Entity,
109+
#[deprecated(since = "0.16.0", note = "Use child_of.parent() instead")]
110+
#[inline]
111+
pub fn get(&self) -> Entity {
112+
self.0
113+
}
102114
}
103115

104116
// TODO: We need to impl either FromWorld or Default so ChildOf can be registered as Reflect.
@@ -108,9 +120,7 @@ pub struct ChildOf {
108120
impl FromWorld for ChildOf {
109121
#[inline(always)]
110122
fn from_world(_world: &mut World) -> Self {
111-
ChildOf {
112-
parent: Entity::PLACEHOLDER,
113-
}
123+
ChildOf(Entity::PLACEHOLDER)
114124
}
115125
}
116126

@@ -316,7 +326,7 @@ impl<'w> EntityWorldMut<'w> {
316326
pub fn with_child(&mut self, bundle: impl Bundle) -> &mut Self {
317327
let parent = self.id();
318328
self.world_scope(|world| {
319-
world.spawn((bundle, ChildOf { parent }));
329+
world.spawn((bundle, ChildOf(parent)));
320330
});
321331
self
322332
}
@@ -329,12 +339,9 @@ impl<'w> EntityWorldMut<'w> {
329339
}
330340

331341
/// Inserts the [`ChildOf`] component with the given `parent` entity, if it exists.
332-
#[deprecated(
333-
since = "0.16.0",
334-
note = "Use entity_mut.insert(ChildOf { parent: entity })"
335-
)]
342+
#[deprecated(since = "0.16.0", note = "Use entity_mut.insert(ChildOf(entity))")]
336343
pub fn set_parent(&mut self, parent: Entity) -> &mut Self {
337-
self.insert(ChildOf { parent });
344+
self.insert(ChildOf(parent));
338345
self
339346
}
340347
}
@@ -394,7 +401,7 @@ impl<'a> EntityCommands<'a> {
394401
/// [`with_children`]: EntityCommands::with_children
395402
pub fn with_child(&mut self, bundle: impl Bundle) -> &mut Self {
396403
let parent = self.id();
397-
self.commands.spawn((bundle, ChildOf { parent }));
404+
self.commands.spawn((bundle, ChildOf(parent)));
398405
self
399406
}
400407

@@ -406,12 +413,9 @@ impl<'a> EntityCommands<'a> {
406413
}
407414

408415
/// Inserts the [`ChildOf`] component with the given `parent` entity, if it exists.
409-
#[deprecated(
410-
since = "0.16.0",
411-
note = "Use entity_commands.insert(ChildOf { parent: entity })"
412-
)]
416+
#[deprecated(since = "0.16.0", note = "Use entity_commands.insert(ChildOf(entity))")]
413417
pub fn set_parent(&mut self, parent: Entity) -> &mut Self {
414-
self.insert(ChildOf { parent });
418+
self.insert(ChildOf(parent));
415419
self
416420
}
417421
}
@@ -427,7 +431,7 @@ pub fn validate_parent_has_component<C: Component>(
427431
return;
428432
};
429433
if !world
430-
.get_entity(child_of.parent)
434+
.get_entity(child_of.parent())
431435
.is_ok_and(|e| e.contains::<C>())
432436
{
433437
// TODO: print name here once Name lives in bevy_ecs
@@ -527,9 +531,9 @@ mod tests {
527531
fn hierarchy() {
528532
let mut world = World::new();
529533
let root = world.spawn_empty().id();
530-
let child1 = world.spawn(ChildOf { parent: root }).id();
531-
let grandchild = world.spawn(ChildOf { parent: child1 }).id();
532-
let child2 = world.spawn(ChildOf { parent: root }).id();
534+
let child1 = world.spawn(ChildOf(root)).id();
535+
let grandchild = world.spawn(ChildOf(child1)).id();
536+
let child2 = world.spawn(ChildOf(root)).id();
533537

534538
// Spawn
535539
let hierarchy = get_hierarchy(&world, root);
@@ -550,7 +554,7 @@ mod tests {
550554
assert_eq!(hierarchy, Node::new_with(root, vec![Node::new(child2)]));
551555

552556
// Insert
553-
world.entity_mut(child1).insert(ChildOf { parent: root });
557+
world.entity_mut(child1).insert(ChildOf(root));
554558
let hierarchy = get_hierarchy(&world, root);
555559
assert_eq!(
556560
hierarchy,
@@ -638,7 +642,7 @@ mod tests {
638642
fn self_parenting_invalid() {
639643
let mut world = World::new();
640644
let id = world.spawn_empty().id();
641-
world.entity_mut(id).insert(ChildOf { parent: id });
645+
world.entity_mut(id).insert(ChildOf(id));
642646
assert!(
643647
world.entity(id).get::<ChildOf>().is_none(),
644648
"invalid ChildOf relationships should self-remove"
@@ -650,7 +654,7 @@ mod tests {
650654
let mut world = World::new();
651655
let parent = world.spawn_empty().id();
652656
world.entity_mut(parent).despawn();
653-
let id = world.spawn(ChildOf { parent }).id();
657+
let id = world.spawn(ChildOf(parent)).id();
654658
assert!(
655659
world.entity(id).get::<ChildOf>().is_none(),
656660
"invalid ChildOf relationships should self-remove"
@@ -661,10 +665,10 @@ mod tests {
661665
fn reinsert_same_parent() {
662666
let mut world = World::new();
663667
let parent = world.spawn_empty().id();
664-
let id = world.spawn(ChildOf { parent }).id();
665-
world.entity_mut(id).insert(ChildOf { parent });
668+
let id = world.spawn(ChildOf(parent)).id();
669+
world.entity_mut(id).insert(ChildOf(parent));
666670
assert_eq!(
667-
Some(&ChildOf { parent }),
671+
Some(&ChildOf(parent)),
668672
world.entity(id).get::<ChildOf>(),
669673
"ChildOf should still be there"
670674
);
@@ -699,11 +703,11 @@ mod tests {
699703

700704
assert_eq!(
701705
world.entity(child_a).get::<ChildOf>().unwrap(),
702-
&ChildOf { parent }
706+
&ChildOf(parent)
703707
);
704708
assert_eq!(
705709
world.entity(child_c).get::<ChildOf>().unwrap(),
706-
&ChildOf { parent }
710+
&ChildOf(parent)
707711
);
708712
assert!(world.entity(child_b).get::<ChildOf>().is_none());
709713
}
@@ -739,7 +743,7 @@ mod tests {
739743
assert_eq!(children.0, [child]);
740744
assert_eq!(
741745
world.entity(child).get::<ChildOf>().unwrap(),
742-
&ChildOf { parent }
746+
&ChildOf(parent)
743747
);
744748
}
745749

@@ -762,11 +766,11 @@ mod tests {
762766

763767
assert_eq!(
764768
world.entity(child_a).get::<ChildOf>().unwrap(),
765-
&ChildOf { parent }
769+
&ChildOf(parent)
766770
);
767771
assert_eq!(
768772
world.entity(child_b).get::<ChildOf>().unwrap(),
769-
&ChildOf { parent }
773+
&ChildOf(parent)
770774
);
771775
assert_eq!(
772776
world.entity(parent).get::<Children>().unwrap().0,
@@ -781,15 +785,15 @@ mod tests {
781785
);
782786
assert_eq!(
783787
world.entity(child_a).get::<ChildOf>().unwrap(),
784-
&ChildOf { parent }
788+
&ChildOf(parent)
785789
);
786790
assert_eq!(
787791
world.entity(child_c).get::<ChildOf>().unwrap(),
788-
&ChildOf { parent }
792+
&ChildOf(parent)
789793
);
790794
assert_eq!(
791795
world.entity(child_d).get::<ChildOf>().unwrap(),
792-
&ChildOf { parent }
796+
&ChildOf(parent)
793797
);
794798
assert_eq!(
795799
world.entity(parent).get::<Children>().unwrap().0,
@@ -844,11 +848,11 @@ mod tests {
844848

845849
assert_eq!(
846850
world.entity(child_a).get::<ChildOf>().unwrap(),
847-
&ChildOf { parent }
851+
&ChildOf(parent)
848852
);
849853
assert_eq!(
850854
world.entity(child_b).get::<ChildOf>().unwrap(),
851-
&ChildOf { parent }
855+
&ChildOf(parent)
852856
);
853857
assert_eq!(
854858
world.entity(parent).get::<Children>().unwrap().0,
@@ -863,11 +867,11 @@ mod tests {
863867
);
864868
assert_eq!(
865869
world.entity(child_c).get::<ChildOf>().unwrap(),
866-
&ChildOf { parent }
870+
&ChildOf(parent)
867871
);
868872
assert_eq!(
869873
world.entity(child_d).get::<ChildOf>().unwrap(),
870-
&ChildOf { parent }
874+
&ChildOf(parent)
871875
);
872876
assert_eq!(
873877
world.entity(parent).get::<Children>().unwrap().0,
@@ -974,11 +978,10 @@ mod tests {
974978
let mut world = World::new();
975979
let parent = world.spawn_empty().id();
976980
let other = world.spawn_empty().id();
977-
let child = world.spawn(ChildOf { parent }).id();
978-
world.entity_mut(child).insert_with_relationship_hook_mode(
979-
ChildOf { parent: other },
980-
RelationshipHookMode::Skip,
981-
);
981+
let child = world.spawn(ChildOf(parent)).id();
982+
world
983+
.entity_mut(child)
984+
.insert_with_relationship_hook_mode(ChildOf(other), RelationshipHookMode::Skip);
982985
assert_eq!(
983986
&**world.entity(parent).get::<Children>().unwrap(),
984987
&[child],

crates/bevy_ecs/src/relationship/related_methods.rs

+3-3
Original file line numberDiff line numberDiff line change
@@ -521,9 +521,9 @@ mod tests {
521521
let mut world = World::new();
522522

523523
let a = world.spawn_empty().id();
524-
let b = world.spawn(ChildOf { parent: a }).id();
525-
let c = world.spawn(ChildOf { parent: a }).id();
526-
let d = world.spawn(ChildOf { parent: b }).id();
524+
let b = world.spawn(ChildOf(a)).id();
525+
let c = world.spawn(ChildOf(a)).id();
526+
let d = world.spawn(ChildOf(b)).id();
527527

528528
world
529529
.entity_mut(a)

crates/bevy_input_focus/src/lib.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ impl<E: Event + Clone> Traversal<FocusedInput<E>> for WindowTraversal {
165165

166166
// Send event to parent, if it has one.
167167
if let Some(child_of) = child_of {
168-
return Some(child_of.parent);
168+
return Some(child_of.parent());
169169
};
170170

171171
// Otherwise, send it to the window entity (unless this is a window entity).
@@ -338,7 +338,7 @@ impl IsFocused for World {
338338
if e == entity {
339339
return true;
340340
}
341-
if let Some(parent) = self.entity(e).get::<ChildOf>().map(|c| c.parent) {
341+
if let Some(parent) = self.entity(e).get::<ChildOf>().map(ChildOf::parent) {
342342
e = parent;
343343
} else {
344344
return false;

crates/bevy_input_focus/src/tab_navigation.rs

+2-16
Original file line numberDiff line numberDiff line change
@@ -375,22 +375,8 @@ mod tests {
375375
let world = app.world_mut();
376376

377377
let tab_group_entity = world.spawn(TabGroup::new(0)).id();
378-
let tab_entity_1 = world
379-
.spawn((
380-
TabIndex(0),
381-
ChildOf {
382-
parent: tab_group_entity,
383-
},
384-
))
385-
.id();
386-
let tab_entity_2 = world
387-
.spawn((
388-
TabIndex(1),
389-
ChildOf {
390-
parent: tab_group_entity,
391-
},
392-
))
393-
.id();
378+
let tab_entity_1 = world.spawn((TabIndex(0), ChildOf(tab_group_entity))).id();
379+
let tab_entity_2 = world.spawn((TabIndex(1), ChildOf(tab_group_entity))).id();
394380

395381
let mut system_state: SystemState<TabNavigation> = SystemState::new(world);
396382
let tab_navigation = system_state.get(world);

crates/bevy_picking/src/events.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ where
9292

9393
// Send event to parent, if it has one.
9494
if let Some(child_of) = child_of {
95-
return Some(child_of.parent);
95+
return Some(child_of.parent());
9696
};
9797

9898
// Otherwise, send it to the window entity (unless this is a window entity).

crates/bevy_render/src/view/visibility/mod.rs

+2-4
Original file line numberDiff line numberDiff line change
@@ -411,7 +411,7 @@ fn visibility_propagate_system(
411411
Visibility::Hidden => false,
412412
// fall back to true if no parent is found or parent lacks components
413413
Visibility::Inherited => child_of
414-
.and_then(|c| visibility_query.get(c.parent).ok())
414+
.and_then(|c| visibility_query.get(c.parent()).ok())
415415
.is_none_or(|(_, x)| x.get()),
416416
};
417417
let (_, mut inherited_visibility) = visibility_query
@@ -786,9 +786,7 @@ mod test {
786786
.entity_mut(parent2)
787787
.insert(Visibility::Visible);
788788
// Simulate a change in the parent component
789-
app.world_mut()
790-
.entity_mut(child2)
791-
.insert(ChildOf { parent: parent2 }); // example of changing parent
789+
app.world_mut().entity_mut(child2).insert(ChildOf(parent2)); // example of changing parent
792790

793791
// Run the system again to propagate changes
794792
app.update();

crates/bevy_scene/src/dynamic_scene.rs

+3-3
Original file line numberDiff line numberDiff line change
@@ -325,7 +325,7 @@ mod tests {
325325
.unwrap()
326326
.get::<ChildOf>()
327327
.unwrap()
328-
.parent,
328+
.parent(),
329329
"something about reloading the scene is touching entities with the same scene Ids"
330330
);
331331
assert_eq!(
@@ -335,7 +335,7 @@ mod tests {
335335
.unwrap()
336336
.get::<ChildOf>()
337337
.unwrap()
338-
.parent,
338+
.parent(),
339339
"something about reloading the scene is touching components not defined in the scene but on entities defined in the scene"
340340
);
341341
assert_eq!(
@@ -345,7 +345,7 @@ mod tests {
345345
.unwrap()
346346
.get::<ChildOf>()
347347
.expect("something is wrong with this test, and the scene components don't have a parent/child relationship")
348-
.parent,
348+
.parent(),
349349
"something is wrong with this test or the code reloading scenes since the relationship between scene entities is broken"
350350
);
351351
}

0 commit comments

Comments
 (0)