From 81c83dd2bc9d2d2204552ef30ad2588788b938d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20=C5=81abor?= Date: Sun, 15 Jan 2023 11:05:58 +0100 Subject: [PATCH 1/5] ReflectComponentFns --- crates/bevy_ecs/src/reflect.rs | 47 ++++++++++--------- .../bevy_scene/src/dynamic_scene_builder.rs | 11 ++--- 2 files changed, 30 insertions(+), 28 deletions(-) diff --git a/crates/bevy_ecs/src/reflect.rs b/crates/bevy_ecs/src/reflect.rs index b865844731b66..279469c9f69e3 100644 --- a/crates/bevy_ecs/src/reflect.rs +++ b/crates/bevy_ecs/src/reflect.rs @@ -5,7 +5,7 @@ use crate::{ component::Component, entity::{Entity, EntityMap, MapEntities, MapEntitiesError}, system::Resource, - world::{unsafe_world_cell::UnsafeWorldCell, FromWorld, World}, + world::{unsafe_world_cell::UnsafeWorldCell, EntityMut, EntityRef, FromWorld, World}, }; use bevy_reflect::{ impl_from_reflect_value, impl_reflect_value, FromType, Reflect, ReflectDeserialize, @@ -44,13 +44,15 @@ pub struct ReflectComponentFns { /// Function pointer implementing [`ReflectComponent::insert()`]. pub insert: fn(&mut World, Entity, &dyn Reflect), /// Function pointer implementing [`ReflectComponent::apply()`]. - pub apply: fn(&mut World, Entity, &dyn Reflect), + pub apply: fn(EntityMut, &dyn Reflect), /// Function pointer implementing [`ReflectComponent::apply_or_insert()`]. pub apply_or_insert: fn(&mut World, Entity, &dyn Reflect), /// Function pointer implementing [`ReflectComponent::remove()`]. - pub remove: fn(&mut World, Entity), + pub remove: fn(EntityMut), + /// Function pointer implementing [`ReflectComponent::contains()`]. + pub contains: fn(EntityRef) -> bool, /// Function pointer implementing [`ReflectComponent::reflect()`]. - pub reflect: fn(&World, Entity) -> Option<&dyn Reflect>, + pub reflect: fn(EntityRef) -> Option<&dyn Reflect>, /// Function pointer implementing [`ReflectComponent::reflect_mut()`]. /// /// # Safety @@ -85,9 +87,9 @@ impl ReflectComponent { /// /// # Panics /// - /// Panics if there is no [`Component`] of the given type or the `entity` does not exist. - pub fn apply(&self, world: &mut World, entity: Entity, component: &dyn Reflect) { - (self.0.apply)(world, entity, component); + /// Panics if there is no [`Component`] of the given type. + pub fn apply(&self, entity: EntityMut, component: &dyn Reflect) { + (self.0.apply)(entity, component); } /// Uses reflection to set the value of this [`Component`] type in the entity to the given value or insert a new one if it does not exist. @@ -103,14 +105,19 @@ impl ReflectComponent { /// /// # Panics /// - /// Panics if there is no [`Component`] of the given type or the `entity` does not exist. - pub fn remove(&self, world: &mut World, entity: Entity) { - (self.0.remove)(world, entity); + /// Panics if there is no [`Component`] of the given type. + pub fn remove(&self, entity: EntityMut) { + (self.0.remove)(entity); + } + + /// Returns whether entity contains this [`Component`] + pub fn contains(&self, entity: EntityRef) -> bool { + (self.0.contains)(entity) } /// Gets the value of this [`Component`] type from the entity as a reflected reference. - pub fn reflect<'a>(&self, world: &'a World, entity: Entity) -> Option<&'a dyn Reflect> { - (self.0.reflect)(world, entity) + pub fn reflect<'a>(&self, entity: EntityRef<'a>) -> Option<&'a dyn Reflect> { + (self.0.reflect)(entity) } /// Gets the value of this [`Component`] type from the entity as a mutable reflected reference. @@ -181,8 +188,8 @@ impl FromType for ReflectComponent { component.apply(reflected_component); world.entity_mut(entity).insert(component); }, - apply: |world, entity, reflected_component| { - let mut component = world.get_mut::(entity).unwrap(); + apply: |mut entity, reflected_component| { + let mut component = entity.get_mut::().unwrap(); component.apply(reflected_component); }, apply_or_insert: |world, entity, reflected_component| { @@ -194,9 +201,10 @@ impl FromType for ReflectComponent { world.entity_mut(entity).insert(component); } }, - remove: |world, entity| { - world.entity_mut(entity).remove::(); + remove: |mut entity| { + entity.remove::(); }, + contains: |entity| entity.contains::(), copy: |source_world, destination_world, source_entity, destination_entity| { let source_component = source_world.get::(source_entity).unwrap(); let mut destination_component = C::from_world(destination_world); @@ -205,12 +213,7 @@ impl FromType for ReflectComponent { .entity_mut(destination_entity) .insert(destination_component); }, - reflect: |world, entity| { - world - .get_entity(entity)? - .get::() - .map(|c| c as &dyn Reflect) - }, + reflect: |entity| entity.get::().map(|c| c as &dyn Reflect), reflect_mut: |world, entity| { // SAFETY: reflect_mut is an unsafe function pointer used by // 1. `reflect_unchecked_mut` which must be called with an UnsafeWorldCell with access to the the component `C` on the `entity`, and diff --git a/crates/bevy_scene/src/dynamic_scene_builder.rs b/crates/bevy_scene/src/dynamic_scene_builder.rs index 98bf10e27f5b8..1187f087b6809 100644 --- a/crates/bevy_scene/src/dynamic_scene_builder.rs +++ b/crates/bevy_scene/src/dynamic_scene_builder.rs @@ -124,19 +124,18 @@ impl<'w> DynamicSceneBuilder<'w> { components: Vec::new(), }; - for component_id in self.original_world.entity(entity).archetype().components() { + let entity = self.original_world.entity(entity); + for component_id in entity.archetype().components() { let reflect_component = self .original_world .components() .get_info(component_id) .and_then(|info| type_registry.get(info.type_id().unwrap())) - .and_then(|registration| registration.data::()); + .and_then(|registration| registration.data::()) + .and_then(|reflect_component| reflect_component.reflect(entity)); if let Some(reflect_component) = reflect_component { - if let Some(component) = reflect_component.reflect(self.original_world, entity) - { - entry.components.push(component.clone_value()); - } + entry.components.push(reflect_component.clone_value()); } } self.extracted_scene.insert(index, entry); From 4936e4990ac0b21dbd00b90974988e6bcac512a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20=C5=81abor?= Date: Thu, 19 Jan 2023 08:34:08 +0100 Subject: [PATCH 2/5] EntityMut reflect_mut Nit --- crates/bevy_ecs/src/reflect.rs | 62 ++++++++++--------- .../bevy_scene/src/dynamic_scene_builder.rs | 2 +- 2 files changed, 35 insertions(+), 29 deletions(-) diff --git a/crates/bevy_ecs/src/reflect.rs b/crates/bevy_ecs/src/reflect.rs index 279469c9f69e3..a61d856f04b6e 100644 --- a/crates/bevy_ecs/src/reflect.rs +++ b/crates/bevy_ecs/src/reflect.rs @@ -5,7 +5,10 @@ use crate::{ component::Component, entity::{Entity, EntityMap, MapEntities, MapEntitiesError}, system::Resource, - world::{unsafe_world_cell::UnsafeWorldCell, EntityMut, EntityRef, FromWorld, World}, + world::{ + unsafe_world_cell::{UnsafeWorldCell, UnsafeWorldCellEntityRef}, + EntityMut, EntityRef, FromWorld, World, + }, }; use bevy_reflect::{ impl_from_reflect_value, impl_reflect_value, FromType, Reflect, ReflectDeserialize, @@ -44,20 +47,23 @@ pub struct ReflectComponentFns { /// Function pointer implementing [`ReflectComponent::insert()`]. pub insert: fn(&mut World, Entity, &dyn Reflect), /// Function pointer implementing [`ReflectComponent::apply()`]. - pub apply: fn(EntityMut, &dyn Reflect), + pub apply: fn(&mut EntityMut, &dyn Reflect), /// Function pointer implementing [`ReflectComponent::apply_or_insert()`]. pub apply_or_insert: fn(&mut World, Entity, &dyn Reflect), /// Function pointer implementing [`ReflectComponent::remove()`]. - pub remove: fn(EntityMut), + pub remove: fn(&mut EntityMut), /// Function pointer implementing [`ReflectComponent::contains()`]. - pub contains: fn(EntityRef) -> bool, + pub contains: fn(&EntityRef) -> bool, /// Function pointer implementing [`ReflectComponent::reflect()`]. - pub reflect: fn(EntityRef) -> Option<&dyn Reflect>, + pub reflect: for<'a> fn(&'a EntityRef) -> Option<&'a dyn Reflect>, /// Function pointer implementing [`ReflectComponent::reflect_mut()`]. + pub reflect_mut: for<'a> fn(&'a mut EntityMut) -> Option>, + /// Function pointer implementing [`ReflectComponent::reflect_unchecked_mut()`]. /// /// # Safety - /// The function may only be called with an [`UnsafeWorldCell`] that can be used to mutably access the relevant component on the given entity. - pub reflect_mut: unsafe fn(UnsafeWorldCell<'_>, Entity) -> Option>, + /// The function may only be called with an [`UnsafeWorldCellEntityRef`] that can be used to mutably access the relevant component on the given entity. + pub reflect_unchecked_mut: + unsafe fn(UnsafeWorldCellEntityRef<'_>) -> Option>, /// Function pointer implementing [`ReflectComponent::copy()`]. pub copy: fn(&World, &mut World, Entity, Entity), } @@ -88,7 +94,7 @@ impl ReflectComponent { /// # Panics /// /// Panics if there is no [`Component`] of the given type. - pub fn apply(&self, entity: EntityMut, component: &dyn Reflect) { + pub fn apply(&self, entity: &mut EntityMut, component: &dyn Reflect) { (self.0.apply)(entity, component); } @@ -106,42 +112,37 @@ impl ReflectComponent { /// # Panics /// /// Panics if there is no [`Component`] of the given type. - pub fn remove(&self, entity: EntityMut) { + pub fn remove(&self, entity: &mut EntityMut) { (self.0.remove)(entity); } /// Returns whether entity contains this [`Component`] - pub fn contains(&self, entity: EntityRef) -> bool { + pub fn contains(&self, entity: &EntityRef) -> bool { (self.0.contains)(entity) } /// Gets the value of this [`Component`] type from the entity as a reflected reference. - pub fn reflect<'a>(&self, entity: EntityRef<'a>) -> Option<&'a dyn Reflect> { + pub fn reflect<'a>(&self, entity: &'a EntityRef<'a>) -> Option<&'a dyn Reflect> { (self.0.reflect)(entity) } /// Gets the value of this [`Component`] type from the entity as a mutable reflected reference. - pub fn reflect_mut<'a>( - &self, - world: &'a mut World, - entity: Entity, - ) -> Option> { + pub fn reflect_mut<'a>(&self, entity: &'a mut EntityMut<'a>) -> Option> { // SAFETY: unique world access - unsafe { (self.0.reflect_mut)(world.as_unsafe_world_cell(), entity) } + (self.0.reflect_mut)(entity) } /// # Safety /// This method does not prevent you from having two mutable pointers to the same data, /// violating Rust's aliasing rules. To avoid this: - /// * Only call this method with a [`UnsafeWorldCell`] that may be used to mutably access the component on the entity `entity` + /// * Only call this method with a [`UnsafeWorldCellEntityRef`] that may be used to mutably access the component on the entity `entity` /// * Don't call this method more than once in the same scope for a given [`Component`]. pub unsafe fn reflect_unchecked_mut<'a>( &self, - world: UnsafeWorldCell<'a>, - entity: Entity, + entity: UnsafeWorldCellEntityRef<'a>, ) -> Option> { // SAFETY: safety requirements deferred to caller - (self.0.reflect_mut)(world, entity) + (self.0.reflect_unchecked_mut)(entity) } /// Gets the value of this [`Component`] type from entity from `source_world` and [applies](Self::apply()) it to the value of this [`Component`] type in entity in `destination_world`. @@ -188,7 +189,7 @@ impl FromType for ReflectComponent { component.apply(reflected_component); world.entity_mut(entity).insert(component); }, - apply: |mut entity, reflected_component| { + apply: |entity, reflected_component| { let mut component = entity.get_mut::().unwrap(); component.apply(reflected_component); }, @@ -201,7 +202,7 @@ impl FromType for ReflectComponent { world.entity_mut(entity).insert(component); } }, - remove: |mut entity| { + remove: |entity| { entity.remove::(); }, contains: |entity| entity.contains::(), @@ -214,12 +215,17 @@ impl FromType for ReflectComponent { .insert(destination_component); }, reflect: |entity| entity.get::().map(|c| c as &dyn Reflect), - reflect_mut: |world, entity| { - // SAFETY: reflect_mut is an unsafe function pointer used by - // 1. `reflect_unchecked_mut` which must be called with an UnsafeWorldCell with access to the the component `C` on the `entity`, and - // 2. `reflect_mut`, which has mutable world access + reflect_mut: |entity| { + entity.get_mut::().map(|c| Mut { + value: c.value as &mut dyn Reflect, + ticks: c.ticks, + }) + }, + reflect_unchecked_mut: |entity| { + // SAFETY: reflect_unchecked_mut is an unsafe function pointer used by + // `reflect_unchecked_mut` which must be called with an UnsafeWorldCellEntityReff with access to the the component `C` on the `entity` unsafe { - world.get_entity(entity)?.get_mut::().map(|c| Mut { + entity.get_mut::().map(|c| Mut { value: c.value as &mut dyn Reflect, ticks: c.ticks, }) diff --git a/crates/bevy_scene/src/dynamic_scene_builder.rs b/crates/bevy_scene/src/dynamic_scene_builder.rs index 1187f087b6809..e688887564c04 100644 --- a/crates/bevy_scene/src/dynamic_scene_builder.rs +++ b/crates/bevy_scene/src/dynamic_scene_builder.rs @@ -132,7 +132,7 @@ impl<'w> DynamicSceneBuilder<'w> { .get_info(component_id) .and_then(|info| type_registry.get(info.type_id().unwrap())) .and_then(|registration| registration.data::()) - .and_then(|reflect_component| reflect_component.reflect(entity)); + .and_then(|reflect_component| reflect_component.reflect(&entity)); if let Some(reflect_component) = reflect_component { entry.components.push(reflect_component.clone_value()); From 1648e06470e23fcfd259e2a207add0cc5326c8f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20=C5=81abor?= Date: Thu, 19 Jan 2023 18:31:11 +0100 Subject: [PATCH 3/5] Review items --- crates/bevy_ecs/src/reflect.rs | 8 ++++---- crates/bevy_scene/src/dynamic_scene_builder.rs | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/bevy_ecs/src/reflect.rs b/crates/bevy_ecs/src/reflect.rs index a61d856f04b6e..78d5d0e9deb82 100644 --- a/crates/bevy_ecs/src/reflect.rs +++ b/crates/bevy_ecs/src/reflect.rs @@ -53,9 +53,9 @@ pub struct ReflectComponentFns { /// Function pointer implementing [`ReflectComponent::remove()`]. pub remove: fn(&mut EntityMut), /// Function pointer implementing [`ReflectComponent::contains()`]. - pub contains: fn(&EntityRef) -> bool, + pub contains: fn(EntityRef) -> bool, /// Function pointer implementing [`ReflectComponent::reflect()`]. - pub reflect: for<'a> fn(&'a EntityRef) -> Option<&'a dyn Reflect>, + pub reflect: fn(EntityRef) -> Option<&dyn Reflect>, /// Function pointer implementing [`ReflectComponent::reflect_mut()`]. pub reflect_mut: for<'a> fn(&'a mut EntityMut) -> Option>, /// Function pointer implementing [`ReflectComponent::reflect_unchecked_mut()`]. @@ -117,12 +117,12 @@ impl ReflectComponent { } /// Returns whether entity contains this [`Component`] - pub fn contains(&self, entity: &EntityRef) -> bool { + pub fn contains(&self, entity: EntityRef) -> bool { (self.0.contains)(entity) } /// Gets the value of this [`Component`] type from the entity as a reflected reference. - pub fn reflect<'a>(&self, entity: &'a EntityRef<'a>) -> Option<&'a dyn Reflect> { + pub fn reflect<'a>(&self, entity: EntityRef<'a>) -> Option<&'a dyn Reflect> { (self.0.reflect)(entity) } diff --git a/crates/bevy_scene/src/dynamic_scene_builder.rs b/crates/bevy_scene/src/dynamic_scene_builder.rs index e688887564c04..1187f087b6809 100644 --- a/crates/bevy_scene/src/dynamic_scene_builder.rs +++ b/crates/bevy_scene/src/dynamic_scene_builder.rs @@ -132,7 +132,7 @@ impl<'w> DynamicSceneBuilder<'w> { .get_info(component_id) .and_then(|info| type_registry.get(info.type_id().unwrap())) .and_then(|registration| registration.data::()) - .and_then(|reflect_component| reflect_component.reflect(&entity)); + .and_then(|reflect_component| reflect_component.reflect(entity)); if let Some(reflect_component) = reflect_component { entry.components.push(reflect_component.clone_value()); From f586377e768a0f9af9df51b3fc86570b44247e17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20=C5=81abor?= Date: Fri, 20 Jan 2023 16:14:24 +0100 Subject: [PATCH 4/5] Use entity references everywhere Fix Nit --- crates/bevy_ecs/src/reflect.rs | 36 ++++++++++---------------- crates/bevy_scene/src/dynamic_scene.rs | 3 ++- 2 files changed, 16 insertions(+), 23 deletions(-) diff --git a/crates/bevy_ecs/src/reflect.rs b/crates/bevy_ecs/src/reflect.rs index 78d5d0e9deb82..a5b2034da1109 100644 --- a/crates/bevy_ecs/src/reflect.rs +++ b/crates/bevy_ecs/src/reflect.rs @@ -45,11 +45,11 @@ pub struct ReflectComponent(ReflectComponentFns); #[derive(Clone)] pub struct ReflectComponentFns { /// Function pointer implementing [`ReflectComponent::insert()`]. - pub insert: fn(&mut World, Entity, &dyn Reflect), + pub insert: fn(&mut EntityMut, &dyn Reflect), /// Function pointer implementing [`ReflectComponent::apply()`]. pub apply: fn(&mut EntityMut, &dyn Reflect), /// Function pointer implementing [`ReflectComponent::apply_or_insert()`]. - pub apply_or_insert: fn(&mut World, Entity, &dyn Reflect), + pub apply_or_insert: fn(&mut EntityMut, &dyn Reflect), /// Function pointer implementing [`ReflectComponent::remove()`]. pub remove: fn(&mut EntityMut), /// Function pointer implementing [`ReflectComponent::contains()`]. @@ -57,7 +57,7 @@ pub struct ReflectComponentFns { /// Function pointer implementing [`ReflectComponent::reflect()`]. pub reflect: fn(EntityRef) -> Option<&dyn Reflect>, /// Function pointer implementing [`ReflectComponent::reflect_mut()`]. - pub reflect_mut: for<'a> fn(&'a mut EntityMut) -> Option>, + pub reflect_mut: for<'a> fn(&'a mut EntityMut<'_>) -> Option>, /// Function pointer implementing [`ReflectComponent::reflect_unchecked_mut()`]. /// /// # Safety @@ -81,12 +81,8 @@ impl ReflectComponentFns { impl ReflectComponent { /// Insert a reflected [`Component`] into the entity like [`insert()`](crate::world::EntityMut::insert). - /// - /// # Panics - /// - /// Panics if there is no such entity. - pub fn insert(&self, world: &mut World, entity: Entity, component: &dyn Reflect) { - (self.0.insert)(world, entity, component); + pub fn insert(&self, entity: &mut EntityMut, component: &dyn Reflect) { + (self.0.insert)(entity, component); } /// Uses reflection to set the value of this [`Component`] type in the entity to the given value. @@ -99,12 +95,8 @@ impl ReflectComponent { } /// Uses reflection to set the value of this [`Component`] type in the entity to the given value or insert a new one if it does not exist. - /// - /// # Panics - /// - /// Panics if the `entity` does not exist. - pub fn apply_or_insert(&self, world: &mut World, entity: Entity, component: &dyn Reflect) { - (self.0.apply_or_insert)(world, entity, component); + pub fn apply_or_insert(&self, entity: &mut EntityMut, component: &dyn Reflect) { + (self.0.apply_or_insert)(entity, component); } /// Removes this [`Component`] type from the entity. Does nothing if it doesn't exist. @@ -184,22 +176,22 @@ impl ReflectComponent { impl FromType for ReflectComponent { fn from_type() -> Self { ReflectComponent(ReflectComponentFns { - insert: |world, entity, reflected_component| { - let mut component = C::from_world(world); + insert: |entity, reflected_component| { + let mut component = entity.world_scope(|world| C::from_world(world)); component.apply(reflected_component); - world.entity_mut(entity).insert(component); + entity.insert(component); }, apply: |entity, reflected_component| { let mut component = entity.get_mut::().unwrap(); component.apply(reflected_component); }, - apply_or_insert: |world, entity, reflected_component| { - if let Some(mut component) = world.get_mut::(entity) { + apply_or_insert: |entity, reflected_component| { + if let Some(mut component) = entity.get_mut::() { component.apply(reflected_component); } else { - let mut component = C::from_world(world); + let mut component = entity.world_scope(|world| C::from_world(world)); component.apply(reflected_component); - world.entity_mut(entity).insert(component); + entity.insert(component); } }, remove: |entity| { diff --git a/crates/bevy_scene/src/dynamic_scene.rs b/crates/bevy_scene/src/dynamic_scene.rs index c0eb067b2ca8b..cb5bd1a387fce 100644 --- a/crates/bevy_scene/src/dynamic_scene.rs +++ b/crates/bevy_scene/src/dynamic_scene.rs @@ -71,6 +71,7 @@ impl DynamicScene { let entity = *entity_map .entry(bevy_ecs::entity::Entity::from_raw(scene_entity.entity)) .or_insert_with(|| world.spawn_empty().id()); + let entity_mut = &mut world.entity_mut(entity); // Apply/ add each component to the given entity. for component in &scene_entity.components { @@ -89,7 +90,7 @@ impl DynamicScene { // If the entity already has the given component attached, // just apply the (possibly) new value, otherwise add the // component to the entity. - reflect_component.apply_or_insert(world, entity, &**component); + reflect_component.apply_or_insert(entity_mut, &**component); } } From 6246d1765280a6078818513bcf1bbad7bf4fc197 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20=C5=81abor?= Date: Sun, 29 Jan 2023 00:45:12 +0100 Subject: [PATCH 5/5] Rebase --- crates/bevy_ecs/src/reflect.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/reflect.rs b/crates/bevy_ecs/src/reflect.rs index a5b2034da1109..1b3a2eaf0726b 100644 --- a/crates/bevy_ecs/src/reflect.rs +++ b/crates/bevy_ecs/src/reflect.rs @@ -119,8 +119,7 @@ impl ReflectComponent { } /// Gets the value of this [`Component`] type from the entity as a mutable reflected reference. - pub fn reflect_mut<'a>(&self, entity: &'a mut EntityMut<'a>) -> Option> { - // SAFETY: unique world access + pub fn reflect_mut<'a>(&self, entity: &'a mut EntityMut<'_>) -> Option> { (self.0.reflect_mut)(entity) } @@ -215,7 +214,7 @@ impl FromType for ReflectComponent { }, reflect_unchecked_mut: |entity| { // SAFETY: reflect_unchecked_mut is an unsafe function pointer used by - // `reflect_unchecked_mut` which must be called with an UnsafeWorldCellEntityReff with access to the the component `C` on the `entity` + // `reflect_unchecked_mut` which must be called with an UnsafeWorldCellEntityRef with access to the the component `C` on the `entity` unsafe { entity.get_mut::().map(|c| Mut { value: c.value as &mut dyn Reflect,