From d1cad57d8cc259d5153f4642fdf7015b5995d600 Mon Sep 17 00:00:00 2001 From: Ellen Date: Thu, 22 Jul 2021 23:25:37 +0100 Subject: [PATCH 1/6] Dedupe move logic in remove_bundle and remove_bundle_intersection --- crates/bevy_ecs/src/world/entity_ref.rs | 91 +++++++++++++------------ 1 file changed, 48 insertions(+), 43 deletions(-) diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index 11a0a8b842800..17748dda0664a 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -340,6 +340,34 @@ impl<'w> EntityMut<'w> { }) }; + unsafe { + Self::move_entity_from_remove::( + entity, + &mut self.location, + old_location.archetype_id, + old_location, + entities, + archetypes, + storages, + new_archetype_id, + ); + } + + Some(result) + } + + /// if DROP is true we will drop removed components, if its false we will forget them + unsafe fn move_entity_from_remove( + entity: Entity, + self_location: &mut EntityLocation, + old_archetype_id: ArchetypeId, + old_location: EntityLocation, + entities: &mut Entities, + archetypes: &mut Archetypes, + storages: &mut Storages, + new_archetype_id: ArchetypeId, + ) { + let old_archetype = &mut archetypes[old_archetype_id]; let remove_result = old_archetype.swap_remove(old_location.index); if let Some(swapped_entity) = remove_result.swapped_entity { entities.meta[swapped_entity.id as usize].location = old_location; @@ -355,10 +383,11 @@ impl<'w> EntityMut<'w> { .tables .get_2_mut(old_table_id, new_archetype.table_id()); - // SAFE: table_row exists. All "missing" components have been extracted into the bundle - // above and the caller takes ownership - let move_result = - unsafe { old_table.move_to_and_forget_missing_unchecked(old_table_row, new_table) }; + // SAFE: table_row exists. All "missing" components have been extracted into the bundle above and the caller takes ownership + let move_result = match DROP { + true => old_table.move_to_and_drop_missing_unchecked(old_table_row, new_table), + false => old_table.move_to_and_forget_missing_unchecked(old_table_row, new_table), + }; // SAFE: new_table_row is a valid position in new_archetype's table let new_location = unsafe { new_archetype.allocate(entity, move_result.new_row) }; @@ -366,17 +395,15 @@ impl<'w> EntityMut<'w> { // if an entity was moved into this entity's table spot, update its table row if let Some(swapped_entity) = move_result.swapped_entity { let swapped_location = entities.get(swapped_entity).unwrap(); - let archetype = &mut archetypes[swapped_location.archetype_id]; - archetype.set_entity_table_row(swapped_location.index, old_table_row); + archetypes[swapped_location.archetype_id] + .set_entity_table_row(swapped_location.index, old_table_row); } new_location }; - self.location = new_location; - entities.meta[self.entity.id as usize].location = new_location; - - Some(result) + *self_location = new_location; + entities.meta[entity.id as usize].location = new_location; } /// Remove any components in the bundle that the entity has. @@ -425,40 +452,18 @@ impl<'w> EntityMut<'w> { } } - let remove_result = old_archetype.swap_remove(old_location.index); - if let Some(swapped_entity) = remove_result.swapped_entity { - entities.meta[swapped_entity.id as usize].location = old_location; + unsafe { + Self::move_entity_from_remove::( + entity, + &mut self.location, + old_location.archetype_id, + old_location, + entities, + archetypes, + storages, + new_archetype_id, + ) } - let old_table_row = remove_result.table_row; - let old_table_id = old_archetype.table_id(); - let new_archetype = &mut archetypes[new_archetype_id]; - - let new_location = if old_table_id == new_archetype.table_id() { - unsafe { new_archetype.allocate(entity, old_table_row) } - } else { - let (old_table, new_table) = storages - .tables - .get_2_mut(old_table_id, new_archetype.table_id()); - - // SAFE: table_row exists - let move_result = - unsafe { old_table.move_to_and_drop_missing_unchecked(old_table_row, new_table) }; - - // SAFE: new_table_row is a valid position in new_archetype's table - let new_location = unsafe { new_archetype.allocate(entity, move_result.new_row) }; - - // if an entity was moved into this entity's table spot, update its table row - if let Some(swapped_entity) = move_result.swapped_entity { - let swapped_location = entities.get(swapped_entity).unwrap(); - archetypes[swapped_location.archetype_id] - .set_entity_table_row(swapped_location.index, old_table_row); - } - - new_location - }; - - self.location = new_location; - entities.meta[self.entity.id as usize].location = new_location; } pub fn insert(&mut self, value: T) -> &mut Self { From f0113c6dea59d2d3e74401e70601f0c4eeaa9e05 Mon Sep 17 00:00:00 2001 From: Ellen Date: Thu, 22 Jul 2021 23:28:47 +0100 Subject: [PATCH 2/6] unused unsafe --- crates/bevy_ecs/src/world/entity_ref.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index 17748dda0664a..65f293708ae07 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -367,6 +367,7 @@ impl<'w> EntityMut<'w> { storages: &mut Storages, new_archetype_id: ArchetypeId, ) { + #![allow(unused_unsafe)] let old_archetype = &mut archetypes[old_archetype_id]; let remove_result = old_archetype.swap_remove(old_location.index); if let Some(swapped_entity) = remove_result.swapped_entity { @@ -383,7 +384,7 @@ impl<'w> EntityMut<'w> { .tables .get_2_mut(old_table_id, new_archetype.table_id()); - // SAFE: table_row exists. All "missing" components have been extracted into the bundle above and the caller takes ownership + // SAFE: table_row exists let move_result = match DROP { true => old_table.move_to_and_drop_missing_unchecked(old_table_row, new_table), false => old_table.move_to_and_forget_missing_unchecked(old_table_row, new_table), From 48ea6f69fdc844979a8076b8fe605d00e977a8dd Mon Sep 17 00:00:00 2001 From: Ellen Date: Thu, 22 Jul 2021 23:40:59 +0100 Subject: [PATCH 3/6] go away clippy --- crates/bevy_ecs/src/world/entity_ref.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index 65f293708ae07..b6021a9f249f0 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -357,6 +357,8 @@ impl<'w> EntityMut<'w> { } /// if DROP is true we will drop removed components, if its false we will forget them + #[allow(clippy::too_many_arguments)] + #[allow(clippy::match_bool)] unsafe fn move_entity_from_remove( entity: Entity, self_location: &mut EntityLocation, From 85828f1bc29b5d37471ac83a84776d661be45482 Mon Sep 17 00:00:00 2001 From: Ellen Date: Sat, 24 Jul 2021 03:29:55 +0100 Subject: [PATCH 4/6] voice changer --- crates/bevy_ecs/src/world/entity_ref.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index b6021a9f249f0..4fa53b48cb61a 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -356,7 +356,7 @@ impl<'w> EntityMut<'w> { Some(result) } - /// if DROP is true we will drop removed components, if its false we will forget them + /// when DROP is true removed components will be dropped otherwise they will be forgotten #[allow(clippy::too_many_arguments)] #[allow(clippy::match_bool)] unsafe fn move_entity_from_remove( From 9af0b3ae6543f09643c125f4d91c159ecc6aebfa Mon Sep 17 00:00:00 2001 From: Ellen Date: Sat, 24 Jul 2021 22:35:35 +0100 Subject: [PATCH 5/6] nits --- crates/bevy_ecs/src/world/entity_ref.rs | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index 4fa53b48cb61a..56877f5b63fb1 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -356,7 +356,14 @@ impl<'w> EntityMut<'w> { Some(result) } + /// Safety: `new_archetype_id` must have the same or a subset of the components + /// in `old_archetype_id`. Probably more safety stuff too, audit a call to + /// this fn as if the code here was written inline + /// /// when DROP is true removed components will be dropped otherwise they will be forgotten + /// + // We use a const generic here so that we are less reliant on + // inlining for rustc to optimize out the `match DROP` #[allow(clippy::too_many_arguments)] #[allow(clippy::match_bool)] unsafe fn move_entity_from_remove( @@ -369,7 +376,6 @@ impl<'w> EntityMut<'w> { storages: &mut Storages, new_archetype_id: ArchetypeId, ) { - #![allow(unused_unsafe)] let old_archetype = &mut archetypes[old_archetype_id]; let remove_result = old_archetype.swap_remove(old_location.index); if let Some(swapped_entity) = remove_result.swapped_entity { @@ -380,20 +386,20 @@ impl<'w> EntityMut<'w> { let new_archetype = &mut archetypes[new_archetype_id]; let new_location = if old_table_id == new_archetype.table_id() { - unsafe { new_archetype.allocate(entity, old_table_row) } + new_archetype.allocate(entity, old_table_row) } else { let (old_table, new_table) = storages .tables .get_2_mut(old_table_id, new_archetype.table_id()); - // SAFE: table_row exists + // SAFE: old_table_row exists let move_result = match DROP { true => old_table.move_to_and_drop_missing_unchecked(old_table_row, new_table), false => old_table.move_to_and_forget_missing_unchecked(old_table_row, new_table), }; - // SAFE: new_table_row is a valid position in new_archetype's table - let new_location = unsafe { new_archetype.allocate(entity, move_result.new_row) }; + // SAFE: move_result.new_row is a valid position in new_archetype's table + let new_location = new_archetype.allocate(entity, move_result.new_row); // if an entity was moved into this entity's table spot, update its table row if let Some(swapped_entity) = move_result.swapped_entity { From ef69ae7cea5e37a5fec67a3ffc0a2947e6934f50 Mon Sep 17 00:00:00 2001 From: Ellen Date: Tue, 27 Jul 2021 06:01:07 +0100 Subject: [PATCH 6/6] goodbye match bool you will be missed --- crates/bevy_ecs/src/world/entity_ref.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index 56877f5b63fb1..6e7185dc5b625 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -365,7 +365,6 @@ impl<'w> EntityMut<'w> { // We use a const generic here so that we are less reliant on // inlining for rustc to optimize out the `match DROP` #[allow(clippy::too_many_arguments)] - #[allow(clippy::match_bool)] unsafe fn move_entity_from_remove( entity: Entity, self_location: &mut EntityLocation, @@ -393,9 +392,10 @@ impl<'w> EntityMut<'w> { .get_2_mut(old_table_id, new_archetype.table_id()); // SAFE: old_table_row exists - let move_result = match DROP { - true => old_table.move_to_and_drop_missing_unchecked(old_table_row, new_table), - false => old_table.move_to_and_forget_missing_unchecked(old_table_row, new_table), + let move_result = if DROP { + old_table.move_to_and_drop_missing_unchecked(old_table_row, new_table) + } else { + old_table.move_to_and_forget_missing_unchecked(old_table_row, new_table) }; // SAFE: move_result.new_row is a valid position in new_archetype's table