allow EntityCloner to move components without Clone or Reflect

[eugineerd]

Objective

Fix #18079

Solution

• EntityCloner can now move components that don't have Clone or Reflect implementation.
• Components with ComponentCloneBehavior::Ignore will not be moved.
• Components with ComponentCloneBehavior::Custom will be cloned using their defined ComponentCloneFn and then removed from the source entity to respect their queue_deferred logic.
• Relationships still need to be Clone or Reflect to be movable.
• Custom relationship data is now correctly preserved when cloned or moved using EntityCloner.

Testing

• Added new tests for moving components

[alice-i-cecile]

@urben1680 requesting your review here :)

[urben1680]

Oh I would love this landing on 0.17, thank you for implementing this. I am still not confident enough myself in these regions of the ECS.

[eugineerd]

After trying to debug where the source part of relationship gets new values for the target entity on cloning, I've finally found that it's done by entity mapping inside write_target_component, which is a generic function used by component_clone_via_clone. This means that to support moving relationships we'd have to store component's map_entities function somewhere (likely on ComponentDescriptor, which I'm hesitant to add more special stuff to since dynamic components will have to be aware of it too) if we want to support moving relationships without Clone bound. At this point I'm questioning whether it even makes sense to "move" a subset of components on a hierarchy of entities like that. Are there actually any use cases for that action?

It's also tempting to split component moving functionality into a different struct because of how many edge cases there are now, but even if we do that I think it should be done in a different PR.

Now, as for cloning data on relationship components: unsurprisingly, it currently works only for the source part of relationship. I think supporting cloning data on the target part will require some more design work to solve, so it would make sense to have it as a separate issue.

---

@urben1680 @alice-i-cecile, how should we proceed with this PR at this point?

[urben1680]

At this point I'm questioning whether it even makes sense to "move" a subset of components on a hierarchy of entities like that. Are there actually any use cases for that action?

In my crate, I want to implement reversible structural changes to entities. For example when component A gets overwritten by a new value via a command, I want to "backup" the old value first. Backup means here that I use an EntityCloner to move the value into a disabled buffer entity. At "undo" I do the same with the new value and a second buffer entity, so I can return the value from the first.

This component A may very well be a Relationship component. However, other than what we try here, I cannot store it at an entity because I don't want a usual entity to be suddenly related to my buffer entities that I try to keep hidden. So for my crate I already have to find an alternative to backup relationship components.

tl;dr I have no use case for moving relationship components.

It also seems weird that a user has a collection of entities that are not yet related, and relate them by moving the relationship components from an existing "family" instead of doing it "properly" by relating them directly.

how should we proceed with this PR at this point?

I think we want to somehow map the custom relationship target cloning behavior to Ignore. But as you noticed, comparing fn for equality is not reliable. 🫤

Maybe we need a ComponentCloneBehavior::RelationshipTarget(fn(...)) instead of Custom for them so you can match them in an untyped context.

[eugineerd]

I feel like the biggest problem with the current implementation right now is that cloning behavior has to work around component hooks, and the existence of RelationshipHookMode is a proof of that. It would've been so much easier if we could suppress on_replace/on_insert/on_remove hooks and implement all the transition logic in separate on_move/on_clone hooks. That would've given us more explicit control over how cloning/moving component works and protected from having to think about at which point in cloning logic other hooks (if any) will run. Besides that, there is value in differentiating between "data clone" (the Clone implementation) and cloning effect logic (on_clone hook), since "data clone" can be useful in other contexts as well.

With all that in mind, I believe the best course of action for this PR would be to disallow moving relationship components without Clone or Reflect implementation for now. Those will probably be quite rare and moving them doesn't currently work on main either, so nothing of value will really be lost.

I will look into moving EntityCloner to on_clone/on_move hooks later, but I think it will require large refactors of existing code and don't expect it to happen this close to the end of 0.17 release cycle.

[urben1680]

Having extra hooks for these situations sounds like a good idea. There might be other complex hooks out there that are not relationships that make this approach here not work as intended.

[eugineerd]

Alright, I think this should fix some of the current problems with the implementation of relationship cloning/moving using EntityCloner.

• Fixed data not cloning on RelationshipTarget.
• Fixed Relationship component disappearing when moving components.

Handling all the edge cases resulted in some very messy code, but at least it should behave correctly now. I'd really like to clean it up, but since I don't have a good estimate on how long that will take, I believe fixing known bugs outweighs it for now.

[urben1680]

The tests with components containing vectors made me think that maybe, in the follow-up PR, we want to prefer bitwise moves over clone moves. It would also be more like what would happen if you manually take and insert the components.
If we clone the potentially large allocation in the vector we might needlessly waste performance.

[alice-i-cecile]

Excellent work: I'm really glad this is possible, and I appreciate the cleanup in terms of tests and docs that you've done while here.