Skip to content
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

Extend cloning functionality and add convenience methods to EntityWorldMut and EntityCommands #16826

Merged
merged 3 commits into from
Dec 16, 2024
Merged

Extend cloning functionality and add convenience methods to EntityWorldMut and EntityCommands #16826

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1d6e8d1
add `clone_components`, `move_components`, and variants
JaySpruce Dec 15, 2024
4e6abaf
move variant functionality to builder, add `clone_with`
JaySpruce Dec 16, 2024
97642e8
implement suggestions
JaySpruce Dec 16, 2024
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
193 changes: 135 additions & 58 deletions crates/bevy_ecs/src/entity/clone_entities.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ pub struct EntityCloner {
filter_allows_components: bool,
filter: Arc<HashSet<ComponentId>>,
clone_handlers_overrides: Arc<HashMap<ComponentId, ComponentCloneHandler>>,
move_components: bool,
}

impl EntityCloner {
Expand All @@ -35,17 +36,21 @@ impl EntityCloner {
.filter(|id| self.is_cloning_allowed(id)),
);

for component in components {
for component in &components {
let global_handlers = world.components().get_component_clone_handlers();
let handler = match self.clone_handlers_overrides.get(&component) {
None => global_handlers.get_handler(component),
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);
self.component_id = Some(*component);
(handler)(&mut world.into(), self);
}

if self.move_components {
world.entity_mut(self.source).remove_by_ids(&components);
}
}

fn is_cloning_allowed(&self, component: &ComponentId) -> bool {
Expand Down Expand Up @@ -145,6 +150,8 @@ pub struct EntityCloneBuilder<'w> {
filter_allows_components: bool,
filter: HashSet<ComponentId>,
clone_handlers_overrides: HashMap<ComponentId, ComponentCloneHandler>,
attach_required_components: bool,
move_components: bool,
}

impl<'w> EntityCloneBuilder<'w> {
Expand All @@ -155,6 +162,8 @@ impl<'w> EntityCloneBuilder<'w> {
filter_allows_components: false,
filter: Default::default(),
clone_handlers_overrides: Default::default(),
attach_required_components: false,
move_components: false,
}
}

Expand All @@ -165,6 +174,7 @@ impl<'w> EntityCloneBuilder<'w> {
filter_allows_components,
filter,
clone_handlers_overrides,
move_components,
..
} = self;

Expand All @@ -175,29 +185,51 @@ impl<'w> EntityCloneBuilder<'w> {
filter_allows_components,
filter: Arc::new(filter),
clone_handlers_overrides: Arc::new(clone_handlers_overrides),
move_components,
}
.clone_entity(world);

world.flush_commands();
}

/// Allows for a scoped mode where any changes to the filter that allow/deny components
/// will also allow/deny those components' required components, recursively.
pub fn with_required_components(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool. @cart is almost certainly going to want to want this on by default, possibly without the ability to disable it. The vision is to treat these as "one indivisible object" wherever possible, at least for "required" components :)

I agree with that philosophy, and I think the toggle should basically be "replace required components with their default values". The default value should be "copy all required components that you can".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit off-topic for this PR, but I wonder if DynamicSceneBuilder should also extract required components in that case?

Copy link
Contributor Author

@JaySpruce JaySpruce Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's essentially what the toggle is, since components are ultimately inserted with normal insert methods. I can switch the default though.

Here's a couple colors for the bikeshed:

  • default_required_components
  • with_default_required_components
  • replace_required_components
  • replace_required_components_with_default
  • without_required_components

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we should probably be unifying the mechanisms between this and the scenes stuff. Another PR / issue though! As for the bikeshed, go with the long and clear one please: this is quite niche :)

&mut self,
builder: impl FnOnce(&mut EntityCloneBuilder) + Send + Sync + 'static,
) -> &mut Self {
self.attach_required_components = true;
builder(self);
self.attach_required_components = false;
self
}

/// Sets the cloner to remove any components that were cloned,
/// effectively moving them from the source entity to the target.
///
/// This only applies to components that are allowed through the filter
/// at the time [`EntityCloneBuilder::clone_entity`] is called.
pub fn enable_move_on_clone(&mut self) -> &mut Self {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bikeshed: I think these methods might be clearer if they were move_components and keep_components. The enable/disable terminology was confusing to me at first.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should just be one method: move_components that takes a bool. That way it would be in line with the other EntityCloneBuilder options (like add_observers). It was designed to look somewhat similar to std::fs::OpenOptions.

self.move_components = true;
self
}

/// The default setting for the cloner. The source entity will keep
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bit about this being the default should be in the second line, not the first.

