Moving components instead of duplicating should lift the Clone/Reflect requirement

[urben1680]

What problem does this solve or what need does it fill?

With EntityCloner, one can give an entity the component of another. By default the original entity keeps it's component and this operation requires the component to be clone-able.

However, if the component is moved, so the original entity loses the component, this should be no requirement. But it currently is. Move operations just fail silently

#[derive(Component)] // adding Clone or Reflect makes the assertions not fail
struct NoCloneImpl;

let mut world = World::new();
let entity1 = world.spawn_empty().id();
let entity2 = world
    .spawn(NoCloneImpl)
    .move_components::<NoCloneImpl>(entity1) // fails silently
    .id();

assert_eq!(world.entity(entity1).contains::<NoCloneImpl>(), true); // panic
assert_eq!(world.entity(entity2).contains::<NoCloneImpl>(), false); // panic

(The same effect can be observed with EntityCloner)

What solution would you like?

Make it possible to move components without them implementing Clone nor Reflect.

Additional context

This was an open todo from @eugineerd, mentioning that it is tricky to not have the component be dropped twice. I just wanted to open this issue so it is written down somewhere.

(Not sure if this is a bug issue or feature request issue)

[eugineerd]

To give some context about the problem:
move_components in EntityCloner works by using EntityWorldMut::remove_by_ids, but this drops the components' values. What we want is actually something like EntityWorldMut::forget_by_ids (not too difficult to implement). The real problem is that we would still (probably?) want to run remove hooks and observers on moved components.
To do that, EntityWorldMut::forget_by_ids would have to run these hooks just like EntityWorldMut::remove_by_ids does:

1. copy component data from source
2. call forget_by_ids
3. insert component data into target

However, hooks and observers can move data out of components, so after step 2 we might have invalid component data stored. Instead, the correct way to do it looks something like this:

1. run remove hooks/observes
2. copy component data from source
3. call forget_by_ids without running any hooks/observers
4. insert component data into target

So the question really is how to design such interface without having to reimplement most of EntityWorldMut::remove_bundle.

[grind086]

I noticed a couple of TODO: BundleRemover? comments and gave that a shot. It seems like it'd be a good way to go, but I borked something when I tried it.

I think it's possible to clean up a lot of the repetitive *_with_caller methods this way too. Rather than repetitive special-cased internal methods, those things could be configured on the BundleRemover. A few helper functions could also be turned into methods on BundleRemover (or just moved to bundle.rs).

[urben1680]

This change could also require some adjustments on relations.

The cloning behavior currently is to skip Relationship(Target) components. If moving becomes unconditional then this could break it, like when moving such a component into an entity where it is already present. RelationshipTargets may be possible to be combined, but not Relationship. A mechanism needs to ensure these still are not moved.

[urben1680]

Does this become easier now that there is a BundleRemover thanks to #18521?

[eugineerd]

Does this become easier now that there is a BundleRemover thanks to #18521?

Yes, as far as I can see. It would require restructuring the current EntityCloner::clone_entity_internal code to run component cloning inside the BundleRemover's pre_remove function, which shouldn't be too difficult to do.