Removing entity crashes program

[nickyvanurk]

I am working on my projectile system. I am destroying projectiles after a timeout and on colliding with another object. I keep getting error messages and I suspect the flaw is within ecsy itself. Currently my code looks like this:

WeaponSystem.ts

 const projectile = this.world.createEntity()
          .addComponent(Object3d, {value: this.bulletMesh.clone()})
          .addComponent(Transform, {position, rotation})
          .addComponent(Physics, {velocity})
          .addComponent(SphereCollider, {isTrigger: true, radius: 0.1})
          .addComponent(DestroyOnCollision)
          .addComponent(Timeout, {
            timer: 500,
            addComponents: [Destroy]
          });

DestroySystem.ts

export class DestroySystem extends System {
  static queries: any = {
    entities: {
      components: [Destroy],
      listen: {
        added: true
      }
    },
    colliding: {
      components: [CollisionStart, DestroyOnCollision],
      listen: {
        added: true
      }
    }
  };

  execute() {
    this.queries.entities.added.forEach((entity: Entity) => {
      entity.remove();
    });

    this.queries.colliding.added.forEach((entity: Entity) => {
      entity.remove();
    });
  }
}

When I try this order of registering my systems:

this.world
      .registerSystem(DestroySystem)
      .registerSystem(CameraSystem)
      .registerSystem(RaycasterSystem)
      .registerSystem(Input)
      .registerSystem(PlayerInput)
      .registerSystem(TimeoutSystem)
      .registerSystem(WeaponSystem)
      .registerSystem(PhysicsSystem)
      .registerSystem(TransformSystem)
      .registerSystem(WebGlRendererSystem);

I get the following error:

Uncaught TypeError: Cannot read property 'value' of undefined
    at TransformSystem.updateTransform (app.bundle.js:69191)
    at app.bundle.js:69186
    at Array.forEach (<anonymous>)
    at TransformSystem.execute (app.bundle.js:69185)
    at SystemManager.executeSystem (app.bundle.js:375)
    at app.bundle.js:390
    at Array.forEach (<anonymous>)
    at SystemManager.execute (app.bundle.js:388)
    at World.execute (app.bundle.js:1441)
    at Game.run (app.bundle.js:68374)

Using the following queries in TransformSystem.ts

    this.queries.transforms.added.forEach((transformEntity: Entity) => {
        this.updateTransform(transformEntity);
    });

    this.queries.transforms.changed.forEach((transformEntity: Entity) => {
        this.updateTransform(transformEntity);
    });

When I put the DestroySystem all the way at the top like so:

this.world
      .registerSystem(DestroySystem)
      .registerSystem(CameraSystem)
      .registerSystem(RaycasterSystem)
      .registerSystem(Input)
      .registerSystem(PlayerInput)
      .registerSystem(TimeoutSystem)
      .registerSystem(WeaponSystem)
      .registerSystem(PhysicsSystem)
      .registerSystem(TransformSystem)
      .registerSystem(WebGlRendererSystem);

It appears to work (other than the rendering system rendering an already destroyed object) but after a couple of seconds of shooting I get:

Uncaught Error: Tried to remove entity not in list
    at EntityManager.removeEntity (app.bundle.js:1104)
    at Entity.remove (app.bundle.js:760)
    at app.bundle.js:68592
    at Array.forEach (<anonymous>)
    at DestroySystem.execute (app.bundle.js:68591)
    at SystemManager.executeSystem (app.bundle.js:375)
    at app.bundle.js:390
    at Array.forEach (<anonymous>)
    at SystemManager.execute (app.bundle.js:388)
    at World.execute (app.bundle.js:1441)

I hope this is enough information to discover the flaw, my source can be found here: https://github.com/nickyvanurk/3d-multiplayer-browser-shooter/tree/master/client/src

[fernandojsg]

With the order you have initially DestroySystem > TransformSystem if you remove the component on DestroySystem the entity will get removed and all its components too. As we are using by default deferred removal method, it will get disposed at the end of the frame. But within the frame you could still access the removed components by calling: entity.getRemovedComponent(Object3D), or if you don't know if the component could be removed or not, you could do entity.getComponent(Object3D, true), note the true as the second parameter, indicating that it should look in the "alive" components as well as in the "dead (soon to be removed)" components, which I believe is your case in here https://github.com/nickyvanurk/3d-multiplayer-browser-shooter/blob/master/client/src/systems/transform-system.ts#L29

[nickyvanurk]

But the TransformSystem should not operate on removed entities/components because I didn't set the true flag. This is my intention, once I flag an entity for removal it should not be operated on on the following systems but as you can see in my first error message, it does it anyway. It seems the components get removed but the entity is still alive. Thus resulting in undefined components.

[fernandojsg]