/// all components that are cloned.
pub fn disable_move_on_clone(&mut self) -> &mut Self {
self.move_components = false;
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
/// [`deny_all`](`Self::deny_all`) before calling any of the `allow` methods.
pub fn allow<T: Bundle>(&mut self) -> &mut Self {
if self.filter_allows_components {
T::get_component_ids(self.world.components(), &mut |id| {
if let Some(id) = id {
self.filter.insert(id);
}
});
} else {
T::get_component_ids(self.world.components(), &mut |id| {
if let Some(id) = id {
self.filter.remove(&id);
}
});
let bundle = self.world.register_bundle::<T>();
let ids = bundle.explicit_components().to_owned();
for id in ids {
self.filter_allow(id);
}
self
}
Expand All @@ -207,12 +239,8 @@ impl<'w> EntityCloneBuilder<'w> {
/// Note that all components are allowed by default, to clone only explicitly allowed components make sure to call
/// [`deny_all`](`Self::deny_all`) before calling any of the `allow` methods.
pub fn allow_by_ids(&mut self, ids: impl IntoIterator<Item = ComponentId>) -> &mut Self {
if self.filter_allows_components {
self.filter.extend(ids);
} else {
ids.into_iter().for_each(|id| {
self.filter.remove(&id);
});
for id in ids {
self.filter_allow(id);
}
self
}
Expand All @@ -222,15 +250,10 @@ impl<'w> EntityCloneBuilder<'w> {
/// Note that all components are allowed by default, to clone only explicitly allowed components make sure to call
/// [`deny_all`](`Self::deny_all`) before calling any of the `allow` methods.
pub fn allow_by_type_ids(&mut self, ids: impl IntoIterator<Item = TypeId>) -> &mut Self {
let ids = ids
.into_iter()
.filter_map(|id| self.world.components().get_id(id));
if self.filter_allows_components {
self.filter.extend(ids);
} else {
ids.into_iter().for_each(|id| {
self.filter.remove(&id);
});
for type_id in ids {
if let Some(id) = self.world.components().get_id(type_id) {
self.filter_allow(id);
}
}
self
}
Expand All @@ -244,45 +267,28 @@ impl<'w> EntityCloneBuilder<'w> {

/// Disallows all components of the bundle from being cloned.
pub fn deny<T: Bundle>(&mut self) -> &mut Self {
if self.filter_allows_components {
T::get_component_ids(self.world.components(), &mut |id| {
if let Some(id) = id {
self.filter.remove(&id);
}
});
} else {
T::get_component_ids(self.world.components(), &mut |id| {
if let Some(id) = id {
self.filter.insert(id);
}
});
let bundle = self.world.register_bundle::<T>();
let ids = bundle.explicit_components().to_owned();
for id in ids {
self.filter_deny(id);
}
self
}

/// Extends the list of components that shouldn't be cloned.
pub fn deny_by_ids(&mut self, ids: impl IntoIterator<Item = ComponentId>) -> &mut Self {
if self.filter_allows_components {
ids.into_iter().for_each(|id| {
self.filter.remove(&id);
});
} else {
self.filter.extend(ids);
for id in ids {
self.filter_deny(id);
}
self
}

/// Extends the list of components that shouldn't be cloned by type ids.
pub fn deny_by_type_ids(&mut self, ids: impl IntoIterator<Item = TypeId>) -> &mut Self {
let ids = ids
.into_iter()
.filter_map(|id| self.world.components().get_id(id));
if self.filter_allows_components {
ids.into_iter().for_each(|id| {
self.filter.remove(&id);
});
} else {
self.filter.extend(ids);
for type_id in ids {
if let Some(id) = self.world.components().get_id(type_id) {
self.filter_deny(id);
}
}
self
}
Expand Down Expand Up @@ -315,11 +321,52 @@ impl<'w> EntityCloneBuilder<'w> {
}
self
}

/// Helper function that allows a component through the filter.
fn filter_allow(&mut self, id: ComponentId) {
if self.filter_allows_components {
self.filter.insert(id);
} else {
self.filter.remove(&id);
}
if self.attach_required_components {
if let Some(info) = self.world.components().get_info(id) {
for required_id in info.required_components().iter_ids() {
if self.filter_allows_components {
self.filter.insert(required_id);
} else {
self.filter.remove(&required_id);
}
}
}
}
}

/// Helper function that disallows a component through the filter.
fn filter_deny(&mut self, id: ComponentId) {
if self.filter_allows_components {
self.filter.remove(&id);
} else {
self.filter.insert(id);
}
if self.attach_required_components {
if let Some(info) = self.world.components().get_info(id) {
for required_id in info.required_components().iter_ids() {
if self.filter_allows_components {
self.filter.remove(&required_id);
} else {
self.filter.insert(required_id);
}
}
}
}
}
}

#[cfg(test)]
mod tests {
use crate::{self as bevy_ecs, component::Component, entity::EntityCloneBuilder, world::World};
use bevy_ecs_macros::require;

#[cfg(feature = "bevy_reflect")]
#[test]
Expand Down Expand Up @@ -520,4 +567,34 @@ mod tests {
assert!(world.get::<B>(e_clone).is_none());
assert!(world.get::<C>(e_clone).is_none());
}

#[test]
fn clone_entity_with_required_components() {
#[derive(Component, Clone, PartialEq, Debug)]
#[require(B)]
struct A;

#[derive(Component, Clone, PartialEq, Debug, Default)]
#[require(C(|| C(5)))]
struct B;

#[derive(Component, Clone, PartialEq, Debug)]
struct C(u32);

let mut world = World::default();

let e = world.spawn(A).id();
let e_clone = world.spawn_empty().id();

let mut builder = EntityCloneBuilder::new(&mut world);
builder.deny_all();
builder.with_required_components(|builder| {
builder.allow::<B>();
});
builder.clone_entity(e, e_clone);

assert_eq!(world.entity(e_clone).get::<A>(), None);
assert_eq!(world.entity(e_clone).get::<B>(), Some(&B));
assert_eq!(world.entity(e_clone).get::<C>(), Some(&C(5)));
}
}
Loading
Loading