From be22569db7177addb3d9ac9d49d424d4df0032dc Mon Sep 17 00:00:00 2001 From: Cameron <51241057+maniwani@users.noreply.github.com> Date: Sun, 26 Feb 2023 00:09:19 +0000 Subject: [PATCH] `EntityMut`: rename `remove_intersection` to `remove` and `remove` to `take` (#7810) # Objective - A more intuitive distinction between the two. `remove_intersection` is verbose and unclear. - `EntityMut::remove` and `Commands::remove` should match. ## Solution - What the title says. --- ## Migration Guide Before ```rust fn clear_children(parent: Entity, world: &mut World) { if let Some(children) = world.entity_mut(parent).remove::() { for &child in &children.0 { world.entity_mut(child).remove_intersection::(); } } } ``` After ```rust fn clear_children(parent: Entity, world: &mut World) { if let Some(children) = world.entity_mut(parent).take::() { for &child in &children.0 { world.entity_mut(child).remove::(); } } } ``` --- crates/bevy_ecs/src/archetype.rs | 22 ++++++------- crates/bevy_ecs/src/lib.rs | 22 ++++++------- crates/bevy_ecs/src/reflect.rs | 4 --- crates/bevy_ecs/src/system/commands/mod.rs | 4 +-- crates/bevy_ecs/src/world/entity_ref.rs | 32 ++++++++++--------- .../ui/entity_ref_mut_lifetime_safety.rs | 4 +-- .../ui/entity_ref_mut_lifetime_safety.stderr | 8 ++--- crates/bevy_hierarchy/src/child_builder.rs | 4 +-- 8 files changed, 46 insertions(+), 54 deletions(-) diff --git a/crates/bevy_ecs/src/archetype.rs b/crates/bevy_ecs/src/archetype.rs index bd2ceb5dcc..aa7a4189f6 100644 --- a/crates/bevy_ecs/src/archetype.rs +++ b/crates/bevy_ecs/src/archetype.rs @@ -150,7 +150,7 @@ impl BundleComponentStatus for SpawnBundleStatus { pub struct Edges { add_bundle: SparseArray, remove_bundle: SparseArray>, - remove_bundle_intersection: SparseArray>, + take_bundle: SparseArray>, } impl Edges { @@ -222,32 +222,28 @@ impl Edges { } /// Checks the cache for the target archetype when removing a bundle to the - /// source archetype. For more information, see [`EntityMut::remove_intersection`]. + /// source archetype. For more information, see [`EntityMut::remove`]. /// /// If this returns `None`, it means there has not been a transition from /// the source archetype via the provided bundle. /// - /// [`EntityMut::remove_intersection`]: crate::world::EntityMut::remove_intersection + /// [`EntityMut::remove`]: crate::world::EntityMut::remove #[inline] - pub fn get_remove_bundle_intersection( - &self, - bundle_id: BundleId, - ) -> Option> { - self.remove_bundle_intersection.get(bundle_id).cloned() + pub fn get_take_bundle(&self, bundle_id: BundleId) -> Option> { + self.take_bundle.get(bundle_id).cloned() } /// Caches the target archetype when removing a bundle to the source archetype. - /// For more information, see [`EntityMut::remove_intersection`]. + /// For more information, see [`EntityMut::take`]. /// - /// [`EntityMut::remove_intersection`]: crate::world::EntityMut::remove_intersection + /// [`EntityMut::take`]: crate::world::EntityMut::take #[inline] - pub(crate) fn insert_remove_bundle_intersection( + pub(crate) fn insert_take_bundle( &mut self, bundle_id: BundleId, archetype_id: Option, ) { - self.remove_bundle_intersection - .insert(bundle_id, archetype_id); + self.take_bundle.insert(bundle_id, archetype_id); } } diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 9f00ffd804..d8d8e5948a 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -191,7 +191,7 @@ mod tests { assert_eq!(world.get::(e2).unwrap().0, 42); assert_eq!( - world.entity_mut(e1).remove::().unwrap(), + world.entity_mut(e1).take::().unwrap(), FooBundle { x: TableStored("xyz"), y: SparseStored(123), @@ -240,7 +240,7 @@ mod tests { assert_eq!(world.get::(e3).unwrap().0, 1); assert_eq!(world.get::(e3).unwrap().0, 2); assert_eq!( - world.entity_mut(e3).remove::().unwrap(), + world.entity_mut(e3).take::().unwrap(), NestedBundle { a: A(1), foo: FooBundle { @@ -283,7 +283,7 @@ mod tests { assert_eq!(world.get::(e4), None); assert_eq!( - world.entity_mut(e4).remove::().unwrap(), + world.entity_mut(e4).take::().unwrap(), BundleWithIgnored { c: C, ignored: Ignored, @@ -596,7 +596,7 @@ mod tests { &[(e1, A(1), B(3)), (e2, A(2), B(4))] ); - assert_eq!(world.entity_mut(e1).remove::(), Some(A(1))); + assert_eq!(world.entity_mut(e1).take::(), Some(A(1))); assert_eq!( world .query::<(Entity, &A, &B)>() @@ -656,7 +656,7 @@ mod tests { } for (i, entity) in entities.iter().cloned().enumerate() { - assert_eq!(world.entity_mut(entity).remove::(), Some(A(i))); + assert_eq!(world.entity_mut(entity).take::(), Some(A(i))); } } @@ -675,7 +675,7 @@ mod tests { for (i, entity) in entities.iter().cloned().enumerate() { assert_eq!( - world.entity_mut(entity).remove::(), + world.entity_mut(entity).take::(), Some(SparseStored(i as u32)) ); } @@ -685,7 +685,7 @@ mod tests { fn remove_missing() { let mut world = World::new(); let e = world.spawn((TableStored("abc"), A(123))).id(); - assert!(world.entity_mut(e).remove::().is_none()); + assert!(world.entity_mut(e).take::().is_none()); } #[test] @@ -1187,7 +1187,7 @@ mod tests { } #[test] - fn remove_intersection() { + fn remove() { let mut world = World::default(); let e1 = world.spawn((A(1), B(1), TableStored("a"))).id(); @@ -1201,7 +1201,7 @@ mod tests { "C is not in the entity, so it should not exist" ); - e.remove_intersection::<(A, B, C)>(); + e.remove::<(A, B, C)>(); assert_eq!( e.get::(), Some(&TableStored("a")), @@ -1225,7 +1225,7 @@ mod tests { } #[test] - fn remove() { + fn take() { let mut world = World::default(); world.spawn((A(1), B(1), TableStored("1"))); let e2 = world.spawn((A(2), B(2), TableStored("2"))).id(); @@ -1238,7 +1238,7 @@ mod tests { .collect::>(); assert_eq!(results, vec![(1, "1"), (2, "2"), (3, "3"),]); - let removed_bundle = world.entity_mut(e2).remove::<(B, TableStored)>().unwrap(); + let removed_bundle = world.entity_mut(e2).take::<(B, TableStored)>().unwrap(); assert_eq!(removed_bundle, (B(2), TableStored("2"))); let results = query diff --git a/crates/bevy_ecs/src/reflect.rs b/crates/bevy_ecs/src/reflect.rs index d5f2729d8c..bb4dbb1cf3 100644 --- a/crates/bevy_ecs/src/reflect.rs +++ b/crates/bevy_ecs/src/reflect.rs @@ -99,10 +99,6 @@ impl ReflectComponent { } /// Removes this [`Component`] type from the entity. Does nothing if it doesn't exist. - /// - /// # Panics - /// - /// Panics if there is no [`Component`] of the given type. pub fn remove(&self, entity: &mut EntityMut) { (self.0.remove)(entity); } diff --git a/crates/bevy_ecs/src/system/commands/mod.rs b/crates/bevy_ecs/src/system/commands/mod.rs index 3761cdb24a..73df904177 100644 --- a/crates/bevy_ecs/src/system/commands/mod.rs +++ b/crates/bevy_ecs/src/system/commands/mod.rs @@ -951,9 +951,7 @@ where { fn write(self, world: &mut World) { if let Some(mut entity_mut) = world.get_entity_mut(self.entity) { - // remove intersection to gracefully handle components that were removed before running - // this command - entity_mut.remove_intersection::(); + entity_mut.remove::(); } } } diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index 5a6f179cee..bb5eb7ddb3 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -279,11 +279,13 @@ impl<'w> EntityMut<'w> { self } - // TODO: move to BundleInfo - /// Removes a [`Bundle`] of components from the entity and returns the bundle. + /// Removes all components in the [`Bundle`] from the entity and returns their previous values. /// - /// Returns `None` if the entity does not contain the bundle. - pub fn remove(&mut self) -> Option { + /// **Note:** If the entity does not have every component in the bundle, this method will not + /// remove any of them. + // TODO: BundleRemover? + #[must_use] + pub fn take(&mut self) -> Option { let archetypes = &mut self.world.archetypes; let storages = &mut self.world.storages; let components = &mut self.world.components; @@ -408,9 +410,9 @@ impl<'w> EntityMut<'w> { entities.set(entity.index(), new_location); } - // TODO: move to BundleInfo - /// Remove any components in the bundle that the entity has. - pub fn remove_intersection(&mut self) { + /// Removes any components in the [`Bundle`] from the entity. + // TODO: BundleRemover? + pub fn remove(&mut self) -> &mut Self { let archetypes = &mut self.world.archetypes; let storages = &mut self.world.storages; let components = &mut self.world.components; @@ -435,7 +437,7 @@ impl<'w> EntityMut<'w> { }; if new_archetype_id == old_location.archetype_id { - return; + return self; } let old_archetype = &mut archetypes[old_location.archetype_id]; @@ -469,6 +471,8 @@ impl<'w> EntityMut<'w> { new_archetype_id, ); } + + self } pub fn despawn(self) { @@ -647,11 +651,9 @@ unsafe fn remove_bundle_from_archetype( let remove_bundle_result = { let current_archetype = &mut archetypes[archetype_id]; if intersection { - current_archetype - .edges() - .get_remove_bundle_intersection(bundle_info.id) - } else { current_archetype.edges().get_remove_bundle(bundle_info.id) + } else { + current_archetype.edges().get_take_bundle(bundle_info.id) } }; let result = if let Some(result) = remove_bundle_result { @@ -679,7 +681,7 @@ unsafe fn remove_bundle_from_archetype( // graph current_archetype .edges_mut() - .insert_remove_bundle(bundle_info.id, None); + .insert_take_bundle(bundle_info.id, None); return None; } } @@ -718,11 +720,11 @@ unsafe fn remove_bundle_from_archetype( if intersection { current_archetype .edges_mut() - .insert_remove_bundle_intersection(bundle_info.id, result); + .insert_remove_bundle(bundle_info.id, result); } else { current_archetype .edges_mut() - .insert_remove_bundle(bundle_info.id, result); + .insert_take_bundle(bundle_info.id, result); } result } diff --git a/crates/bevy_ecs_compile_fail_tests/tests/ui/entity_ref_mut_lifetime_safety.rs b/crates/bevy_ecs_compile_fail_tests/tests/ui/entity_ref_mut_lifetime_safety.rs index 2ab786b9f3..9d047da4f6 100644 --- a/crates/bevy_ecs_compile_fail_tests/tests/ui/entity_ref_mut_lifetime_safety.rs +++ b/crates/bevy_ecs_compile_fail_tests/tests/ui/entity_ref_mut_lifetime_safety.rs @@ -14,7 +14,7 @@ fn main() { { let gotten: &A = e_mut.get::().unwrap(); - let gotten2: A = e_mut.remove::().unwrap(); + let gotten2: A = e_mut.take::().unwrap(); assert_eq!(gotten, &gotten2); // oops UB } @@ -22,7 +22,7 @@ fn main() { { let mut gotten: Mut = e_mut.get_mut::().unwrap(); - let mut gotten2: A = e_mut.remove::().unwrap(); + let mut gotten2: A = e_mut.take::().unwrap(); assert_eq!(&mut *gotten, &mut gotten2); // oops UB } diff --git a/crates/bevy_ecs_compile_fail_tests/tests/ui/entity_ref_mut_lifetime_safety.stderr b/crates/bevy_ecs_compile_fail_tests/tests/ui/entity_ref_mut_lifetime_safety.stderr index e37857d1d3..c12e0d0292 100644 --- a/crates/bevy_ecs_compile_fail_tests/tests/ui/entity_ref_mut_lifetime_safety.stderr +++ b/crates/bevy_ecs_compile_fail_tests/tests/ui/entity_ref_mut_lifetime_safety.stderr @@ -3,8 +3,8 @@ error[E0502]: cannot borrow `e_mut` as mutable because it is also borrowed as im | 16 | let gotten: &A = e_mut.get::().unwrap(); | ---------------- immutable borrow occurs here -17 | let gotten2: A = e_mut.remove::().unwrap(); - | ^^^^^^^^^^^^^^^^^^^ mutable borrow occurs here +17 | let gotten2: A = e_mut.take::().unwrap(); + | ^^^^^^^^^^^^^^^^^ mutable borrow occurs here 18 | assert_eq!(gotten, &gotten2); // oops UB | ---------------------------- immutable borrow later used here @@ -13,8 +13,8 @@ error[E0499]: cannot borrow `e_mut` as mutable more than once at a time | 24 | let mut gotten: Mut = e_mut.get_mut::().unwrap(); | -------------------- first mutable borrow occurs here -25 | let mut gotten2: A = e_mut.remove::().unwrap(); - | ^^^^^^^^^^^^^^^^^^^ second mutable borrow occurs here +25 | let mut gotten2: A = e_mut.take::().unwrap(); + | ^^^^^^^^^^^^^^^^^ second mutable borrow occurs here 26 | assert_eq!(&mut *gotten, &mut gotten2); // oops UB | ------ first borrow later used here diff --git a/crates/bevy_hierarchy/src/child_builder.rs b/crates/bevy_hierarchy/src/child_builder.rs index 395667a6e7..2d899686d5 100644 --- a/crates/bevy_hierarchy/src/child_builder.rs +++ b/crates/bevy_hierarchy/src/child_builder.rs @@ -141,7 +141,7 @@ fn remove_children(parent: Entity, children: &[Entity], world: &mut World) { } fn clear_children(parent: Entity, world: &mut World) { - if let Some(children) = world.entity_mut(parent).remove::() { + if let Some(children) = world.entity_mut(parent).take::() { for &child in &children.0 { world.entity_mut(child).remove::(); } @@ -532,7 +532,7 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { fn remove_parent(&mut self) -> &mut Self { let child = self.id(); - if let Some(parent) = self.remove::().map(|p| p.get()) { + if let Some(parent) = self.take::().map(|p| p.get()) { self.world_scope(|world| { remove_from_children(world, parent, child); push_events(world, [HierarchyEvent::ChildRemoved { child, parent }]);