Sync{Bodies|Colliders}ToPhysics systems fail to remove things from the physics world

[dylemma]

I see the big "do not use" message in the readme, but I was pointed here after asking for advice in the Amethyst Discord.

I've got a toy app that I'm trying to integrate with NCollide, where most entities will have a CollisionObjectHandle. I want to make sure that when those entities are deleted, the corresponding collision object is deleted from the CollisionWorld. That seems to be one of the things you aim to do with this project, but for NPhysics objects.

Unfortunately, it looks like the currently-implemented approach doesn't actually work.

When the entities are despawned, the sync_bodies_to_physics and sync_colliders_to_physics systems look up their respective handles from storage in order to remove them from the physics world... but by that point the handles are seemingly already deleted.

I modified the amethyst.rs example, adding a pair of systems to spawn and despawn a handful of balls: dylemma@d8160c3

The result:

Deleting entity Entity(5, Generation(1))
[ERROR][nphysics_ecs_dumb::systems::sync_bodies_to_physics] Missing body with id: 5
[ERROR][nphysics_ecs_dumb::systems::sync_bodies_to_physics] Missing body with id: 5
[ERROR][nphysics_ecs_dumb::systems::sync_colliders_to_physics] Missing collider with id: 5
Deleting entity Entity(6, Generation(1))
[ERROR][nphysics_ecs_dumb::systems::sync_bodies_to_physics] Missing body with id: 6
[ERROR][nphysics_ecs_dumb::systems::sync_bodies_to_physics] Missing body with id: 6
[ERROR][nphysics_ecs_dumb::systems::sync_colliders_to_physics] Missing collider with id: 6
Deleting entity Entity(7, Generation(1))
[ERROR][nphysics_ecs_dumb::systems::sync_bodies_to_physics] Missing body with id: 7
[ERROR][nphysics_ecs_dumb::systems::sync_bodies_to_physics] Missing body with id: 7
[ERROR][nphysics_ecs_dumb::systems::sync_colliders_to_physics] Missing collider with id: 7
...and so on

[distransient]

Hey! Just pinging to say thanks for opening the issue. I'm gonna see if I could investigate this sometime soon.

Main reason why the description warns people to not use the repository is because it currently has git hash dependencies still (and probably more bugs like this). I think it's not too far off a 0.1.0 release, however. At which point I'll probably remove that warning.

[dylemma]

I'm glad to hear back!

If it's any help, I tried two approaches to fix this in my toy app.

• Write my own Storage trait for types that are Event + Clone and have it send a clone of any value that gets removed as an event to its EventChannel. I ended up getting stuck trying to add subscribers to it in my System implementation... I think this change would have to be implemented in specs, but I could be wrong there... I still consider myself a Rust noob.
• Set up a kind of reference loop between the Entity/Storage layer and the CollisionWorld.
  • Have CollisionObjects in the CollisionWorld hold an Option<Entity> in their data field, which is initially None.
  • A Collider component holds a CollisionObjectHandle, which you get while constructing the CollisionObject.
  • Use FlaggedStorage for the Collider so we can see when new colliders are added/removed.
  • In the SyncCollisionWorld system, iterate the entity ids that were added to the collider storage. Follow the entity.id --> Entity --> Collider --> CollisionObjectHandle --> CollisionObject chain of references to get a mutable reference to the new CollisionObject. Set its Option<Entity> to Some(entity).
  • Collect removed entity ids from the collider storage into a BitSet. Iterate over the CollisionObjects in the CollisionWorld. If that CollisionObject's Option<Entity> has an id() that was in the BitSet, mark that object to be removed, and remove all of the marked objects after iterating.

The second approach seems to work, but it feels dirty and somewhat unextensible. That said, it might solve the issue here, which is that there's currently no way to get back to the Entities for a collision. Nevermind, now that I poked around more in your source I think you already solved that.

[dylemma]

I put some more time in and figured out how to make my first approach work. I made a DetailedComponentEvent<E> enum that's basically the same as ComponentEvent but where the removal includes an E value. Here's my implementation: https://github.com/dylemma/Falldown.rs/blob/master/src/storage/mod.rs

[distransient]

The custom storage part is interesting to me. This problem of synchronization between libraries that force all data into World-like instances and an ECS application is truly frustrating and error prone. It feels like every line of code has a bit of interconnected state in it that's bound to blow up somehow that's completely impossible to test for.