Merge pull request #5 from TheRawMeatball/adress-final-review

crates/bevy_ecs/README.md

@@ -193,7 +193,16 @@ Components can be stored in:
 * **Tables**: Fast and cache friendly iteration, but slower adding and removing of components. This is the default storage type.
 * **Sparse Sets**: Fast adding and removing of components, but slower iteration.
 
-Component storage types are configurable, and they default to table storage if the storage is not manually defined. The [`component_storage.rs`](examples/component_storage.rs) example shows how to configure the storage type for a component.
+Component storage types are configurable, and they default to table storage if the storage is not manually defined.
+
+```rs
+#[derive(Component)]
+struct TableStoredComponent;
+
+#[derive(Component)]
+#[component(storage = "SparseSet")]
+struct SparseStoredComponent;
+```
 
 ### Component Bundles
 

crates/bevy_ecs/macros/src/lib.rs

@@ -115,7 +115,6 @@ pub fn derive_bundle(input: TokenStream) -> TokenStream {
     let mut field_component_ids = Vec::new();
     let mut field_get_components = Vec::new();
     let mut field_from_components = Vec::new();
-    let mut is_dense_exprs = Vec::new();
     for ((field_type, is_bundle), field) in
         field_type.iter().zip(is_bundle.iter()).zip(field.iter())
     {
@@ -129,9 +128,6 @@ pub fn derive_bundle(input: TokenStream) -> TokenStream {
             field_from_components.push(quote! {
                 #field: <#field_type as #ecs_path::bundle::Bundle>::from_components(&mut func),
             });
-            is_dense_exprs.push(quote! {
-                <#field_type as #ecs_path::bundle::Bundle>::IS_DENSE
-            });
         } else {
             field_component_ids.push(quote! {
                 component_ids.push(components.get_or_insert_id::<#field_type>());
@@ -143,12 +139,6 @@ pub fn derive_bundle(input: TokenStream) -> TokenStream {
             field_from_components.push(quote! {
                 #field: func().cast::<#field_type>().read(),
             });
-            is_dense_exprs.push(quote! {
-                match <<#field_type as #ecs_path::component::Component>::Storage as #ecs_path::component::ComponentStorage>::STORAGE_TYPE {
-                    #ecs_path::component::StorageType::Table => true,
-                    #ecs_path::component::StorageType::SparseSet => false,
-                }
-            });
         }
     }
     let field_len = field.len();
@@ -157,11 +147,8 @@ pub fn derive_bundle(input: TokenStream) -> TokenStream {
     let struct_name = &ast.ident;
 
     TokenStream::from(quote! {
-        // TODO: TypeInfo no longer exists, verify safety again
-        /// SAFE: TypeInfo is returned in field-definition-order. [from_components] and [get_components] use field-definition-order
+        /// SAFE: ComponentId 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 {
-            const IS_DENSE: bool = true #(&& #is_dense_exprs)*;
-
             fn component_ids(
                 components: &mut #ecs_path::component::Components,
             ) -> Vec<#ecs_path::component::ComponentId> {

crates/bevy_ecs/src/bundle.rs

@@ -76,8 +76,6 @@ use std::{any::TypeId, collections::HashMap};
 /// - [Bundle::from_components] must call `func` exactly once for each [ComponentId] returned by
 ///   [Bundle::component_ids].
 pub unsafe trait Bundle: Send + Sync + 'static {
-    const IS_DENSE: bool;
-
     /// Gets this [Bundle]'s component ids, in the order of this bundle's Components
     fn component_ids(components: &mut Components) -> Vec<ComponentId>;
 
@@ -99,21 +97,13 @@ pub unsafe trait Bundle: Send + Sync + 'static {
 
 macro_rules! tuple_impl {
     ($($name: ident),*) => {
-        // TODO: TypeInfo no longer exists, verify safety again
-        /// SAFE: TypeInfo is returned in tuple-order. [Bundle::from_components] and [Bundle::get_components] use tuple-order
+        /// SAFE: Component 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_ids(components: &mut Components) -> Vec<ComponentId> {
                 vec![$(components.get_or_insert_id::<$name>()),*]
             }
 
-            const IS_DENSE: bool = true $(&&
-                match <$name::Storage as $crate::component::ComponentStorage>::STORAGE_TYPE {
-                    StorageType::Table => true,
-                    StorageType::SparseSet => false,
-                }
-            )*;
-
             #[allow(unused_variables, unused_mut)]
             #[allow(clippy::unused_unit)]
             unsafe fn from_components(mut func: impl FnMut() -> *mut u8) -> Self {

crates/bevy_ecs/src/component.rs

@@ -163,6 +163,8 @@ impl SparseSetIndex for ComponentId {
 #[derive(Debug)]
 pub struct ComponentDescriptor {
     name: String,
+    // SAFETY: This must remain private. It must match the statically known StorageType of the
+    // associated rust component type if one exists.
     storage_type: StorageType,
     // SAFETY: This must remain private. It must only be set to "true" if this component is
     // actually Send + Sync
@@ -237,7 +239,11 @@ pub struct Components {
 #[derive(Debug, Error)]
 pub enum ComponentsError {
     #[error("A component of type {name:?} ({type_id:?}) already exists")]
-    ComponentAlreadyExists { type_id: TypeId, name: String },
+    ComponentAlreadyExists {
+        type_id: TypeId,
+        name: String,
+        existing_id: ComponentId,
+    },
 }
 
 impl Components {
@@ -247,10 +253,12 @@ impl Components {
     ) -> Result<ComponentId, ComponentsError> {
         let index = self.components.len();
         if let Some(type_id) = descriptor.type_id {
-            if self.indices.contains_key(&type_id) {
+            let i = self.indices.get(&type_id);
+            if let Some(&index) = i {
                 return Err(ComponentsError::ComponentAlreadyExists {
                     type_id,
                     name: descriptor.name,
+                    existing_id: self.components[index].id,
                 });
             }
             self.indices.insert(type_id, index);
@@ -267,13 +275,6 @@ impl Components {
         unsafe { self.get_or_insert_with(TypeId::of::<T>(), ComponentDescriptor::new::<T>) }
     }
 
-    #[inline]
-    pub fn get_or_insert_info<T: Component>(&mut self) -> &ComponentInfo {
-        let id = self.get_or_insert_id::<T>();
-        // SAFE: component_info with the given `id` initialized above
-        unsafe { self.get_info_unchecked(id) }
-    }
-
     #[inline]
     pub fn len(&self) -> usize {
         self.components.len()

crates/bevy_ecs/src/lib.rs

@@ -82,6 +82,10 @@ mod tests {
         }
     }
 
+    #[derive(Component, Clone, Debug)]
+    #[component(storage = "SparseSet")]
+    struct DropCkSparse(DropCk);
+
     #[derive(Component, Copy, Clone, PartialEq, Eq, Debug)]
     #[component(storage = "Table")]
     struct TableStored(&'static str);
@@ -1343,7 +1347,10 @@ mod tests {
         let (dropck2, dropped2) = DropCk::new_pair();
         let mut world = World::default();
 
-        world.spawn().insert(dropck1).insert(dropck2);
+        world
+            .spawn()
+            .insert(DropCkSparse(dropck1))
+            .insert(DropCkSparse(dropck2));
         assert_eq!(dropped1.load(Ordering::Relaxed), 1);
         assert_eq!(dropped2.load(Ordering::Relaxed), 0);
         drop(world);

crates/bevy_ecs/src/query/fetch.rs

@@ -232,12 +232,9 @@ pub struct ReadState<T> {
 // read
 unsafe impl<T: Component> FetchState for ReadState<T> {
     fn init(world: &mut World) -> Self {
-        let component_info = world.components.get_or_insert_info::<T>();
-        if T::Storage::STORAGE_TYPE == StorageType::SparseSet {
-            world.storages.sparse_sets.get_or_insert(component_info);
-        };
+        let component_id = world.register_or_get_id::<T>();
         ReadState {
-            component_id: component_info.id(),
+            component_id,
             marker: PhantomData,
         }
     }
@@ -414,9 +411,9 @@ pub struct WriteState<T> {
 // written
 unsafe impl<T: Component> FetchState for WriteState<T> {
     fn init(world: &mut World) -> Self {
-        let component_info = world.components.get_or_insert_info::<T>();
+        let component_id = world.register_or_get_id::<T>();
         WriteState {
-            component_id: component_info.id(),
+            component_id,
             marker: PhantomData,
         }
     }
@@ -746,9 +743,9 @@ pub struct ChangeTrackersState<T> {
 // read
 unsafe impl<T: Component> FetchState for ChangeTrackersState<T> {
     fn init(world: &mut World) -> Self {
-        let component_info = world.components.get_or_insert_info::<T>();
+        let component_id = world.register_or_get_id::<T>();
         Self {
-            component_id: component_info.id(),
+            component_id,
             marker: PhantomData,
         }
     }

crates/bevy_ecs/src/query/filter.rs

@@ -88,9 +88,9 @@ pub struct WithState<T> {
 // SAFETY: no component access or archetype component access
 unsafe impl<T: Component> FetchState for WithState<T> {
     fn init(world: &mut World) -> Self {
-        let component_info = world.components.get_or_insert_info::<T>();
+        let component_id = world.register_or_get_id::<T>();
         Self {
-            component_id: component_info.id(),
+            component_id,
             marker: PhantomData,
         }
     }
@@ -207,9 +207,9 @@ pub struct WithoutState<T> {
 // SAFETY: no component access or archetype component access
 unsafe impl<T: Component> FetchState for WithoutState<T> {
     fn init(world: &mut World) -> Self {
-        let component_info = world.components.get_or_insert_info::<T>();
+        let component_id = world.register_or_get_id::<T>();
         Self {
-            component_id: component_info.id(),
+            component_id,
             marker: PhantomData,
         }
     }
@@ -472,10 +472,10 @@ macro_rules! impl_tick_filter {
         // SAFETY: this reads the T component. archetype component access and component access are updated to reflect that
         unsafe impl<T: Component> FetchState for $state_name<T> {
             fn init(world: &mut World) -> Self {
-                let component_info = world.components.get_or_insert_info::<T>();
+                let component_id = world.register_or_get_id::<T>();
                 Self {
-                    component_id: component_info.id(),
-                    storage_type: component_info.storage_type(),
+                    component_id,
+                    storage_type: T::Storage::STORAGE_TYPE,
                     marker: PhantomData,
                 }
             }

crates/bevy_ecs/src/world/mod.rs

@@ -190,6 +190,13 @@ impl World {
         Ok(component_id)
     }
 
+    pub(crate) fn register_or_get_id<T: Component>(&mut self) -> ComponentId {
+        self.register_component(ComponentDescriptor::new::<T>())
+            .unwrap_or_else(|e| match e {
+                ComponentsError::ComponentAlreadyExists { existing_id, .. } => existing_id,
+            })
+    }
+
     /// Retrieves an [EntityRef] that exposes read-only operations for the given `entity`.
     /// This will panic if the `entity` does not exist. Use [World::get_entity] if you want
     /// to check for entity existence instead of implicitly panic-ing.

crates/bevy_sprite/src/lib.rs

@@ -88,8 +88,7 @@ impl Plugin for SpritePlugin {
                 frustum_culling::atlas_frustum_culling_system,
             );
         }
-        let world = &mut app.world;
-        let world_cell = world.cell();
+        let world_cell = app.world.cell();
         let mut render_graph = world_cell.get_resource_mut::<RenderGraph>().unwrap();
         let mut pipelines = world_cell
             .get_resource_mut::<Assets<PipelineDescriptor>>()