From 8bcdc165da3703db8862637ca3ccdcdbc7646a50 Mon Sep 17 00:00:00 2001 From: bjorn3 Date: Fri, 16 Jul 2021 15:15:42 +0200 Subject: [PATCH 1/8] Use ComponentDescriptor inside ComponentInfo --- crates/bevy_ecs/src/component/mod.rs | 32 ++++++++-------------------- 1 file changed, 9 insertions(+), 23 deletions(-) diff --git a/crates/bevy_ecs/src/component/mod.rs b/crates/bevy_ecs/src/component/mod.rs index ccb8a6f993e6d..e925d5afd7a8c 100644 --- a/crates/bevy_ecs/src/component/mod.rs +++ b/crates/bevy_ecs/src/component/mod.rs @@ -56,15 +56,8 @@ impl Default for StorageType { #[derive(Debug)] pub struct ComponentInfo { - name: String, id: ComponentId, - type_id: Option, - // SAFETY: This must remain private. It must only be set to "true" if this component is - // actually Send + Sync - is_send_and_sync: bool, - layout: Layout, - drop: unsafe fn(*mut u8), - storage_type: StorageType, + descriptor: ComponentDescriptor, } impl ComponentInfo { @@ -75,44 +68,36 @@ impl ComponentInfo { #[inline] pub fn name(&self) -> &str { - &self.name + &self.descriptor.name } #[inline] pub fn type_id(&self) -> Option { - self.type_id + self.descriptor.type_id } #[inline] pub fn layout(&self) -> Layout { - self.layout + self.descriptor.layout } #[inline] pub fn drop(&self) -> unsafe fn(*mut u8) { - self.drop + self.descriptor.drop } #[inline] pub fn storage_type(&self) -> StorageType { - self.storage_type + self.descriptor.storage_type } #[inline] pub fn is_send_and_sync(&self) -> bool { - self.is_send_and_sync + self.descriptor.is_send_and_sync } fn new(id: ComponentId, descriptor: ComponentDescriptor) -> Self { - ComponentInfo { - id, - name: descriptor.name, - storage_type: descriptor.storage_type, - type_id: descriptor.type_id, - is_send_and_sync: descriptor.is_send_and_sync, - drop: descriptor.drop, - layout: descriptor.layout, - } + ComponentInfo { id, descriptor } } } @@ -142,6 +127,7 @@ impl SparseSetIndex for ComponentId { } } +#[derive(Debug)] pub struct ComponentDescriptor { name: String, storage_type: StorageType, From b3993caf8b2f200247925d8fd95c320b28186ef2 Mon Sep 17 00:00:00 2001 From: bjorn3 Date: Fri, 16 Jul 2021 15:19:53 +0200 Subject: [PATCH 2/8] Add safety comment to TypeInfo --- crates/bevy_ecs/src/component/type_info.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/bevy_ecs/src/component/type_info.rs b/crates/bevy_ecs/src/component/type_info.rs index 58839632f438f..7cb096a1fb209 100644 --- a/crates/bevy_ecs/src/component/type_info.rs +++ b/crates/bevy_ecs/src/component/type_info.rs @@ -7,6 +7,8 @@ pub struct TypeInfo { layout: Layout, drop: unsafe fn(*mut u8), type_name: &'static str, + // SAFETY: This must remain private. It must only be set to "true" if this type is actually + // Send + Sync is_send_and_sync: bool, } From 12fcaf63db09111f9c13b8b86783fd9193ebaf78 Mon Sep 17 00:00:00 2001 From: bjorn3 Date: Fri, 16 Jul 2021 16:52:01 +0200 Subject: [PATCH 3/8] Make Bundle work with ComponentId instead of TypeInfo This allows it to work with dynamic or untyped components if we get any. It would also be useful for the relationships PR. --- crates/bevy_ecs/macros/src/lib.rs | 26 ++++++++++++------ crates/bevy_ecs/src/bundle.rs | 41 ++++++++++++++++------------ crates/bevy_ecs/src/lib.rs | 29 ++++++++++++-------- crates/bevy_ecs/src/storage/table.rs | 1 + 4 files changed, 59 insertions(+), 38 deletions(-) diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index 27383ff71ee55..cf9c5a50509b4 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -110,15 +110,15 @@ pub fn derive_bundle(input: TokenStream) -> TokenStream { .map(|field| &field.ty) .collect::>(); - let mut field_type_infos = Vec::new(); + let mut field_component_ids = Vec::new(); let mut field_get_components = Vec::new(); let mut field_from_components = Vec::new(); for ((field_type, is_bundle), field) in field_type.iter().zip(is_bundle.iter()).zip(field.iter()) { if *is_bundle { - field_type_infos.push(quote! { - type_info.extend(<#field_type as #ecs_path::bundle::Bundle>::type_info()); + field_component_ids.push(quote! { + component_id.extend(<#field_type as #ecs_path::bundle::Bundle>::component_id(components)); }); field_get_components.push(quote! { self.#field.get_components(&mut func); @@ -127,8 +127,14 @@ pub fn derive_bundle(input: TokenStream) -> TokenStream { #field: <#field_type as #ecs_path::bundle::Bundle>::from_components(&mut func), }); } else { - field_type_infos.push(quote! { - type_info.push(#ecs_path::component::TypeInfo::of::<#field_type>()); + field_component_ids.push(quote! { + // SAFE: The [`TypeInfo`] matches the [`TypeId`] + component_id.push(unsafe { + components.get_or_insert_with( + std::any::TypeId::of::<#field_type>(), + || #ecs_path::component::TypeInfo::of::<#field_type>(), + ) + }); }); field_get_components.push(quote! { func((&mut self.#field as *mut #field_type).cast::()); @@ -147,10 +153,12 @@ pub fn derive_bundle(input: TokenStream) -> TokenStream { TokenStream::from(quote! { /// SAFE: TypeInfo is returned in field-definition-order. [from_components] and [get_components] use field-definition-order unsafe impl #impl_generics #ecs_path::bundle::Bundle for #struct_name#ty_generics #where_clause { - fn type_info() -> Vec<#ecs_path::component::TypeInfo> { - let mut type_info = Vec::with_capacity(#field_len); - #(#field_type_infos)* - type_info + fn component_id( + components: &mut #ecs_path::component::Components, + ) -> Vec<#ecs_path::component::ComponentId> { + let mut component_id = Vec::with_capacity(#field_len); + #(#field_component_ids)* + component_id } #[allow(unused_variables, unused_mut, non_snake_case)] diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index 00f3bac9e527e..c282345d9f287 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -38,13 +38,13 @@ use std::{any::TypeId, collections::HashMap}; /// ``` /// /// # Safety -/// [Bundle::type_info] must return the TypeInfo for each component type in the bundle, in the +/// [Bundle::component_id] must return the ComponentId for each component type in the bundle, in the /// _exact_ order that [Bundle::get_components] is called. -/// [Bundle::from_components] must call `func` exactly once for each [TypeInfo] returned by -/// [Bundle::type_info] +/// [Bundle::from_components] must call `func` exactly once for each [ComponentId] returned by +/// [Bundle::component_id] pub unsafe trait Bundle: Send + Sync + 'static { - /// Gets this [Bundle]'s components type info, in the order of this bundle's Components - fn type_info() -> Vec; + /// Gets this [Bundle]'s component ids, in the order of this bundle's Components + fn component_id(components: &mut Components) -> Vec; /// Calls `func`, which should return data for each component in the bundle, in the order of /// this bundle's Components @@ -66,8 +66,11 @@ macro_rules! tuple_impl { ($($name: ident),*) => { /// SAFE: TypeInfo is returned in tuple-order. [Bundle::from_components] and [Bundle::get_components] use tuple-order unsafe impl<$($name: Component),*> Bundle for ($($name,)*) { - fn type_info() -> Vec { - vec![$(TypeInfo::of::<$name>()),*] + #[allow(unused_variables)] + fn component_id(components: &mut Components) -> Vec { + vec![$( + components.get_or_insert_with(TypeId::of::<$name>(), || TypeInfo::of::<$name>()) + ),*] } #[allow(unused_variables, unused_mut)] @@ -205,10 +208,12 @@ impl Bundles { ) -> &'a BundleInfo { let bundle_infos = &mut self.bundle_infos; let id = self.bundle_ids.entry(TypeId::of::()).or_insert_with(|| { - let type_info = T::type_info(); + let component_id = T::component_id(components); let id = BundleId(bundle_infos.len()); - let bundle_info = - initialize_bundle(std::any::type_name::(), &type_info, id, components); + // SAFE: T::component_id ensures info was created + let bundle_info = unsafe { + initialize_bundle(std::any::type_name::(), &component_id, id, components) + }; bundle_infos.push(bundle_info); id }); @@ -217,21 +222,23 @@ impl Bundles { } } -fn initialize_bundle( +/// # Safety +/// +/// `component_id` must be valid [ComponentId]'s +unsafe fn initialize_bundle( bundle_type_name: &'static str, - type_info: &[TypeInfo], + component_id: &[ComponentId], id: BundleId, components: &mut Components, ) -> BundleInfo { let mut component_ids = Vec::new(); let mut storage_types = Vec::new(); - for type_info in type_info { - let component_id = components.get_or_insert_with(type_info.type_id(), || type_info.clone()); - // SAFE: get_with_type_info ensures info was created - let info = unsafe { components.get_info_unchecked(component_id) }; + for &component_id in component_id { + // SAFE: component_id exists and is therefore valid + let component_info = components.get_info_unchecked(component_id); component_ids.push(component_id); - storage_types.push(info.storage_type()); + storage_types.push(component_info.storage_type()); } let mut deduped = component_ids.clone(); diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index d95a15bd0804b..75aeb3ca5b112 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -41,7 +41,7 @@ mod tests { use crate as bevy_ecs; use crate::{ bundle::Bundle, - component::{Component, ComponentDescriptor, ComponentId, StorageType, TypeInfo}, + component::{Component, ComponentDescriptor, ComponentId, StorageType}, entity::Entity, query::{ Added, ChangeTrackers, Changed, FilterFetch, FilteredAccess, With, Without, WorldQuery, @@ -102,21 +102,26 @@ mod tests { #[test] fn bundle_derive() { + let mut world = World::new(); + #[derive(Bundle, PartialEq, Debug)] struct Foo { x: &'static str, y: i32, } - assert_eq!( - ::type_info(), - vec![TypeInfo::of::<&'static str>(), TypeInfo::of::(),] - ); - - let mut world = World::new(); world .register_component(ComponentDescriptor::new::(StorageType::SparseSet)) .unwrap(); + + assert_eq!( + ::component_id(world.components_mut()), + vec![ + world.components_mut().get_or_insert_id::<&'static str>(), + world.components_mut().get_or_insert_id::(), + ] + ); + let e1 = world.spawn().insert_bundle(Foo { x: "abc", y: 123 }).id(); let e2 = world.spawn().insert_bundle(("def", 456, true)).id(); assert_eq!(*world.get::<&str>(e1).unwrap(), "abc"); @@ -146,12 +151,12 @@ mod tests { } assert_eq!( - ::type_info(), + ::component_id(world.components_mut()), vec![ - TypeInfo::of::(), - TypeInfo::of::<&'static str>(), - TypeInfo::of::(), - TypeInfo::of::(), + world.components_mut().get_or_insert_id::(), + world.components_mut().get_or_insert_id::<&'static str>(), + world.components_mut().get_or_insert_id::(), + world.components_mut().get_or_insert_id::(), ] ); diff --git a/crates/bevy_ecs/src/storage/table.rs b/crates/bevy_ecs/src/storage/table.rs index 8fc334b56975a..6d06b1b377e83 100644 --- a/crates/bevy_ecs/src/storage/table.rs +++ b/crates/bevy_ecs/src/storage/table.rs @@ -505,6 +505,7 @@ mod tests { entity::Entity, storage::Table, }; + use std::any::TypeId; #[test] fn table() { From 222d674080058bffd82f4e9e4f593839c6077fa1 Mon Sep 17 00:00:00 2001 From: bjorn3 Date: Fri, 16 Jul 2021 17:10:59 +0200 Subject: [PATCH 4/8] Remove type_id from TypeInfo This allow TypeInfo to exist for non-rust types. It is not yet possible to construct such a TypeInfo. --- crates/bevy_ecs/src/bundle.rs | 5 +- crates/bevy_ecs/src/component/mod.rs | 70 +++++++++++++++------- crates/bevy_ecs/src/component/type_info.rs | 10 +--- crates/bevy_ecs/src/storage/table.rs | 4 +- 4 files changed, 58 insertions(+), 31 deletions(-) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index c282345d9f287..09bbdca384b26 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -69,7 +69,10 @@ macro_rules! tuple_impl { #[allow(unused_variables)] fn component_id(components: &mut Components) -> Vec { vec![$( - components.get_or_insert_with(TypeId::of::<$name>(), || TypeInfo::of::<$name>()) + // SAFE: The [`TypeInfo`] matches the [`TypeId`] + unsafe { + components.get_or_insert_with(TypeId::of::<$name>(), || TypeInfo::of::<$name>()) + } ),*] } diff --git a/crates/bevy_ecs/src/component/mod.rs b/crates/bevy_ecs/src/component/mod.rs index e925d5afd7a8c..630519065f1a2 100644 --- a/crates/bevy_ecs/src/component/mod.rs +++ b/crates/bevy_ecs/src/component/mod.rs @@ -151,6 +151,24 @@ impl ComponentDescriptor { } } + /// # Safety + /// + /// The [`TypeInfo`] must match the [`TypeId`] + pub unsafe fn from_type_info( + storage_type: StorageType, + type_id: Option, + type_info: TypeInfo, + ) -> Self { + Self { + name: type_info.type_name().to_string(), + storage_type, + is_send_and_sync: type_info.is_send_and_sync(), + type_id, + layout: type_info.layout(), + drop: type_info.drop(), + } + } + #[inline] pub fn storage_type(&self) -> StorageType { self.storage_type @@ -167,19 +185,6 @@ impl ComponentDescriptor { } } -impl From for ComponentDescriptor { - fn from(type_info: TypeInfo) -> Self { - Self { - name: type_info.type_name().to_string(), - storage_type: StorageType::default(), - is_send_and_sync: type_info.is_send_and_sync(), - type_id: Some(type_info.type_id()), - drop: type_info.drop(), - layout: type_info.layout(), - } - } -} - #[derive(Debug, Default)] pub struct Components { components: Vec, @@ -217,7 +222,8 @@ impl Components { #[inline] pub fn get_or_insert_id(&mut self) -> ComponentId { - self.get_or_insert_with(TypeId::of::(), TypeInfo::of::) + // SAFE: The [`TypeInfo`] matches the [`TypeId`] + unsafe { self.get_or_insert_with(TypeId::of::(), TypeInfo::of::) } } #[inline] @@ -265,16 +271,23 @@ impl Components { #[inline] pub fn get_or_insert_resource_id(&mut self) -> ComponentId { - self.get_or_insert_resource_with(TypeId::of::(), TypeInfo::of::) + // SAFE: The [`TypeInfo`] matches the [`TypeId`] + unsafe { self.get_or_insert_resource_with(TypeId::of::(), TypeInfo::of::) } } #[inline] pub fn get_or_insert_non_send_resource_id(&mut self) -> ComponentId { - self.get_or_insert_resource_with(TypeId::of::(), TypeInfo::of_non_send_and_sync::) + // SAFE: The [`TypeInfo`] matches the [`TypeId`] + unsafe { + self.get_or_insert_resource_with(TypeId::of::(), TypeInfo::of_non_send_and_sync::) + } } + /// # Safety + /// + /// The [`TypeInfo`] must match the [`TypeId`] #[inline] - fn get_or_insert_resource_with( + unsafe fn get_or_insert_resource_with( &mut self, type_id: TypeId, func: impl FnOnce() -> TypeInfo, @@ -283,15 +296,25 @@ impl Components { let index = self.resource_indices.entry(type_id).or_insert_with(|| { let type_info = func(); let index = components.len(); - components.push(ComponentInfo::new(ComponentId(index), type_info.into())); + components.push(ComponentInfo::new( + ComponentId(index), + ComponentDescriptor::from_type_info( + StorageType::default(), + Some(type_id), + type_info, + ), + )); index }); ComponentId(*index) } + /// # Safety + /// + /// The [`TypeInfo`] must match the [`TypeId`] #[inline] - pub(crate) fn get_or_insert_with( + pub(crate) unsafe fn get_or_insert_with( &mut self, type_id: TypeId, func: impl FnOnce() -> TypeInfo, @@ -300,7 +323,14 @@ impl Components { let index = self.indices.entry(type_id).or_insert_with(|| { let type_info = func(); let index = components.len(); - components.push(ComponentInfo::new(ComponentId(index), type_info.into())); + components.push(ComponentInfo::new( + ComponentId(index), + ComponentDescriptor::from_type_info( + StorageType::default(), + Some(type_id), + type_info, + ), + )); index }); diff --git a/crates/bevy_ecs/src/component/type_info.rs b/crates/bevy_ecs/src/component/type_info.rs index 7cb096a1fb209..5a0aed233e220 100644 --- a/crates/bevy_ecs/src/component/type_info.rs +++ b/crates/bevy_ecs/src/component/type_info.rs @@ -1,9 +1,8 @@ -use std::{alloc::Layout, any::TypeId}; +use std::alloc::Layout; /// Metadata required to store a component. #[derive(Debug, Clone, PartialEq, Eq)] pub struct TypeInfo { - type_id: TypeId, layout: Layout, drop: unsafe fn(*mut u8), type_name: &'static str, @@ -16,7 +15,6 @@ impl TypeInfo { /// Metadata for `T`. pub fn of() -> Self { Self { - type_id: TypeId::of::(), layout: Layout::new::(), is_send_and_sync: true, drop: Self::drop_ptr::, @@ -26,7 +24,6 @@ impl TypeInfo { pub fn of_non_send_and_sync() -> Self { Self { - type_id: TypeId::of::(), layout: Layout::new::(), is_send_and_sync: false, drop: Self::drop_ptr::, @@ -34,11 +31,6 @@ impl TypeInfo { } } - #[inline] - pub fn type_id(&self) -> TypeId { - self.type_id - } - #[inline] pub fn layout(&self) -> Layout { self.layout diff --git a/crates/bevy_ecs/src/storage/table.rs b/crates/bevy_ecs/src/storage/table.rs index 6d06b1b377e83..3cf6cb9303da3 100644 --- a/crates/bevy_ecs/src/storage/table.rs +++ b/crates/bevy_ecs/src/storage/table.rs @@ -511,7 +511,9 @@ mod tests { fn table() { let mut components = Components::default(); let type_info = TypeInfo::of::(); - let component_id = components.get_or_insert_with(type_info.type_id(), || type_info); + // SAFE: The [`TypeInfo`] matches the [`TypeId`] + let component_id = + unsafe { components.get_or_insert_with(TypeId::of::(), || type_info) }; let columns = &[component_id]; let mut table = Table::with_capacity(0, columns.len()); table.add_column(components.get_info(component_id).unwrap()); From e222fbde6c9123c9bc89d738d668f7deea4476d8 Mon Sep 17 00:00:00 2001 From: bjorn3 Date: Fri, 16 Jul 2021 18:18:22 +0200 Subject: [PATCH 5/8] Don't use TypeInfo::drop_ptr in BlobVec tests --- crates/bevy_ecs/src/component/type_info.rs | 2 +- crates/bevy_ecs/src/storage/blob_vec.rs | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/crates/bevy_ecs/src/component/type_info.rs b/crates/bevy_ecs/src/component/type_info.rs index 5a0aed233e220..f745044260bd1 100644 --- a/crates/bevy_ecs/src/component/type_info.rs +++ b/crates/bevy_ecs/src/component/type_info.rs @@ -51,7 +51,7 @@ impl TypeInfo { self.type_name } - pub(crate) unsafe fn drop_ptr(x: *mut u8) { + pub(super) unsafe fn drop_ptr(x: *mut u8) { x.cast::().drop_in_place() } } diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs index d724728c177b2..49c92d92d1eb9 100644 --- a/crates/bevy_ecs/src/storage/blob_vec.rs +++ b/crates/bevy_ecs/src/storage/blob_vec.rs @@ -267,9 +267,13 @@ const fn padding_needed_for(layout: &Layout, align: usize) -> usize { #[cfg(test)] mod tests { use super::BlobVec; - use crate::component::TypeInfo; use std::{alloc::Layout, cell::RefCell, rc::Rc}; + // SAFETY: The pointer points to a valid value of type `T` and it is safe to drop this value. + unsafe fn drop_ptr(x: *mut u8) { + x.cast::().drop_in_place() + } + /// # Safety /// /// `blob_vec` must have a layout that matches Layout::new::() @@ -300,7 +304,7 @@ mod tests { #[test] fn resize_test() { let item_layout = Layout::new::(); - let drop = TypeInfo::drop_ptr::; + let drop = drop_ptr::; let mut blob_vec = BlobVec::new(item_layout, drop, 64); unsafe { for i in 0..1_000 { @@ -330,7 +334,7 @@ mod tests { let drop_counter = Rc::new(RefCell::new(0)); { let item_layout = Layout::new::(); - let drop = TypeInfo::drop_ptr::; + let drop = drop_ptr::; let mut blob_vec = BlobVec::new(item_layout, drop, 2); assert_eq!(blob_vec.capacity(), 2); unsafe { @@ -390,7 +394,7 @@ mod tests { #[test] fn blob_vec_drop_empty_capacity() { let item_layout = Layout::new::(); - let drop = TypeInfo::drop_ptr::; + let drop = drop_ptr::; let _ = BlobVec::new(item_layout, drop, 0); } } From 721071dc1410a3cc7eb96afebcfdd55f747a83e0 Mon Sep 17 00:00:00 2001 From: bjorn3 Date: Fri, 16 Jul 2021 18:22:16 +0200 Subject: [PATCH 6/8] Use get_or_insert_id instead of get_or_insert_with where possible --- crates/bevy_ecs/macros/src/lib.rs | 8 +------- crates/bevy_ecs/src/bundle.rs | 9 ++------- crates/bevy_ecs/src/storage/table.rs | 12 ++---------- 3 files changed, 5 insertions(+), 24 deletions(-) diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index cf9c5a50509b4..eeeff34c65242 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -128,13 +128,7 @@ pub fn derive_bundle(input: TokenStream) -> TokenStream { }); } else { field_component_ids.push(quote! { - // SAFE: The [`TypeInfo`] matches the [`TypeId`] - component_id.push(unsafe { - components.get_or_insert_with( - std::any::TypeId::of::<#field_type>(), - || #ecs_path::component::TypeInfo::of::<#field_type>(), - ) - }); + component_id.push(components.get_or_insert_id::<#field_type>()); }); field_get_components.push(quote! { func((&mut self.#field as *mut #field_type).cast::()); diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index 09bbdca384b26..dd284773717b9 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -2,7 +2,7 @@ pub use bevy_ecs_macros::Bundle; use crate::{ archetype::ComponentStatus, - component::{Component, ComponentId, ComponentTicks, Components, StorageType, TypeInfo}, + component::{Component, ComponentId, ComponentTicks, Components, StorageType}, entity::Entity, storage::{SparseSetIndex, SparseSets, Table}, }; @@ -68,12 +68,7 @@ macro_rules! tuple_impl { unsafe impl<$($name: Component),*> Bundle for ($($name,)*) { #[allow(unused_variables)] fn component_id(components: &mut Components) -> Vec { - vec![$( - // SAFE: The [`TypeInfo`] matches the [`TypeId`] - unsafe { - components.get_or_insert_with(TypeId::of::<$name>(), || TypeInfo::of::<$name>()) - } - ),*] + vec![$(components.get_or_insert_id::<$name>()),*] } #[allow(unused_variables, unused_mut)] diff --git a/crates/bevy_ecs/src/storage/table.rs b/crates/bevy_ecs/src/storage/table.rs index 3cf6cb9303da3..3b015076132ec 100644 --- a/crates/bevy_ecs/src/storage/table.rs +++ b/crates/bevy_ecs/src/storage/table.rs @@ -500,20 +500,12 @@ impl IndexMut for Tables { #[cfg(test)] mod tests { - use crate::{ - component::{Components, TypeInfo}, - entity::Entity, - storage::Table, - }; - use std::any::TypeId; + use crate::{component::Components, entity::Entity, storage::Table}; #[test] fn table() { let mut components = Components::default(); - let type_info = TypeInfo::of::(); - // SAFE: The [`TypeInfo`] matches the [`TypeId`] - let component_id = - unsafe { components.get_or_insert_with(TypeId::of::(), || type_info) }; + let component_id = components.get_or_insert_id::(); let columns = &[component_id]; let mut table = Table::with_capacity(0, columns.len()); table.add_column(components.get_info(component_id).unwrap()); From db6db967e950dcf41d10bb802479ea84e716deb3 Mon Sep 17 00:00:00 2001 From: bjorn3 Date: Fri, 16 Jul 2021 18:33:11 +0200 Subject: [PATCH 7/8] Fold TypeInfo into ComponentDescriptor --- .../src/{component/mod.rs => component.rs} | 82 ++++++++----------- crates/bevy_ecs/src/component/type_info.rs | 57 ------------- 2 files changed, 36 insertions(+), 103 deletions(-) rename crates/bevy_ecs/src/{component/mod.rs => component.rs} (85%) delete mode 100644 crates/bevy_ecs/src/component/type_info.rs diff --git a/crates/bevy_ecs/src/component/mod.rs b/crates/bevy_ecs/src/component.rs similarity index 85% rename from crates/bevy_ecs/src/component/mod.rs rename to crates/bevy_ecs/src/component.rs index 630519065f1a2..6e71a1a7d9328 100644 --- a/crates/bevy_ecs/src/component/mod.rs +++ b/crates/bevy_ecs/src/component.rs @@ -1,7 +1,3 @@ -mod type_info; - -pub use type_info::*; - use crate::storage::SparseSetIndex; use std::{ alloc::Layout, @@ -140,6 +136,11 @@ pub struct ComponentDescriptor { } impl ComponentDescriptor { + // SAFETY: The pointer points to a valid value of type `T` and it is safe to drop this value. + unsafe fn drop_ptr(x: *mut u8) { + x.cast::().drop_in_place() + } + pub fn new(storage_type: StorageType) -> Self { Self { name: std::any::type_name::().to_string(), @@ -147,25 +148,18 @@ impl ComponentDescriptor { is_send_and_sync: true, type_id: Some(TypeId::of::()), layout: Layout::new::(), - drop: TypeInfo::drop_ptr::, + drop: Self::drop_ptr::, } } - /// # Safety - /// - /// The [`TypeInfo`] must match the [`TypeId`] - pub unsafe fn from_type_info( - storage_type: StorageType, - type_id: Option, - type_info: TypeInfo, - ) -> Self { + fn new_non_send(storage_type: StorageType) -> Self { Self { - name: type_info.type_name().to_string(), + name: std::any::type_name::().to_string(), storage_type, - is_send_and_sync: type_info.is_send_and_sync(), - type_id, - layout: type_info.layout(), - drop: type_info.drop(), + is_send_and_sync: false, + type_id: Some(TypeId::of::()), + layout: Layout::new::(), + drop: Self::drop_ptr::, } } @@ -222,8 +216,12 @@ impl Components { #[inline] pub fn get_or_insert_id(&mut self) -> ComponentId { - // SAFE: The [`TypeInfo`] matches the [`TypeId`] - unsafe { self.get_or_insert_with(TypeId::of::(), TypeInfo::of::) } + // SAFE: The [`ComponentDescriptor`] matches the [`TypeId`] + unsafe { + self.get_or_insert_with(TypeId::of::(), || { + ComponentDescriptor::new::(StorageType::default()) + }) + } } #[inline] @@ -271,39 +269,38 @@ impl Components { #[inline] pub fn get_or_insert_resource_id(&mut self) -> ComponentId { - // SAFE: The [`TypeInfo`] matches the [`TypeId`] - unsafe { self.get_or_insert_resource_with(TypeId::of::(), TypeInfo::of::) } + // SAFE: The [`ComponentDescriptor`] matches the [`TypeId`] + unsafe { + self.get_or_insert_resource_with(TypeId::of::(), || { + ComponentDescriptor::new::(StorageType::default()) + }) + } } #[inline] pub fn get_or_insert_non_send_resource_id(&mut self) -> ComponentId { - // SAFE: The [`TypeInfo`] matches the [`TypeId`] + // SAFE: The [`ComponentDescriptor`] matches the [`TypeId`] unsafe { - self.get_or_insert_resource_with(TypeId::of::(), TypeInfo::of_non_send_and_sync::) + self.get_or_insert_resource_with(TypeId::of::(), || { + ComponentDescriptor::new_non_send::(StorageType::default()) + }) } } /// # Safety /// - /// The [`TypeInfo`] must match the [`TypeId`] + /// The [`ComponentDescriptor`] must match the [`TypeId`] #[inline] unsafe fn get_or_insert_resource_with( &mut self, type_id: TypeId, - func: impl FnOnce() -> TypeInfo, + func: impl FnOnce() -> ComponentDescriptor, ) -> ComponentId { let components = &mut self.components; let index = self.resource_indices.entry(type_id).or_insert_with(|| { - let type_info = func(); + let descriptor = func(); let index = components.len(); - components.push(ComponentInfo::new( - ComponentId(index), - ComponentDescriptor::from_type_info( - StorageType::default(), - Some(type_id), - type_info, - ), - )); + components.push(ComponentInfo::new(ComponentId(index), descriptor)); index }); @@ -312,25 +309,18 @@ impl Components { /// # Safety /// - /// The [`TypeInfo`] must match the [`TypeId`] + /// The [`ComponentDescriptor`] must match the [`TypeId`] #[inline] pub(crate) unsafe fn get_or_insert_with( &mut self, type_id: TypeId, - func: impl FnOnce() -> TypeInfo, + func: impl FnOnce() -> ComponentDescriptor, ) -> ComponentId { let components = &mut self.components; let index = self.indices.entry(type_id).or_insert_with(|| { - let type_info = func(); + let descriptor = func(); let index = components.len(); - components.push(ComponentInfo::new( - ComponentId(index), - ComponentDescriptor::from_type_info( - StorageType::default(), - Some(type_id), - type_info, - ), - )); + components.push(ComponentInfo::new(ComponentId(index), descriptor)); index }); diff --git a/crates/bevy_ecs/src/component/type_info.rs b/crates/bevy_ecs/src/component/type_info.rs deleted file mode 100644 index f745044260bd1..0000000000000 --- a/crates/bevy_ecs/src/component/type_info.rs +++ /dev/null @@ -1,57 +0,0 @@ -use std::alloc::Layout; - -/// Metadata required to store a component. -#[derive(Debug, Clone, PartialEq, Eq)] -pub struct TypeInfo { - layout: Layout, - drop: unsafe fn(*mut u8), - type_name: &'static str, - // SAFETY: This must remain private. It must only be set to "true" if this type is actually - // Send + Sync - is_send_and_sync: bool, -} - -impl TypeInfo { - /// Metadata for `T`. - pub fn of() -> Self { - Self { - layout: Layout::new::(), - is_send_and_sync: true, - drop: Self::drop_ptr::, - type_name: core::any::type_name::(), - } - } - - pub fn of_non_send_and_sync() -> Self { - Self { - layout: Layout::new::(), - is_send_and_sync: false, - drop: Self::drop_ptr::, - type_name: core::any::type_name::(), - } - } - - #[inline] - pub fn layout(&self) -> Layout { - self.layout - } - - #[inline] - pub fn drop(&self) -> unsafe fn(*mut u8) { - self.drop - } - - #[inline] - pub fn is_send_and_sync(&self) -> bool { - self.is_send_and_sync - } - - #[inline] - pub fn type_name(&self) -> &'static str { - self.type_name - } - - pub(super) unsafe fn drop_ptr(x: *mut u8) { - x.cast::().drop_in_place() - } -} From edbc8bf2dd50b6d51963aa3b388d493ea0cb7ff4 Mon Sep 17 00:00:00 2001 From: bjorn3 Date: Wed, 28 Jul 2021 10:26:20 +0200 Subject: [PATCH 8/8] Fix review comments --- crates/bevy_ecs/macros/src/lib.rs | 10 +++++----- crates/bevy_ecs/src/bundle.rs | 14 ++++++-------- crates/bevy_ecs/src/lib.rs | 4 ++-- 3 files changed, 13 insertions(+), 15 deletions(-) diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index eeeff34c65242..d4d307f8a53e6 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -118,7 +118,7 @@ pub fn derive_bundle(input: TokenStream) -> TokenStream { { if *is_bundle { field_component_ids.push(quote! { - component_id.extend(<#field_type as #ecs_path::bundle::Bundle>::component_id(components)); + component_ids.extend(<#field_type as #ecs_path::bundle::Bundle>::component_ids(components)); }); field_get_components.push(quote! { self.#field.get_components(&mut func); @@ -128,7 +128,7 @@ pub fn derive_bundle(input: TokenStream) -> TokenStream { }); } else { field_component_ids.push(quote! { - component_id.push(components.get_or_insert_id::<#field_type>()); + component_ids.push(components.get_or_insert_id::<#field_type>()); }); field_get_components.push(quote! { func((&mut self.#field as *mut #field_type).cast::()); @@ -147,12 +147,12 @@ pub fn derive_bundle(input: TokenStream) -> TokenStream { TokenStream::from(quote! { /// SAFE: TypeInfo is returned in field-definition-order. [from_components] and [get_components] use field-definition-order unsafe impl #impl_generics #ecs_path::bundle::Bundle for #struct_name#ty_generics #where_clause { - fn component_id( + fn component_ids( components: &mut #ecs_path::component::Components, ) -> Vec<#ecs_path::component::ComponentId> { - let mut component_id = Vec::with_capacity(#field_len); + let mut component_ids = Vec::with_capacity(#field_len); #(#field_component_ids)* - component_id + component_ids } #[allow(unused_variables, unused_mut, non_snake_case)] diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index dd284773717b9..ab8f346a55116 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -44,7 +44,7 @@ use std::{any::TypeId, collections::HashMap}; /// [Bundle::component_id] pub unsafe trait Bundle: Send + Sync + 'static { /// Gets this [Bundle]'s component ids, in the order of this bundle's Components - fn component_id(components: &mut Components) -> Vec; + fn component_ids(components: &mut Components) -> Vec; /// Calls `func`, which should return data for each component in the bundle, in the order of /// this bundle's Components @@ -67,7 +67,7 @@ macro_rules! tuple_impl { /// SAFE: TypeInfo is returned in tuple-order. [Bundle::from_components] and [Bundle::get_components] use tuple-order unsafe impl<$($name: Component),*> Bundle for ($($name,)*) { #[allow(unused_variables)] - fn component_id(components: &mut Components) -> Vec { + fn component_ids(components: &mut Components) -> Vec { vec![$(components.get_or_insert_id::<$name>()),*] } @@ -206,11 +206,11 @@ impl Bundles { ) -> &'a BundleInfo { let bundle_infos = &mut self.bundle_infos; let id = self.bundle_ids.entry(TypeId::of::()).or_insert_with(|| { - let component_id = T::component_id(components); + let component_ids = T::component_ids(components); let id = BundleId(bundle_infos.len()); // SAFE: T::component_id ensures info was created let bundle_info = unsafe { - initialize_bundle(std::any::type_name::(), &component_id, id, components) + initialize_bundle(std::any::type_name::(), component_ids, id, components) }; bundle_infos.push(bundle_info); id @@ -225,17 +225,15 @@ impl Bundles { /// `component_id` must be valid [ComponentId]'s unsafe fn initialize_bundle( bundle_type_name: &'static str, - component_id: &[ComponentId], + component_ids: Vec, id: BundleId, components: &mut Components, ) -> BundleInfo { - let mut component_ids = Vec::new(); let mut storage_types = Vec::new(); - for &component_id in component_id { + for &component_id in &component_ids { // SAFE: component_id exists and is therefore valid let component_info = components.get_info_unchecked(component_id); - component_ids.push(component_id); storage_types.push(component_info.storage_type()); } diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 75aeb3ca5b112..fa50ae8efd22a 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -115,7 +115,7 @@ mod tests { .unwrap(); assert_eq!( - ::component_id(world.components_mut()), + ::component_ids(world.components_mut()), vec![ world.components_mut().get_or_insert_id::<&'static str>(), world.components_mut().get_or_insert_id::(), @@ -151,7 +151,7 @@ mod tests { } assert_eq!( - ::component_id(world.components_mut()), + ::component_ids(world.components_mut()), vec![ world.components_mut().get_or_insert_id::(), world.components_mut().get_or_insert_id::<&'static str>(),