From e94367e8fefe7e71c8f72c7844812ed870798574 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sat, 10 Dec 2022 02:22:37 -0800 Subject: [PATCH 1/5] Use T::Storage::STORAGE_TYPE over Components --- crates/bevy_ecs/macros/src/lib.rs | 6 ++++- crates/bevy_ecs/src/bundle.rs | 43 ++++++++++++------------------- 2 files changed, 22 insertions(+), 27 deletions(-) diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index 53121a6c7f883..640ea1e4a173a 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -191,7 +191,11 @@ pub fn derive_bundle(input: TokenStream) -> TokenStream { } #[allow(unused_variables)] - fn get_components(self, func: &mut impl FnMut(#ecs_path::ptr::OwningPtr<'_>)) { + #[inline] + unsafe fn get_components( + self, + func: &mut impl FnMut(#ecs_path::component::StorageType, #ecs_path::ptr::OwningPtr<'_>) + ) { #(#field_get_components)* } } diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index e0f9b341b3553..7ba1554ee8fb4 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -9,7 +9,7 @@ use crate::{ Archetype, ArchetypeId, ArchetypeRow, Archetypes, BundleComponentStatus, ComponentStatus, SpawnBundleStatus, }, - component::{Component, ComponentId, Components, StorageType, Tick}, + component::{Component, ComponentId, Components, ComponentStorage, StorageType, Tick}, entity::{Entities, Entity, EntityLocation}, storage::{SparseSetIndex, SparseSets, Storages, Table, TableRow}, }; @@ -161,8 +161,11 @@ pub unsafe trait Bundle: Send + Sync + 'static { /// Calls `func` on each value, in the order of this bundle's [`Component`]s. This passes /// ownership of the component values to `func`. + /// + /// # Safety + /// The passed in `StorageType` needs to be correct for the provided component #[doc(hidden)] - fn get_components(self, func: &mut impl FnMut(OwningPtr<'_>)); + unsafe fn get_components(self, func: &mut impl FnMut(StorageType, OwningPtr<'_>)); } // SAFETY: @@ -188,8 +191,9 @@ unsafe impl Bundle for C { func(ctx).read() } - fn get_components(self, func: &mut impl FnMut(OwningPtr<'_>)) { - OwningPtr::make(self, func); + #[inline] + unsafe fn get_components(self, func: &mut impl FnMut(StorageType, OwningPtr<'_>)) { + OwningPtr::make(self, |ptr| func(C::Storage::STORAGE_TYPE, ptr)); } } @@ -217,7 +221,8 @@ macro_rules! tuple_impl { } #[allow(unused_variables, unused_mut)] - fn get_components(self, func: &mut impl FnMut(OwningPtr<'_>)) { + #[inline(always)] + unsafe fn get_components(self, func: &mut impl FnMut(StorageType, OwningPtr<'_>)) { #[allow(non_snake_case)] let ($(mut $name,)*) = self; $( @@ -254,7 +259,6 @@ impl SparseSetIndex for BundleId { pub struct BundleInfo { pub(crate) id: BundleId, pub(crate) component_ids: Vec, - pub(crate) storage_types: Vec, } impl BundleInfo { @@ -268,11 +272,6 @@ impl BundleInfo { &self.component_ids } - #[inline] - pub fn storage_types(&self) -> &[StorageType] { - &self.storage_types - } - pub(crate) fn get_bundle_inserter<'a, 'b>( &'b self, entities: &'a mut Entities, @@ -386,9 +385,9 @@ impl BundleInfo { // NOTE: get_components calls this closure on each component in "bundle order". // bundle_info.component_ids are also in "bundle order" let mut bundle_component = 0; - bundle.get_components(&mut |component_ptr| { + bundle.get_components(&mut |storage_type, component_ptr| { let component_id = *self.component_ids.get_unchecked(bundle_component); - match self.storage_types[bundle_component] { + match storage_type { StorageType::Table => { let column = table.get_column_mut(component_id).unwrap(); // SAFETY: bundle_component is a valid index for this bundle @@ -402,8 +401,10 @@ impl BundleInfo { } } StorageType::SparseSet => { - let sparse_set = sparse_sets.get_mut(component_id).unwrap(); - sparse_set.insert(entity, component_ptr, change_tick); + sparse_sets + .get_mut(component_id) + .unwrap() + .insert(entity, component_ptr, change_tick); } } bundle_component += 1; @@ -709,7 +710,7 @@ impl Bundles { let id = BundleId(bundle_infos.len()); // SAFETY: T::component_id ensures info was created let bundle_info = unsafe { - initialize_bundle(std::any::type_name::(), component_ids, id, components) + initialize_bundle(std::any::type_name::(), component_ids, id) }; bundle_infos.push(bundle_info); id @@ -726,16 +727,7 @@ unsafe fn initialize_bundle( bundle_type_name: &'static str, component_ids: Vec, id: BundleId, - components: &mut Components, ) -> BundleInfo { - let mut storage_types = Vec::new(); - - for &component_id in &component_ids { - // SAFETY: component_id exists and is therefore valid - let component_info = components.get_info_unchecked(component_id); - storage_types.push(component_info.storage_type()); - } - let mut deduped = component_ids.clone(); deduped.sort(); deduped.dedup(); @@ -748,6 +740,5 @@ unsafe fn initialize_bundle( BundleInfo { id, component_ids, - storage_types, } } From d1e005b8754ebae0bb1f5e98a729fbe59f912afb Mon Sep 17 00:00:00 2001 From: james7132 Date: Sat, 10 Dec 2022 03:24:20 -0800 Subject: [PATCH 2/5] Formatting --- crates/bevy_ecs/src/bundle.rs | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index 7ba1554ee8fb4..f35b77f3a9d0b 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -9,7 +9,7 @@ use crate::{ Archetype, ArchetypeId, ArchetypeRow, Archetypes, BundleComponentStatus, ComponentStatus, SpawnBundleStatus, }, - component::{Component, ComponentId, Components, ComponentStorage, StorageType, Tick}, + component::{Component, ComponentId, ComponentStorage, Components, StorageType, Tick}, entity::{Entities, Entity, EntityLocation}, storage::{SparseSetIndex, SparseSets, Storages, Table, TableRow}, }; @@ -401,10 +401,11 @@ impl BundleInfo { } } StorageType::SparseSet => { - sparse_sets - .get_mut(component_id) - .unwrap() - .insert(entity, component_ptr, change_tick); + sparse_sets.get_mut(component_id).unwrap().insert( + entity, + component_ptr, + change_tick, + ); } } bundle_component += 1; @@ -708,10 +709,9 @@ impl Bundles { let mut component_ids = Vec::new(); T::component_ids(components, storages, &mut |id| component_ids.push(id)); let id = BundleId(bundle_infos.len()); - // SAFETY: T::component_id ensures info was created - let bundle_info = unsafe { - initialize_bundle(std::any::type_name::(), component_ids, id) - }; + let bundle_info = + // SAFETY: T::component_id ensures info was created + unsafe { initialize_bundle(std::any::type_name::(), component_ids, id) }; bundle_infos.push(bundle_info); id }); @@ -737,8 +737,5 @@ unsafe fn initialize_bundle( bundle_type_name ); - BundleInfo { - id, - component_ids, - } + BundleInfo { id, component_ids } } From 92e456e4dadc962229047f84945d18c6ec6e87e0 Mon Sep 17 00:00:00 2001 From: James Liu Date: Sat, 10 Dec 2022 11:56:35 -0800 Subject: [PATCH 3/5] Formatting the safety comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: François --- crates/bevy_ecs/src/bundle.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index f35b77f3a9d0b..69385535a4d91 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -163,6 +163,7 @@ pub unsafe trait Bundle: Send + Sync + 'static { /// ownership of the component values to `func`. /// /// # Safety + /// /// The passed in `StorageType` needs to be correct for the provided component #[doc(hidden)] unsafe fn get_components(self, func: &mut impl FnMut(StorageType, OwningPtr<'_>)); From 02d0367f5fb080fa5670ca29b7244422519cac59 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sat, 10 Dec 2022 17:13:42 -0800 Subject: [PATCH 4/5] Make get_components safe --- crates/bevy_ecs/macros/src/lib.rs | 7 +++++-- crates/bevy_ecs/src/bundle.rs | 19 ++++++++++--------- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index 640ea1e4a173a..c9be553b0f472 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -170,7 +170,10 @@ pub fn derive_bundle(input: TokenStream) -> TokenStream { let struct_name = &ast.ident; TokenStream::from(quote! { - /// SAFETY: ComponentId is returned in field-definition-order. [from_components] and [get_components] use field-definition-order + // SAFETY: + // - ComponentId is returned in field-definition-order. [from_components] and [get_components] use field-definition-order + // - `Bundle::get_components` is exactly once for each member. Rely's on the Component -> Bundle implementation to properly pass + // the correct `StorageType` into the callback. unsafe impl #impl_generics #ecs_path::bundle::Bundle for #struct_name #ty_generics #where_clause { fn component_ids( components: &mut #ecs_path::component::Components, @@ -192,7 +195,7 @@ pub fn derive_bundle(input: TokenStream) -> TokenStream { #[allow(unused_variables)] #[inline] - unsafe fn get_components( + fn get_components( self, func: &mut impl FnMut(#ecs_path::component::StorageType, #ecs_path::ptr::OwningPtr<'_>) ) { diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index 69385535a4d91..0b2d28127188d 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -130,7 +130,6 @@ use std::{any::TypeId, collections::HashMap}; /// That is, there is no safe way to implement this trait, and you must not do so. /// If you want a type to implement [`Bundle`], you must use [`derive@Bundle`](derive@Bundle). /// -/// /// [`Query`]: crate::system::Query // Some safety points: // - [`Bundle::component_ids`] must return the [`ComponentId`] for each component type in the @@ -159,19 +158,19 @@ pub unsafe trait Bundle: Send + Sync + 'static { F: for<'a> FnMut(&'a mut T) -> OwningPtr<'a>, Self: Sized; + // SAFETY: + // The `StorageType` argument passed into [`Bundle::get_components`] must be correct for the + // component being fetched. + // /// Calls `func` on each value, in the order of this bundle's [`Component`]s. This passes /// ownership of the component values to `func`. - /// - /// # Safety - /// - /// The passed in `StorageType` needs to be correct for the provided component #[doc(hidden)] - unsafe fn get_components(self, func: &mut impl FnMut(StorageType, OwningPtr<'_>)); + fn get_components(self, func: &mut impl FnMut(StorageType, OwningPtr<'_>)); } // SAFETY: // - `Bundle::component_ids` calls `ids` for C's component id (and nothing else) -// - `Bundle::get_components` is called exactly once for C. +// - `Bundle::get_components` is called exactly once for C and passes the component's storage type based on it's associated constant. // - `Bundle::from_components` calls `func` exactly once for C, which is the exact value returned by `Bundle::component_ids`. unsafe impl Bundle for C { fn component_ids( @@ -193,7 +192,7 @@ unsafe impl Bundle for C { } #[inline] - unsafe fn get_components(self, func: &mut impl FnMut(StorageType, OwningPtr<'_>)) { + fn get_components(self, func: &mut impl FnMut(StorageType, OwningPtr<'_>)) { OwningPtr::make(self, |ptr| func(C::Storage::STORAGE_TYPE, ptr)); } } @@ -204,6 +203,8 @@ macro_rules! tuple_impl { // - `Bundle::component_ids` calls `ids` for each component type in the // bundle, in the exact order that `Bundle::get_components` is called. // - `Bundle::from_components` calls `func` exactly once for each `ComponentId` returned by `Bundle::component_ids`. + // - `Bundle::get_components` is exactly once for each member. Rely's on the above implementation to pass the correct + // `StorageType` into the callback. unsafe impl<$($name: Bundle),*> Bundle for ($($name,)*) { #[allow(unused_variables)] fn component_ids(components: &mut Components, storages: &mut Storages, ids: &mut impl FnMut(ComponentId)){ @@ -223,7 +224,7 @@ macro_rules! tuple_impl { #[allow(unused_variables, unused_mut)] #[inline(always)] - unsafe fn get_components(self, func: &mut impl FnMut(StorageType, OwningPtr<'_>)) { + fn get_components(self, func: &mut impl FnMut(StorageType, OwningPtr<'_>)) { #[allow(non_snake_case)] let ($(mut $name,)*) = self; $( From 22aae4ac9d65c099962bfcda66988ad2cfa15c54 Mon Sep 17 00:00:00 2001 From: James Liu Date: Sat, 10 Dec 2022 21:43:09 -0800 Subject: [PATCH 5/5] Fix typo. Co-authored-by: Alice Cecile --- crates/bevy_ecs/src/bundle.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index 0b2d28127188d..3236929f50972 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -203,7 +203,7 @@ macro_rules! tuple_impl { // - `Bundle::component_ids` calls `ids` for each component type in the // bundle, in the exact order that `Bundle::get_components` is called. // - `Bundle::from_components` calls `func` exactly once for each `ComponentId` returned by `Bundle::component_ids`. - // - `Bundle::get_components` is exactly once for each member. Rely's on the above implementation to pass the correct + // - `Bundle::get_components` is called exactly once for each member. Relies on the above implementation to pass the correct // `StorageType` into the callback. unsafe impl<$($name: Bundle),*> Bundle for ($($name,)*) { #[allow(unused_variables)]