So in tha case you should change your queries to skip the entities that has the Destroy component on them, otherwise the query still matches and that's why you are getting them on these lists

[nickyvanurk]

I am lost. Why do the queries match after DestroySystem? They should not match because the components were removed. They should match the removed query but I am not using that query in the systems after DestroySystem.

EDIT: I think I get it, the actual entity doesn't get removed untill the end of the frame thus resulting in the error. I thought the actual entity also would not show up in the systems following DestroySystem. But how does it work because when I do entity.remove() ALL components get removed even Destroy so there is no way for me to use Not(Destroy)

EDIT "If you remove the component on DestroySystem the entity will get removed and all its components too" reading this again it doesn't seem to make sense, if an entity is flagged as removed why would it still show up in the systems following DestroySystem?

[fernandojsg]

They will show up on removed event queries, as the components were removed so the query event matches in case you want to free resources or something. But they won't show up on the standard query results. So this.queries.myQuery.removed will get populated after DestroySystem is being executed with any component that matches it from the removed entity, while the this.queries.myQuery.results will not. Can you setup a demo on your code so I can easily reproduce it and see where the problem could be happening?

[fernandojsg]

(*) Just to clarify the comment you mention. When I say will get removed and its components too I meant will get marked to be removed at the end of the frame as we are doing deferred removal both for entities and components

[nickyvanurk]

Yeah right, but I am not using removed in TransformSystem, only the added and changed queries. Even when I enable the getComponent(..., true) flag (behaviour I don't want) I am still getting the last error. Something is trying to delete an already deleted object. I think it's related to this added changed queries holding on to removed entities. You can add me on discord so I can show you the problem if you want: Mr. Sockpuppet#9355

EDIT: Here you can see the problem most likely. The changed query hold on to entities that are not alive (possibility the added too).

[fernandojsg]

I just realized one issue, if you are removing entities or components within the list you are traversing, you are mutating it and you can't just use forEach. (re https://github.com/MozillaReality/ecsy/issues/163) eg:

query = [1,2,3,4];
queryforEach(i => query.splice(query.indexOf(i), 1));
// query = [2,3]

you should traverse it in the inverse order:

for (i = results.length - 1; i > 0; i--) {
   results[i].remove()
}

I can't join on discord right now, but I'll prepare a logger that shows the steps you are going into so you can paste them here, and probably tomorrow we could talk if so.

[nickyvanurk]

Still getting the errors even with this loop :(

EDIT: I've located the error, I'm sure the changed and added queries still hold on to the entity.alive = false entities.

I don't see the error when I do a quick fix like so:

    for (let i = this.queries.colliding.added.length - 1; i >= 0; --i) {
      this.queries.colliding.added[i].remove();
    }

    for (let i = this.queries.entities.added.length - 1; i >= 0; --i) {
      // @ts-ignore
      if (this.queries.entities.added[i].alive)
        this.queries.entities.added[i].remove();
    }

So for now I use that if statement before removing + placing DestroySystem all the way at the top as a quick fix until the problem is fixed.

It seems it's the same error as #138

[fernandojsg]

Tagging this as bug and I'll keep track of it. There is a concept of Disabled/Detached entities that could help coming to an agreement with this issue, will be discussed https://github.com/MozillaReality/ecsy/issues/180.

[nickyvanurk]

Awesome. I expected it to be a 'simple' fix tho, filtering the query entities for their alive state before returning it but it seems it's more complex than that? Do you have an estimation for when this bug would be resolved?

[fernandojsg]

The problem is that there is not really a perfect answer to what to expect when you add and remove an entity within the same frame. Like in your case you would expect it to not appear on added but it could be other situations where you want to detect that that event had happened. for example having something like a key down/up within the same frame and you want to detect a clicked tagComponent, that would probably be ok to keep it in added. Not sure about timing as we are going through many new updates in here, mainly I want to get the benchmarks #177 so we can measure how it could affect all this type of sensitive changes. But this issue is something we want to address as soon as possible so it will be top priority for us.

[nickyvanurk]

I understand. It would, to me, be most intuitive if the entity after explicitly typing entity.remove() would not exists on the added/changed queries in the systems that come after it but of course it's available for the systems that preceded it. I also think for most users this is the expected behavior. Currently I have to type if (entity.alive) ... in those added/changed queries which is something I shouldn't have to do because those queries are based on components and therefore there should be an entity (that's alive presumably) attached to is. My suggestion is to make entity.remove() work as expected and if you need the entities to still show up in the added/changed/removed queries the ecsy user could simply add a TagComponent like Destroy to the entity and handle the actual destruction later down the line in the DestroySystem (that's how I do it within my project, or at least wanted to). That way entities with components are always expected to be alive.

Appreciate all the work and communication!