Remove ChildOf::get and Deref impl (#18080)

# Objective

There are currently three ways to access the parent stored on a ChildOf
relationship:

1. `child_of.parent` (field accessor)
2. `child_of.get()` (get function)
3. `**child_of` (Deref impl)

I will assert that we should only have one (the field accessor), and
that the existence of the other implementations causes confusion and
legibility issues. The deref approach is heinous, and `child_of.get()`
is significantly less clear than `child_of.parent`.

## Solution

Remove `impl Deref for ChildOf` and `ChildOf::get`.

The one "downside" I'm seeing is that:

```rust
entity.get::<ChildOf>().map(ChildOf::get)
```
Becomes this:

```rust
entity.get::<ChildOf>().map(|c| c.parent)
```

I strongly believe that this is worth the increased clarity and
consistency. I'm also not really a huge fan of the "pass function
pointer to map" syntax. I think most people don't think this way about
maps. They think in terms of a function that takes the item in the
Option and returns the result of some action on it.

## Migration Guide

```rust
// Before
**child_of
// After
child_of.parent

// Before
child_of.get()
// After
child_of.parent

// Before
entity.get::<ChildOf>().map(ChildOf::get)
// After
entity.get::<ChildOf>().map(|c| c.parent)
```
This commit is contained in:
Carter Anderson 2025-02-27 15:11:03 -08:00 committed by GitHub
parent 0122b85cc3
commit b73811d40e
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
11 changed files with 44 additions and 60 deletions

View File

@ -99,22 +99,6 @@ pub struct ChildOf {
pub parent: Entity, pub parent: Entity,
} }
impl ChildOf {
/// Returns the parent entity, which is the "target" of this relationship.
pub fn get(&self) -> Entity {
self.parent
}
}
impl Deref for ChildOf {
type Target = Entity;
#[inline]
fn deref(&self) -> &Self::Target {
&self.parent
}
}
// TODO: We need to impl either FromWorld or Default so ChildOf can be registered as Reflect. // TODO: We need to impl either FromWorld or Default so ChildOf can be registered as Reflect.
// This is because Reflect deserialize by creating an instance and apply a patch on top. // This is because Reflect deserialize by creating an instance and apply a patch on top.
// However ChildOf should only ever be set with a real user-defined entity. Its worth looking into // However ChildOf should only ever be set with a real user-defined entity. Its worth looking into
@ -280,7 +264,7 @@ pub fn validate_parent_has_component<C: Component>(
return; return;
}; };
if !world if !world
.get_entity(child_of.get()) .get_entity(child_of.parent)
.is_ok_and(|e| e.contains::<C>()) .is_ok_and(|e| e.contains::<C>())
{ {
// TODO: print name here once Name lives in bevy_ecs // TODO: print name here once Name lives in bevy_ecs

View File

@ -151,17 +151,17 @@ impl<E: Event + Clone> Event for FocusedInput<E> {
#[derive(QueryData)] #[derive(QueryData)]
/// These are for accessing components defined on the targeted entity /// These are for accessing components defined on the targeted entity
pub struct WindowTraversal { pub struct WindowTraversal {
parent: Option<&'static ChildOf>, child_of: Option<&'static ChildOf>,
window: Option<&'static Window>, window: Option<&'static Window>,
} }
impl<E: Event + Clone> Traversal<FocusedInput<E>> for WindowTraversal { impl<E: Event + Clone> Traversal<FocusedInput<E>> for WindowTraversal {
fn traverse(item: Self::Item<'_>, event: &FocusedInput<E>) -> Option<Entity> { fn traverse(item: Self::Item<'_>, event: &FocusedInput<E>) -> Option<Entity> {
let WindowTraversalItem { parent, window } = item; let WindowTraversalItem { child_of, window } = item;
// Send event to parent, if it has one. // Send event to parent, if it has one.
if let Some(parent) = parent { if let Some(child_of) = child_of {
return Some(parent.get()); return Some(child_of.parent);
}; };
// Otherwise, send it to the window entity (unless this is a window entity). // Otherwise, send it to the window entity (unless this is a window entity).
@ -334,7 +334,7 @@ impl IsFocused for World {
if e == entity { if e == entity {
return true; return true;
} }
if let Some(parent) = self.entity(e).get::<ChildOf>().map(ChildOf::get) { if let Some(parent) = self.entity(e).get::<ChildOf>().map(|c| c.parent) {
e = parent; e = parent;
} else { } else {
return false; return false;

View File

@ -79,7 +79,7 @@ pub struct Pointer<E: Debug + Clone + Reflect> {
/// propagates to the pointer's window and stops there. /// propagates to the pointer's window and stops there.
#[derive(QueryData)] #[derive(QueryData)]
pub struct PointerTraversal { pub struct PointerTraversal {
parent: Option<&'static ChildOf>, child_of: Option<&'static ChildOf>,
window: Option<&'static Window>, window: Option<&'static Window>,
} }
@ -88,11 +88,11 @@ where
E: Debug + Clone + Reflect, E: Debug + Clone + Reflect,
{ {
fn traverse(item: Self::Item<'_>, pointer: &Pointer<E>) -> Option<Entity> { fn traverse(item: Self::Item<'_>, pointer: &Pointer<E>) -> Option<Entity> {
let PointerTraversalItem { parent, window } = item; let PointerTraversalItem { child_of, window } = item;
// Send event to parent, if it has one. // Send event to parent, if it has one.
if let Some(parent) = parent { if let Some(child_of) = child_of {
return Some(parent.get()); return Some(child_of.parent);
}; };
// Otherwise, send it to the window entity (unless this is a window entity). // Otherwise, send it to the window entity (unless this is a window entity).

View File

@ -405,13 +405,13 @@ fn visibility_propagate_system(
mut visibility_query: Query<(&Visibility, &mut InheritedVisibility)>, mut visibility_query: Query<(&Visibility, &mut InheritedVisibility)>,
children_query: Query<&Children, (With<Visibility>, With<InheritedVisibility>)>, children_query: Query<&Children, (With<Visibility>, With<InheritedVisibility>)>,
) { ) {
for (entity, visibility, parent, children) in &changed { for (entity, visibility, child_of, children) in &changed {
let is_visible = match visibility { let is_visible = match visibility {
Visibility::Visible => true, Visibility::Visible => true,
Visibility::Hidden => false, Visibility::Hidden => false,
// fall back to true if no parent is found or parent lacks components // fall back to true if no parent is found or parent lacks components
Visibility::Inherited => parent Visibility::Inherited => child_of
.and_then(|p| visibility_query.get(p.get()).ok()) .and_then(|c| visibility_query.get(c.parent).ok())
.is_none_or(|(_, x)| x.get()), .is_none_or(|(_, x)| x.get()),
}; };
let (_, mut inherited_visibility) = visibility_query let (_, mut inherited_visibility) = visibility_query

View File

@ -316,7 +316,7 @@ mod tests {
.unwrap() .unwrap()
.get::<ChildOf>() .get::<ChildOf>()
.unwrap() .unwrap()
.get(), .parent,
"something about reloading the scene is touching entities with the same scene Ids" "something about reloading the scene is touching entities with the same scene Ids"
); );
assert_eq!( assert_eq!(
@ -326,7 +326,7 @@ mod tests {
.unwrap() .unwrap()
.get::<ChildOf>() .get::<ChildOf>()
.unwrap() .unwrap()
.get(), .parent,
"something about reloading the scene is touching components not defined in the scene but on entities defined in the scene" "something about reloading the scene is touching components not defined in the scene but on entities defined in the scene"
); );
assert_eq!( assert_eq!(
@ -336,7 +336,7 @@ mod tests {
.unwrap() .unwrap()
.get::<ChildOf>() .get::<ChildOf>()
.expect("something is wrong with this test, and the scene components don't have a parent/child relationship") .expect("something is wrong with this test, and the scene components don't have a parent/child relationship")
.get(), .parent,
"something is wrong with this test or the code reloading scenes since the relationship between scene entities is broken" "something is wrong with this test or the code reloading scenes since the relationship between scene entities is broken"
); );
} }

View File

@ -508,14 +508,14 @@ pub fn detect_text_needs_rerender<Root: Component>(
// - Span component changed. // - Span component changed.
// - Span TextFont changed. // - Span TextFont changed.
// - Span children changed (can include additions and removals). // - Span children changed (can include additions and removals).
for (entity, maybe_span_parent, has_text_block) in changed_spans.iter() { for (entity, maybe_span_child_of, has_text_block) in changed_spans.iter() {
if has_text_block { if has_text_block {
once!(warn!("found entity {} with a TextSpan that has a TextLayout, which should only be on root \ once!(warn!("found entity {} with a TextSpan that has a TextLayout, which should only be on root \
text entities (that have {}); this warning only prints once", text entities (that have {}); this warning only prints once",
entity, core::any::type_name::<Root>())); entity, core::any::type_name::<Root>()));
} }
let Some(span_parent) = maybe_span_parent else { let Some(span_child_of) = maybe_span_child_of else {
once!(warn!( once!(warn!(
"found entity {} with a TextSpan that has no parent; it should have an ancestor \ "found entity {} with a TextSpan that has no parent; it should have an ancestor \
with a root text component ({}); this warning only prints once", with a root text component ({}); this warning only prints once",
@ -524,13 +524,13 @@ pub fn detect_text_needs_rerender<Root: Component>(
)); ));
continue; continue;
}; };
let mut parent: Entity = span_parent.get(); let mut parent: Entity = span_child_of.parent;
// Search for the nearest ancestor with ComputedTextBlock. // Search for the nearest ancestor with ComputedTextBlock.
// Note: We assume the perf cost from duplicate visits in the case that multiple spans in a block are visited // Note: We assume the perf cost from duplicate visits in the case that multiple spans in a block are visited
// is outweighed by the expense of tracking visited spans. // is outweighed by the expense of tracking visited spans.
loop { loop {
let Ok((maybe_parent, maybe_computed, has_span)) = computed.get_mut(parent) else { let Ok((maybe_child_of, maybe_computed, has_span)) = computed.get_mut(parent) else {
once!(warn!("found entity {} with a TextSpan that is part of a broken hierarchy with a ChildOf \ once!(warn!("found entity {} with a TextSpan that is part of a broken hierarchy with a ChildOf \
component that points at non-existent entity {}; this warning only prints once", component that points at non-existent entity {}; this warning only prints once",
entity, parent)); entity, parent));
@ -546,7 +546,7 @@ pub fn detect_text_needs_rerender<Root: Component>(
entity, parent)); entity, parent));
break; break;
} }
let Some(next_parent) = maybe_parent else { let Some(next_child_of) = maybe_child_of else {
once!(warn!( once!(warn!(
"found entity {} with a TextSpan that has no ancestor with the root text \ "found entity {} with a TextSpan that has no ancestor with the root text \
component ({}); this warning only prints once", component ({}); this warning only prints once",
@ -555,7 +555,7 @@ pub fn detect_text_needs_rerender<Root: Component>(
)); ));
break; break;
}; };
parent = next_parent.get(); parent = next_child_of.parent;
} }
} }
} }

View File

@ -52,8 +52,8 @@ pub fn compute_transform_leaves(
) { ) {
leaves leaves
.par_iter_mut() .par_iter_mut()
.for_each(|(transform, mut global_transform, parent)| { .for_each(|(transform, mut global_transform, child_of)| {
let Ok(parent_transform) = parents.get(parent.get()) else { let Ok(parent_transform) = parents.get(child_of.parent) else {
return; return;
}; };
if parent_transform.is_changed() if parent_transform.is_changed()
@ -102,7 +102,7 @@ mod serial {
(Ref<Transform>, &mut GlobalTransform, Option<&Children>), (Ref<Transform>, &mut GlobalTransform, Option<&Children>),
(With<ChildOf>, With<Children>), (With<ChildOf>, With<Children>),
>, >,
parent_query: Query<(Entity, Ref<ChildOf>), With<GlobalTransform>>, child_query: Query<(Entity, Ref<ChildOf>), With<GlobalTransform>>,
mut orphaned_entities: Local<Vec<Entity>>, mut orphaned_entities: Local<Vec<Entity>>,
) { ) {
orphaned_entities.clear(); orphaned_entities.clear();
@ -115,9 +115,9 @@ mod serial {
*global_transform = GlobalTransform::from(*transform); *global_transform = GlobalTransform::from(*transform);
} }
for (child, actual_parent) in parent_query.iter_many(children) { for (child, child_of) in child_query.iter_many(children) {
assert_eq!( assert_eq!(
actual_parent.get(), entity, child_of.parent, entity,
"Malformed hierarchy. This probably means that your hierarchy has been improperly maintained, or contains a cycle" "Malformed hierarchy. This probably means that your hierarchy has been improperly maintained, or contains a cycle"
); );
// SAFETY: // SAFETY:
@ -137,9 +137,9 @@ mod serial {
propagate_recursive( propagate_recursive(
&global_transform, &global_transform,
&transform_query, &transform_query,
&parent_query, &child_query,
child, child,
changed || actual_parent.is_changed(), changed || child_of.is_changed(),
); );
} }
} }
@ -170,7 +170,7 @@ mod serial {
(Ref<Transform>, &mut GlobalTransform, Option<&Children>), (Ref<Transform>, &mut GlobalTransform, Option<&Children>),
(With<ChildOf>, With<Children>), (With<ChildOf>, With<Children>),
>, >,
parent_query: &Query<(Entity, Ref<ChildOf>), With<GlobalTransform>>, child_query: &Query<(Entity, Ref<ChildOf>), With<GlobalTransform>>,
entity: Entity, entity: Entity,
mut changed: bool, mut changed: bool,
) { ) {
@ -215,9 +215,9 @@ mod serial {
}; };
let Some(children) = children else { return }; let Some(children) = children else { return };
for (child, actual_parent) in parent_query.iter_many(children) { for (child, child_of) in child_query.iter_many(children) {
assert_eq!( assert_eq!(
actual_parent.get(), entity, child_of.parent, entity,
"Malformed hierarchy. This probably means that your hierarchy has been improperly maintained, or contains a cycle" "Malformed hierarchy. This probably means that your hierarchy has been improperly maintained, or contains a cycle"
); );
// SAFETY: The caller guarantees that `transform_query` will not be fetched for any // SAFETY: The caller guarantees that `transform_query` will not be fetched for any
@ -229,9 +229,9 @@ mod serial {
propagate_recursive( propagate_recursive(
global_matrix.as_ref(), global_matrix.as_ref(),
transform_query, transform_query,
parent_query, child_query,
child, child,
changed || actual_parent.is_changed(), changed || child_of.is_changed(),
); );
} }
} }
@ -463,7 +463,7 @@ mod parallel {
let mut last_child = None; let mut last_child = None;
let new_children = children_iter.map( let new_children = children_iter.map(
|(child, (transform, mut global_transform), (children, child_of))| { |(child, (transform, mut global_transform), (children, child_of))| {
assert_eq!(child_of.get(), parent); assert_eq!(child_of.parent, parent);
if p_global_transform.is_changed() if p_global_transform.is_changed()
|| transform.is_changed() || transform.is_changed()
|| global_transform.is_added() || global_transform.is_added()

View File

@ -127,12 +127,12 @@ pub fn ui_layout_system(
computed_node_query computed_node_query
.iter() .iter()
.for_each(|(entity, maybe_parent)| { .for_each(|(entity, maybe_child_of)| {
if let Some(parent) = maybe_parent { if let Some(child_of) = maybe_child_of {
// Note: This does not cover the case where a parent's Node component was removed. // Note: This does not cover the case where a parent's Node component was removed.
// Users are responsible for fixing hierarchies if they do that (it is not recommended). // Users are responsible for fixing hierarchies if they do that (it is not recommended).
// Detecting it here would be a permanent perf burden on the hot path. // Detecting it here would be a permanent perf burden on the hot path.
if parent.is_changed() && !ui_children.is_ui_node(parent.get()) { if child_of.is_changed() && !ui_children.is_ui_node(child_of.parent) {
warn!( warn!(
"Node ({entity}) is in a non-UI entity hierarchy. You are using an entity \ "Node ({entity}) is in a non-UI entity hierarchy. You are using an entity \
with UI components as a child of an entity without UI components, your UI layout may be broken." with UI components as a child of an entity without UI components, your UI layout may be broken."

View File

@ -227,9 +227,9 @@ fn update_adapter(
) -> TreeUpdate { ) -> TreeUpdate {
let mut to_update = vec![]; let mut to_update = vec![];
let mut window_children = vec![]; let mut window_children = vec![];
for (entity, node, children, parent) in &nodes { for (entity, node, children, child_of) in &nodes {
let mut node = (**node).clone(); let mut node = (**node).clone();
queue_node_for_update(entity, parent, &node_entities, &mut window_children); queue_node_for_update(entity, child_of, &node_entities, &mut window_children);
add_children_nodes(children, &node_entities, &mut node); add_children_nodes(children, &node_entities, &mut node);
let node_id = NodeId(entity.to_bits()); let node_id = NodeId(entity.to_bits());
to_update.push((node_id, node)); to_update.push((node_id, node));
@ -258,7 +258,7 @@ fn queue_node_for_update(
window_children: &mut Vec<NodeId>, window_children: &mut Vec<NodeId>,
) { ) {
let should_push = if let Some(child_of) = child_of { let should_push = if let Some(child_of) = child_of {
!node_entities.contains(child_of.get()) !node_entities.contains(child_of.parent)
} else { } else {
true true
}; };

View File

@ -109,7 +109,7 @@ fn assign_clips(
} }
// Go to the next parent. // Go to the next parent.
current = children.get(entity).ok().map(ChildOf::get); current = children.get(entity).ok().map(|c| c.parent);
} }
} }

View File

@ -108,9 +108,9 @@ fn button_system(
mut counter_query: Query<&mut Counter>, mut counter_query: Query<&mut Counter>,
) { ) {
// Update parent counter on click // Update parent counter on click
for (interaction, parent) in &mut interaction_query { for (interaction, child_of) in &mut interaction_query {
if matches!(interaction, Interaction::Pressed) { if matches!(interaction, Interaction::Pressed) {
let mut counter = counter_query.get_mut(parent.get()).unwrap(); let mut counter = counter_query.get_mut(child_of.parent).unwrap();
counter.0 += 1; counter.0 += 1;
} }
} }