Ralith / hecs

A handy ECS
Apache License 2.0
967 stars 82 forks source link

Add ability to take components off a `TakenEntity` #358

Closed sanbox-irl closed 9 months ago

sanbox-irl commented 9 months ago

I'd like to do the following:

let mut taken_entity = ecs.take(interacting_entity).unwrap();
let foo: Foo = taken_entity.remove::<(Foo,)>().unwrap().0;

This would be as an alternative to:

let foo: Foo = ecs.remove::<(Foo,)>(interacting_entity).unwrap().0);
ecs.despawn(interacting_entity).unwrap();
Ralith commented 9 months ago

That seems reasonable. TakenEntity would need to either be extended with extra dynamic state to track which components have been removed, or remove would need to consume self, in which case the implementation could be provided for all DynamicBundles, by immediately dropping components that aren't taken. Thoughts?

Performing type look-up for the output bundle might be a little bit quadratic in general, but removed bundles will probably tend to be small, and for TakenEntity we could probably take advantage of the archetype's pre-existing index structures.

Ralith commented 9 months ago

On second thought, a clean and cheeky solution for TakenEntity would be to basically make it a wrapper for &mut World and delegate to World::remove until you actually consume it. That might undermine your presumed goal of avoiding an archetype move, though.

adamreichold commented 9 months ago

On second thought, a clean and cheeky solution for TakenEntity would be to basically make it a wrapper for &mut World and delegate to World::remove until you actually consume it. That might undermine your presumed goal of avoiding an archetype move, though.

I agree with that for obvious reasons. ;)

I would then add that making it a wrapper does not really add anything IMHO, just offer a way to efficiently transfer entities and all their components between worlds. Whether user code then uses one-world-per-entity (as with TakenEntity) or groups them in some domain specific manner does not matter. Also there is no need to decide which part of the world API to reproduce on TakenEntity and it enables amortizing setup costs by reusing the same world in multiple transfer, e.g. in multiple simulation time steps.

Ralith commented 9 months ago

making it a wrapper does not really add anything

Yeah, fair. User might as well just call remove and despawn on World.

just offer a way to efficiently transfer entities and all their components between worlds

I don't entirely follow what this is proposing. Is it related to the feature request here?

sanbox-irl commented 9 months ago

Based on this conversation, I'm thinking it's more or less just fine to do remove and then despawn, so I'll close this issue then!