From 4dc6e041b68a23673159c37288b06324a507826a Mon Sep 17 00:00:00 2001 From: devil-ira Date: Tue, 1 Nov 2022 13:51:16 +0100 Subject: [PATCH 1/6] Well, that was a lie. --- crates/bevy_hierarchy/src/child_builder.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/bevy_hierarchy/src/child_builder.rs b/crates/bevy_hierarchy/src/child_builder.rs index 721e3c06b1ef2..6599d85955fe8 100644 --- a/crates/bevy_hierarchy/src/child_builder.rs +++ b/crates/bevy_hierarchy/src/child_builder.rs @@ -515,9 +515,10 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { fn remove_children(&mut self, children: &[Entity]) -> &mut Self { let parent = self.id(); - // SAFETY: This doesn't change the parent's location + // SAFETY: The EntityLocation is updated afterwards. let world = unsafe { self.world_mut() }; remove_children(parent, children, world); + self.update_location(); self } } From 06f2774c5b4f19be2d0e4cd5ca55ab95d1574689 Mon Sep 17 00:00:00 2001 From: devil-ira Date: Tue, 1 Nov 2022 13:53:39 +0100 Subject: [PATCH 2/6] Remove unhelpful safety comments. --- crates/bevy_hierarchy/src/child_builder.rs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/crates/bevy_hierarchy/src/child_builder.rs b/crates/bevy_hierarchy/src/child_builder.rs index 6599d85955fe8..e194d2f2a4ae4 100644 --- a/crates/bevy_hierarchy/src/child_builder.rs +++ b/crates/bevy_hierarchy/src/child_builder.rs @@ -461,8 +461,7 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { let mut builder = WorldChildBuilder { current_entity: None, parent_entities: vec![entity], - // SAFETY: self.update_location() is called below. It is impossible to make EntityMut - // function calls on `self` within the scope defined here + // SAFETY: The EntityLocation is updated afterwards. world: unsafe { self.world_mut() }, }; @@ -475,10 +474,9 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { fn push_children(&mut self, children: &[Entity]) -> &mut Self { let parent = self.id(); { - // SAFETY: parent entity is not modified and its location is updated manually + // SAFETY: The EntityLocation is updated afterwards. let world = unsafe { self.world_mut() }; update_old_parents(world, parent, children); - // Inserting a bundle in the children entities may change the parent entity's location if they were of the same archetype self.update_location(); } if let Some(mut children_component) = self.get_mut::() { @@ -495,10 +493,9 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { fn insert_children(&mut self, index: usize, children: &[Entity]) -> &mut Self { let parent = self.id(); { - // SAFETY: parent entity is not modified and its location is updated manually + // SAFETY: The EntityLocation is updated afterwards. let world = unsafe { self.world_mut() }; update_old_parents(world, parent, children); - // Inserting a bundle in the children entities may change the parent entity's location if they were of the same archetype self.update_location(); } @@ -515,7 +512,7 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { fn remove_children(&mut self, children: &[Entity]) -> &mut Self { let parent = self.id(); - // SAFETY: The EntityLocation is updated afterwards. + // SAFETY: The EntityLocation is updated afterwards. let world = unsafe { self.world_mut() }; remove_children(parent, children, world); self.update_location(); From 5fe95819cdb9ced8f4599f8169a72010429334b7 Mon Sep 17 00:00:00 2001 From: devil-ira Date: Tue, 1 Nov 2022 13:59:24 +0100 Subject: [PATCH 3/6] reworded --- crates/bevy_hierarchy/src/child_builder.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/bevy_hierarchy/src/child_builder.rs b/crates/bevy_hierarchy/src/child_builder.rs index e194d2f2a4ae4..0f53ed91a99e0 100644 --- a/crates/bevy_hierarchy/src/child_builder.rs +++ b/crates/bevy_hierarchy/src/child_builder.rs @@ -461,7 +461,7 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { let mut builder = WorldChildBuilder { current_entity: None, parent_entities: vec![entity], - // SAFETY: The EntityLocation is updated afterwards. + // SAFETY: The EntityLocation is updated after mutable world access. world: unsafe { self.world_mut() }, }; @@ -474,7 +474,7 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { fn push_children(&mut self, children: &[Entity]) -> &mut Self { let parent = self.id(); { - // SAFETY: The EntityLocation is updated afterwards. + // SAFETY: The EntityLocation is updated after mutable world access before any methods are called on self. let world = unsafe { self.world_mut() }; update_old_parents(world, parent, children); self.update_location(); @@ -493,7 +493,7 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { fn insert_children(&mut self, index: usize, children: &[Entity]) -> &mut Self { let parent = self.id(); { - // SAFETY: The EntityLocation is updated afterwards. + // SAFETY: The EntityLocation is updated after mutable world access before any methods are called on self. let world = unsafe { self.world_mut() }; update_old_parents(world, parent, children); self.update_location(); @@ -512,7 +512,7 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { fn remove_children(&mut self, children: &[Entity]) -> &mut Self { let parent = self.id(); - // SAFETY: The EntityLocation is updated afterwards. + // SAFETY: The EntityLocation is updated after mutable world access. let world = unsafe { self.world_mut() }; remove_children(parent, children, world); self.update_location(); From 9483f1a47bfaec8071528cfd37b4898a672b9cd7 Mon Sep 17 00:00:00 2001 From: devil-ira Date: Fri, 4 Nov 2022 12:54:04 +0100 Subject: [PATCH 4/6] reworded again --- crates/bevy_hierarchy/src/child_builder.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/bevy_hierarchy/src/child_builder.rs b/crates/bevy_hierarchy/src/child_builder.rs index 0f53ed91a99e0..b23c2ef268e1d 100644 --- a/crates/bevy_hierarchy/src/child_builder.rs +++ b/crates/bevy_hierarchy/src/child_builder.rs @@ -461,7 +461,7 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { let mut builder = WorldChildBuilder { current_entity: None, parent_entities: vec![entity], - // SAFETY: The EntityLocation is updated after mutable world access. + // SAFETY: The EntityLocation is updated before any other methods are called on self. world: unsafe { self.world_mut() }, }; @@ -474,7 +474,7 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { fn push_children(&mut self, children: &[Entity]) -> &mut Self { let parent = self.id(); { - // SAFETY: The EntityLocation is updated after mutable world access before any methods are called on self. + // SAFETY: The EntityLocation is updated before any other methods are called on self. let world = unsafe { self.world_mut() }; update_old_parents(world, parent, children); self.update_location(); @@ -493,7 +493,7 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { fn insert_children(&mut self, index: usize, children: &[Entity]) -> &mut Self { let parent = self.id(); { - // SAFETY: The EntityLocation is updated after mutable world access before any methods are called on self. + // SAFETY: The EntityLocation is updated before any other methods are called on self. let world = unsafe { self.world_mut() }; update_old_parents(world, parent, children); self.update_location(); @@ -512,7 +512,7 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { fn remove_children(&mut self, children: &[Entity]) -> &mut Self { let parent = self.id(); - // SAFETY: The EntityLocation is updated after mutable world access. + // SAFETY: The EntityLocation is updated before any other methods are called on self. let world = unsafe { self.world_mut() }; remove_children(parent, children, world); self.update_location(); From 44536507a97426eaba6c63cf44917dd7f46616e5 Mon Sep 17 00:00:00 2001 From: devil-ira Date: Fri, 4 Nov 2022 13:30:04 +0100 Subject: [PATCH 5/6] remove unnecessary unsafe --- crates/bevy_hierarchy/src/hierarchy.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/crates/bevy_hierarchy/src/hierarchy.rs b/crates/bevy_hierarchy/src/hierarchy.rs index 93c2be96f96a2..df81324387b2b 100644 --- a/crates/bevy_hierarchy/src/hierarchy.rs +++ b/crates/bevy_hierarchy/src/hierarchy.rs @@ -104,7 +104,7 @@ impl<'w, 's, 'a> DespawnRecursiveExt for EntityCommands<'w, 's, 'a> { impl<'w> DespawnRecursiveExt for EntityMut<'w> { /// Despawns the provided entity and its children. - fn despawn_recursive(mut self) { + fn despawn_recursive(self) { let entity = self.id(); #[cfg(feature = "trace")] @@ -114,11 +114,7 @@ impl<'w> DespawnRecursiveExt for EntityMut<'w> { ) .entered(); - // SAFETY: EntityMut is consumed so even though the location is no longer - // valid, it cannot be accessed again with the invalid location. - unsafe { - despawn_with_children_recursive(self.world_mut(), entity); - } + despawn_with_children_recursive(self.into_world_mut(), entity); } fn despawn_descendants(&mut self) { From 201a3f9902ce66127dea87ba078da6ca430f2db3 Mon Sep 17 00:00:00 2001 From: devil-ira Date: Fri, 4 Nov 2022 13:54:44 +0100 Subject: [PATCH 6/6] Add `EntityMut::world_scope` --- crates/bevy_ecs/src/world/entity_ref.rs | 8 +++++- crates/bevy_hierarchy/src/child_builder.rs | 33 ++++++++-------------- 2 files changed, 18 insertions(+), 23 deletions(-) diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index 29ff36fb903e2..d7049057d9d80 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -526,7 +526,7 @@ impl<'w> EntityMut<'w> { /// Returns this `EntityMut`'s world. /// - /// See [`EntityMut::into_world_mut`] for a safe alternative. + /// See [`EntityMut::world_scope`] or [`EntityMut::into_world_mut`] for a safe alternative. /// /// # Safety /// Caller must not modify the world in a way that changes the current entity's location @@ -543,6 +543,12 @@ impl<'w> EntityMut<'w> { self.world } + /// Gives mutable access to this `EntityMut`'s [`World`] in a temporary scope. + pub fn world_scope(&mut self, f: impl FnOnce(&mut World)) { + f(self.world); + self.update_location(); + } + /// Updates the internal entity location to match the current location in the internal /// [`World`]. This is only needed if the user called [`EntityMut::world`], which enables the /// location to change. diff --git a/crates/bevy_hierarchy/src/child_builder.rs b/crates/bevy_hierarchy/src/child_builder.rs index b23c2ef268e1d..a5c5d6708a667 100644 --- a/crates/bevy_hierarchy/src/child_builder.rs +++ b/crates/bevy_hierarchy/src/child_builder.rs @@ -456,29 +456,23 @@ pub trait BuildWorldChildren { impl<'w> BuildWorldChildren for EntityMut<'w> { fn with_children(&mut self, spawn_children: impl FnOnce(&mut WorldChildBuilder)) -> &mut Self { - { - let entity = self.id(); + let entity = self.id(); + self.world_scope(|world| { let mut builder = WorldChildBuilder { current_entity: None, parent_entities: vec![entity], - // SAFETY: The EntityLocation is updated before any other methods are called on self. - world: unsafe { self.world_mut() }, + world, }; - spawn_children(&mut builder); - } - self.update_location(); + }); self } fn push_children(&mut self, children: &[Entity]) -> &mut Self { let parent = self.id(); - { - // SAFETY: The EntityLocation is updated before any other methods are called on self. - let world = unsafe { self.world_mut() }; + self.world_scope(|world| { update_old_parents(world, parent, children); - self.update_location(); - } + }); if let Some(mut children_component) = self.get_mut::() { children_component .0 @@ -492,13 +486,9 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { fn insert_children(&mut self, index: usize, children: &[Entity]) -> &mut Self { let parent = self.id(); - { - // SAFETY: The EntityLocation is updated before any other methods are called on self. - let world = unsafe { self.world_mut() }; + self.world_scope(|world| { update_old_parents(world, parent, children); - self.update_location(); - } - + }); if let Some(mut children_component) = self.get_mut::() { children_component .0 @@ -512,10 +502,9 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { fn remove_children(&mut self, children: &[Entity]) -> &mut Self { let parent = self.id(); - // SAFETY: The EntityLocation is updated before any other methods are called on self. - let world = unsafe { self.world_mut() }; - remove_children(parent, children, world); - self.update_location(); + self.world_scope(|world| { + remove_children(parent, children, world); + }); self } }