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::<Children>() {
        for &child in &children.0 {
            world.entity_mut(child).remove_intersection::<Parent>();
        }
    }
}
```

After
```rust
fn clear_children(parent: Entity, world: &mut World) {
    if let Some(children) = world.entity_mut(parent).take::<Children>() {
        for &child in &children.0 {
            world.entity_mut(child).remove::<Parent>();
        }
    }
}
```
This commit is contained in:
Cameron 2023-02-26 00:09:19 +00:00
parent 9c98f8adc3
commit be22569db7
8 changed files with 46 additions and 54 deletions

View File

@ -150,7 +150,7 @@ impl BundleComponentStatus for SpawnBundleStatus {
pub struct Edges { pub struct Edges {
add_bundle: SparseArray<BundleId, AddBundle>, add_bundle: SparseArray<BundleId, AddBundle>,
remove_bundle: SparseArray<BundleId, Option<ArchetypeId>>, remove_bundle: SparseArray<BundleId, Option<ArchetypeId>>,
remove_bundle_intersection: SparseArray<BundleId, Option<ArchetypeId>>, take_bundle: SparseArray<BundleId, Option<ArchetypeId>>,
} }
impl Edges { impl Edges {
@ -222,32 +222,28 @@ impl Edges {
} }
/// Checks the cache for the target archetype when removing a bundle to the /// 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 /// If this returns `None`, it means there has not been a transition from
/// the source archetype via the provided bundle. /// the source archetype via the provided bundle.
/// ///
/// [`EntityMut::remove_intersection`]: crate::world::EntityMut::remove_intersection /// [`EntityMut::remove`]: crate::world::EntityMut::remove
#[inline] #[inline]
pub fn get_remove_bundle_intersection( pub fn get_take_bundle(&self, bundle_id: BundleId) -> Option<Option<ArchetypeId>> {
&self, self.take_bundle.get(bundle_id).cloned()
bundle_id: BundleId,
) -> Option<Option<ArchetypeId>> {
self.remove_bundle_intersection.get(bundle_id).cloned()
} }
/// Caches the target archetype when removing a bundle to the source archetype. /// 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] #[inline]
pub(crate) fn insert_remove_bundle_intersection( pub(crate) fn insert_take_bundle(
&mut self, &mut self,
bundle_id: BundleId, bundle_id: BundleId,
archetype_id: Option<ArchetypeId>, archetype_id: Option<ArchetypeId>,
) { ) {
self.remove_bundle_intersection self.take_bundle.insert(bundle_id, archetype_id);
.insert(bundle_id, archetype_id);
} }
} }

View File

@ -191,7 +191,7 @@ mod tests {
assert_eq!(world.get::<SparseStored>(e2).unwrap().0, 42); assert_eq!(world.get::<SparseStored>(e2).unwrap().0, 42);
assert_eq!( assert_eq!(
world.entity_mut(e1).remove::<FooBundle>().unwrap(), world.entity_mut(e1).take::<FooBundle>().unwrap(),
FooBundle { FooBundle {
x: TableStored("xyz"), x: TableStored("xyz"),
y: SparseStored(123), y: SparseStored(123),
@ -240,7 +240,7 @@ mod tests {
assert_eq!(world.get::<A>(e3).unwrap().0, 1); assert_eq!(world.get::<A>(e3).unwrap().0, 1);
assert_eq!(world.get::<B>(e3).unwrap().0, 2); assert_eq!(world.get::<B>(e3).unwrap().0, 2);
assert_eq!( assert_eq!(
world.entity_mut(e3).remove::<NestedBundle>().unwrap(), world.entity_mut(e3).take::<NestedBundle>().unwrap(),
NestedBundle { NestedBundle {
a: A(1), a: A(1),
foo: FooBundle { foo: FooBundle {
@ -283,7 +283,7 @@ mod tests {
assert_eq!(world.get::<Ignored>(e4), None); assert_eq!(world.get::<Ignored>(e4), None);
assert_eq!( assert_eq!(
world.entity_mut(e4).remove::<BundleWithIgnored>().unwrap(), world.entity_mut(e4).take::<BundleWithIgnored>().unwrap(),
BundleWithIgnored { BundleWithIgnored {
c: C, c: C,
ignored: Ignored, ignored: Ignored,
@ -596,7 +596,7 @@ mod tests {
&[(e1, A(1), B(3)), (e2, A(2), B(4))] &[(e1, A(1), B(3)), (e2, A(2), B(4))]
); );
assert_eq!(world.entity_mut(e1).remove::<A>(), Some(A(1))); assert_eq!(world.entity_mut(e1).take::<A>(), Some(A(1)));
assert_eq!( assert_eq!(
world world
.query::<(Entity, &A, &B)>() .query::<(Entity, &A, &B)>()
@ -656,7 +656,7 @@ mod tests {
} }
for (i, entity) in entities.iter().cloned().enumerate() { for (i, entity) in entities.iter().cloned().enumerate() {
assert_eq!(world.entity_mut(entity).remove::<A>(), Some(A(i))); assert_eq!(world.entity_mut(entity).take::<A>(), Some(A(i)));
} }
} }
@ -675,7 +675,7 @@ mod tests {
for (i, entity) in entities.iter().cloned().enumerate() { for (i, entity) in entities.iter().cloned().enumerate() {
assert_eq!( assert_eq!(
world.entity_mut(entity).remove::<SparseStored>(), world.entity_mut(entity).take::<SparseStored>(),
Some(SparseStored(i as u32)) Some(SparseStored(i as u32))
); );
} }
@ -685,7 +685,7 @@ mod tests {
fn remove_missing() { fn remove_missing() {
let mut world = World::new(); let mut world = World::new();
let e = world.spawn((TableStored("abc"), A(123))).id(); let e = world.spawn((TableStored("abc"), A(123))).id();
assert!(world.entity_mut(e).remove::<B>().is_none()); assert!(world.entity_mut(e).take::<B>().is_none());
} }
#[test] #[test]
@ -1187,7 +1187,7 @@ mod tests {
} }
#[test] #[test]
fn remove_intersection() { fn remove() {
let mut world = World::default(); let mut world = World::default();
let e1 = world.spawn((A(1), B(1), TableStored("a"))).id(); 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" "C is not in the entity, so it should not exist"
); );
e.remove_intersection::<(A, B, C)>(); e.remove::<(A, B, C)>();
assert_eq!( assert_eq!(
e.get::<TableStored>(), e.get::<TableStored>(),
Some(&TableStored("a")), Some(&TableStored("a")),
@ -1225,7 +1225,7 @@ mod tests {
} }
#[test] #[test]
fn remove() { fn take() {
let mut world = World::default(); let mut world = World::default();
world.spawn((A(1), B(1), TableStored("1"))); world.spawn((A(1), B(1), TableStored("1")));
let e2 = world.spawn((A(2), B(2), TableStored("2"))).id(); let e2 = world.spawn((A(2), B(2), TableStored("2"))).id();
@ -1238,7 +1238,7 @@ mod tests {
.collect::<Vec<_>>(); .collect::<Vec<_>>();
assert_eq!(results, vec![(1, "1"), (2, "2"), (3, "3"),]); 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"))); assert_eq!(removed_bundle, (B(2), TableStored("2")));
let results = query let results = query

View File

@ -99,10 +99,6 @@ impl ReflectComponent {
} }
/// Removes this [`Component`] type from the entity. Does nothing if it doesn't exist. /// 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) { pub fn remove(&self, entity: &mut EntityMut) {
(self.0.remove)(entity); (self.0.remove)(entity);
} }

View File

@ -951,9 +951,7 @@ where
{ {
fn write(self, world: &mut World) { fn write(self, world: &mut World) {
if let Some(mut entity_mut) = world.get_entity_mut(self.entity) { if let Some(mut entity_mut) = world.get_entity_mut(self.entity) {
// remove intersection to gracefully handle components that were removed before running entity_mut.remove::<T>();
// this command
entity_mut.remove_intersection::<T>();
} }
} }
} }

View File

@ -279,11 +279,13 @@ impl<'w> EntityMut<'w> {
self self
} }
// TODO: move to BundleInfo /// Removes all components in the [`Bundle`] from the entity and returns their previous values.
/// Removes a [`Bundle`] of components from the entity and returns the bundle.
/// ///
/// Returns `None` if the entity does not contain the bundle. /// **Note:** If the entity does not have every component in the bundle, this method will not
pub fn remove<T: Bundle>(&mut self) -> Option<T> { /// remove any of them.
// TODO: BundleRemover?
#[must_use]
pub fn take<T: Bundle>(&mut self) -> Option<T> {
let archetypes = &mut self.world.archetypes; let archetypes = &mut self.world.archetypes;
let storages = &mut self.world.storages; let storages = &mut self.world.storages;
let components = &mut self.world.components; let components = &mut self.world.components;
@ -408,9 +410,9 @@ impl<'w> EntityMut<'w> {
entities.set(entity.index(), new_location); entities.set(entity.index(), new_location);
} }
// TODO: move to BundleInfo /// Removes any components in the [`Bundle`] from the entity.
/// Remove any components in the bundle that the entity has. // TODO: BundleRemover?
pub fn remove_intersection<T: Bundle>(&mut self) { pub fn remove<T: Bundle>(&mut self) -> &mut Self {
let archetypes = &mut self.world.archetypes; let archetypes = &mut self.world.archetypes;
let storages = &mut self.world.storages; let storages = &mut self.world.storages;
let components = &mut self.world.components; let components = &mut self.world.components;
@ -435,7 +437,7 @@ impl<'w> EntityMut<'w> {
}; };
if new_archetype_id == old_location.archetype_id { if new_archetype_id == old_location.archetype_id {
return; return self;
} }
let old_archetype = &mut archetypes[old_location.archetype_id]; let old_archetype = &mut archetypes[old_location.archetype_id];
@ -469,6 +471,8 @@ impl<'w> EntityMut<'w> {
new_archetype_id, new_archetype_id,
); );
} }
self
} }
pub fn despawn(self) { pub fn despawn(self) {
@ -647,11 +651,9 @@ unsafe fn remove_bundle_from_archetype(
let remove_bundle_result = { let remove_bundle_result = {
let current_archetype = &mut archetypes[archetype_id]; let current_archetype = &mut archetypes[archetype_id];
if intersection { if intersection {
current_archetype
.edges()
.get_remove_bundle_intersection(bundle_info.id)
} else {
current_archetype.edges().get_remove_bundle(bundle_info.id) 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 { let result = if let Some(result) = remove_bundle_result {
@ -679,7 +681,7 @@ unsafe fn remove_bundle_from_archetype(
// graph // graph
current_archetype current_archetype
.edges_mut() .edges_mut()
.insert_remove_bundle(bundle_info.id, None); .insert_take_bundle(bundle_info.id, None);
return None; return None;
} }
} }
@ -718,11 +720,11 @@ unsafe fn remove_bundle_from_archetype(
if intersection { if intersection {
current_archetype current_archetype
.edges_mut() .edges_mut()
.insert_remove_bundle_intersection(bundle_info.id, result); .insert_remove_bundle(bundle_info.id, result);
} else { } else {
current_archetype current_archetype
.edges_mut() .edges_mut()
.insert_remove_bundle(bundle_info.id, result); .insert_take_bundle(bundle_info.id, result);
} }
result result
} }

View File

@ -14,7 +14,7 @@ fn main() {
{ {
let gotten: &A = e_mut.get::<A>().unwrap(); let gotten: &A = e_mut.get::<A>().unwrap();
let gotten2: A = e_mut.remove::<A>().unwrap(); let gotten2: A = e_mut.take::<A>().unwrap();
assert_eq!(gotten, &gotten2); // oops UB assert_eq!(gotten, &gotten2); // oops UB
} }
@ -22,7 +22,7 @@ fn main() {
{ {
let mut gotten: Mut<A> = e_mut.get_mut::<A>().unwrap(); let mut gotten: Mut<A> = e_mut.get_mut::<A>().unwrap();
let mut gotten2: A = e_mut.remove::<A>().unwrap(); let mut gotten2: A = e_mut.take::<A>().unwrap();
assert_eq!(&mut *gotten, &mut gotten2); // oops UB assert_eq!(&mut *gotten, &mut gotten2); // oops UB
} }

View File

@ -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::<A>().unwrap(); 16 | let gotten: &A = e_mut.get::<A>().unwrap();
| ---------------- immutable borrow occurs here | ---------------- immutable borrow occurs here
17 | let gotten2: A = e_mut.remove::<A>().unwrap(); 17 | let gotten2: A = e_mut.take::<A>().unwrap();
| ^^^^^^^^^^^^^^^^^^^ mutable borrow occurs here | ^^^^^^^^^^^^^^^^^ mutable borrow occurs here
18 | assert_eq!(gotten, &gotten2); // oops UB 18 | assert_eq!(gotten, &gotten2); // oops UB
| ---------------------------- immutable borrow later used here | ---------------------------- 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<A> = e_mut.get_mut::<A>().unwrap(); 24 | let mut gotten: Mut<A> = e_mut.get_mut::<A>().unwrap();
| -------------------- first mutable borrow occurs here | -------------------- first mutable borrow occurs here
25 | let mut gotten2: A = e_mut.remove::<A>().unwrap(); 25 | let mut gotten2: A = e_mut.take::<A>().unwrap();
| ^^^^^^^^^^^^^^^^^^^ second mutable borrow occurs here | ^^^^^^^^^^^^^^^^^ second mutable borrow occurs here
26 | assert_eq!(&mut *gotten, &mut gotten2); // oops UB 26 | assert_eq!(&mut *gotten, &mut gotten2); // oops UB
| ------ first borrow later used here | ------ first borrow later used here

View File

@ -141,7 +141,7 @@ fn remove_children(parent: Entity, children: &[Entity], world: &mut World) {
} }
fn clear_children(parent: Entity, world: &mut World) { fn clear_children(parent: Entity, world: &mut World) {
if let Some(children) = world.entity_mut(parent).remove::<Children>() { if let Some(children) = world.entity_mut(parent).take::<Children>() {
for &child in &children.0 { for &child in &children.0 {
world.entity_mut(child).remove::<Parent>(); world.entity_mut(child).remove::<Parent>();
} }
@ -532,7 +532,7 @@ impl<'w> BuildWorldChildren for EntityMut<'w> {
fn remove_parent(&mut self) -> &mut Self { fn remove_parent(&mut self) -> &mut Self {
let child = self.id(); let child = self.id();
if let Some(parent) = self.remove::<Parent>().map(|p| p.get()) { if let Some(parent) = self.take::<Parent>().map(|p| p.get()) {
self.world_scope(|world| { self.world_scope(|world| {
remove_from_children(world, parent, child); remove_from_children(world, parent, child);
push_events(world, [HierarchyEvent::ChildRemoved { child, parent }]); push_events(world, [HierarchyEvent::ChildRemoved { child, parent }]);