EntityCloner allow/deny by BundleId and allow_if_new

[urben1680]

Objective

I use the EntityCloner for my reversible crate and cache operations by storing the relevant BundleId. If I want to generate an EntityCloner from that I need to fetch the component ids first. (1)
Also, to only clone components the target does not know yet requires me to create a new bundle from ids and the target archetype. (2)

Solution

1. It is more convenient to give the BundleId to the cloner directly so I added methods for that.
2. After Fix EntityCloner replacing required components. #19326 EntityCloner becomes aware of the target archetype and with allow_if_new methods can itself determine which components to clone.

I renamed the internal field filter_required to filter_if_new because functionally this describes the behavior of handling required components as well.

It made no sense to me to also add deny_if_new. Who would want a component to not be cloned if it the target does not contain it?

Testing

Added a new test

Showcase

EntityCloner::build(&mut world)
    .deny_all()
    .allow_if_new::<A>() // do not clone if target already contains A
    .allow_if_new::<(B, C)>() // if target only contains B, C will be cloned
    .allow_by_bundle_id(bundle_id) // behaves like allow::<(B, C)> if this id belongs to this bundle type
    ...

[urben1680]

Would appreciate a review from you @eugineerd :)

[urben1680]

I am wondering though, when id is already contained in the target, should that also skip required_id of this id even if the target does not contain these? Then this solution here is not enough.

[eugineerd]

Yes, this should probably skip required component ids in that case. I believe that's how insert_if_new works at least. I'm not really sure how to add this behavior correctly without introducing more performance regressions (yet another set to check in if implemented naively). Maybe it would make sense to do this using a sorted vec set that would store (ComponentId, FilterType) or something similar, but I don't know if that will actually be more performant.

[urben1680]

I have a few ideas, for now this PR here is blocked on the fix I decided to do extra so it be shipped in a point release.

[urben1680]

Hm seems like this issue also arises for #19635 when considering the source archetype.

When I deny_all and allow::<A> where A requires B, then B gets cloned even when the source entity lacks A.

[urben1680]

I think #19326 did not do the correct implementation for the deny filter.

For example see this method:

fn filter_deny(&mut self, id: ComponentId) {
    if self.entity_cloner.filter_allows_components {
        self.entity_cloner.filter.remove(&id);
    } else {
        self.entity_cloner.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.entity_cloner.filter_allows_components {
                    self.entity_cloner.filter_required.remove(&required_id);
                } else {
                    self.entity_cloner.filter_required.insert(required_id);
                }
            }
        }
    }
}

The context is an EntityCloner that otherwise allows all components do be cloned, this is the default behavior if deny_all is not called.

Now let's assume the source entity has the components A and B which both require C.
Now we call deny::<A>. That will cause C to be removed from filter_required. So both B and C will not be cloned. But with A still being cloned, it requires C! The ecs will still generate a C with a default value, but not the clone of the source.

This test therefore fails:

#[test]
fn deny_with_non_unique_required() {
    #[derive(Component, Clone, PartialEq, Debug, Default)]
    #[require(C(5))]
    struct A;

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

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


    let mut world = World::default();

    let e = world.spawn((A, B, C(10))).id();
    let e_clone = world.spawn_empty().id();

    EntityCloner::build(&mut world)
        .deny::<A>()
        .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(10))); // fails, is C(5)
}

I will fix this issue in this PR too.

[urben1680]

Blocked by #19635