From 0b0ab274b113bdaab3c28331ed744ceeedbb14cc Mon Sep 17 00:00:00 2001 From: eugineerd Date: Wed, 4 Dec 2024 09:23:39 +0000 Subject: [PATCH 01/40] add simple entity cloning benchmark --- benches/Cargo.toml | 5 + benches/benches/bevy_ecs/entity_cloning.rs | 195 +++++++++++++++++++++ 2 files changed, 200 insertions(+) create mode 100644 benches/benches/bevy_ecs/entity_cloning.rs diff --git a/benches/Cargo.toml b/benches/Cargo.toml index 6375af6d5e198..2fbc186a6efec 100644 --- a/benches/Cargo.toml +++ b/benches/Cargo.toml @@ -35,6 +35,11 @@ name = "change_detection" path = "benches/bevy_ecs/change_detection.rs" harness = false +[[bench]] +name = "entity_cloning" +path = "benches/bevy_ecs/entity_cloning.rs" +harness = false + [[bench]] name = "ecs" path = "benches/bevy_ecs/benches.rs" diff --git a/benches/benches/bevy_ecs/entity_cloning.rs b/benches/benches/bevy_ecs/entity_cloning.rs new file mode 100644 index 0000000000000..028a9c70b549b --- /dev/null +++ b/benches/benches/bevy_ecs/entity_cloning.rs @@ -0,0 +1,195 @@ +use bevy_ecs::bundle::Bundle; +use bevy_ecs::reflect::AppTypeRegistry; +use bevy_ecs::{component::Component, reflect::ReflectComponent, world::World}; +use bevy_hierarchy::{BuildChildren, CloneEntityHierarchyExt}; +use bevy_math::Mat4; +use bevy_reflect::{GetTypeRegistration, Reflect}; +use criterion::{black_box, criterion_group, criterion_main, Bencher, Criterion}; + +criterion_group!(benches, reflect_benches, clone_benches); +criterion_main!(benches); + +#[derive(Component, Reflect, Default)] +#[reflect(Component)] +struct ReflectComponent1(Mat4); + +#[derive(Component, Reflect, Default)] +#[reflect(Component)] +struct ReflectComponent2(Mat4); + +#[derive(Component, Reflect, Default)] +#[reflect(Component)] +struct ReflectComponent3(Mat4); + +#[derive(Component, Reflect, Default)] +#[reflect(Component)] +struct ReflectComponent4(Mat4); + +#[derive(Component, Reflect, Default)] +#[reflect(Component)] +struct ReflectComponent5(Mat4); + +#[derive(Component, Reflect, Default)] +#[reflect(Component)] +struct ReflectComponent6(Mat4); + +#[derive(Component, Reflect, Default)] +#[reflect(Component)] +struct ReflectComponent7(Mat4); + +#[derive(Component, Reflect, Default)] +#[reflect(Component)] +struct ReflectComponent8(Mat4); + +#[derive(Component, Reflect, Default)] +#[reflect(Component)] +struct ReflectComponent9(Mat4); + +#[derive(Component, Reflect, Default)] +#[reflect(Component)] +struct ReflectComponent10(Mat4); + +#[derive(Component, Reflect, Default, Clone)] +#[reflect(Component)] +struct CloneComponent1(Mat4); + +#[derive(Component, Reflect, Default, Clone)] +#[reflect(Component)] +struct CloneComponent2(Mat4); + +#[derive(Component, Reflect, Default, Clone)] +#[reflect(Component)] +struct CloneComponent3(Mat4); + +#[derive(Component, Reflect, Default, Clone)] +#[reflect(Component)] +struct CloneComponent4(Mat4); + +#[derive(Component, Reflect, Default, Clone)] +#[reflect(Component)] +struct CloneComponent5(Mat4); + +#[derive(Component, Reflect, Default, Clone)] +#[reflect(Component)] +struct CloneComponent6(Mat4); + +#[derive(Component, Reflect, Default, Clone)] +#[reflect(Component)] +struct CloneComponent7(Mat4); + +#[derive(Component, Reflect, Default, Clone)] +#[reflect(Component)] +struct CloneComponent8(Mat4); +#[derive(Component, Reflect, Default, Clone)] +#[reflect(Component)] +struct CloneComponent9(Mat4); + +#[derive(Component, Reflect, Default, Clone)] +#[reflect(Component)] +struct CloneComponent10(Mat4); + +fn hierarchy( + b: &mut Bencher, + width: usize, + height: usize, +) { + let mut world = World::default(); + let registry = AppTypeRegistry::default(); + { + let mut r = registry.write(); + r.register::(); + } + world.insert_resource(registry); + + let id = world.spawn(black_box(C::default())).id(); + + let mut hierarchy_level = vec![id]; + + for _ in 0..height { + let current_hierarchy_level = hierarchy_level.clone(); + hierarchy_level.clear(); + for parent_id in current_hierarchy_level { + for _ in 0..width { + let child_id = world + .spawn(black_box(C::default())) + .set_parent(parent_id) + .id(); + hierarchy_level.push(child_id) + } + } + } + world.flush(); + + b.iter(move || { + world.commands().clone_entity_with(id, |builder| { + builder.recursive(true); + }); + world.flush(); + }); +} + +fn simple(b: &mut Bencher) { + let mut world = World::default(); + let registry = AppTypeRegistry::default(); + { + let mut r = registry.write(); + r.register::(); + } + world.insert_resource(registry); + let id = world.spawn(black_box(C::default())).id(); + + b.iter(move || { + world.commands().clone_entity(id); + world.flush(); + }); +} + +fn reflect_benches(c: &mut Criterion) { + c.bench_function("many components reflect", |b| { + simple::<( + ReflectComponent1, + ReflectComponent2, + ReflectComponent3, + ReflectComponent4, + ReflectComponent5, + ReflectComponent6, + ReflectComponent7, + ReflectComponent8, + ReflectComponent9, + ReflectComponent10, + )>(b); + }); + + c.bench_function("hierarchy wide reflect", |b| { + hierarchy::(b, 5, 4); + }); + + c.bench_function("hierarchy tall reflect", |b| { + hierarchy::(b, 1, 50); + }); +} + +fn clone_benches(c: &mut Criterion) { + c.bench_function("many components clone", |b| { + simple::<( + CloneComponent1, + CloneComponent2, + CloneComponent3, + CloneComponent4, + CloneComponent5, + CloneComponent6, + CloneComponent7, + CloneComponent8, + CloneComponent9, + CloneComponent10, + )>(b); + }); + + c.bench_function("hierarchy wide clone", |b| { + hierarchy::(b, 5, 4); + }); + + c.bench_function("hierarchy tall clone", |b| { + hierarchy::(b, 1, 50); + }); +} From 6316f3c601fb7bd87096262e62188a61750fa0d4 Mon Sep 17 00:00:00 2001 From: eugineerd Date: Wed, 4 Dec 2024 16:47:05 +0000 Subject: [PATCH 02/40] first draft of faster cloning --- benches/benches/bevy_ecs/entity_cloning.rs | 30 ++ crates/bevy_ecs/src/component.rs | 30 +- crates/bevy_ecs/src/entity/clone_entities.rs | 267 +++++++++++++++--- .../bevy_ecs/src/observer/entity_observer.rs | 8 +- crates/bevy_hierarchy/src/hierarchy.rs | 29 +- 5 files changed, 286 insertions(+), 78 deletions(-) diff --git a/benches/benches/bevy_ecs/entity_cloning.rs b/benches/benches/bevy_ecs/entity_cloning.rs index 028a9c70b549b..54959838c8f94 100644 --- a/benches/benches/bevy_ecs/entity_cloning.rs +++ b/benches/benches/bevy_ecs/entity_cloning.rs @@ -167,6 +167,21 @@ fn reflect_benches(c: &mut Criterion) { c.bench_function("hierarchy tall reflect", |b| { hierarchy::(b, 1, 50); }); + + c.bench_function("hierarchy many reflect", |b| { + hierarchy::<( + ReflectComponent1, + ReflectComponent2, + ReflectComponent3, + ReflectComponent4, + ReflectComponent5, + ReflectComponent6, + ReflectComponent7, + ReflectComponent8, + ReflectComponent9, + ReflectComponent10, + )>(b, 3, 3); + }); } fn clone_benches(c: &mut Criterion) { @@ -192,4 +207,19 @@ fn clone_benches(c: &mut Criterion) { c.bench_function("hierarchy tall clone", |b| { hierarchy::(b, 1, 50); }); + + c.bench_function("hierarchy many clone", |b| { + hierarchy::<( + CloneComponent1, + CloneComponent2, + CloneComponent3, + CloneComponent4, + CloneComponent5, + CloneComponent6, + CloneComponent7, + CloneComponent8, + CloneComponent9, + CloneComponent10, + )>(b, 3, 3); + }); } diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index acffd5b3d10fd..5b3d4296ff557 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -5,7 +5,7 @@ use crate::{ archetype::ArchetypeFlags, bundle::BundleInfo, change_detection::MAX_CHANGE_AGE, - entity::{Entity, EntityCloner}, + entity::{ComponentCloneCtx, Entity}, query::DebugCheckedUnwrap, storage::{SparseSetIndex, SparseSets, Storages, Table, TableRow}, system::{Local, Resource, SystemParam}, @@ -883,7 +883,7 @@ impl ComponentDescriptor { } /// Function type that can be used to clone an entity. -pub type ComponentCloneFn = fn(&mut DeferredWorld, &EntityCloner); +pub type ComponentCloneFn = fn(&mut DeferredWorld, &mut ComponentCloneCtx); /// An enum instructing how to clone a component. #[derive(Debug, Default)] @@ -1964,18 +1964,12 @@ impl RequiredComponents { /// /// See [`ComponentCloneHandlers`] for more details. pub fn component_clone_via_clone( - world: &mut DeferredWorld, - entity_cloner: &EntityCloner, + _world: &mut DeferredWorld, + ctx: &mut ComponentCloneCtx, ) { - let component = world - .entity(entity_cloner.source()) - .get::() - .expect("Component must exists on source entity") - .clone(); - world - .commands() - .entity(entity_cloner.target()) - .insert(component); + if let Some(component) = ctx.read_source_component::() { + ctx.write_target_component(component.clone()); + } } /// Component [clone handler function](ComponentCloneFn) implemented using reflect. @@ -1984,10 +1978,10 @@ pub fn component_clone_via_clone( /// /// See [`ComponentCloneHandlers`] for more details. #[cfg(feature = "bevy_reflect")] -pub fn component_clone_via_reflect(world: &mut DeferredWorld, entity_cloner: &EntityCloner) { - let component_id = entity_cloner.component_id(); - let source = entity_cloner.source(); - let target = entity_cloner.target(); +pub fn component_clone_via_reflect(world: &mut DeferredWorld, ctx: &mut ComponentCloneCtx) { + let component_id = ctx.component_id(); + let source = ctx.source(); + let target = ctx.target(); world.commands().queue(move |world: &mut World| { world.resource_scope::(|world, registry| { let registry = registry.read(); @@ -2019,7 +2013,7 @@ pub fn component_clone_via_reflect(world: &mut DeferredWorld, entity_cloner: &En /// Noop implementation of component clone handler function. /// /// See [`ComponentCloneHandlers`] for more details. -pub fn component_clone_ignore(_world: &mut DeferredWorld, _entity_cloner: &EntityCloner) {} +pub fn component_clone_ignore(_world: &mut DeferredWorld, _ctx: &mut ComponentCloneCtx) {} /// Wrapper for components clone specialization using autoderef. #[doc(hidden)] diff --git a/crates/bevy_ecs/src/entity/clone_entities.rs b/crates/bevy_ecs/src/entity/clone_entities.rs index 710b96e5a3080..57579166c3e9f 100644 --- a/crates/bevy_ecs/src/entity/clone_entities.rs +++ b/crates/bevy_ecs/src/entity/clone_entities.rs @@ -1,20 +1,99 @@ -use alloc::sync::Arc; +use alloc::{alloc::Layout, sync::Arc}; +use bevy_ptr::{OwningPtr, Ptr}; use core::any::TypeId; +use core::ptr::NonNull; +use std::mem::MaybeUninit; use bevy_utils::{HashMap, HashSet}; use crate::{ + archetype::Archetype, bundle::Bundle, - component::{component_clone_ignore, Component, ComponentCloneHandler, ComponentId}, + component::{ + component_clone_ignore, Component, ComponentCloneHandler, ComponentId, Components, + StorageType, + }, entity::Entity, - world::World, + world::{DeferredWorld, World}, }; -/// A helper struct to clone an entity. Used internally by [`EntityCloneBuilder::clone_entity`] and custom clone handlers. +pub struct ComponentCloneCtx<'a> { + source: Entity, + target: Entity, + component_id: ComponentId, + source_component_ptr: Ptr<'a>, + target_component_ptr: NonNull, + target_component_written: bool, + components: &'a Components, + entity_cloner: &'a EntityCloner, + // #[cfg(feature = "bevy_reflect")] + // type_registry: Option<&'a bevy_reflect::TypeRegistry>, +} + +impl<'a> ComponentCloneCtx<'a> { + /// Returns the current source entity. + pub fn source(&self) -> Entity { + self.source + } + + /// Returns the current target entity. + pub fn target(&self) -> Entity { + self.target + } + + /// Returns the [`ComponentId`] of currently cloned component. + pub fn component_id(&self) -> ComponentId { + self.component_id + } + + pub fn read_source_component(&self) -> Option<&T> { + if self + .components + .component_id::() + .is_some_and(|id| id == self.component_id) + { + // SAFETY: + // 1. Components and ComponentId are from the same world + // 2. source_component_ptr holds valid data of the type referenced by ComponentId + unsafe { Some(self.source_component_ptr.deref::()) } + } else { + None + } + } + + pub fn write_target_component(&mut self, component: T) { + let short_name = disqualified::ShortName::of::(); + if self.target_component_written { + panic!("Trying to write component '{short_name}' multiple times") + } + if !self + .components + .component_id::() + .is_some_and(|id| id == self.component_id) + { + panic!("Component '{short_name}' does not match ComponentId of this ComponentCloneCtx"); + } + // SAFETY: + // 1. Components and ComponentId are from the same world + // 2. target_component_ptr holds valid data of the type referenced by ComponentId + unsafe { self.target_component_ptr.cast::().write(component) }; + self.target_component_written = true + } + + pub fn clone_entity_fn(&self, source: Entity, target: Entity) -> EntityCloner { + self.entity_cloner.with_source_and_target(source, target) + } + + // #[cfg(feature = "bevy_reflect")] + // pub fn type_registry(&self) -> Option<&App> { + // self.type_registry.as_ref().read() + // } +} + +/// A helper struct to clone an entity. Used internally by [`EntityCloneBuilder::clone_entity`]. pub struct EntityCloner { source: Entity, target: Entity, - component_id: Option, filter_allows_components: bool, filter: Arc>, clone_handlers_overrides: Arc>, @@ -23,28 +102,157 @@ pub struct EntityCloner { impl EntityCloner { /// Clones and inserts components from the `source` entity into `target` entity using the stored configuration. pub fn clone_entity(&mut self, world: &mut World) { - let source_entity = world - .get_entity(self.source) - .expect("Source entity must exist"); + let (app_registry, source_entity, storages, components, mut deferred_world) = unsafe { + let world = world.as_unsafe_world_cell(); + let source_entity = world + .get_entity(self.source) + .expect("Source entity must exist"); + + #[cfg(feature = "bevy_reflect")] + let app_registry = world.get_resource::(); + #[cfg(not(feature = "bevy_reflect"))] + let app_registry = None; + + ( + app_registry, + source_entity, + world.storages(), + world.components(), + world.into_deferred(), + ) + }; let archetype = source_entity.archetype(); + let source_location = source_entity.location(); + + // // To clone components without table moves we need to use `insert_by_ids`. + // // For this, we need 2 things: + // // 1. list of component ids + // // 2. list of aligned pointers to component data + // // + // // To reduce the number of memory allocations and provide better data locality, we need to store all this data + // // in one big buffer. + // // The structure of this buffer is as follows: + // // +--------------+------------------------+ + // // | ComponentIds | Aligned component data | + // // +--------------+------------------------+ + // // This means that we need to preallocate buffer of size (component_count * sizeof:: + Archetype row size) + + // // First, calculate ComponentIds portion of buffer. + // let mut buffer_layout = Layout::array::(archetype.component_count()).unwrap(); + // // Next, add each component aligned data layout to it. + // for component in archetype.components() { + // let layout = world.components().get_info(component).unwrap().layout(); + // let (new_layout, _) = buffer_layout.extend(layout.pad_to_align()).unwrap(); + // buffer_layout = new_layout; + // } + // let buffer_size = archetype + // .components() + // .fold(buffer_size, |mut buffer_size, id| { + // let layout = world.components().get_info(id).unwrap().layout(); + + // // Round up current size to meet alignment + // let align = layout.align(); + // buffer_size = (buffer_size + align - 1) & !(align - 1); + + // // Add size of current layout + // buffer_size += layout.size(); + + // buffer_size + // }); + // let buffer_layout = + + let mut component_data_size = 0; + for component in archetype.components() { + let layout = components.get_info(component).unwrap().layout(); + + // Round up current size to meet alignment + let align = layout.align(); + component_data_size = (component_data_size + align - 1) & !(align - 1); + + // Add size of current layout + component_data_size += layout.size(); + } + let mut component_ids: Vec = Vec::with_capacity(archetype.component_count()); + let mut component_data: Vec = Vec::with_capacity(component_data_size); + component_data.push(0); + let mut component_data_offsets = Vec::with_capacity(archetype.component_count()); + let mut component_data_offset = 0; + + for component in archetype.components() { + if !self.is_cloning_allowed(&component) { + continue; + } - let mut components = Vec::with_capacity(archetype.component_count()); - components.extend( - archetype - .components() - .filter(|id| self.is_cloning_allowed(id)), - ); - - for component in components { - let global_handlers = world.components().get_component_clone_handlers(); + let global_handlers = components.get_component_clone_handlers(); let handler = match self.clone_handlers_overrides.get(&component) { None => global_handlers.get_handler(component), Some(ComponentCloneHandler::Default) => global_handlers.get_default_handler(), Some(ComponentCloneHandler::Ignore) => component_clone_ignore, Some(ComponentCloneHandler::Custom(handler)) => *handler, }; - self.component_id = Some(component); - (handler)(&mut world.into(), self); + + let source_component_ptr = match archetype.get_storage_type(component) { + Some(StorageType::Table) => unsafe { + storages + .tables + .get(source_location.table_id) + .unwrap() + .get_component(component, source_location.table_row) + .unwrap() + }, + Some(StorageType::SparseSet) => storages + .sparse_sets + .get(component) + .unwrap() + .get(self.source) + .unwrap(), + None => continue, + }; + + let mut ctx = ComponentCloneCtx { + source: self.source, + target: self.target, + component_id: component, + entity_cloner: &self, + components, + source_component_ptr, + target_component_ptr: unsafe { + NonNull::new_unchecked(component_data.as_mut_ptr()) + }, + target_component_written: false, + // type_registry, + }; + + (handler)(&mut deferred_world, &mut ctx); + + if !ctx.target_component_written { + continue; + } + + component_ids.push(component); + component_data_offsets.push(component_data_offset); + + let layout = components.get_info(component).unwrap().layout(); + + // Round up current size to meet alignment + let align = layout.align(); + component_data_offset = (component_data_offset + align - 1) & !(align - 1); + + // Add size of current layout + component_data_offset += layout.size(); + } + + world.flush(); + + unsafe { + world.entity_mut(self.target).insert_by_ids( + &component_ids, + component_data_offsets.iter().map(|offset| { + OwningPtr::new({ + NonNull::new_unchecked(component_data.as_mut_ptr()).add(*offset) + }) + }), + ); } } @@ -53,24 +261,8 @@ impl EntityCloner { || (!self.filter_allows_components && !self.filter.contains(component)) } - /// Returns the current source entity. - pub fn source(&self) -> Entity { - self.source - } - - /// Returns the current target entity. - pub fn target(&self) -> Entity { - self.target - } - - /// Returns the [`ComponentId`] of currently cloned component. - pub fn component_id(&self) -> ComponentId { - self.component_id - .expect("ComponentId must be set in clone_entity") - } - /// Reuse existing [`EntityCloner`] configuration with new source and target. - pub fn with_source_and_target(&self, source: Entity, target: Entity) -> EntityCloner { + fn with_source_and_target(&self, source: Entity, target: Entity) -> EntityCloner { EntityCloner { source, target, @@ -170,14 +362,11 @@ impl<'w> EntityCloneBuilder<'w> { EntityCloner { source, target, - component_id: None, filter_allows_components, filter: Arc::new(filter), clone_handlers_overrides: Arc::new(clone_handlers_overrides), } .clone_entity(world); - - world.flush_commands(); } /// Adds all components of the bundle to the list of components to clone. diff --git a/crates/bevy_ecs/src/observer/entity_observer.rs b/crates/bevy_ecs/src/observer/entity_observer.rs index 8433d3cea358b..9c2a85a326273 100644 --- a/crates/bevy_ecs/src/observer/entity_observer.rs +++ b/crates/bevy_ecs/src/observer/entity_observer.rs @@ -1,6 +1,6 @@ use crate::{ component::{Component, ComponentCloneHandler, ComponentHooks, StorageType}, - entity::{Entity, EntityCloneBuilder, EntityCloner}, + entity::{ComponentCloneCtx, Entity, EntityCloneBuilder}, observer::ObserverState, world::{DeferredWorld, World}, }; @@ -64,9 +64,9 @@ impl CloneEntityWithObserversExt for EntityCloneBuilder<'_> { } } -fn component_clone_observed_by(world: &mut DeferredWorld, entity_cloner: &EntityCloner) { - let target = entity_cloner.target(); - let source = entity_cloner.source(); +fn component_clone_observed_by(world: &mut DeferredWorld, ctx: &mut ComponentCloneCtx) { + let target = ctx.target(); + let source = ctx.source(); world.commands().queue(move |world: &mut World| { let observed_by = world diff --git a/crates/bevy_hierarchy/src/hierarchy.rs b/crates/bevy_hierarchy/src/hierarchy.rs index d48e74d864e23..234d4e86f3eeb 100644 --- a/crates/bevy_hierarchy/src/hierarchy.rs +++ b/crates/bevy_hierarchy/src/hierarchy.rs @@ -4,7 +4,7 @@ use crate::{ }; use bevy_ecs::{ component::ComponentCloneHandler, - entity::{Entity, EntityCloneBuilder, EntityCloner}, + entity::{ComponentCloneCtx, Entity, EntityCloneBuilder}, system::EntityCommands, world::{Command, DeferredWorld, EntityWorldMut, World}, }; @@ -233,34 +233,29 @@ impl CloneEntityHierarchyExt for EntityCloneBuilder<'_> { } /// Clone handler for the [`Children`] component. Allows to clone the entity recursively. -fn component_clone_children(world: &mut DeferredWorld, entity_cloner: &EntityCloner) { - let children = world - .get::(entity_cloner.source()) +fn component_clone_children(world: &mut DeferredWorld, ctx: &mut ComponentCloneCtx) { + let children = ctx + .read_source_component::() .expect("Source entity must have Children component") - .iter() - .cloned() - .collect::>(); - let parent = entity_cloner.target(); + .iter(); + let parent = ctx.target(); for child in children { let child_clone = world.commands().spawn_empty().id(); - let mut entity_cloner = entity_cloner.with_source_and_target(child, child_clone); + let mut clone_entity = ctx.clone_entity_fn(*child, child_clone); world.commands().queue(move |world: &mut World| { - entity_cloner.clone_entity(world); + clone_entity.clone_entity(world); world.entity_mut(child_clone).set_parent(parent); }); } } /// Clone handler for the [`Parent`] component. Allows to add clone as a child to the parent entity. -fn component_clone_parent(world: &mut DeferredWorld, entity_cloner: &EntityCloner) { - let parent = world - .get::(entity_cloner.source()) +fn component_clone_parent(world: &mut DeferredWorld, ctx: &mut ComponentCloneCtx) { + let parent = ctx + .read_source_component::() .map(|p| p.0) .expect("Source entity must have Parent component"); - world - .commands() - .entity(entity_cloner.target()) - .set_parent(parent); + world.commands().entity(ctx.target()).set_parent(parent); } #[cfg(test)] From d4ac17a0839f202aabe72a5400a0b63898755412 Mon Sep 17 00:00:00 2001 From: eugineerd Date: Sat, 7 Dec 2024 12:55:32 +0000 Subject: [PATCH 03/40] use bumpalo to store component data --- crates/bevy_ecs/Cargo.toml | 1 + crates/bevy_ecs/src/entity/clone_entities.rs | 257 +++++++++---------- crates/bevy_hierarchy/src/hierarchy.rs | 4 +- 3 files changed, 125 insertions(+), 137 deletions(-) diff --git a/crates/bevy_ecs/Cargo.toml b/crates/bevy_ecs/Cargo.toml index 30117f71277d5..173380baa2b8d 100644 --- a/crates/bevy_ecs/Cargo.toml +++ b/crates/bevy_ecs/Cargo.toml @@ -43,6 +43,7 @@ derive_more = { version = "1", default-features = false, features = [ nonmax = "0.5" arrayvec = { version = "0.7.4", optional = true } smallvec = { version = "1", features = ["union"] } +bumpalo = "3" [dev-dependencies] rand = "0.8" diff --git a/crates/bevy_ecs/src/entity/clone_entities.rs b/crates/bevy_ecs/src/entity/clone_entities.rs index 57579166c3e9f..4049510852866 100644 --- a/crates/bevy_ecs/src/entity/clone_entities.rs +++ b/crates/bevy_ecs/src/entity/clone_entities.rs @@ -1,44 +1,77 @@ -use alloc::{alloc::Layout, sync::Arc}; -use bevy_ptr::{OwningPtr, Ptr}; +use alloc::sync::Arc; +use bevy_ptr::{Ptr, PtrMut}; +use bumpalo::Bump; use core::any::TypeId; -use core::ptr::NonNull; -use std::mem::MaybeUninit; use bevy_utils::{HashMap, HashSet}; use crate::{ - archetype::Archetype, bundle::Bundle, component::{ component_clone_ignore, Component, ComponentCloneHandler, ComponentId, Components, - StorageType, }, entity::Entity, - world::{DeferredWorld, World}, + world::World, }; -pub struct ComponentCloneCtx<'a> { - source: Entity, - target: Entity, +/// Context for component clone handlers. +/// +/// Provides fast access to useful resources like [`AppTypeRegistry`](crate::reflect::AppTypeRegistry) +/// and allows component clone handler to get information about component being cloned. +pub struct ComponentCloneCtx<'a, 'b> { component_id: ComponentId, source_component_ptr: Ptr<'a>, - target_component_ptr: NonNull, target_component_written: bool, + target_components_ptrs: &'a mut Vec>, + target_components_buffer: &'b Bump, components: &'a Components, entity_cloner: &'a EntityCloner, - // #[cfg(feature = "bevy_reflect")] - // type_registry: Option<&'a bevy_reflect::TypeRegistry>, + #[cfg(feature = "bevy_reflect")] + type_registry: Option<&'a crate::reflect::AppTypeRegistry>, } -impl<'a> ComponentCloneCtx<'a> { +impl<'a, 'b> ComponentCloneCtx<'a, 'b> { + /// Create a new instance of `ComponentCloneCtx` that can be passed to component clone handlers. + /// + /// # Safety + /// Caller must ensure that: + /// - `components` and `component_id` are from the same world. + /// - `source_component_ptr` points to a valid component of type represented by `component_id`. + unsafe fn new( + component_id: ComponentId, + source_component_ptr: Ptr<'a>, + target_components_ptrs: &'a mut Vec>, + target_components_buffer: &'b Bump, + components: &'a Components, + entity_cloner: &'a EntityCloner, + #[cfg(feature = "bevy_reflect")] type_registry: Option<&'a crate::reflect::AppTypeRegistry>, + ) -> Self { + Self { + component_id, + source_component_ptr, + target_components_ptrs, + target_component_written: false, + target_components_buffer, + components, + entity_cloner, + #[cfg(feature = "bevy_reflect")] + type_registry, + } + } + + /// Returns true if [`write_target_component`](`Self::write_target_component`) was called before. + pub fn target_component_written(&self) -> bool { + self.target_component_written + } + /// Returns the current source entity. pub fn source(&self) -> Entity { - self.source + self.entity_cloner.source } /// Returns the current target entity. pub fn target(&self) -> Entity { - self.target + self.entity_cloner.target } /// Returns the [`ComponentId`] of currently cloned component. @@ -46,6 +79,8 @@ impl<'a> ComponentCloneCtx<'a> { self.component_id } + /// Returns a reference to the component on the source entity. + /// Will return `None` if ComponentId of requested component does not match ComponentId of source component pub fn read_source_component(&self) -> Option<&T> { if self .components @@ -53,41 +88,63 @@ impl<'a> ComponentCloneCtx<'a> { .is_some_and(|id| id == self.component_id) { // SAFETY: - // 1. Components and ComponentId are from the same world - // 2. source_component_ptr holds valid data of the type referenced by ComponentId + // - Components and ComponentId are from the same world + // - source_component_ptr holds valid data of the type referenced by ComponentId unsafe { Some(self.source_component_ptr.deref::()) } } else { None } } + /// Writes component data to target entity. + /// + /// # Panics + /// This will panic if: + /// - `write_target_component` called more than once. + /// - Component being written is not registered in the world. + /// - `ComponentId` of component being written does not match expected `ComponentId`. pub fn write_target_component(&mut self, component: T) { let short_name = disqualified::ShortName::of::(); if self.target_component_written { panic!("Trying to write component '{short_name}' multiple times") } - if !self - .components - .component_id::() - .is_some_and(|id| id == self.component_id) - { + let Some(component_id) = self.components.component_id::() else { + panic!("Component '{short_name}' is not registered") + }; + if component_id != self.component_id { panic!("Component '{short_name}' does not match ComponentId of this ComponentCloneCtx"); } - // SAFETY: - // 1. Components and ComponentId are from the same world - // 2. target_component_ptr holds valid data of the type referenced by ComponentId - unsafe { self.target_component_ptr.cast::().write(component) }; + let component_ref = self.target_components_buffer.alloc(component); + self.target_components_ptrs + .push(PtrMut::from(component_ref)); self.target_component_written = true } - pub fn clone_entity_fn(&self, source: Entity, target: Entity) -> EntityCloner { - self.entity_cloner.with_source_and_target(source, target) + /// Return a reference to this context's `EntityCloner` instance. + /// + /// This can be used to issue clone commands using the same cloning configuration: + /// ``` + /// fn clone_handler(&mut world: DeferredWorld, ctx: &mut ComponentCloneCtx) { + /// let another_target = world.spawn_empty(); + /// let mut entity_cloner = ctx + /// .entity_cloner() + /// .with_source_and_target(ctx.source(), another_target); + /// world.commands().queue(move |world| { + /// entity_cloner.clone_entity(world); + /// }); + /// } + /// ``` + pub fn entity_cloner(&self) -> &EntityCloner { + self.entity_cloner } - // #[cfg(feature = "bevy_reflect")] - // pub fn type_registry(&self) -> Option<&App> { - // self.type_registry.as_ref().read() - // } + /// Returns [`AppTypeRegistry`](`crate::reflect::AppTypeRegistry`) if it exists in the world. + /// + /// NOTE: Prefer this method instead of manually reading the resource from the world. + #[cfg(feature = "bevy_reflect")] + pub fn type_registry(&self) -> Option<&crate::reflect::AppTypeRegistry> { + self.type_registry + } } /// A helper struct to clone an entity. Used internally by [`EntityCloneBuilder::clone_entity`]. @@ -102,7 +159,7 @@ pub struct EntityCloner { impl EntityCloner { /// Clones and inserts components from the `source` entity into `target` entity using the stored configuration. pub fn clone_entity(&mut self, world: &mut World) { - let (app_registry, source_entity, storages, components, mut deferred_world) = unsafe { + let (type_registry, source_entity, components, mut deferred_world) = unsafe { let world = world.as_unsafe_world_cell(); let source_entity = world .get_entity(self.source) @@ -116,67 +173,15 @@ impl EntityCloner { ( app_registry, source_entity, - world.storages(), world.components(), world.into_deferred(), ) }; let archetype = source_entity.archetype(); - let source_location = source_entity.location(); - - // // To clone components without table moves we need to use `insert_by_ids`. - // // For this, we need 2 things: - // // 1. list of component ids - // // 2. list of aligned pointers to component data - // // - // // To reduce the number of memory allocations and provide better data locality, we need to store all this data - // // in one big buffer. - // // The structure of this buffer is as follows: - // // +--------------+------------------------+ - // // | ComponentIds | Aligned component data | - // // +--------------+------------------------+ - // // This means that we need to preallocate buffer of size (component_count * sizeof:: + Archetype row size) - - // // First, calculate ComponentIds portion of buffer. - // let mut buffer_layout = Layout::array::(archetype.component_count()).unwrap(); - // // Next, add each component aligned data layout to it. - // for component in archetype.components() { - // let layout = world.components().get_info(component).unwrap().layout(); - // let (new_layout, _) = buffer_layout.extend(layout.pad_to_align()).unwrap(); - // buffer_layout = new_layout; - // } - // let buffer_size = archetype - // .components() - // .fold(buffer_size, |mut buffer_size, id| { - // let layout = world.components().get_info(id).unwrap().layout(); - - // // Round up current size to meet alignment - // let align = layout.align(); - // buffer_size = (buffer_size + align - 1) & !(align - 1); - - // // Add size of current layout - // buffer_size += layout.size(); - - // buffer_size - // }); - // let buffer_layout = - - let mut component_data_size = 0; - for component in archetype.components() { - let layout = components.get_info(component).unwrap().layout(); - - // Round up current size to meet alignment - let align = layout.align(); - component_data_size = (component_data_size + align - 1) & !(align - 1); - // Add size of current layout - component_data_size += layout.size(); - } + let component_data = Bump::new(); let mut component_ids: Vec = Vec::with_capacity(archetype.component_count()); - let mut component_data: Vec = Vec::with_capacity(component_data_size); - component_data.push(0); - let mut component_data_offsets = Vec::with_capacity(archetype.component_count()); - let mut component_data_offset = 0; + let mut component_data_ptrs: Vec = Vec::with_capacity(archetype.component_count()); for component in archetype.components() { if !self.is_cloning_allowed(&component) { @@ -191,67 +196,47 @@ impl EntityCloner { Some(ComponentCloneHandler::Custom(handler)) => *handler, }; - let source_component_ptr = match archetype.get_storage_type(component) { - Some(StorageType::Table) => unsafe { - storages - .tables - .get(source_location.table_id) - .unwrap() - .get_component(component, source_location.table_row) - .unwrap() - }, - Some(StorageType::SparseSet) => storages - .sparse_sets - .get(component) - .unwrap() - .get(self.source) - .unwrap(), - None => continue, + // SAFETY: There are no other mutable references to source entity. + let Some(source_component_ptr) = (unsafe { source_entity.get_by_id(component) }) else { + continue; }; - let mut ctx = ComponentCloneCtx { - source: self.source, - target: self.target, - component_id: component, - entity_cloner: &self, - components, - source_component_ptr, - target_component_ptr: unsafe { - NonNull::new_unchecked(component_data.as_mut_ptr()) - }, - target_component_written: false, - // type_registry, + // SAFETY: + // - `components` and `component` are from the same world + // - `source_component_ptr` is valid and points to the same type as represented by `component` + let mut ctx = unsafe { + ComponentCloneCtx::new( + component, + source_component_ptr, + &mut component_data_ptrs, + &component_data, + components, + &self, + #[cfg(feature = "bevy_reflect")] + type_registry, + ) }; (handler)(&mut deferred_world, &mut ctx); - if !ctx.target_component_written { - continue; + if ctx.target_component_written { + component_ids.push(component) } - - component_ids.push(component); - component_data_offsets.push(component_data_offset); - - let layout = components.get_info(component).unwrap().layout(); - - // Round up current size to meet alignment - let align = layout.align(); - component_data_offset = (component_data_offset + align - 1) & !(align - 1); - - // Add size of current layout - component_data_offset += layout.size(); } world.flush(); + if !world.entities.contains(self.target) { + panic!("Target entity does not exist"); + } + + // SAFETY: + // - All `component_ids` are from the same world as `target` entity + // - All `component_data_ptrs` are valid types represented by `component_ids` unsafe { world.entity_mut(self.target).insert_by_ids( &component_ids, - component_data_offsets.iter().map(|offset| { - OwningPtr::new({ - NonNull::new_unchecked(component_data.as_mut_ptr()).add(*offset) - }) - }), + component_data_ptrs.into_iter().map(|ptr| ptr.promote()), ); } } @@ -262,7 +247,7 @@ impl EntityCloner { } /// Reuse existing [`EntityCloner`] configuration with new source and target. - fn with_source_and_target(&self, source: Entity, target: Entity) -> EntityCloner { + pub fn with_source_and_target(&self, source: Entity, target: Entity) -> EntityCloner { EntityCloner { source, target, diff --git a/crates/bevy_hierarchy/src/hierarchy.rs b/crates/bevy_hierarchy/src/hierarchy.rs index 234d4e86f3eeb..b8539fb01b0d0 100644 --- a/crates/bevy_hierarchy/src/hierarchy.rs +++ b/crates/bevy_hierarchy/src/hierarchy.rs @@ -241,7 +241,9 @@ fn component_clone_children(world: &mut DeferredWorld, ctx: &mut ComponentCloneC let parent = ctx.target(); for child in children { let child_clone = world.commands().spawn_empty().id(); - let mut clone_entity = ctx.clone_entity_fn(*child, child_clone); + let mut clone_entity = ctx + .entity_cloner() + .with_source_and_target(*child, child_clone); world.commands().queue(move |world: &mut World| { clone_entity.clone_entity(world); world.entity_mut(child_clone).set_parent(parent); From c710dff62e50c5dc67672ed62186a6275f5d8b87 Mon Sep 17 00:00:00 2001 From: eugineerd Date: Sat, 7 Dec 2024 21:05:25 +0000 Subject: [PATCH 04/40] add `read_source_component_reflect` --- crates/bevy_ecs/src/component.rs | 25 ++++---- crates/bevy_ecs/src/entity/clone_entities.rs | 66 +++++++++++++++++++- 2 files changed, 76 insertions(+), 15 deletions(-) diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index 5b3d4296ff557..1c4699e8357cb 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -1979,34 +1979,31 @@ pub fn component_clone_via_clone( /// See [`ComponentCloneHandlers`] for more details. #[cfg(feature = "bevy_reflect")] pub fn component_clone_via_reflect(world: &mut DeferredWorld, ctx: &mut ComponentCloneCtx) { - let component_id = ctx.component_id(); - let source = ctx.source(); + let Some(source_component_reflect) = ctx.read_source_component_reflect() else { + return; + }; + + let component_info = world.components().get_info(ctx.component_id()).unwrap(); + let type_id = component_info.type_id().unwrap(); + + let source_component = source_component_reflect.clone_value(); let target = ctx.target(); + world.commands().queue(move |world: &mut World| { world.resource_scope::(|world, registry| { let registry = registry.read(); - let component_info = world - .components() - .get_info(component_id) - .expect("Component must be registered"); - let Some(type_id) = component_info.type_id() else { - return; - }; let Some(reflect_component) = registry.get_type_data::(type_id) else { return; }; - let source_component = reflect_component - .reflect(world.get_entity(source).expect("Source entity must exist")) - .expect("Source entity must have reflected component") - .clone_value(); + let mut target = world .get_entity_mut(target) .expect("Target entity must exist"); reflect_component.apply_or_insert(&mut target, &*source_component, ®istry); - }); + }) }); } diff --git a/crates/bevy_ecs/src/entity/clone_entities.rs b/crates/bevy_ecs/src/entity/clone_entities.rs index 4049510852866..0b6c2f63582bb 100644 --- a/crates/bevy_ecs/src/entity/clone_entities.rs +++ b/crates/bevy_ecs/src/entity/clone_entities.rs @@ -80,7 +80,8 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> { } /// Returns a reference to the component on the source entity. - /// Will return `None` if ComponentId of requested component does not match ComponentId of source component + /// + /// Will return `None` if `ComponentId` of requested component does not match `ComponentId` of source component pub fn read_source_component(&self) -> Option<&T> { if self .components @@ -96,6 +97,22 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> { } } + /// Returns a reference to the component on the source entity as [`&dyn Reflect`](bevy_reflect::Reflect). + /// + /// Will return `None` if: + /// - World does not have [`AppTypeRegistry`](`crate::reflect::AppTypeRegistry`). + /// - Component does not implement [`ReflectFromPtr`](bevy_reflect::ReflectFromPtr). + /// - Component is not registered. + /// - Component does not have [`TypeId`] + #[cfg(feature = "bevy_reflect")] + pub fn read_source_component_reflect(&self) -> Option<&dyn bevy_reflect::Reflect> { + let registry = self.type_registry?.read(); + let type_id = self.components.get_info(self.component_id)?.type_id()?; + let reflect_from_ptr = registry.get_type_data::(type_id)?; + // SAFETY: `source_component_ptr` stores data represented by `component_id`, which we used to get `ReflectFromPtr`. + unsafe { Some(reflect_from_ptr.as_reflect(self.source_component_ptr)) } + } + /// Writes component data to target entity. /// /// # Panics @@ -120,6 +137,53 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> { self.target_component_written = true } + // /// Writes component data to target entity. + // /// + // /// # Panics + // /// This will panic if: + // /// - World does not have [`AppTypeRegistry`](`crate::reflect::AppTypeRegistry`). + // /// - Component does not implement [`ReflectFromPtr`](bevy_reflect::ReflectFromPtr). + // /// - Component is not registered. + // /// - Component does not have [`TypeId`] + // #[cfg(feature = "bevy_reflect")] + // pub fn write_target_component_reflect(&mut self, component: impl bevy_reflect::Reflect) { + // let registry = self.type_registry.unwrap().read(); + // let component_info = self.components.get_info(self.component_id).unwrap(); + // let component_layout = component_info.layout(); + // let type_id = component_info.type_id().unwrap(); + // let reflect_from_ptr = registry + // .get_type_data::(type_id) + // .unwrap(); + + // let target_component_data_ptr = + // self.target_components_buffer.alloc_layout(component_layout); + // unsafe { + // core::ptr::copy_nonoverlapping( + // self.source_component_ptr.as_ptr(), + // target_component_data_ptr.as_ptr(), + // component_layout.size(), + // ); + // let target_reflect = + // reflect_from_ptr.as_reflect_mut(PtrMut::new(target_component_data_ptr)); + // *target_reflect = component; + // } + + // let short_name = disqualified::ShortName::of::(); + // if self.target_component_written { + // panic!("Trying to write component '{short_name}' multiple times") + // } + // let Some(component_id) = self.components.component_id::() else { + // panic!("Component '{short_name}' is not registered") + // }; + // if component_id != self.component_id { + // panic!("Component '{short_name}' does not match ComponentId of this ComponentCloneCtx"); + // } + // let component_ref = self.target_components_buffer.alloc(component); + // self.target_components_ptrs + // .push(PtrMut::from(component_ref)); + // self.target_component_written = true + // } + /// Return a reference to this context's `EntityCloner` instance. /// /// This can be used to issue clone commands using the same cloning configuration: From 581d2d0a456c84abc637a6aca16e55fdbb00ac7a Mon Sep 17 00:00:00 2001 From: eugineerd Date: Sat, 7 Dec 2024 21:49:34 +0000 Subject: [PATCH 05/40] redesign `ComponentCloneHandler` --- crates/bevy_ecs/src/component.rs | 46 ++++++++++++++----- crates/bevy_ecs/src/entity/clone_entities.rs | 19 ++++---- .../bevy_ecs/src/observer/entity_observer.rs | 8 ++-- .../bevy_hierarchy/src/components/children.rs | 2 +- .../bevy_hierarchy/src/components/parent.rs | 2 +- crates/bevy_hierarchy/src/hierarchy.rs | 8 ++-- 6 files changed, 55 insertions(+), 30 deletions(-) diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index 5b3d4296ff557..5388b0511eafd 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -885,16 +885,42 @@ impl ComponentDescriptor { /// Function type that can be used to clone an entity. pub type ComponentCloneFn = fn(&mut DeferredWorld, &mut ComponentCloneCtx); -/// An enum instructing how to clone a component. +/// A struct instructing which clone handler to use when cloning a component. #[derive(Debug, Default)] -pub enum ComponentCloneHandler { - #[default] +pub struct ComponentCloneHandler(Option); + +impl ComponentCloneHandler { /// Use the global default function to clone the component with this handler. - Default, + pub fn default_handler() -> Self { + Self(None) + } + /// Do not clone the component. When a command to clone an entity is issued, component with this handler will be skipped. - Ignore, + pub fn ignore() -> Self { + Self(Some(component_clone_ignore)) + } + + /// Set clone handler based on `Clone` trait. + /// + /// If set as a handler for a component that is not the same as the one used to create this handler, it will panic. + pub fn clone_handler() -> Self { + Self(Some(component_clone_via_clone::)) + } + + /// Set clone handler based on `Reflect` trait. + pub fn reflect_handler() -> Self { + Self(Some(component_clone_via_reflect)) + } + /// Set a custom handler for the component. - Custom(ComponentCloneFn), + pub fn custom_handler(handler: ComponentCloneFn) -> Self { + Self(Some(handler)) + } + + /// Get [`ComponentCloneFn`] representing this handler or `None` if set to default handler. + pub fn get_handler(&self) -> Option { + self.0 + } } /// A registry of component clone handlers. Allows to set global default and per-component clone function for all components in the world. @@ -925,11 +951,7 @@ impl ComponentCloneHandlers { if id.0 >= self.handlers.len() { self.handlers.resize(id.0 + 1, None); } - match handler { - ComponentCloneHandler::Default => self.handlers[id.0] = None, - ComponentCloneHandler::Ignore => self.handlers[id.0] = Some(component_clone_ignore), - ComponentCloneHandler::Custom(handler) => self.handlers[id.0] = Some(handler), - }; + self.handlers[id.0] = handler.0; } /// Checks if the specified component is registered. If not, the component will use the default global handler. @@ -2043,6 +2065,6 @@ pub trait ComponentCloneViaClone { } impl ComponentCloneViaClone for &ComponentCloneSpecializationWrapper { fn get_component_clone_handler(&self) -> ComponentCloneHandler { - ComponentCloneHandler::Custom(component_clone_via_clone::) + ComponentCloneHandler::clone_handler::() } } diff --git a/crates/bevy_ecs/src/entity/clone_entities.rs b/crates/bevy_ecs/src/entity/clone_entities.rs index 4049510852866..996bab8a3b030 100644 --- a/crates/bevy_ecs/src/entity/clone_entities.rs +++ b/crates/bevy_ecs/src/entity/clone_entities.rs @@ -7,9 +7,7 @@ use bevy_utils::{HashMap, HashSet}; use crate::{ bundle::Bundle, - component::{ - component_clone_ignore, Component, ComponentCloneHandler, ComponentId, Components, - }, + component::{Component, ComponentCloneHandler, ComponentId, Components}, entity::Entity, world::World, }; @@ -159,6 +157,11 @@ pub struct EntityCloner { impl EntityCloner { /// Clones and inserts components from the `source` entity into `target` entity using the stored configuration. pub fn clone_entity(&mut self, world: &mut World) { + // SAFETY: + // - `source_entity` is read-only. + // - `type_registry` is read-only. + // - `components` is read-only. + // - `deferred_world` disallows structural ecs changes, which means all read-only resources above a not affected. let (type_registry, source_entity, components, mut deferred_world) = unsafe { let world = world.as_unsafe_world_cell(); let source_entity = world @@ -190,10 +193,10 @@ impl EntityCloner { let global_handlers = components.get_component_clone_handlers(); let handler = match self.clone_handlers_overrides.get(&component) { + Some(handler) => handler + .get_handler() + .unwrap_or_else(|| global_handlers.get_default_handler()), None => global_handlers.get_handler(component), - Some(ComponentCloneHandler::Default) => global_handlers.get_default_handler(), - Some(ComponentCloneHandler::Ignore) => component_clone_ignore, - Some(ComponentCloneHandler::Custom(handler)) => *handler, }; // SAFETY: There are no other mutable references to source entity. @@ -289,7 +292,7 @@ impl EntityCloner { /// It should be noted that if `Component` is implemented manually or if `Clone` implementation is conditional /// (like when deriving `Clone` for a type with a generic parameter without `Clone` bound), /// the component will be cloned using the [default cloning strategy](crate::component::ComponentCloneHandlers::get_default_handler). -/// To use `Clone`-based handler ([`component_clone_via_clone`](crate::component::component_clone_via_clone)) in this case it should be set manually using one +/// To use `Clone`-based handler ([`ComponentCloneHandler::clone_handler`]) in this case it should be set manually using one /// of the methods mentioned in the [Handlers](#handlers) section /// /// Here's an example of how to do it using [`get_component_clone_handler`](Component::get_component_clone_handler): @@ -302,7 +305,7 @@ impl EntityCloner { /// impl Component for SomeComponent { /// const STORAGE_TYPE: StorageType = StorageType::Table; /// fn get_component_clone_handler() -> ComponentCloneHandler { -/// ComponentCloneHandler::Custom(component_clone_via_clone::) +/// ComponentCloneHandler::clone_handler::() /// } /// } /// ``` diff --git a/crates/bevy_ecs/src/observer/entity_observer.rs b/crates/bevy_ecs/src/observer/entity_observer.rs index 9c2a85a326273..6533b08de0efa 100644 --- a/crates/bevy_ecs/src/observer/entity_observer.rs +++ b/crates/bevy_ecs/src/observer/entity_observer.rs @@ -42,7 +42,7 @@ impl Component for ObservedBy { } fn get_component_clone_handler() -> ComponentCloneHandler { - ComponentCloneHandler::Ignore + ComponentCloneHandler::ignore() } } @@ -55,9 +55,9 @@ pub trait CloneEntityWithObserversExt { impl CloneEntityWithObserversExt for EntityCloneBuilder<'_> { fn add_observers(&mut self, add_observers: bool) -> &mut Self { if add_observers { - self.override_component_clone_handler::(ComponentCloneHandler::Custom( - component_clone_observed_by, - )) + self.override_component_clone_handler::( + ComponentCloneHandler::custom_handler(component_clone_observed_by), + ) } else { self.remove_component_clone_handler_override::() } diff --git a/crates/bevy_hierarchy/src/components/children.rs b/crates/bevy_hierarchy/src/components/children.rs index b811c1da2dd0e..ca05c4334bd18 100644 --- a/crates/bevy_hierarchy/src/components/children.rs +++ b/crates/bevy_hierarchy/src/components/children.rs @@ -44,7 +44,7 @@ impl Component for Children { const STORAGE_TYPE: StorageType = StorageType::Table; fn get_component_clone_handler() -> ComponentCloneHandler { - ComponentCloneHandler::Ignore + ComponentCloneHandler::ignore() } } diff --git a/crates/bevy_hierarchy/src/components/parent.rs b/crates/bevy_hierarchy/src/components/parent.rs index 445857bb18d6f..3c8c103b4277c 100644 --- a/crates/bevy_hierarchy/src/components/parent.rs +++ b/crates/bevy_hierarchy/src/components/parent.rs @@ -44,7 +44,7 @@ impl Component for Parent { const STORAGE_TYPE: StorageType = StorageType::Table; fn get_component_clone_handler() -> ComponentCloneHandler { - ComponentCloneHandler::Ignore + ComponentCloneHandler::ignore() } } diff --git a/crates/bevy_hierarchy/src/hierarchy.rs b/crates/bevy_hierarchy/src/hierarchy.rs index b8539fb01b0d0..3f488fc1123ad 100644 --- a/crates/bevy_hierarchy/src/hierarchy.rs +++ b/crates/bevy_hierarchy/src/hierarchy.rs @@ -214,16 +214,16 @@ pub trait CloneEntityHierarchyExt { impl CloneEntityHierarchyExt for EntityCloneBuilder<'_> { fn recursive(&mut self, recursive: bool) -> &mut Self { if recursive { - self.override_component_clone_handler::(ComponentCloneHandler::Custom( - component_clone_children, - )) + self.override_component_clone_handler::( + ComponentCloneHandler::custom_handler(component_clone_children), + ) } else { self.remove_component_clone_handler_override::() } } fn as_child(&mut self, as_child: bool) -> &mut Self { if as_child { - self.override_component_clone_handler::(ComponentCloneHandler::Custom( + self.override_component_clone_handler::(ComponentCloneHandler::custom_handler( component_clone_parent, )) } else { From c65f20d6dbdd8d5229bea3a10ccdf3b72073da72 Mon Sep 17 00:00:00 2001 From: eugineerd Date: Sat, 7 Dec 2024 21:53:40 +0000 Subject: [PATCH 06/40] clippy fixes --- crates/bevy_ecs/src/entity/clone_entities.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/entity/clone_entities.rs b/crates/bevy_ecs/src/entity/clone_entities.rs index 996bab8a3b030..2debdde585e18 100644 --- a/crates/bevy_ecs/src/entity/clone_entities.rs +++ b/crates/bevy_ecs/src/entity/clone_entities.rs @@ -78,7 +78,7 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> { } /// Returns a reference to the component on the source entity. - /// Will return `None` if ComponentId of requested component does not match ComponentId of source component + /// Will return `None` if `ComponentId` of requested component does not match `ComponentId` of source component pub fn read_source_component(&self) -> Option<&T> { if self .components @@ -115,7 +115,7 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> { let component_ref = self.target_components_buffer.alloc(component); self.target_components_ptrs .push(PtrMut::from(component_ref)); - self.target_component_written = true + self.target_component_written = true; } /// Return a reference to this context's `EntityCloner` instance. @@ -214,7 +214,7 @@ impl EntityCloner { &mut component_data_ptrs, &component_data, components, - &self, + self, #[cfg(feature = "bevy_reflect")] type_registry, ) @@ -223,7 +223,7 @@ impl EntityCloner { (handler)(&mut deferred_world, &mut ctx); if ctx.target_component_written { - component_ids.push(component) + component_ids.push(component); } } From 676b7f2599784b95b7a5565931f3f1d140cd5271 Mon Sep 17 00:00:00 2001 From: eugineerd Date: Sun, 8 Dec 2024 19:51:48 +0000 Subject: [PATCH 07/40] implement fast entity cloning for reflect handler (for types with ReflectDefault only) --- crates/bevy_ecs/src/component.rs | 66 +++++++--- crates/bevy_ecs/src/entity/clone_entities.rs | 124 ++++++++++++------- 2 files changed, 124 insertions(+), 66 deletions(-) diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index 1c4699e8357cb..ad74aa56a4ec2 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -1979,31 +1979,57 @@ pub fn component_clone_via_clone( /// See [`ComponentCloneHandlers`] for more details. #[cfg(feature = "bevy_reflect")] pub fn component_clone_via_reflect(world: &mut DeferredWorld, ctx: &mut ComponentCloneCtx) { - let Some(source_component_reflect) = ctx.read_source_component_reflect() else { + fn clone_fast(ctx: &mut ComponentCloneCtx) -> Option<()> { + let registry = ctx.type_registry()?; + let component = { + let source_component_reflect = ctx.read_source_component_reflect()?; + let registry = registry.read(); + let component_info = ctx.components().get_info(ctx.component_id())?; + let type_id = component_info.type_id()?; + let reflect_default = + registry.get_type_data::(type_id)?; + let mut component = reflect_default.default(); + component.apply(source_component_reflect.as_partial_reflect()); + component + }; + ctx.write_target_component_reflect(component); + + Some(()) + } + + fn clone_slow( + component_id: ComponentId, + source: Entity, + target: Entity, + registry: &crate::reflect::AppTypeRegistry, + world: &mut World, + ) -> Option<()> { + let registry = registry.read(); + + let component_info = world.components().get_info(component_id)?; + let type_id = component_info.type_id()?; + let reflect_component = + registry.get_type_data::(type_id)?; + let source_component = reflect_component + .reflect(world.get_entity(source).ok()?)? + .clone_value(); + let mut target = world.get_entity_mut(target).ok()?; + reflect_component.apply_or_insert(&mut target, &*source_component, ®istry); + Some(()) + } + + // Try to clone fast first, if it doesn't work use the slow path + if clone_fast(ctx).is_some() { return; }; - let component_info = world.components().get_info(ctx.component_id()).unwrap(); - let type_id = component_info.type_id().unwrap(); - - let source_component = source_component_reflect.clone_value(); + let component_id = ctx.component_id(); + let source = ctx.source(); let target = ctx.target(); - world.commands().queue(move |world: &mut World| { - world.resource_scope::(|world, registry| { - let registry = registry.read(); - - let Some(reflect_component) = - registry.get_type_data::(type_id) - else { - return; - }; - - let mut target = world - .get_entity_mut(target) - .expect("Target entity must exist"); - reflect_component.apply_or_insert(&mut target, &*source_component, ®istry); - }) + world.resource_scope(|world, registry| { + clone_slow(component_id, source, target, &*registry, world); + }); }); } diff --git a/crates/bevy_ecs/src/entity/clone_entities.rs b/crates/bevy_ecs/src/entity/clone_entities.rs index 0b6c2f63582bb..2aba7f7f8f477 100644 --- a/crates/bevy_ecs/src/entity/clone_entities.rs +++ b/crates/bevy_ecs/src/entity/clone_entities.rs @@ -137,52 +137,47 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> { self.target_component_written = true } - // /// Writes component data to target entity. - // /// - // /// # Panics - // /// This will panic if: - // /// - World does not have [`AppTypeRegistry`](`crate::reflect::AppTypeRegistry`). - // /// - Component does not implement [`ReflectFromPtr`](bevy_reflect::ReflectFromPtr). - // /// - Component is not registered. - // /// - Component does not have [`TypeId`] - // #[cfg(feature = "bevy_reflect")] - // pub fn write_target_component_reflect(&mut self, component: impl bevy_reflect::Reflect) { - // let registry = self.type_registry.unwrap().read(); - // let component_info = self.components.get_info(self.component_id).unwrap(); - // let component_layout = component_info.layout(); - // let type_id = component_info.type_id().unwrap(); - // let reflect_from_ptr = registry - // .get_type_data::(type_id) - // .unwrap(); - - // let target_component_data_ptr = - // self.target_components_buffer.alloc_layout(component_layout); - // unsafe { - // core::ptr::copy_nonoverlapping( - // self.source_component_ptr.as_ptr(), - // target_component_data_ptr.as_ptr(), - // component_layout.size(), - // ); - // let target_reflect = - // reflect_from_ptr.as_reflect_mut(PtrMut::new(target_component_data_ptr)); - // *target_reflect = component; - // } - - // let short_name = disqualified::ShortName::of::(); - // if self.target_component_written { - // panic!("Trying to write component '{short_name}' multiple times") - // } - // let Some(component_id) = self.components.component_id::() else { - // panic!("Component '{short_name}' is not registered") - // }; - // if component_id != self.component_id { - // panic!("Component '{short_name}' does not match ComponentId of this ComponentCloneCtx"); - // } - // let component_ref = self.target_components_buffer.alloc(component); - // self.target_components_ptrs - // .push(PtrMut::from(component_ref)); - // self.target_component_written = true - // } + /// Writes component data to target entity. + /// + /// # Panics + /// This will panic if: + /// - World does not have [`AppTypeRegistry`](`crate::reflect::AppTypeRegistry`). + /// - Component does not implement [`ReflectFromPtr`](bevy_reflect::ReflectFromPtr). + /// - Component is not registered. + /// - Component does not have [`TypeId`] + /// - Passed component's [`TypeId`] does not match source component [`TypeId`] + #[cfg(feature = "bevy_reflect")] + pub fn write_target_component_reflect(&mut self, component: Box) { + let source_type_id = self + .components + .get_info(self.component_id) + .unwrap() + .type_id() + .unwrap(); + let component_type_id = component.reflect_type_info().type_id(); + if source_type_id != component_type_id { + panic!("Passed component TypeId does not match source component TypeId") + } + let component_info = self.components.get_info(self.component_id).unwrap(); + let component_layout = component_info.layout(); + + let component_data_ptr = Box::into_raw(component) as *mut u8; + let target_component_data_ptr = + self.target_components_buffer.alloc_layout(component_layout); + // SAFETY: + // - target_component_data_ptr and component_data have the same data type. + // - component_data_ptr has layout of component_layout + unsafe { + core::ptr::copy_nonoverlapping( + component_data_ptr, + target_component_data_ptr.as_ptr(), + component_layout.size(), + ); + alloc::alloc::dealloc(component_data_ptr, component_layout); + } + + self.target_component_written = true + } /// Return a reference to this context's `EntityCloner` instance. /// @@ -202,6 +197,11 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> { self.entity_cloner } + /// Returns instance of [`Components`]. + pub fn components(&self) -> &Components { + self.components + } + /// Returns [`AppTypeRegistry`](`crate::reflect::AppTypeRegistry`) if it exists in the world. /// /// NOTE: Prefer this method instead of manually reading the resource from the world. @@ -585,6 +585,38 @@ mod tests { assert!(world.get::(e_clone).is_some_and(|c| *c == component)); } + // TODO: remove this when 13432 lands + #[cfg(feature = "bevy_reflect")] + #[test] + fn clone_entity_using_reflect_with_default() { + use crate::reflect::{AppTypeRegistry, ReflectComponent}; + use bevy_reflect::{std_traits::ReflectDefault, Reflect}; + + #[derive(Component, Reflect, Clone, PartialEq, Eq, Default)] + #[reflect(Component, Default)] + struct A { + field: usize, + field2: Vec, + } + + let mut world = World::default(); + world.init_resource::(); + let registry = world.get_resource::().unwrap(); + registry.write().register::(); + + let component = A { + field: 5, + field2: vec![1, 2, 3, 4, 5], + }; + + let e = world.spawn(component.clone()).id(); + let e_clone = world.spawn_empty().id(); + + EntityCloneBuilder::new(&mut world).clone_entity(e, e_clone); + + assert!(world.get::(e_clone).is_some_and(|c| *c == component)); + } + #[test] fn clone_entity_using_clone() { #[derive(Component, Clone, PartialEq, Eq)] From fd7307591c28a20dfa49f642e99cb2192b223891 Mon Sep 17 00:00:00 2001 From: eugineerd Date: Sun, 8 Dec 2024 19:52:14 +0000 Subject: [PATCH 08/40] improve entity cloning benchmark --- benches/benches/bevy_ecs/entity_cloning.rs | 172 +++++++-------------- 1 file changed, 59 insertions(+), 113 deletions(-) diff --git a/benches/benches/bevy_ecs/entity_cloning.rs b/benches/benches/bevy_ecs/entity_cloning.rs index 54959838c8f94..76b38c7f4ec21 100644 --- a/benches/benches/bevy_ecs/entity_cloning.rs +++ b/benches/benches/bevy_ecs/entity_cloning.rs @@ -3,95 +3,59 @@ use bevy_ecs::reflect::AppTypeRegistry; use bevy_ecs::{component::Component, reflect::ReflectComponent, world::World}; use bevy_hierarchy::{BuildChildren, CloneEntityHierarchyExt}; use bevy_math::Mat4; +use bevy_reflect::prelude::ReflectDefault; use bevy_reflect::{GetTypeRegistration, Reflect}; use criterion::{black_box, criterion_group, criterion_main, Bencher, Criterion}; criterion_group!(benches, reflect_benches, clone_benches); criterion_main!(benches); -#[derive(Component, Reflect, Default)] -#[reflect(Component)] -struct ReflectComponent1(Mat4); - -#[derive(Component, Reflect, Default)] -#[reflect(Component)] -struct ReflectComponent2(Mat4); - -#[derive(Component, Reflect, Default)] -#[reflect(Component)] -struct ReflectComponent3(Mat4); - -#[derive(Component, Reflect, Default)] -#[reflect(Component)] -struct ReflectComponent4(Mat4); - -#[derive(Component, Reflect, Default)] -#[reflect(Component)] -struct ReflectComponent5(Mat4); - -#[derive(Component, Reflect, Default)] -#[reflect(Component)] -struct ReflectComponent6(Mat4); - -#[derive(Component, Reflect, Default)] -#[reflect(Component)] -struct ReflectComponent7(Mat4); - -#[derive(Component, Reflect, Default)] -#[reflect(Component)] -struct ReflectComponent8(Mat4); - -#[derive(Component, Reflect, Default)] -#[reflect(Component)] -struct ReflectComponent9(Mat4); - -#[derive(Component, Reflect, Default)] -#[reflect(Component)] -struct ReflectComponent10(Mat4); - #[derive(Component, Reflect, Default, Clone)] -#[reflect(Component)] -struct CloneComponent1(Mat4); +#[reflect(Component, Default)] +struct C1(Mat4); #[derive(Component, Reflect, Default, Clone)] -#[reflect(Component)] -struct CloneComponent2(Mat4); +#[reflect(Component, Default)] +struct C2(Mat4); #[derive(Component, Reflect, Default, Clone)] -#[reflect(Component)] -struct CloneComponent3(Mat4); +#[reflect(Component, Default)] +struct C3(Mat4); #[derive(Component, Reflect, Default, Clone)] -#[reflect(Component)] -struct CloneComponent4(Mat4); +#[reflect(Component, Default)] +struct C4(Mat4); #[derive(Component, Reflect, Default, Clone)] -#[reflect(Component)] -struct CloneComponent5(Mat4); +#[reflect(Component, Default)] +struct C5(Mat4); #[derive(Component, Reflect, Default, Clone)] -#[reflect(Component)] -struct CloneComponent6(Mat4); +#[reflect(Component, Default)] +struct C6(Mat4); #[derive(Component, Reflect, Default, Clone)] -#[reflect(Component)] -struct CloneComponent7(Mat4); +#[reflect(Component, Default)] +struct C7(Mat4); #[derive(Component, Reflect, Default, Clone)] -#[reflect(Component)] -struct CloneComponent8(Mat4); +#[reflect(Component, Default)] +struct C8(Mat4); #[derive(Component, Reflect, Default, Clone)] -#[reflect(Component)] -struct CloneComponent9(Mat4); +#[reflect(Component, Default)] +struct C9(Mat4); #[derive(Component, Reflect, Default, Clone)] -#[reflect(Component)] -struct CloneComponent10(Mat4); +#[reflect(Component, Default)] +struct C10(Mat4); + +type ComplexBundle = (C1, C2, C3, C4, C5, C6, C7, C8, C9, C10); fn hierarchy( b: &mut Bencher, width: usize, height: usize, + clone_via_reflect: bool, ) { let mut world = World::default(); let registry = AppTypeRegistry::default(); @@ -100,6 +64,19 @@ fn hierarchy( r.register::(); } world.insert_resource(registry); + world.register_bundle::(); + if clone_via_reflect { + let mut components = Vec::new(); + C::get_component_ids(world.components(), &mut |id| components.push(id.unwrap())); + for component in components { + world + .get_component_clone_handlers_mut() + .set_component_handler( + component, + bevy_ecs::component::ComponentCloneHandler::Default, + ); + } + } let id = world.spawn(black_box(C::default())).id(); @@ -128,7 +105,7 @@ fn hierarchy( }); } -fn simple(b: &mut Bencher) { +fn simple(b: &mut Bencher, clone_via_reflect: bool) { let mut world = World::default(); let registry = AppTypeRegistry::default(); { @@ -136,6 +113,19 @@ fn simple(b: &mut Bencher) { r.register::(); } world.insert_resource(registry); + world.register_bundle::(); + if clone_via_reflect { + let mut components = Vec::new(); + C::get_component_ids(world.components(), &mut |id| components.push(id.unwrap())); + for component in components { + world + .get_component_clone_handlers_mut() + .set_component_handler( + component, + bevy_ecs::component::ComponentCloneHandler::Default, + ); + } + } let id = world.spawn(black_box(C::default())).id(); b.iter(move || { @@ -146,80 +136,36 @@ fn simple(b: &mut Bencher) { fn reflect_benches(c: &mut Criterion) { c.bench_function("many components reflect", |b| { - simple::<( - ReflectComponent1, - ReflectComponent2, - ReflectComponent3, - ReflectComponent4, - ReflectComponent5, - ReflectComponent6, - ReflectComponent7, - ReflectComponent8, - ReflectComponent9, - ReflectComponent10, - )>(b); + simple::(b, true); }); c.bench_function("hierarchy wide reflect", |b| { - hierarchy::(b, 5, 4); + hierarchy::(b, 10, 4, true); }); c.bench_function("hierarchy tall reflect", |b| { - hierarchy::(b, 1, 50); + hierarchy::(b, 1, 50, true); }); c.bench_function("hierarchy many reflect", |b| { - hierarchy::<( - ReflectComponent1, - ReflectComponent2, - ReflectComponent3, - ReflectComponent4, - ReflectComponent5, - ReflectComponent6, - ReflectComponent7, - ReflectComponent8, - ReflectComponent9, - ReflectComponent10, - )>(b, 3, 3); + hierarchy::(b, 5, 5, true); }); } fn clone_benches(c: &mut Criterion) { c.bench_function("many components clone", |b| { - simple::<( - CloneComponent1, - CloneComponent2, - CloneComponent3, - CloneComponent4, - CloneComponent5, - CloneComponent6, - CloneComponent7, - CloneComponent8, - CloneComponent9, - CloneComponent10, - )>(b); + simple::(b, false); }); c.bench_function("hierarchy wide clone", |b| { - hierarchy::(b, 5, 4); + hierarchy::(b, 10, 4, false); }); c.bench_function("hierarchy tall clone", |b| { - hierarchy::(b, 1, 50); + hierarchy::(b, 1, 50, false); }); c.bench_function("hierarchy many clone", |b| { - hierarchy::<( - CloneComponent1, - CloneComponent2, - CloneComponent3, - CloneComponent4, - CloneComponent5, - CloneComponent6, - CloneComponent7, - CloneComponent8, - CloneComponent9, - CloneComponent10, - )>(b, 3, 3); + hierarchy::(b, 5, 5, false); }); } From 56e6ab2ff4fcf842283bf34ad1fd6cf6e7650b45 Mon Sep 17 00:00:00 2001 From: eugineerd Date: Sun, 8 Dec 2024 20:07:52 +0000 Subject: [PATCH 09/40] clippy fixes --- benches/benches/bevy_ecs/entity_cloning.rs | 8 ++++---- crates/bevy_ecs/src/component.rs | 2 +- crates/bevy_ecs/src/entity/clone_entities.rs | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/benches/benches/bevy_ecs/entity_cloning.rs b/benches/benches/bevy_ecs/entity_cloning.rs index 76b38c7f4ec21..1469ef0ec49af 100644 --- a/benches/benches/bevy_ecs/entity_cloning.rs +++ b/benches/benches/bevy_ecs/entity_cloning.rs @@ -73,7 +73,7 @@ fn hierarchy( .get_component_clone_handlers_mut() .set_component_handler( component, - bevy_ecs::component::ComponentCloneHandler::Default, + bevy_ecs::component::ComponentCloneHandler::default_handler(), ); } } @@ -98,7 +98,7 @@ fn hierarchy( world.flush(); b.iter(move || { - world.commands().clone_entity_with(id, |builder| { + world.commands().entity(id).clone_with(|builder| { builder.recursive(true); }); world.flush(); @@ -122,14 +122,14 @@ fn simple(b: &mut Bencher, clone_via_ .get_component_clone_handlers_mut() .set_component_handler( component, - bevy_ecs::component::ComponentCloneHandler::Default, + bevy_ecs::component::ComponentCloneHandler::default_handler(), ); } } let id = world.spawn(black_box(C::default())).id(); b.iter(move || { - world.commands().clone_entity(id); + world.commands().entity(id).clone(); world.flush(); }); } diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index f41e3a07b4463..4de5fa75a8047 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -2136,7 +2136,7 @@ pub fn component_clone_via_reflect(world: &mut DeferredWorld, ctx: &mut Componen let target = ctx.target(); world.commands().queue(move |world: &mut World| { world.resource_scope(|world, registry| { - clone_slow(component_id, source, target, &*registry, world); + clone_slow(component_id, source, target, ®istry, world); }); }); } diff --git a/crates/bevy_ecs/src/entity/clone_entities.rs b/crates/bevy_ecs/src/entity/clone_entities.rs index f8ca331586f7c..3d441161ffdfd 100644 --- a/crates/bevy_ecs/src/entity/clone_entities.rs +++ b/crates/bevy_ecs/src/entity/clone_entities.rs @@ -159,7 +159,7 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> { let component_info = self.components.get_info(self.component_id).unwrap(); let component_layout = component_info.layout(); - let component_data_ptr = Box::into_raw(component) as *mut u8; + let component_data_ptr = Box::into_raw(component).cast::(); let target_component_data_ptr = self.target_components_buffer.alloc_layout(component_layout); // SAFETY: From b4f53d59dfd1a8bde40e8bbbcb081c900a9a1258 Mon Sep 17 00:00:00 2001 From: eugineerd Date: Sun, 8 Dec 2024 21:17:43 +0000 Subject: [PATCH 10/40] fix doctests --- crates/bevy_ecs/src/entity/clone_entities.rs | 1 + crates/bevy_ecs/src/world/mod.rs | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/entity/clone_entities.rs b/crates/bevy_ecs/src/entity/clone_entities.rs index 3d441161ffdfd..573b175568045 100644 --- a/crates/bevy_ecs/src/entity/clone_entities.rs +++ b/crates/bevy_ecs/src/entity/clone_entities.rs @@ -181,6 +181,7 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> { /// /// This can be used to issue clone commands using the same cloning configuration: /// ``` + /// # use bevy_ecs::{DeferredWorld, ComponentCloneCtx}; /// fn clone_handler(&mut world: DeferredWorld, ctx: &mut ComponentCloneCtx) { /// let another_target = world.spawn_empty(); /// let mut entity_cloner = ctx diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 491c3648dd043..d6cb0d7493289 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -3348,12 +3348,12 @@ impl World { /// ``` /// # use bevy_ecs::prelude::*; /// use bevy_ecs::component::{ComponentId, ComponentCloneHandler}; - /// use bevy_ecs::entity::EntityCloner; + /// use bevy_ecs::entity::ComponentCloneCtx; /// use bevy_ecs::world::DeferredWorld; /// /// fn custom_clone_handler( /// _world: &mut DeferredWorld, - /// _entity_cloner: &EntityCloner, + /// _ctx: &mut ComponentCloneCtx, /// ) { /// // Custom cloning logic for component /// } @@ -3366,7 +3366,7 @@ impl World { /// let component_id = world.register_component::(); /// /// world.get_component_clone_handlers_mut() - /// .set_component_handler(component_id, ComponentCloneHandler::Custom(custom_clone_handler)) + /// .set_component_handler(component_id, ComponentCloneHandler::custom_handler(custom_clone_handler)) /// ``` pub fn get_component_clone_handlers_mut(&mut self) -> &mut ComponentCloneHandlers { self.components.get_component_clone_handlers_mut() From 86990573c7e7503b6d79a00a69ad4beea8777704 Mon Sep 17 00:00:00 2001 From: eugineerd Date: Sun, 8 Dec 2024 21:31:01 +0000 Subject: [PATCH 11/40] actually fix doctests --- crates/bevy_ecs/src/entity/clone_entities.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/entity/clone_entities.rs b/crates/bevy_ecs/src/entity/clone_entities.rs index 573b175568045..2b6224cd7c070 100644 --- a/crates/bevy_ecs/src/entity/clone_entities.rs +++ b/crates/bevy_ecs/src/entity/clone_entities.rs @@ -181,13 +181,14 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> { /// /// This can be used to issue clone commands using the same cloning configuration: /// ``` - /// # use bevy_ecs::{DeferredWorld, ComponentCloneCtx}; - /// fn clone_handler(&mut world: DeferredWorld, ctx: &mut ComponentCloneCtx) { - /// let another_target = world.spawn_empty(); + /// # use bevy_ecs::world::{DeferredWorld, World}; + /// # use bevy_ecs::entity::ComponentCloneCtx; + /// fn clone_handler(world: &mut DeferredWorld, ctx: &mut ComponentCloneCtx) { + /// let another_target = world.commands().spawn_empty().id(); /// let mut entity_cloner = ctx /// .entity_cloner() /// .with_source_and_target(ctx.source(), another_target); - /// world.commands().queue(move |world| { + /// world.commands().queue(move |world: &mut World| { /// entity_cloner.clone_entity(world); /// }); /// } From 4cf7e6eca122a1c22ec979a6f17b6fe57edc75a9 Mon Sep 17 00:00:00 2001 From: eugineerd Date: Sun, 8 Dec 2024 21:46:47 +0000 Subject: [PATCH 12/40] last doc fixes (hopefully) --- crates/bevy_ecs/src/component.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index 4de5fa75a8047..4eaa2168a1c5e 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -1021,7 +1021,7 @@ pub struct ComponentCloneHandlers { } impl ComponentCloneHandlers { - /// Sets the default handler for this registry. All components with [`Default`](ComponentCloneHandler::Default) handler, as well as any component that does not have an + /// Sets the default handler for this registry. All components with [`default`](ComponentCloneHandler::default_handler) handler, as well as any component that does not have an /// explicitly registered clone function will use this handler. /// /// See [Handlers section of `EntityCloneBuilder`](crate::entity::EntityCloneBuilder#handlers) to understand how this affects handler priority. From 8b6c9a1e0c94c46b239ddbcc7ec076450737dc48 Mon Sep 17 00:00:00 2001 From: eugineerd Date: Tue, 10 Dec 2024 12:31:36 +0000 Subject: [PATCH 13/40] fix entity cloning using reflect tests to actually use correct handlers --- crates/bevy_ecs/src/entity/clone_entities.rs | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/entity/clone_entities.rs b/crates/bevy_ecs/src/entity/clone_entities.rs index 2b6224cd7c070..3c4ec75e3b5fd 100644 --- a/crates/bevy_ecs/src/entity/clone_entities.rs +++ b/crates/bevy_ecs/src/entity/clone_entities.rs @@ -299,6 +299,8 @@ impl EntityCloner { panic!("Target entity does not exist"); } + debug_assert_eq!(component_data_ptrs.len(), component_ids.len()); + // SAFETY: // - All `component_ids` are from the same world as `target` entity // - All `component_data_ptrs` are valid types represented by `component_ids` @@ -562,7 +564,10 @@ impl<'w> EntityCloneBuilder<'w> { #[cfg(test)] mod tests { - use crate::{self as bevy_ecs, component::Component, entity::EntityCloneBuilder, world::World}; + use crate::{ + self as bevy_ecs, component::Component, component::ComponentCloneHandler, + entity::EntityCloneBuilder, world::World, + }; #[cfg(feature = "bevy_reflect")] #[test] @@ -581,6 +586,12 @@ mod tests { let registry = world.get_resource::().unwrap(); registry.write().register::(); + world.register_component::(); + let id = world.component_id::().unwrap(); + world + .get_component_clone_handlers_mut() + .set_component_handler(id, ComponentCloneHandler::reflect_handler()); + let component = A { field: 5 }; let e = world.spawn(component.clone()).id(); @@ -610,6 +621,12 @@ mod tests { let registry = world.get_resource::().unwrap(); registry.write().register::(); + world.register_component::(); + let id = world.component_id::().unwrap(); + world + .get_component_clone_handlers_mut() + .set_component_handler(id, ComponentCloneHandler::reflect_handler()); + let component = A { field: 5, field2: vec![1, 2, 3, 4, 5], From 3aefdaf4bc1b54bccd46602e72d2058ab52305d7 Mon Sep 17 00:00:00 2001 From: eugineerd Date: Tue, 10 Dec 2024 12:36:06 +0000 Subject: [PATCH 14/40] actually write pointers in `write_target_component_reflect` --- crates/bevy_ecs/src/entity/clone_entities.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/bevy_ecs/src/entity/clone_entities.rs b/crates/bevy_ecs/src/entity/clone_entities.rs index 3c4ec75e3b5fd..88cf5de6f1664 100644 --- a/crates/bevy_ecs/src/entity/clone_entities.rs +++ b/crates/bevy_ecs/src/entity/clone_entities.rs @@ -171,6 +171,8 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> { target_component_data_ptr.as_ptr(), component_layout.size(), ); + self.target_components_ptrs + .push(PtrMut::new(target_component_data_ptr)); alloc::alloc::dealloc(component_data_ptr, component_layout); } From 81558c04b6a5aeb590a4164c1335bb2e0baeafa6 Mon Sep 17 00:00:00 2001 From: eugineerd Date: Tue, 10 Dec 2024 12:44:11 +0000 Subject: [PATCH 15/40] don't allow double write in `write_target_component_reflect` --- crates/bevy_ecs/src/entity/clone_entities.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/entity/clone_entities.rs b/crates/bevy_ecs/src/entity/clone_entities.rs index 88cf5de6f1664..ab05d7bce8af3 100644 --- a/crates/bevy_ecs/src/entity/clone_entities.rs +++ b/crates/bevy_ecs/src/entity/clone_entities.rs @@ -146,6 +146,9 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> { /// - Passed component's [`TypeId`] does not match source component [`TypeId`] #[cfg(feature = "bevy_reflect")] pub fn write_target_component_reflect(&mut self, component: Box) { + if self.target_component_written { + panic!("Trying to write component multiple times") + } let source_type_id = self .components .get_info(self.component_id) @@ -567,8 +570,10 @@ impl<'w> EntityCloneBuilder<'w> { #[cfg(test)] mod tests { use crate::{ - self as bevy_ecs, component::Component, component::ComponentCloneHandler, - entity::EntityCloneBuilder, world::World, + self as bevy_ecs, + component::{Component, ComponentCloneHandler}, + entity::EntityCloneBuilder, + world::World, }; #[cfg(feature = "bevy_reflect")] From a9739a11b867a97e31d6a1553504a5b844c06d02 Mon Sep 17 00:00:00 2001 From: eugineerd Date: Tue, 10 Dec 2024 13:48:29 +0000 Subject: [PATCH 16/40] fix `entity_cloning` benchmark after rename --- benches/benches/bevy_ecs/entity_cloning.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/benches/benches/bevy_ecs/entity_cloning.rs b/benches/benches/bevy_ecs/entity_cloning.rs index 1469ef0ec49af..03b3e736245fa 100644 --- a/benches/benches/bevy_ecs/entity_cloning.rs +++ b/benches/benches/bevy_ecs/entity_cloning.rs @@ -73,7 +73,7 @@ fn hierarchy( .get_component_clone_handlers_mut() .set_component_handler( component, - bevy_ecs::component::ComponentCloneHandler::default_handler(), + bevy_ecs::component::ComponentCloneHandler::reflect_handler(), ); } } @@ -98,7 +98,7 @@ fn hierarchy( world.flush(); b.iter(move || { - world.commands().entity(id).clone_with(|builder| { + world.commands().entity(id).clone_and_spawn_with(|builder| { builder.recursive(true); }); world.flush(); @@ -122,14 +122,14 @@ fn simple(b: &mut Bencher, clone_via_ .get_component_clone_handlers_mut() .set_component_handler( component, - bevy_ecs::component::ComponentCloneHandler::default_handler(), + bevy_ecs::component::ComponentCloneHandler::reflect_handler(), ); } } let id = world.spawn(black_box(C::default())).id(); b.iter(move || { - world.commands().entity(id).clone(); + world.commands().entity(id).clone_and_spawn(); world.flush(); }); } From 588eba7745016e81e19ad2a120686abd835212cd Mon Sep 17 00:00:00 2001 From: eugineerd Date: Mon, 16 Dec 2024 20:21:31 +0000 Subject: [PATCH 17/40] use `ReflectFromReflect` in `component_clone_via_reflect` fast path --- benches/benches/bevy_ecs/entity_cloning.rs | 21 +++++------ crates/bevy_ecs/src/component.rs | 19 +++++++--- crates/bevy_ecs/src/entity/clone_entities.rs | 38 ++++++++++++++------ 3 files changed, 53 insertions(+), 25 deletions(-) diff --git a/benches/benches/bevy_ecs/entity_cloning.rs b/benches/benches/bevy_ecs/entity_cloning.rs index 03b3e736245fa..5e6b1339dc7c9 100644 --- a/benches/benches/bevy_ecs/entity_cloning.rs +++ b/benches/benches/bevy_ecs/entity_cloning.rs @@ -11,42 +11,43 @@ criterion_group!(benches, reflect_benches, clone_benches); criterion_main!(benches); #[derive(Component, Reflect, Default, Clone)] -#[reflect(Component, Default)] +#[reflect(Component)] struct C1(Mat4); #[derive(Component, Reflect, Default, Clone)] -#[reflect(Component, Default)] +#[reflect(Component)] struct C2(Mat4); #[derive(Component, Reflect, Default, Clone)] -#[reflect(Component, Default)] +#[reflect(Component)] struct C3(Mat4); #[derive(Component, Reflect, Default, Clone)] -#[reflect(Component, Default)] +#[reflect(Component)] struct C4(Mat4); #[derive(Component, Reflect, Default, Clone)] -#[reflect(Component, Default)] +#[reflect(Component)] struct C5(Mat4); #[derive(Component, Reflect, Default, Clone)] -#[reflect(Component, Default)] +#[reflect(Component)] struct C6(Mat4); #[derive(Component, Reflect, Default, Clone)] -#[reflect(Component, Default)] +#[reflect(Component)] struct C7(Mat4); #[derive(Component, Reflect, Default, Clone)] -#[reflect(Component, Default)] +#[reflect(Component)] struct C8(Mat4); + #[derive(Component, Reflect, Default, Clone)] -#[reflect(Component, Default)] +#[reflect(Component)] struct C9(Mat4); #[derive(Component, Reflect, Default, Clone)] -#[reflect(Component, Default)] +#[reflect(Component)] struct C10(Mat4); type ComplexBundle = (C1, C2, C3, C4, C5, C6, C7, C8, C9, C10); diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index 4eaa2168a1c5e..0eea8324d4db4 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -2094,11 +2094,20 @@ pub fn component_clone_via_reflect(world: &mut DeferredWorld, ctx: &mut Componen let registry = registry.read(); let component_info = ctx.components().get_info(ctx.component_id())?; let type_id = component_info.type_id()?; - let reflect_default = - registry.get_type_data::(type_id)?; - let mut component = reflect_default.default(); - component.apply(source_component_reflect.as_partial_reflect()); - component + + if let Some(reflect_from_reflect) = + registry.get_type_data::(type_id) + { + reflect_from_reflect.from_reflect(source_component_reflect.as_partial_reflect()) + } else if let Some(reflect_default) = + registry.get_type_data::(type_id) + { + let mut component = reflect_default.default(); + component.apply(source_component_reflect.as_partial_reflect()); + Some(component) + } else { + None + }? }; ctx.write_target_component_reflect(component); diff --git a/crates/bevy_ecs/src/entity/clone_entities.rs b/crates/bevy_ecs/src/entity/clone_entities.rs index ab05d7bce8af3..3c8a7f4ea12ce 100644 --- a/crates/bevy_ecs/src/entity/clone_entities.rs +++ b/crates/bevy_ecs/src/entity/clone_entities.rs @@ -612,39 +612,57 @@ mod tests { // TODO: remove this when 13432 lands #[cfg(feature = "bevy_reflect")] #[test] - fn clone_entity_using_reflect_with_default() { + fn clone_entity_using_reflect_fast_path() { use crate::reflect::{AppTypeRegistry, ReflectComponent}; use bevy_reflect::{std_traits::ReflectDefault, Reflect}; + // `ReflectDefault`-based fast path #[derive(Component, Reflect, Clone, PartialEq, Eq, Default)] #[reflect(Component, Default)] + #[reflect(from_reflect = false)] struct A { field: usize, field2: Vec, } + // `ReflectFromReflect`-based fast path + #[derive(Component, Reflect, Clone, PartialEq, Eq, Default)] + struct B { + field: usize, + field2: Vec, + } + let mut world = World::default(); world.init_resource::(); let registry = world.get_resource::().unwrap(); - registry.write().register::(); + registry.write().register::<(A, B)>(); - world.register_component::(); - let id = world.component_id::().unwrap(); - world - .get_component_clone_handlers_mut() - .set_component_handler(id, ComponentCloneHandler::reflect_handler()); + let a_id = world.register_component::(); + let b_id = world.register_component::(); + let handlers = world.get_component_clone_handlers_mut(); + handlers.set_component_handler(a_id, ComponentCloneHandler::reflect_handler()); + handlers.set_component_handler(b_id, ComponentCloneHandler::reflect_handler()); - let component = A { + let component_a = A { field: 5, field2: vec![1, 2, 3, 4, 5], }; + let component_b = B { + field: 6, + field2: vec![1, 2, 3, 4, 5], + }; - let e = world.spawn(component.clone()).id(); + let e = world.spawn((component_a.clone(), component_b.clone())).id(); let e_clone = world.spawn_empty().id(); EntityCloneBuilder::new(&mut world).clone_entity(e, e_clone); - assert!(world.get::(e_clone).is_some_and(|c| *c == component)); + assert!(world + .get::(e_clone) + .is_some_and(|c| *c == *world.get::(e).unwrap())); + assert!(world + .get::(e_clone) + .is_some_and(|c| *c == *world.get::(e).unwrap())); } #[test] From 5c7c42ec758f7364f3323fa790ac2d0fa8d06421 Mon Sep 17 00:00:00 2001 From: eugineerd Date: Mon, 16 Dec 2024 22:22:09 +0000 Subject: [PATCH 18/40] use `default_handler` instead of `default` for `ComponentCloneHandler` This is a bit more explicit and not deriving `Default` shouldn't be too much of a problem since this type is not expected to be stored somewhere outside. --- crates/bevy_ecs/src/component.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index 0eea8324d4db4..6266ec2f260eb 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -411,7 +411,7 @@ pub trait Component: Send + Sync + 'static { /// /// See [Handlers section of `EntityCloneBuilder`](crate::entity::EntityCloneBuilder#handlers) to understand how this affects handler priority. fn get_component_clone_handler() -> ComponentCloneHandler { - ComponentCloneHandler::default() + ComponentCloneHandler::default_handler() } } @@ -976,7 +976,7 @@ impl ComponentDescriptor { pub type ComponentCloneFn = fn(&mut DeferredWorld, &mut ComponentCloneCtx); /// A struct instructing which clone handler to use when cloning a component. -#[derive(Debug, Default)] +#[derive(Debug)] pub struct ComponentCloneHandler(Option); impl ComponentCloneHandler { @@ -2172,7 +2172,7 @@ pub trait ComponentCloneBase { } impl ComponentCloneBase for ComponentCloneSpecializationWrapper { fn get_component_clone_handler(&self) -> ComponentCloneHandler { - ComponentCloneHandler::default() + ComponentCloneHandler::default_handler() } } From 7b41bf9f5768ff189c94db63ef043412e42a735a Mon Sep 17 00:00:00 2001 From: eugineerd Date: Mon, 16 Dec 2024 22:40:19 +0000 Subject: [PATCH 19/40] Add clarification for `clone_fast` and `clone_slow` in `component_clone_via_reflect` --- crates/bevy_ecs/src/component.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index e3d3400d72bb3..c45c5399b2e25 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -2143,6 +2143,10 @@ pub fn component_clone_via_clone( /// See [`ComponentCloneHandlers`] for more details. #[cfg(feature = "bevy_reflect")] pub fn component_clone_via_reflect(world: &mut DeferredWorld, ctx: &mut ComponentCloneCtx) { + // This path will be selected if: + // 1. Component has ReflectFromReflect or ReflectDefault registered. + // 2. Component has ReflectFromPtr registered. + // Otherwise, it will fallback to clone_slow. fn clone_fast(ctx: &mut ComponentCloneCtx) -> Option<()> { let registry = ctx.type_registry()?; let component = { @@ -2170,6 +2174,8 @@ pub fn component_clone_via_reflect(world: &mut DeferredWorld, ctx: &mut Componen Some(()) } + // This will try to clone component using ReflectComponent. + // If the component does not have this type data registered, it will be ignored. fn clone_slow( component_id: ComponentId, source: Entity, From 53ec75d4e0638da00e3cc9b88e01ea7c351bad15 Mon Sep 17 00:00:00 2001 From: eugineerd Date: Mon, 16 Dec 2024 22:49:23 +0000 Subject: [PATCH 20/40] fix some post-merge weirdness --- crates/bevy_ecs/src/entity/clone_entities.rs | 27 -------------------- 1 file changed, 27 deletions(-) diff --git a/crates/bevy_ecs/src/entity/clone_entities.rs b/crates/bevy_ecs/src/entity/clone_entities.rs index 1cc63c31506cb..d6a5dde71c723 100644 --- a/crates/bevy_ecs/src/entity/clone_entities.rs +++ b/crates/bevy_ecs/src/entity/clone_entities.rs @@ -469,33 +469,6 @@ impl<'w> EntityCloneBuilder<'w> { self } - /// By default, any components allowed/denied through the filter will automatically - /// allow/deny all of their required components. - /// - /// This method allows for a scoped mode where any changes to the filter - /// will not involve required components. - pub fn without_required_components( - &mut self, - builder: impl FnOnce(&mut EntityCloneBuilder) + Send + Sync + 'static, - ) -> &mut Self { - self.attach_required_components = false; - builder(self); - self.attach_required_components = true; - self - } - - /// Sets whether the cloner should remove any components that were cloned, - /// effectively moving them from the source entity to the target. - /// - /// This is disabled by default. - /// - /// The setting only applies to components that are allowed through the filter - /// at the time [`EntityCloneBuilder::clone_entity`] is called. - pub fn move_components(&mut self, enable: bool) -> &mut Self { - self.move_components = enable; - self - } - /// Adds all components of the bundle to the list of components to clone. /// /// Note that all components are allowed by default, to clone only explicitly allowed components make sure to call From cff4ba6ac5e4bdc888425cc9b5c8e4e83e382abe Mon Sep 17 00:00:00 2001 From: eugineerd <70062110+eugineerd@users.noreply.github.com> Date: Tue, 17 Dec 2024 01:55:50 +0300 Subject: [PATCH 21/40] Update crates/bevy_ecs/src/entity/clone_entities.rs Co-authored-by: Alice Cecile --- crates/bevy_ecs/src/entity/clone_entities.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/entity/clone_entities.rs b/crates/bevy_ecs/src/entity/clone_entities.rs index d6a5dde71c723..5930f05773407 100644 --- a/crates/bevy_ecs/src/entity/clone_entities.rs +++ b/crates/bevy_ecs/src/entity/clone_entities.rs @@ -654,7 +654,7 @@ mod tests { assert!(world.get::(e_clone).is_some_and(|c| *c == component)); } - // TODO: remove this when 13432 lands + // TODO: remove this when https://github.com/bevyengine/bevy/pull/13432 lands #[cfg(feature = "bevy_reflect")] #[test] fn clone_entity_using_reflect_fast_path() { From 9601364cbc65cf39241739f89dd87ab06acb69a8 Mon Sep 17 00:00:00 2001 From: eugineerd Date: Tue, 17 Dec 2024 09:06:52 +0000 Subject: [PATCH 22/40] add test to make sure reflect handler doesn't panic if component can't be cloned --- crates/bevy_ecs/src/entity/clone_entities.rs | 42 ++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/crates/bevy_ecs/src/entity/clone_entities.rs b/crates/bevy_ecs/src/entity/clone_entities.rs index d6a5dde71c723..73006e39fab4a 100644 --- a/crates/bevy_ecs/src/entity/clone_entities.rs +++ b/crates/bevy_ecs/src/entity/clone_entities.rs @@ -764,6 +764,48 @@ mod tests { .is_some_and(|comp| *comp == A { field: 10 })); } + #[cfg(feature = "bevy_reflect")] + #[test] + fn clone_entity_using_reflect_should_skip_without_panic() { + use crate::reflect::{AppTypeRegistry, ReflectComponent}; + use bevy_reflect::Reflect; + + // Not reflected + #[derive(Component, PartialEq, Eq, Default, Debug)] + struct A; + + // No valid type data + #[derive(Component, Reflect, PartialEq, Eq, Default, Debug)] + #[reflect(Component)] + #[reflect(from_reflect = false)] + struct B; + + let mut world = World::default(); + let a_id = world.register_component::(); + let b_id = world.register_component::(); + let handlers = world.get_component_clone_handlers_mut(); + handlers.set_component_handler(a_id, ComponentCloneHandler::reflect_handler()); + handlers.set_component_handler(b_id, ComponentCloneHandler::reflect_handler()); + + // No AppTypeRegistry + let e = world.spawn((A, B)).id(); + let e_clone = world.spawn_empty().id(); + EntityCloneBuilder::new(&mut world).clone_entity(e, e_clone); + assert_eq!(world.get::(e_clone), None); + assert_eq!(world.get::(e_clone), None); + + // With AppTypeRegistry + world.init_resource::(); + let registry = world.get_resource::().unwrap(); + registry.write().register::(); + + let e = world.spawn((A, B)).id(); + let e_clone = world.spawn_empty().id(); + EntityCloneBuilder::new(&mut world).clone_entity(e, e_clone); + assert_eq!(world.get::(e_clone), None); + assert_eq!(world.get::(e_clone), None); + } + #[test] fn clone_entity_with_allow_filter() { #[derive(Component, Clone, PartialEq, Eq)] From 56ff78d64c32ad0e13c490c44b3baee5f2f4451d Mon Sep 17 00:00:00 2001 From: eugineerd Date: Tue, 17 Dec 2024 09:12:23 +0000 Subject: [PATCH 23/40] rewrite `component_clone_via_reflect` to not panic --- crates/bevy_ecs/src/component.rs | 130 ++++++++++++++++--------------- 1 file changed, 68 insertions(+), 62 deletions(-) diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index c45c5399b2e25..dd8fd8eae43c0 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -2140,81 +2140,87 @@ pub fn component_clone_via_clone( /// Can be [set](ComponentCloneHandlers::set_component_handler) as clone handler for any registered component, /// but only reflected components will be cloned. /// -/// See [`ComponentCloneHandlers`] for more details. +/// To clone a component using this handler, the following must be true: +/// - World has [`AppTypeRegistry`](crate::reflect::AppTypeRegistry) +/// - Component has [`TypeId`] +/// - Component is registered +/// - Component has [`ReflectFromPtr`](bevy_reflect::ReflectFromPtr) registered +/// - Component has one of the following registered: [`ReflectFromReflect`](bevy_reflect::ReflectFromReflect), +/// [`ReflectDefault`](bevy_reflect::std_traits::ReflectDefault), [`ReflectFromWorld`](crate::reflect::ReflectFromWorld) +/// +/// If any of the conditions is not satisfied, the component will be skipped. +/// +/// See [`EntityCloneBuilder`](crate::entity::EntityCloneBuilder) for details. #[cfg(feature = "bevy_reflect")] pub fn component_clone_via_reflect(world: &mut DeferredWorld, ctx: &mut ComponentCloneCtx) { // This path will be selected if: // 1. Component has ReflectFromReflect or ReflectDefault registered. // 2. Component has ReflectFromPtr registered. // Otherwise, it will fallback to clone_slow. - fn clone_fast(ctx: &mut ComponentCloneCtx) -> Option<()> { - let registry = ctx.type_registry()?; - let component = { - let source_component_reflect = ctx.read_source_component_reflect()?; - let registry = registry.read(); - let component_info = ctx.components().get_info(ctx.component_id())?; - let type_id = component_info.type_id()?; - - if let Some(reflect_from_reflect) = - registry.get_type_data::(type_id) - { - reflect_from_reflect.from_reflect(source_component_reflect.as_partial_reflect()) - } else if let Some(reflect_default) = - registry.get_type_data::(type_id) - { - let mut component = reflect_default.default(); - component.apply(source_component_reflect.as_partial_reflect()); - Some(component) - } else { - None - }? - }; - ctx.write_target_component_reflect(component); - - Some(()) - } - - // This will try to clone component using ReflectComponent. - // If the component does not have this type data registered, it will be ignored. - fn clone_slow( - component_id: ComponentId, - source: Entity, - target: Entity, - registry: &crate::reflect::AppTypeRegistry, - world: &mut World, - ) -> Option<()> { - let registry = registry.read(); - - let component_info = world.components().get_info(component_id)?; - let type_id = component_info.type_id()?; - let reflect_component = - registry.get_type_data::(type_id)?; - let source_component = reflect_component - .reflect(world.get_entity(source).ok()?)? - .clone_value(); - let mut target = world.get_entity_mut(target).ok()?; - reflect_component.apply_or_insert(&mut target, &*source_component, ®istry); - Some(()) - } - - // Try to clone fast first, if it doesn't work use the slow path - if clone_fast(ctx).is_some() { + let Some(registry) = ctx.type_registry() else { return; }; - - let component_id = ctx.component_id(); - let source = ctx.source(); - let target = ctx.target(); - world.commands().queue(move |world: &mut World| { - world.resource_scope(|world, registry| { - clone_slow(component_id, source, target, ®istry, world); + let Some(source_component_reflect) = ctx.read_source_component_reflect() else { + return; + }; + let Some(component_info) = ctx.components().get_info(ctx.component_id()) else { + return; + }; + let Some(type_id) = component_info.type_id() else { + return; + }; + let registry = registry.read(); + + // Try to clone using ReflectFromReflect + if let Some(reflect_from_reflect) = + registry.get_type_data::(type_id) + { + if let Some(component) = + reflect_from_reflect.from_reflect(source_component_reflect.as_partial_reflect()) + { + drop(registry); + ctx.write_target_component_reflect(component); + return; + } + } + // Else, try to clone using ReflectDefault + if let Some(reflect_default) = + registry.get_type_data::(type_id) + { + let mut component = reflect_default.default(); + component.apply(source_component_reflect.as_partial_reflect()); + drop(registry); + ctx.write_target_component_reflect(component); + return; + } + // Otherwise, try to clone using ReflectFromWorld + if let Some(reflect_from_world) = + registry.get_type_data::(type_id) + { + let reflect_from_world = reflect_from_world.clone(); + let source_component_cloned = source_component_reflect.clone_value(); + let target = ctx.target(); + let component_id = ctx.component_id(); + world.commands().queue(move |world: &mut World| { + let mut component = reflect_from_world.from_world(world); + component.apply(source_component_cloned.as_partial_reflect()); + // SAFETY: + // - component_id is from the same world as target entity + // - component is a valid value represented by component_id + unsafe { + let raw_component = + core::ptr::NonNull::new_unchecked(Box::into_raw(component).cast::()); + world + .entity_mut(target) + .insert_by_id(component_id, OwningPtr::new(raw_component)); + } }); - }); + } } /// Noop implementation of component clone handler function. /// -/// See [`ComponentCloneHandlers`] for more details. +/// See [`EntityCloneBuilder`](crate::entity::EntityCloneBuilder) for details. pub fn component_clone_ignore(_world: &mut DeferredWorld, _ctx: &mut ComponentCloneCtx) {} /// Wrapper for components clone specialization using autoderef. From fe31086ea9349ee363ee5412fcd9e35774dbc9e6 Mon Sep 17 00:00:00 2001 From: eugineerd Date: Tue, 17 Dec 2024 09:13:34 +0000 Subject: [PATCH 24/40] put all reflect tests in one module --- crates/bevy_ecs/src/entity/clone_entities.rs | 291 ++++++++++--------- 1 file changed, 147 insertions(+), 144 deletions(-) diff --git a/crates/bevy_ecs/src/entity/clone_entities.rs b/crates/bevy_ecs/src/entity/clone_entities.rs index 73006e39fab4a..223ae542c41c6 100644 --- a/crates/bevy_ecs/src/entity/clone_entities.rs +++ b/crates/bevy_ecs/src/entity/clone_entities.rs @@ -622,188 +622,191 @@ mod tests { use bevy_ecs_macros::require; #[cfg(feature = "bevy_reflect")] - #[test] - fn clone_entity_using_reflect() { - use crate::reflect::{AppTypeRegistry, ReflectComponent}; - use bevy_reflect::Reflect; - - #[derive(Component, Reflect, Clone, PartialEq, Eq)] - #[reflect(Component)] - struct A { - field: usize, - } - - let mut world = World::default(); - world.init_resource::(); - let registry = world.get_resource::().unwrap(); - registry.write().register::(); + mod reflect { + use super::*; + use crate::reflect::{AppTypeRegistry, ReflectComponent, ReflectFromWorld}; + use bevy_reflect::{std_traits::ReflectDefault, Reflect}; - world.register_component::(); - let id = world.component_id::().unwrap(); - world - .get_component_clone_handlers_mut() - .set_component_handler(id, ComponentCloneHandler::reflect_handler()); + #[test] + fn clone_entity_using_reflect() { + #[derive(Component, Reflect, Clone, PartialEq, Eq)] + #[reflect(Component)] + struct A { + field: usize, + } - let component = A { field: 5 }; + let mut world = World::default(); + world.init_resource::(); + let registry = world.get_resource::().unwrap(); + registry.write().register::(); - let e = world.spawn(component.clone()).id(); - let e_clone = world.spawn_empty().id(); + world.register_component::(); + let id = world.component_id::().unwrap(); + world + .get_component_clone_handlers_mut() + .set_component_handler(id, ComponentCloneHandler::reflect_handler()); - EntityCloneBuilder::new(&mut world).clone_entity(e, e_clone); + let component = A { field: 5 }; - assert!(world.get::(e_clone).is_some_and(|c| *c == component)); - } + let e = world.spawn(component.clone()).id(); + let e_clone = world.spawn_empty().id(); - // TODO: remove this when 13432 lands - #[cfg(feature = "bevy_reflect")] - #[test] - fn clone_entity_using_reflect_fast_path() { - use crate::reflect::{AppTypeRegistry, ReflectComponent}; - use bevy_reflect::{std_traits::ReflectDefault, Reflect}; + EntityCloneBuilder::new(&mut world).clone_entity(e, e_clone); - // `ReflectDefault`-based fast path - #[derive(Component, Reflect, Clone, PartialEq, Eq, Default)] - #[reflect(Component, Default)] - #[reflect(from_reflect = false)] - struct A { - field: usize, - field2: Vec, + assert!(world.get::(e_clone).is_some_and(|c| *c == component)); } - // `ReflectFromReflect`-based fast path - #[derive(Component, Reflect, Clone, PartialEq, Eq, Default)] - struct B { - field: usize, - field2: Vec, - } + // TODO: remove this when 13432 lands + #[test] + fn clone_entity_using_reflect_fast_path() { + // `ReflectDefault`-based fast path + #[derive(Component, Reflect, PartialEq, Eq, Default, Debug)] + #[reflect(Default)] + #[reflect(from_reflect = false)] + struct A { + field: usize, + field2: Vec, + } - let mut world = World::default(); - world.init_resource::(); - let registry = world.get_resource::().unwrap(); - registry.write().register::<(A, B)>(); - - let a_id = world.register_component::(); - let b_id = world.register_component::(); - let handlers = world.get_component_clone_handlers_mut(); - handlers.set_component_handler(a_id, ComponentCloneHandler::reflect_handler()); - handlers.set_component_handler(b_id, ComponentCloneHandler::reflect_handler()); - - let component_a = A { - field: 5, - field2: vec![1, 2, 3, 4, 5], - }; - let component_b = B { - field: 6, - field2: vec![1, 2, 3, 4, 5], - }; + // `ReflectFromReflect`-based fast path + #[derive(Component, Reflect, PartialEq, Eq, Default, Debug)] + struct B { + field: usize, + field2: Vec, + } - let e = world.spawn((component_a.clone(), component_b.clone())).id(); - let e_clone = world.spawn_empty().id(); + // `ReflectFromWorld`-based fast path + #[derive(Component, Reflect, PartialEq, Eq, Default, Debug)] + #[reflect(FromWorld)] + #[reflect(from_reflect = false)] + struct C { + field: usize, + field2: Vec, + } - EntityCloneBuilder::new(&mut world).clone_entity(e, e_clone); + let mut world = World::default(); + world.init_resource::(); + let registry = world.get_resource::().unwrap(); + registry.write().register::<(A, B, C)>(); + + let a_id = world.register_component::(); + let b_id = world.register_component::(); + let c_id = world.register_component::(); + let handlers = world.get_component_clone_handlers_mut(); + handlers.set_component_handler(a_id, ComponentCloneHandler::reflect_handler()); + handlers.set_component_handler(b_id, ComponentCloneHandler::reflect_handler()); + handlers.set_component_handler(c_id, ComponentCloneHandler::reflect_handler()); + + let component_a = A { + field: 5, + field2: vec![1, 2, 3, 4, 5], + }; + let component_b = B { + field: 6, + field2: vec![1, 2, 3, 4, 5], + }; + let component_c = C { + field: 7, + field2: vec![1, 2, 3, 4, 5], + }; - assert!(world - .get::(e_clone) - .is_some_and(|c| *c == *world.get::(e).unwrap())); - assert!(world - .get::(e_clone) - .is_some_and(|c| *c == *world.get::(e).unwrap())); - } + let e = world.spawn((component_a, component_b, component_c)).id(); + let e_clone = world.spawn_empty().id(); - #[test] - fn clone_entity_using_clone() { - #[derive(Component, Clone, PartialEq, Eq)] - struct A { - field: usize, + EntityCloneBuilder::new(&mut world).clone_entity(e, e_clone); + + assert_eq!(world.get::(e_clone), Some(world.get::(e).unwrap())); + assert_eq!(world.get::(e_clone), Some(world.get::(e).unwrap())); + assert_eq!(world.get::(e_clone), Some(world.get::(e).unwrap())); } - let mut world = World::default(); + #[test] + fn clone_entity_specialization() { + #[derive(Component, Reflect, PartialEq, Eq)] + #[reflect(Component)] + struct A { + field: usize, + } - let component = A { field: 5 }; + impl Clone for A { + fn clone(&self) -> Self { + Self { field: 10 } + } + } - let e = world.spawn(component.clone()).id(); - let e_clone = world.spawn_empty().id(); + let mut world = World::default(); + world.init_resource::(); + let registry = world.get_resource::().unwrap(); + registry.write().register::(); - EntityCloneBuilder::new(&mut world).clone_entity(e, e_clone); + let component = A { field: 5 }; - assert!(world.get::(e_clone).is_some_and(|c| *c == component)); - } + let e = world.spawn(component.clone()).id(); + let e_clone = world.spawn_empty().id(); - #[cfg(feature = "bevy_reflect")] - #[test] - fn clone_entity_specialization() { - use crate::reflect::{AppTypeRegistry, ReflectComponent}; - use bevy_reflect::Reflect; + EntityCloneBuilder::new(&mut world).clone_entity(e, e_clone); - #[derive(Component, Reflect, PartialEq, Eq)] - #[reflect(Component)] - struct A { - field: usize, + assert!(world + .get::(e_clone) + .is_some_and(|comp| *comp == A { field: 10 })); } - impl Clone for A { - fn clone(&self) -> Self { - Self { field: 10 } - } - } + #[test] + fn clone_entity_using_reflect_should_skip_without_panic() { + // Not reflected + #[derive(Component, PartialEq, Eq, Default, Debug)] + struct A; - let mut world = World::default(); - world.init_resource::(); - let registry = world.get_resource::().unwrap(); - registry.write().register::(); + // No valid type data + #[derive(Component, Reflect, PartialEq, Eq, Default, Debug)] + #[reflect(Component)] + #[reflect(from_reflect = false)] + struct B; - let component = A { field: 5 }; + let mut world = World::default(); + let a_id = world.register_component::(); + let b_id = world.register_component::(); + let handlers = world.get_component_clone_handlers_mut(); + handlers.set_component_handler(a_id, ComponentCloneHandler::reflect_handler()); + handlers.set_component_handler(b_id, ComponentCloneHandler::reflect_handler()); - let e = world.spawn(component.clone()).id(); - let e_clone = world.spawn_empty().id(); + // No AppTypeRegistry + let e = world.spawn((A, B)).id(); + let e_clone = world.spawn_empty().id(); + EntityCloneBuilder::new(&mut world).clone_entity(e, e_clone); + assert_eq!(world.get::(e_clone), None); + assert_eq!(world.get::(e_clone), None); - EntityCloneBuilder::new(&mut world).clone_entity(e, e_clone); + // With AppTypeRegistry + world.init_resource::(); + let registry = world.get_resource::().unwrap(); + registry.write().register::(); - assert!(world - .get::(e_clone) - .is_some_and(|comp| *comp == A { field: 10 })); + let e = world.spawn((A, B)).id(); + let e_clone = world.spawn_empty().id(); + EntityCloneBuilder::new(&mut world).clone_entity(e, e_clone); + assert_eq!(world.get::(e_clone), None); + assert_eq!(world.get::(e_clone), None); + } } - #[cfg(feature = "bevy_reflect")] #[test] - fn clone_entity_using_reflect_should_skip_without_panic() { - use crate::reflect::{AppTypeRegistry, ReflectComponent}; - use bevy_reflect::Reflect; - - // Not reflected - #[derive(Component, PartialEq, Eq, Default, Debug)] - struct A; - - // No valid type data - #[derive(Component, Reflect, PartialEq, Eq, Default, Debug)] - #[reflect(Component)] - #[reflect(from_reflect = false)] - struct B; + fn clone_entity_using_clone() { + #[derive(Component, Clone, PartialEq, Eq)] + struct A { + field: usize, + } let mut world = World::default(); - let a_id = world.register_component::(); - let b_id = world.register_component::(); - let handlers = world.get_component_clone_handlers_mut(); - handlers.set_component_handler(a_id, ComponentCloneHandler::reflect_handler()); - handlers.set_component_handler(b_id, ComponentCloneHandler::reflect_handler()); - - // No AppTypeRegistry - let e = world.spawn((A, B)).id(); - let e_clone = world.spawn_empty().id(); - EntityCloneBuilder::new(&mut world).clone_entity(e, e_clone); - assert_eq!(world.get::(e_clone), None); - assert_eq!(world.get::(e_clone), None); - // With AppTypeRegistry - world.init_resource::(); - let registry = world.get_resource::().unwrap(); - registry.write().register::(); + let component = A { field: 5 }; - let e = world.spawn((A, B)).id(); + let e = world.spawn(component.clone()).id(); let e_clone = world.spawn_empty().id(); + EntityCloneBuilder::new(&mut world).clone_entity(e, e_clone); - assert_eq!(world.get::(e_clone), None); - assert_eq!(world.get::(e_clone), None); + + assert!(world.get::(e_clone).is_some_and(|c| *c == component)); } #[test] From 52b1f26a5ad22495ca5aba615e6d7eb3dd0e8b65 Mon Sep 17 00:00:00 2001 From: eugineerd Date: Tue, 17 Dec 2024 15:11:08 +0000 Subject: [PATCH 25/40] remove unused import in benchmark --- benches/benches/bevy_ecs/entity_cloning.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/benches/benches/bevy_ecs/entity_cloning.rs b/benches/benches/bevy_ecs/entity_cloning.rs index 5e6b1339dc7c9..d0c71c7c2ae8e 100644 --- a/benches/benches/bevy_ecs/entity_cloning.rs +++ b/benches/benches/bevy_ecs/entity_cloning.rs @@ -3,7 +3,6 @@ use bevy_ecs::reflect::AppTypeRegistry; use bevy_ecs::{component::Component, reflect::ReflectComponent, world::World}; use bevy_hierarchy::{BuildChildren, CloneEntityHierarchyExt}; use bevy_math::Mat4; -use bevy_reflect::prelude::ReflectDefault; use bevy_reflect::{GetTypeRegistration, Reflect}; use criterion::{black_box, criterion_group, criterion_main, Bencher, Criterion}; From e86b9639eb2607e660064d643dc15d03a733ecee Mon Sep 17 00:00:00 2001 From: eugineerd Date: Tue, 17 Dec 2024 15:41:09 +0000 Subject: [PATCH 26/40] remove outdated comment --- crates/bevy_ecs/src/component.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index dd8fd8eae43c0..22d7577e8daa3 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -2153,10 +2153,6 @@ pub fn component_clone_via_clone( /// See [`EntityCloneBuilder`](crate::entity::EntityCloneBuilder) for details. #[cfg(feature = "bevy_reflect")] pub fn component_clone_via_reflect(world: &mut DeferredWorld, ctx: &mut ComponentCloneCtx) { - // This path will be selected if: - // 1. Component has ReflectFromReflect or ReflectDefault registered. - // 2. Component has ReflectFromPtr registered. - // Otherwise, it will fallback to clone_slow. let Some(registry) = ctx.type_registry() else { return; }; From 2d11e57ee731133514b435979b50e7c184b4ab68 Mon Sep 17 00:00:00 2001 From: eugineerd Date: Wed, 18 Dec 2024 06:33:36 +0000 Subject: [PATCH 27/40] fix leak in `component_clone_via_reflect`'s `ReflectFromWorld` path --- crates/bevy_ecs/src/component.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index 22d7577e8daa3..715908c80a961 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -2195,6 +2195,7 @@ pub fn component_clone_via_reflect(world: &mut DeferredWorld, ctx: &mut Componen { let reflect_from_world = reflect_from_world.clone(); let source_component_cloned = source_component_reflect.clone_value(); + let component_layout = component_info.layout(); let target = ctx.target(); let component_id = ctx.component_id(); world.commands().queue(move |world: &mut World| { @@ -2204,11 +2205,12 @@ pub fn component_clone_via_reflect(world: &mut DeferredWorld, ctx: &mut Componen // - component_id is from the same world as target entity // - component is a valid value represented by component_id unsafe { - let raw_component = + let raw_component_ptr = core::ptr::NonNull::new_unchecked(Box::into_raw(component).cast::()); world .entity_mut(target) - .insert_by_id(component_id, OwningPtr::new(raw_component)); + .insert_by_id(component_id, OwningPtr::new(raw_component_ptr)); + alloc::alloc::dealloc(raw_component_ptr.as_ptr(), component_layout); } }); } From 7b0f45d51b143ad14f907c29922d1b52209c3398 Mon Sep 17 00:00:00 2001 From: eugineerd Date: Wed, 18 Dec 2024 07:06:48 +0000 Subject: [PATCH 28/40] make `read_source_component_reflect` return `None` when `ReflectFromPtr`'s `TypeId` does not match component's `TypeId` --- crates/bevy_ecs/src/entity/clone_entities.rs | 46 ++++++++++++++++++-- 1 file changed, 43 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/entity/clone_entities.rs b/crates/bevy_ecs/src/entity/clone_entities.rs index bc4bb2aad714e..0c2d14deb08e8 100644 --- a/crates/bevy_ecs/src/entity/clone_entities.rs +++ b/crates/bevy_ecs/src/entity/clone_entities.rs @@ -102,11 +102,15 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> { /// - Component does not implement [`ReflectFromPtr`](bevy_reflect::ReflectFromPtr). /// - Component is not registered. /// - Component does not have [`TypeId`] + /// - Registered [`ReflectFromPtr`](bevy_reflect::ReflectFromPtr)'s [`TypeId`] does not match component's [`TypeId`] #[cfg(feature = "bevy_reflect")] pub fn read_source_component_reflect(&self) -> Option<&dyn bevy_reflect::Reflect> { let registry = self.type_registry?.read(); let type_id = self.components.get_info(self.component_id)?.type_id()?; let reflect_from_ptr = registry.get_type_data::(type_id)?; + if reflect_from_ptr.type_id() != type_id { + return None; + } // SAFETY: `source_component_ptr` stores data represented by `component_id`, which we used to get `ReflectFromPtr`. unsafe { Some(reflect_from_ptr.as_reflect(self.source_component_ptr)) } } @@ -613,11 +617,12 @@ impl<'w> EntityCloneBuilder<'w> { #[cfg(test)] mod tests { + use super::ComponentCloneCtx; use crate::{ self as bevy_ecs, component::{Component, ComponentCloneHandler}, entity::EntityCloneBuilder, - world::World, + world::{DeferredWorld, World}, }; use bevy_ecs_macros::require; @@ -625,7 +630,7 @@ mod tests { mod reflect { use super::*; use crate::reflect::{AppTypeRegistry, ReflectComponent, ReflectFromWorld}; - use bevy_reflect::{std_traits::ReflectDefault, Reflect}; + use bevy_reflect::{std_traits::ReflectDefault, FromType, Reflect, ReflectFromPtr}; #[test] fn clone_entity_using_reflect() { @@ -658,7 +663,7 @@ mod tests { // TODO: remove this when https://github.com/bevyengine/bevy/pull/13432 lands #[test] - fn clone_entity_using_reflect_fast_path() { + fn clone_entity_using_reflect_all_paths() { // `ReflectDefault`-based fast path #[derive(Component, Reflect, PartialEq, Eq, Default, Debug)] #[reflect(Default)] @@ -720,6 +725,41 @@ mod tests { assert_eq!(world.get::(e_clone), Some(world.get::(e).unwrap())); } + #[test] + fn read_source_component_reflect_should_return_none_on_invalid_reflect_from_ptr() { + #[derive(Component, Reflect)] + struct A; + + #[derive(Component, Reflect)] + struct B; + + fn test_handler(_world: &mut DeferredWorld, ctx: &mut ComponentCloneCtx) { + assert!(ctx.read_source_component_reflect().is_none()) + } + + let mut world = World::default(); + world.init_resource::(); + let registry = world.get_resource::().unwrap(); + { + let mut registry = registry.write(); + registry.register::(); + registry + .get_mut(core::any::TypeId::of::()) + .unwrap() + .insert(>::from_type()); + } + + let a_id = world.register_component::(); + let handlers = world.get_component_clone_handlers_mut(); + handlers + .set_component_handler(a_id, ComponentCloneHandler::custom_handler(test_handler)); + + let e = world.spawn(A).id(); + let e_clone = world.spawn_empty().id(); + + EntityCloneBuilder::new(&mut world).clone_entity(e, e_clone); + } + #[test] fn clone_entity_specialization() { #[derive(Component, Reflect, PartialEq, Eq)] From 3cb6891a1afc56f57c132246b8006fe92161573c Mon Sep 17 00:00:00 2001 From: eugineerd Date: Wed, 18 Dec 2024 07:22:53 +0000 Subject: [PATCH 29/40] use `debug_checked_unwrap` for getting source component ptr It should never fail because `component` is part of entity's archetype --- crates/bevy_ecs/src/entity/clone_entities.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/entity/clone_entities.rs b/crates/bevy_ecs/src/entity/clone_entities.rs index 0c2d14deb08e8..0e74847ead553 100644 --- a/crates/bevy_ecs/src/entity/clone_entities.rs +++ b/crates/bevy_ecs/src/entity/clone_entities.rs @@ -9,6 +9,7 @@ use crate::{ bundle::Bundle, component::{Component, ComponentCloneHandler, ComponentId, Components}, entity::Entity, + query::DebugCheckedUnwrap, world::World, }; @@ -275,10 +276,11 @@ impl EntityCloner { None => global_handlers.get_handler(component), }; - // SAFETY: There are no other mutable references to source entity. - let Some(source_component_ptr) = (unsafe { source_entity.get_by_id(component) }) else { - continue; - }; + // SAFETY: + // - There are no other mutable references to source entity. + // - `component` is from `source_entity`'s archetype + let source_component_ptr = + unsafe { source_entity.get_by_id(component).debug_checked_unwrap() }; // SAFETY: // - `components` and `component` are from the same world From 0de432d5b903cb7a1e7c608f13387f3f33916a44 Mon Sep 17 00:00:00 2001 From: eugineerd Date: Wed, 18 Dec 2024 07:30:50 +0000 Subject: [PATCH 30/40] use `type_id()` instead of `reflect_type_info().type_id()` in `write_target_component_reflect` --- crates/bevy_ecs/src/entity/clone_entities.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/entity/clone_entities.rs b/crates/bevy_ecs/src/entity/clone_entities.rs index 0e74847ead553..67422cdde5a80 100644 --- a/crates/bevy_ecs/src/entity/clone_entities.rs +++ b/crates/bevy_ecs/src/entity/clone_entities.rs @@ -160,7 +160,7 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> { .unwrap() .type_id() .unwrap(); - let component_type_id = component.reflect_type_info().type_id(); + let component_type_id = component.type_id(); if source_type_id != component_type_id { panic!("Passed component TypeId does not match source component TypeId") } From ca17d74c24b15c6987a9fede21d618d2675ad057 Mon Sep 17 00:00:00 2001 From: eugineerd Date: Wed, 18 Dec 2024 13:56:27 +0000 Subject: [PATCH 31/40] store `ComponentInfo` in `ComponentCloneCtx` --- crates/bevy_ecs/src/entity/clone_entities.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/entity/clone_entities.rs b/crates/bevy_ecs/src/entity/clone_entities.rs index 67422cdde5a80..58d71ac8cacd4 100644 --- a/crates/bevy_ecs/src/entity/clone_entities.rs +++ b/crates/bevy_ecs/src/entity/clone_entities.rs @@ -24,6 +24,7 @@ pub struct ComponentCloneCtx<'a, 'b> { target_components_ptrs: &'a mut Vec>, target_components_buffer: &'b Bump, components: &'a Components, + component_info: &'a ComponentInfo, entity_cloner: &'a EntityCloner, #[cfg(feature = "bevy_reflect")] type_registry: Option<&'a crate::reflect::AppTypeRegistry>, @@ -52,6 +53,7 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> { target_component_written: false, target_components_buffer, components, + component_info: components.get_info_unchecked(component_id), entity_cloner, #[cfg(feature = "bevy_reflect")] type_registry, @@ -73,11 +75,16 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> { self.entity_cloner.target } - /// Returns the [`ComponentId`] of currently cloned component. + /// Returns the [`ComponentId`] of the component being cloned. pub fn component_id(&self) -> ComponentId { self.component_id } + /// Returns the [`ComponentInfo`] of the component being cloned. + pub fn component_info(&self) -> &ComponentInfo { + self.component_info + } + /// Returns a reference to the component on the source entity. /// /// Will return `None` if `ComponentId` of requested component does not match `ComponentId` of source component From 3c70318cc9b07fc9916c01a851fedef1b2c5f591 Mon Sep 17 00:00:00 2001 From: eugineerd Date: Wed, 18 Dec 2024 14:19:25 +0000 Subject: [PATCH 32/40] use stored `ComponentInfo` instead of getting it from `Components` --- crates/bevy_ecs/src/component.rs | 9 ++---- crates/bevy_ecs/src/entity/clone_entities.rs | 32 +++++++++----------- 2 files changed, 18 insertions(+), 23 deletions(-) diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index 715908c80a961..7dd547727df8c 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -2159,12 +2159,9 @@ pub fn component_clone_via_reflect(world: &mut DeferredWorld, ctx: &mut Componen let Some(source_component_reflect) = ctx.read_source_component_reflect() else { return; }; - let Some(component_info) = ctx.components().get_info(ctx.component_id()) else { - return; - }; - let Some(type_id) = component_info.type_id() else { - return; - }; + let component_info = ctx.component_info(); + // checked in read_source_component_reflect + let type_id = component_info.type_id().unwrap(); let registry = registry.read(); // Try to clone using ReflectFromReflect diff --git a/crates/bevy_ecs/src/entity/clone_entities.rs b/crates/bevy_ecs/src/entity/clone_entities.rs index 58d71ac8cacd4..0cc667418d005 100644 --- a/crates/bevy_ecs/src/entity/clone_entities.rs +++ b/crates/bevy_ecs/src/entity/clone_entities.rs @@ -7,7 +7,7 @@ use bevy_utils::{HashMap, HashSet}; use crate::{ bundle::Bundle, - component::{Component, ComponentCloneHandler, ComponentId, Components}, + component::{Component, ComponentCloneHandler, ComponentId, ComponentInfo, Components}, entity::Entity, query::DebugCheckedUnwrap, world::World, @@ -90,9 +90,9 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> { /// Will return `None` if `ComponentId` of requested component does not match `ComponentId` of source component pub fn read_source_component(&self) -> Option<&T> { if self - .components - .component_id::() - .is_some_and(|id| id == self.component_id) + .component_info + .type_id() + .is_some_and(|id| id == TypeId::of::()) { // SAFETY: // - Components and ComponentId are from the same world @@ -114,7 +114,7 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> { #[cfg(feature = "bevy_reflect")] pub fn read_source_component_reflect(&self) -> Option<&dyn bevy_reflect::Reflect> { let registry = self.type_registry?.read(); - let type_id = self.components.get_info(self.component_id)?.type_id()?; + let type_id = self.component_info.type_id()?; let reflect_from_ptr = registry.get_type_data::(type_id)?; if reflect_from_ptr.type_id() != type_id { return None; @@ -135,12 +135,13 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> { if self.target_component_written { panic!("Trying to write component '{short_name}' multiple times") } - let Some(component_id) = self.components.component_id::() else { - panic!("Component '{short_name}' is not registered") + if !self + .component_info + .type_id() + .is_some_and(|id| id == TypeId::of::()) + { + panic!("TypeId of component '{short_name}' does not match source component TypeId") }; - if component_id != self.component_id { - panic!("Component '{short_name}' does not match ComponentId of this ComponentCloneCtx"); - } let component_ref = self.target_components_buffer.alloc(component); self.target_components_ptrs .push(PtrMut::from(component_ref)); @@ -153,20 +154,17 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> { /// This will panic if: /// - World does not have [`AppTypeRegistry`](`crate::reflect::AppTypeRegistry`). /// - Component does not implement [`ReflectFromPtr`](bevy_reflect::ReflectFromPtr). - /// - Component is not registered. - /// - Component does not have [`TypeId`] - /// - Passed component's [`TypeId`] does not match source component [`TypeId`] + /// - Source component does not have [`TypeId`]. + /// - Passed component's [`TypeId`] does not match source component [`TypeId`]. #[cfg(feature = "bevy_reflect")] pub fn write_target_component_reflect(&mut self, component: Box) { if self.target_component_written { panic!("Trying to write component multiple times") } let source_type_id = self - .components - .get_info(self.component_id) - .unwrap() + .component_info .type_id() - .unwrap(); + .expect("Source component must have TypeId"); let component_type_id = component.type_id(); if source_type_id != component_type_id { panic!("Passed component TypeId does not match source component TypeId") From 05a307a22061ad67f59f5335f4fc24e184249663 Mon Sep 17 00:00:00 2001 From: eugineerd Date: Wed, 18 Dec 2024 15:23:54 +0000 Subject: [PATCH 33/40] add `write_target_component_ptr` to allow writing component clone handlers for components without `TypeId` --- crates/bevy_ecs/src/entity/clone_entities.rs | 86 +++++++++++++++++++- 1 file changed, 84 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/entity/clone_entities.rs b/crates/bevy_ecs/src/entity/clone_entities.rs index 0cc667418d005..7a93d7a7ce2aa 100644 --- a/crates/bevy_ecs/src/entity/clone_entities.rs +++ b/crates/bevy_ecs/src/entity/clone_entities.rs @@ -1,7 +1,7 @@ use alloc::sync::Arc; use bevy_ptr::{Ptr, PtrMut}; use bumpalo::Bump; -use core::any::TypeId; +use core::{any::TypeId, ptr::NonNull}; use bevy_utils::{HashMap, HashSet}; @@ -148,6 +148,29 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> { self.target_component_written = true; } + /// Writes component data to target entity by providing a pointer to source component data and to uninitialized target component data. + /// + /// Source component pointer points to a component described [`ComponentInfo`] stored in this [`ComponentCloneCtx`]. + /// + /// The uninitialized pointer points to a buffer with layout described by [`ComponentInfo`] stored in this [`ComponentCloneCtx`]. + /// + /// # Safety + /// Caller must ensure that after `clone_fn` is called the second argument ([`NonNull`] pointer) points to a valid component data + /// described by [`ComponentInfo`] stored in this [`ComponentCloneCtx`]. + pub unsafe fn write_target_component_ptr(&mut self, clone_fn: impl FnOnce(Ptr, NonNull)) { + if self.target_component_written { + panic!("Trying to write component multiple times") + } + let layout = self.component_info.layout(); + let target_component_data_ptr = self.target_components_buffer.alloc_layout(layout); + + clone_fn(self.source_component_ptr, target_component_data_ptr); + + self.target_components_ptrs + .push(PtrMut::new(target_component_data_ptr)); + self.target_component_written = true; + } + /// Writes component data to target entity. /// /// # Panics @@ -627,11 +650,13 @@ mod tests { use super::ComponentCloneCtx; use crate::{ self as bevy_ecs, - component::{Component, ComponentCloneHandler}, + component::{Component, ComponentCloneHandler, ComponentDescriptor, StorageType}, entity::EntityCloneBuilder, world::{DeferredWorld, World}, }; + use alloc::alloc::Layout; use bevy_ecs_macros::require; + use bevy_ptr::OwningPtr; #[cfg(feature = "bevy_reflect")] mod reflect { @@ -1004,4 +1029,61 @@ mod tests { assert_eq!(world.entity(e_clone).get::(), Some(&B)); assert_eq!(world.entity(e_clone).get::(), Some(&C(5))); } + + #[test] + fn clone_entity_with_dynamic_components() { + const COMPONENT_SIZE: usize = 10; + fn test_handler(_world: &mut DeferredWorld, ctx: &mut ComponentCloneCtx) { + unsafe { + ctx.write_target_component_ptr(move |source_ptr, target_ptr| { + core::ptr::copy_nonoverlapping( + source_ptr.as_ptr(), + target_ptr.as_ptr(), + COMPONENT_SIZE, + ); + }); + } + } + + let mut world = World::default(); + + let layout = Layout::array::(COMPONENT_SIZE).unwrap(); + let descriptor = unsafe { + ComponentDescriptor::new_with_layout( + "DynamicComp", + StorageType::Table, + layout, + None, + true, + ) + }; + let component_id = world.register_component_with_descriptor(descriptor); + + let handlers = world.get_component_clone_handlers_mut(); + handlers.set_component_handler( + component_id, + ComponentCloneHandler::custom_handler(test_handler), + ); + + let mut entity = world.spawn_empty(); + let data = [5u8; COMPONENT_SIZE]; + + OwningPtr::make(data, |ptr| unsafe { + entity.insert_by_id(component_id, ptr); + }); + let entity = entity.id(); + + let entity_clone = world.spawn_empty().id(); + let builder = EntityCloneBuilder::new(&mut world); + builder.clone_entity(entity, entity_clone); + + let ptr = world.get_by_id(entity, component_id).unwrap(); + let clone_ptr = world.get_by_id(entity_clone, component_id).unwrap(); + unsafe { + assert_eq!( + core::slice::from_raw_parts(ptr.as_ptr(), COMPONENT_SIZE), + core::slice::from_raw_parts(clone_ptr.as_ptr(), COMPONENT_SIZE), + ); + } + } } From 6f03851b27ff52a5cce851ba898e38b6ed057d23 Mon Sep 17 00:00:00 2001 From: eugineerd Date: Wed, 18 Dec 2024 16:12:44 +0000 Subject: [PATCH 34/40] allow `clone_fn` in `write_target_component_ptr` to fail --- crates/bevy_ecs/src/entity/clone_entities.rs | 32 ++++++++++++-------- 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/crates/bevy_ecs/src/entity/clone_entities.rs b/crates/bevy_ecs/src/entity/clone_entities.rs index df1b8ba7d55b7..af15f834009e8 100644 --- a/crates/bevy_ecs/src/entity/clone_entities.rs +++ b/crates/bevy_ecs/src/entity/clone_entities.rs @@ -133,7 +133,7 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> { /// /// # Panics /// This will panic if: - /// - `write_target_component` called more than once. + /// - Component has already been written once. /// - Component being written is not registered in the world. /// - `ComponentId` of component being written does not match expected `ComponentId`. pub fn write_target_component(&mut self, component: T) { @@ -154,27 +154,33 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> { self.target_component_written = true; } - /// Writes component data to target entity by providing a pointer to source component data and to uninitialized target component data. + /// Writes component data to target entity by providing a pointer to source component data and a pointer to uninitialized target component data. /// - /// Source component pointer points to a component described [`ComponentInfo`] stored in this [`ComponentCloneCtx`]. - /// - /// The uninitialized pointer points to a buffer with layout described by [`ComponentInfo`] stored in this [`ComponentCloneCtx`]. + /// This method allows caller to provide a function (`clone_fn`) to clone component using untyped pointers. + /// First argument to `clone_fn` points to source component data ([`Ptr`]), second argument points to uninitialized buffer ([`NonNull`]) allocated with layout + /// described by [`ComponentInfo`] stored in this [`ComponentCloneCtx`]. If cloning is successful and uninitialized buffer contains a valid clone of + /// source component, `clone_fn` should return `true`, otherwise it should return `false`. /// /// # Safety - /// Caller must ensure that after `clone_fn` is called the second argument ([`NonNull`] pointer) points to a valid component data + /// Caller must ensure that if `clone_fn` is called and returns `true`, the second argument ([`NonNull`] pointer) points to a valid component data /// described by [`ComponentInfo`] stored in this [`ComponentCloneCtx`]. - pub unsafe fn write_target_component_ptr(&mut self, clone_fn: impl FnOnce(Ptr, NonNull)) { + /// # Panics + /// This will panic if component has already been written once. + pub unsafe fn write_target_component_ptr( + &mut self, + clone_fn: impl FnOnce(Ptr, NonNull) -> bool, + ) { if self.target_component_written { panic!("Trying to write component multiple times") } let layout = self.component_info.layout(); let target_component_data_ptr = self.target_components_buffer.alloc_layout(layout); - clone_fn(self.source_component_ptr, target_component_data_ptr); - - self.target_components_ptrs - .push(PtrMut::new(target_component_data_ptr)); - self.target_component_written = true; + if clone_fn(self.source_component_ptr, target_component_data_ptr) { + self.target_components_ptrs + .push(PtrMut::new(target_component_data_ptr)); + self.target_component_written = true; + } } /// Writes component data to target entity. @@ -185,6 +191,7 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> { /// - Component does not implement [`ReflectFromPtr`](bevy_reflect::ReflectFromPtr). /// - Source component does not have [`TypeId`]. /// - Passed component's [`TypeId`] does not match source component [`TypeId`]. + /// - Component has already been written once. #[cfg(feature = "bevy_reflect")] pub fn write_target_component_reflect(&mut self, component: Box) { if self.target_component_written { @@ -1047,6 +1054,7 @@ mod tests { target_ptr.as_ptr(), COMPONENT_SIZE, ); + true }); } } From fbaa0cbcdc9f773c3521725cfb76c421b06575cc Mon Sep 17 00:00:00 2001 From: eugineerd Date: Wed, 18 Dec 2024 16:31:14 +0000 Subject: [PATCH 35/40] clippy --- crates/bevy_ecs/src/entity/clone_entities.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/entity/clone_entities.rs b/crates/bevy_ecs/src/entity/clone_entities.rs index af15f834009e8..34a4d97003595 100644 --- a/crates/bevy_ecs/src/entity/clone_entities.rs +++ b/crates/bevy_ecs/src/entity/clone_entities.rs @@ -667,9 +667,9 @@ mod tests { entity::EntityCloneBuilder, world::{DeferredWorld, World}, }; - use alloc::alloc::Layout; use bevy_ecs_macros::require; use bevy_ptr::OwningPtr; + use core::alloc::Layout; #[cfg(feature = "bevy_reflect")] mod reflect { @@ -779,7 +779,7 @@ mod tests { struct B; fn test_handler(_world: &mut DeferredWorld, ctx: &mut ComponentCloneCtx) { - assert!(ctx.read_source_component_reflect().is_none()) + assert!(ctx.read_source_component_reflect().is_none()); } let mut world = World::default(); @@ -1047,6 +1047,7 @@ mod tests { fn clone_entity_with_dynamic_components() { const COMPONENT_SIZE: usize = 10; fn test_handler(_world: &mut DeferredWorld, ctx: &mut ComponentCloneCtx) { + // SAFETY: this handler is only going to be used with a component represented by [u8; COMPONENT_SIZE] unsafe { ctx.write_target_component_ptr(move |source_ptr, target_ptr| { core::ptr::copy_nonoverlapping( @@ -1062,6 +1063,9 @@ mod tests { let mut world = World::default(); let layout = Layout::array::(COMPONENT_SIZE).unwrap(); + // SAFETY: + // - No drop command is required + // - The component will store [u8; COMPONENT_SIZE], which is Send + Sync let descriptor = unsafe { ComponentDescriptor::new_with_layout( "DynamicComp", @@ -1082,6 +1086,9 @@ mod tests { let mut entity = world.spawn_empty(); let data = [5u8; COMPONENT_SIZE]; + // SAFETY: + // - ptr points to data represented by component_id ([u8; COMPONENT_SIZE]) + // - component_id is from the same world as entity OwningPtr::make(data, |ptr| unsafe { entity.insert_by_id(component_id, ptr); }); @@ -1093,6 +1100,7 @@ mod tests { let ptr = world.get_by_id(entity, component_id).unwrap(); let clone_ptr = world.get_by_id(entity_clone, component_id).unwrap(); + // SAFETY: ptr and clone_ptr store component represented by [u8; COMPONENT_SIZE] unsafe { assert_eq!( core::slice::from_raw_parts(ptr.as_ptr(), COMPONENT_SIZE), From 1ae0d0db0fe397d32619b2dbbf9a3982dd171405 Mon Sep 17 00:00:00 2001 From: eugineerd Date: Wed, 18 Dec 2024 16:41:26 +0000 Subject: [PATCH 36/40] `no_std` --- crates/bevy_ecs/src/component.rs | 2 +- crates/bevy_ecs/src/entity/clone_entities.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index 107afadccc39f..86f76dbf78c8c 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -11,7 +11,7 @@ use crate::{ system::{Local, Resource, SystemParam}, world::{DeferredWorld, FromWorld, World}, }; -use alloc::{borrow::Cow, format, vec::Vec}; +use alloc::{borrow::Cow, boxed::Box, format, vec::Vec}; pub use bevy_ecs_macros::Component; use bevy_ptr::{OwningPtr, UnsafeCellDeref}; #[cfg(feature = "bevy_reflect")] diff --git a/crates/bevy_ecs/src/entity/clone_entities.rs b/crates/bevy_ecs/src/entity/clone_entities.rs index 34a4d97003595..2468973344e05 100644 --- a/crates/bevy_ecs/src/entity/clone_entities.rs +++ b/crates/bevy_ecs/src/entity/clone_entities.rs @@ -1,4 +1,4 @@ -use alloc::{borrow::ToOwned, vec::Vec}; +use alloc::{borrow::ToOwned, boxed::Box, vec::Vec}; use bevy_ptr::{Ptr, PtrMut}; use bumpalo::Bump; use core::{any::TypeId, ptr::NonNull}; From a2a78d692cdfe9de19c2db7b4b711c80ca7d0646 Mon Sep 17 00:00:00 2001 From: eugineerd <70062110+eugineerd@users.noreply.github.com> Date: Wed, 18 Dec 2024 21:19:19 +0300 Subject: [PATCH 37/40] Fix missed replacement of `self.components` with `self.component_info` inside `ComponentCloneCtx` Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com> --- crates/bevy_ecs/src/entity/clone_entities.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/entity/clone_entities.rs b/crates/bevy_ecs/src/entity/clone_entities.rs index 2468973344e05..6e1c33d33e53f 100644 --- a/crates/bevy_ecs/src/entity/clone_entities.rs +++ b/crates/bevy_ecs/src/entity/clone_entities.rs @@ -205,8 +205,7 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> { if source_type_id != component_type_id { panic!("Passed component TypeId does not match source component TypeId") } - let component_info = self.components.get_info(self.component_id).unwrap(); - let component_layout = component_info.layout(); + let component_layout = self.component_info.layout(); let component_data_ptr = Box::into_raw(component).cast::(); let target_component_data_ptr = From 35a19029785132e3da56fd6700e983794930dabe Mon Sep 17 00:00:00 2001 From: eugineerd <70062110+eugineerd@users.noreply.github.com> Date: Wed, 18 Dec 2024 21:19:43 +0300 Subject: [PATCH 38/40] add `TypeId` check in `ReflectFromWorld` component clone path Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com> --- crates/bevy_ecs/src/component.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index 86f76dbf78c8c..61c5b5d243386 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -2237,6 +2237,7 @@ pub fn component_clone_via_reflect(world: &mut DeferredWorld, ctx: &mut Componen let component_id = ctx.component_id(); world.commands().queue(move |world: &mut World| { let mut component = reflect_from_world.from_world(world); + assert_eq!(type_id, component.type_id()); component.apply(source_component_cloned.as_partial_reflect()); // SAFETY: // - component_id is from the same world as target entity From b02678d07a19354db8e615aecf679dcbfd61ca31 Mon Sep 17 00:00:00 2001 From: eugineerd Date: Wed, 18 Dec 2024 18:28:18 +0000 Subject: [PATCH 39/40] fix getting wrong `type_id` --- crates/bevy_ecs/src/component.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index 61c5b5d243386..88f6ed8ffd451 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -2237,7 +2237,7 @@ pub fn component_clone_via_reflect(world: &mut DeferredWorld, ctx: &mut Componen let component_id = ctx.component_id(); world.commands().queue(move |world: &mut World| { let mut component = reflect_from_world.from_world(world); - assert_eq!(type_id, component.type_id()); + assert_eq!(type_id, (*component).type_id()); component.apply(source_component_cloned.as_partial_reflect()); // SAFETY: // - component_id is from the same world as target entity From 4abf6b96b6304ce40b9015880b7caedf9a076c1b Mon Sep 17 00:00:00 2001 From: eugineerd Date: Wed, 18 Dec 2024 19:11:30 +0000 Subject: [PATCH 40/40] fix build without default features --- crates/bevy_ecs/src/component.rs | 5 ++++- crates/bevy_ecs/src/entity/clone_entities.rs | 13 +++++++++---- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index 88f6ed8ffd451..f0cd6a6b5bcaf 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -11,7 +11,9 @@ use crate::{ system::{Local, Resource, SystemParam}, world::{DeferredWorld, FromWorld, World}, }; -use alloc::{borrow::Cow, boxed::Box, format, vec::Vec}; +#[cfg(feature = "bevy_reflect")] +use alloc::boxed::Box; +use alloc::{borrow::Cow, format, vec::Vec}; pub use bevy_ecs_macros::Component; use bevy_ptr::{OwningPtr, UnsafeCellDeref}; #[cfg(feature = "bevy_reflect")] @@ -1006,6 +1008,7 @@ impl ComponentCloneHandler { } /// Set clone handler based on `Reflect` trait. + #[cfg(feature = "bevy_reflect")] pub fn reflect_handler() -> Self { Self(Some(component_clone_via_reflect)) } diff --git a/crates/bevy_ecs/src/entity/clone_entities.rs b/crates/bevy_ecs/src/entity/clone_entities.rs index 6e1c33d33e53f..b15723aa57419 100644 --- a/crates/bevy_ecs/src/entity/clone_entities.rs +++ b/crates/bevy_ecs/src/entity/clone_entities.rs @@ -1,10 +1,13 @@ -use alloc::{borrow::ToOwned, boxed::Box, vec::Vec}; +use alloc::{borrow::ToOwned, vec::Vec}; use bevy_ptr::{Ptr, PtrMut}; use bumpalo::Bump; use core::{any::TypeId, ptr::NonNull}; use bevy_utils::{HashMap, HashSet}; +#[cfg(feature = "bevy_reflect")] +use alloc::boxed::Box; + #[cfg(feature = "portable-atomic")] use portable_atomic_util::Arc; @@ -34,6 +37,9 @@ pub struct ComponentCloneCtx<'a, 'b> { entity_cloner: &'a EntityCloner, #[cfg(feature = "bevy_reflect")] type_registry: Option<&'a crate::reflect::AppTypeRegistry>, + #[cfg(not(feature = "bevy_reflect"))] + #[expect(dead_code)] + type_registry: Option<()>, } impl<'a, 'b> ComponentCloneCtx<'a, 'b> { @@ -51,6 +57,7 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> { components: &'a Components, entity_cloner: &'a EntityCloner, #[cfg(feature = "bevy_reflect")] type_registry: Option<&'a crate::reflect::AppTypeRegistry>, + #[cfg(not(feature = "bevy_reflect"))] type_registry: Option<()>, ) -> Self { Self { component_id, @@ -61,7 +68,6 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> { components, component_info: components.get_info_unchecked(component_id), entity_cloner, - #[cfg(feature = "bevy_reflect")] type_registry, } } @@ -288,7 +294,7 @@ impl EntityCloner { #[cfg(feature = "bevy_reflect")] let app_registry = world.get_resource::(); #[cfg(not(feature = "bevy_reflect"))] - let app_registry = None; + let app_registry = Option::<()>::None; ( app_registry, @@ -333,7 +339,6 @@ impl EntityCloner { &component_data, components, self, - #[cfg(feature = "bevy_reflect")] type_registry, ) };