Skip to content

bevy_reflect: Deprecate PartialReflect::clone_value #18284

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alice-i-cecile merged 3 commits into from
Mar 14, 2025
Merged

bevy_reflect: Deprecate PartialReflect::clone_value #18284

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
262f22f
Deprecate `PartialReflect::clone_value`
MrGVSV Mar 12, 2025
fa28e5c
Replace usages of `clone_value`
MrGVSV Mar 12, 2025
15d972a
Fix deallocation bug
MrGVSV Mar 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions benches/benches/bevy_reflect/list.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use criterion::{
criterion_group!(
benches,
concrete_list_apply,
concrete_list_clone_dynamic,
concrete_list_to_dynamic_list,
dynamic_list_apply,
dynamic_list_push
);
Expand Down Expand Up @@ -81,20 +81,20 @@ fn concrete_list_apply(criterion: &mut Criterion) {
list_apply(&mut group, "empty_base_concrete_patch", empty_base, patch);

list_apply(&mut group, "empty_base_dynamic_patch", empty_base, |size| {
patch(size).clone_dynamic()
patch(size).to_dynamic_list()
});

list_apply(&mut group, "same_len_concrete_patch", full_base, patch);

list_apply(&mut group, "same_len_dynamic_patch", full_base, |size| {
patch(size).clone_dynamic()
patch(size).to_dynamic_list()
});

group.finish();
}

fn concrete_list_clone_dynamic(criterion: &mut Criterion) {
let mut group = create_group(criterion, bench!("concrete_list_clone_dynamic"));
fn concrete_list_to_dynamic_list(criterion: &mut Criterion) {
let mut group = create_group(criterion, bench!("concrete_list_to_dynamic_list"));

for size in SIZES {
group.throughput(Throughput::Elements(size as u64));
Expand All @@ -105,7 +105,7 @@ fn concrete_list_clone_dynamic(criterion: &mut Criterion) {
|bencher, &size| {
let v = iter::repeat_n(0, size).collect::<Vec<_>>();

bencher.iter(|| black_box(&v).clone_dynamic());
bencher.iter(|| black_box(&v).to_dynamic_list());
},
);
}
Expand All @@ -127,7 +127,7 @@ fn dynamic_list_push(criterion: &mut Criterion) {
let dst = DynamicList::default();

bencher.iter_batched(
|| (src.clone(), dst.clone_dynamic()),
|| (src.clone(), dst.to_dynamic_list()),
|(src, mut dst)| {
for item in src {
dst.push(item);
Expand All @@ -145,20 +145,20 @@ fn dynamic_list_push(criterion: &mut Criterion) {
fn dynamic_list_apply(criterion: &mut Criterion) {
let mut group = create_group(criterion, bench!("dynamic_list_apply"));

let empty_base = |_: usize| || Vec::<u64>::new().clone_dynamic();
let empty_base = |_: usize| || Vec::<u64>::new().to_dynamic_list();
let full_base = |size: usize| move || iter::repeat_n(0, size).collect::<Vec<u64>>();
let patch = |size: usize| iter::repeat_n(1, size).collect::<Vec<u64>>();

list_apply(&mut group, "empty_base_concrete_patch", empty_base, patch);

list_apply(&mut group, "empty_base_dynamic_patch", empty_base, |size| {
patch(size).clone_dynamic()
patch(size).to_dynamic_list()
});

list_apply(&mut group, "same_len_concrete_patch", full_base, patch);

list_apply(&mut group, "same_len_dynamic_patch", full_base, |size| {
patch(size).clone_dynamic()
patch(size).to_dynamic_list()
});

group.finish();
Expand Down
14 changes: 7 additions & 7 deletions benches/benches/bevy_reflect/map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ fn concrete_map_apply(criterion: &mut Criterion) {
);

map_apply(&mut group, "empty_base_dynamic_patch", empty_base, |size| {
key_range_patch(size).clone_dynamic()
key_range_patch(size).to_dynamic_map()
});

map_apply(
Expand All @@ -122,7 +122,7 @@ fn concrete_map_apply(criterion: &mut Criterion) {
&mut group,
"same_keys_dynamic_patch",
key_range_base,
|size| key_range_patch(size).clone_dynamic(),
|size| key_range_patch(size).to_dynamic_map(),
);

map_apply(
Expand All @@ -136,7 +136,7 @@ fn concrete_map_apply(criterion: &mut Criterion) {
&mut group,
"disjoint_keys_dynamic_patch",
key_range_base,
|size| disjoint_patch(size).clone_dynamic(),
|size| disjoint_patch(size).to_dynamic_map(),
);
}

Expand All @@ -159,7 +159,7 @@ fn dynamic_map_apply(criterion: &mut Criterion) {
(0..size as u64)
.zip(iter::repeat(0))
.collect::<HashMap<u64, u64>>()
.clone_dynamic()
.to_dynamic_map()
}
};

Expand All @@ -183,7 +183,7 @@ fn dynamic_map_apply(criterion: &mut Criterion) {
);

map_apply(&mut group, "empty_base_dynamic_patch", empty_base, |size| {
key_range_patch(size).clone_dynamic()
key_range_patch(size).to_dynamic_map()
});

map_apply(
Expand All @@ -197,7 +197,7 @@ fn dynamic_map_apply(criterion: &mut Criterion) {
&mut group,
"same_keys_dynamic_patch",
key_range_base,
|size| key_range_patch(size).clone_dynamic(),
|size| key_range_patch(size).to_dynamic_map(),
);

map_apply(
Expand All @@ -211,7 +211,7 @@ fn dynamic_map_apply(criterion: &mut Criterion) {
&mut group,
"disjoint_keys_dynamic_patch",
key_range_base,
|size| disjoint_patch(size).clone_dynamic(),
|size| disjoint_patch(size).to_dynamic_map(),
);
}

Expand Down
36 changes: 18 additions & 18 deletions benches/benches/bevy_reflect/struct.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ criterion_group!(
concrete_struct_apply,
concrete_struct_field,
concrete_struct_type_info,
concrete_struct_clone,
dynamic_struct_clone,
concrete_struct_to_dynamic_struct,
dynamic_struct_to_dynamic_struct,
dynamic_struct_apply,
dynamic_struct_get_field,
dynamic_struct_insert,
Expand Down Expand Up @@ -113,7 +113,7 @@ fn concrete_struct_apply(criterion: &mut Criterion) {
bencher.iter_batched(
|| {
let (obj, _) = input();
let patch = obj.clone_dynamic();
let patch = obj.to_dynamic_struct();
(obj, patch)
},
|(mut obj, patch)| obj.apply(black_box(&patch)),
Expand Down Expand Up @@ -170,8 +170,8 @@ fn concrete_struct_type_info(criterion: &mut Criterion) {
}
}

fn concrete_struct_clone(criterion: &mut Criterion) {
let mut group = create_group(criterion, bench!("concrete_struct_clone"));
fn concrete_struct_to_dynamic_struct(criterion: &mut Criterion) {
let mut group = create_group(criterion, bench!("concrete_struct_to_dynamic_struct"));

let structs: [(Box<dyn Struct>, Box<dyn Struct>); 5] = [
(
Expand Down Expand Up @@ -203,28 +203,28 @@ fn concrete_struct_clone(criterion: &mut Criterion) {
BenchmarkId::new("NonGeneric", field_count),
&standard,
|bencher, s| {
bencher.iter(|| s.clone_dynamic());
bencher.iter(|| s.to_dynamic_struct());
},
);
group.bench_with_input(
BenchmarkId::new("Generic", field_count),
&generic,
|bencher, s| {
bencher.iter(|| s.clone_dynamic());
bencher.iter(|| s.to_dynamic_struct());
},
);
}
}

fn dynamic_struct_clone(criterion: &mut Criterion) {
let mut group = create_group(criterion, bench!("dynamic_struct_clone"));
fn dynamic_struct_to_dynamic_struct(criterion: &mut Criterion) {
let mut group = create_group(criterion, bench!("dynamic_struct_to_dynamic_struct"));

let structs: [Box<dyn Struct>; 5] = [
Box::new(Struct1::default().clone_dynamic()),
Box::new(Struct16::default().clone_dynamic()),
Box::new(Struct32::default().clone_dynamic()),
Box::new(Struct64::default().clone_dynamic()),
Box::new(Struct128::default().clone_dynamic()),
Box::new(Struct1::default().to_dynamic_struct()),
Box::new(Struct16::default().to_dynamic_struct()),
Box::new(Struct32::default().to_dynamic_struct()),
Box::new(Struct64::default().to_dynamic_struct()),
Box::new(Struct128::default().to_dynamic_struct()),
];

for s in structs {
Expand All @@ -234,7 +234,7 @@ fn dynamic_struct_clone(criterion: &mut Criterion) {
BenchmarkId::from_parameter(field_count),
&s,
|bencher, s| {
bencher.iter(|| s.clone_dynamic());
bencher.iter(|| s.to_dynamic_struct());
},
);
}
Expand Down Expand Up @@ -265,7 +265,7 @@ fn dynamic_struct_apply(criterion: &mut Criterion) {
&patch,
|bencher, patch| {
bencher.iter_batched(
|| (base.clone_dynamic(), patch()),
|| (base.to_dynamic_struct(), patch()),
|(mut base, patch)| base.apply(black_box(&*patch)),
BatchSize::SmallInput,
);
Expand All @@ -289,7 +289,7 @@ fn dynamic_struct_apply(criterion: &mut Criterion) {
}

bencher.iter_batched(
|| base.clone_dynamic(),
|| base.to_dynamic_struct(),
|mut base| base.apply(black_box(&patch)),
BatchSize::SmallInput,
);
Expand All @@ -315,7 +315,7 @@ fn dynamic_struct_insert(criterion: &mut Criterion) {

let field = format!("field_{}", field_count);
bencher.iter_batched(
|| s.clone_dynamic(),
|| s.to_dynamic_struct(),
|mut s| {
s.insert(black_box(&field), ());
},
Expand Down
16 changes: 12 additions & 4 deletions crates/bevy_asset/src/handle.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -658,7 +658,7 @@ mod tests {
assert_eq!(UntypedHandle::from(typed.clone()), untyped);
}

/// `Reflect::clone_value` should increase the strong count of a strong handle
/// `PartialReflect::reflect_clone`/`PartialReflect::to_dynamic` should increase the strong count of a strong handle
#[test]
fn strong_handle_reflect_clone() {
use crate::{AssetApp, AssetPlugin, Assets, VisitAssetDependencies};
Expand Down Expand Up @@ -689,18 +689,26 @@ mod tests {
);

let reflected: &dyn Reflect = &handle;
let cloned_handle: Box<dyn PartialReflect> = reflected.clone_value();
let _cloned_handle: Box<dyn Reflect> = reflected.reflect_clone().unwrap();

assert_eq!(
Arc::strong_count(strong),
2,
"Cloning the handle with reflect should increase the strong count to 2"
);

let dynamic_handle: Box<dyn PartialReflect> = reflected.to_dynamic();

assert_eq!(
Arc::strong_count(strong),
3,
"Converting the handle to a dynamic should increase the strong count to 3"
);

let from_reflect_handle: Handle<MyAsset> =
FromReflect::from_reflect(&*cloned_handle).unwrap();
FromReflect::from_reflect(&*dynamic_handle).unwrap();

assert_eq!(Arc::strong_count(strong), 3, "Converting the reflected value back to a handle should increase the strong count to 3");
assert_eq!(Arc::strong_count(strong), 4, "Converting the reflected value back to a handle should increase the strong count to 4");
assert!(
from_reflect_handle.is_strong(),
"The cloned handle should still be strong"
Expand Down
27 changes: 24 additions & 3 deletions crates/bevy_ecs/src/component.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2911,12 +2911,14 @@ pub fn component_clone_via_clone<C: Clone + Component>(
/// - Component has [`TypeId`]
/// - Component is registered
/// - Component has [`ReflectFromPtr`](bevy_reflect::ReflectFromPtr) registered
/// - Component has one of the following registered: [`ReflectFromReflect`](bevy_reflect::ReflectFromReflect),
/// - Component can be cloned via [`PartialReflect::reflect_clone`] _or_ 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 [`EntityClonerBuilder`](crate::entity::EntityClonerBuilder) for details.
///
/// [`PartialReflect::reflect_clone`]: bevy_reflect::PartialReflect::reflect_clone
#[cfg(feature = "bevy_reflect")]
pub fn component_clone_via_reflect(
commands: &mut Commands,
Expand All @@ -2934,6 +2936,21 @@ pub fn component_clone_via_reflect(
// checked in read_source_component_reflect
let type_id = component_info.type_id().unwrap();

// Try to clone using `reflect_clone`
if let Ok(mut component) = source_component_reflect.reflect_clone() {
if let Some(reflect_component) =
registry.get_type_data::<crate::reflect::ReflectComponent>(type_id)
{
reflect_component.visit_entities_mut(&mut *component, &mut |entity| {
*entity = ctx.entity_mapper().get_mapped(*entity);
});
}
drop(registry);

ctx.write_target_component_reflect(component);
return;
}

// Try to clone using ReflectFromReflect
if let Some(reflect_from_reflect) =
registry.get_type_data::<bevy_reflect::ReflectFromReflect>(type_id)
Expand Down Expand Up @@ -2977,7 +2994,7 @@ pub fn component_clone_via_reflect(
mapped_entities.push(entity);
});
}
let source_component_cloned = source_component_reflect.clone_value();
let source_component_cloned = source_component_reflect.to_dynamic();
let component_layout = component_info.layout();
let target = ctx.target();
let component_id = ctx.component_id();
Expand Down Expand Up @@ -3008,7 +3025,11 @@ pub fn component_clone_via_reflect(
world
.entity_mut(target)
.insert_by_id(component_id, OwningPtr::new(raw_component_ptr));
alloc::alloc::dealloc(raw_component_ptr.as_ptr(), component_layout);

if component_layout.size() > 0 {
// Ensure we don't attempt to deallocate zero-sized components
alloc::alloc::dealloc(raw_component_ptr.as_ptr(), component_layout);
}
}
});
}
Expand Down
Loading