The added/removed entity listeners are a foot gun. Should they be removed or refactored?

[mreinstein]

Here's an example of how one might shoot themselves in the foot with this. Let's say you have a system that handles moving and colliding bullets against entities, and also needs to cleanup bullets in some way:

function bulletSystem (world) {
    const onFixedUpdate = function (dt) {
        for (const removedBullet of ECS.getEntities(world, [ 'bullet' ], 'removed')) {
            // TODO: cleanup the removed bullet here
        }

         // some other bullet logic here...
    }

    return { onFixedUpdate }
}

Let's say your main game loop looks something like this:

ECS.addSystem(world, bulletSystem)

let accumulator = 0
let lastFrameTime = performance.now()

const FIXED_STEP_MS = 120 / 1000  // run the fixed step 120 times per second

function gameLoop () {
    const newTime = performance.now()
    let frameTime = newTime - lastFrameTime
    lastFrameTime = newTime

    accumulator += frameTime

    while (context.accumulator >= FIXED_STEP_MS) {
        context.accumulator -= FIXED_STEP_MS
        ECS.fixedUpdate(context.world, FIXED_STEP_MS)
    }

    ECS.cleanup(world)
    requestAnimationFrame(gameLoop)
}

the fixed step runs at 120fps. If this game is running at 60 frames per second on a typical monitor, the fixed update steps will run twice for each call to gameLoop. When a bullet is removed from the ECS world, the fixed update step in the bullet system will run twice and see the same removed bullet entity each time. This is because the ECS.cleanup call runs at the end of each gameLoop, NOT after each fixed step.

[mreinstein]

I don't have concrete proof, but I think it might also be the case that the not filter ! may not work correctly with the added and removed listeners.

[mreinstein]

There are a few solutions that come to mind:

• just drop the feature entirely. Easy to do, but sucks having no way to query for removed/added entities.
• adjust the documentation to try to explain clearly about this pitfall (tough because it's very dependent on how one sets up their systems, and where they decide to run ECS.cleanup)
• re-work the event handlng somehow to avoid this problem. At the moment it's unclear to me how to do this. What would the pattern look like?

[mreinstein]

There is another foot gun in here too: if multiple systems are doing cleanup, the order can cause some removed entities to be missed. For example:

function systemA (world) {
    const onFixedUpdate = function () {
        for (const removedBullet of ECS.getEntities(world, [ 'bullet' ], 'removed')) {
            // TODO: cleanup the removed bullet here
    ⋮

function systemB (world) {
    const onFixedUpdate = function () {
        ECS.removeEntity(world, someBullet)
    ⋮


ECS.addSystem(world, systemA)
ECS.addSystem(world, systemB)

Two systems are registered which both listen for remove events on bullet entities. systems are run in the order they are added, so systemA will run first. It detects no bullets were removed this frame. Then system B runs, and it removes a bullet from the ECS world. After system B runs, the game loop calls ECS.cleanup(world) and system A will never see the bullet removed by system B from last frame.

This could be fixed by storing all the removed entities in a buffer, and then upon cleanup() move those buffered entities to the actual removed list, so that every system will see them next frame.

[mreinstein]

Another foot gun: added and removed events fire when an entity matches the filter, not just when the entities themselves are added/removed from the world. An example:

const a = ECS.createEntity(world)
ECS.addComponentToEntity(world, a, 'bullet')
ECS.addComponentToEntity(world, a, 'burning')

const r = ECS.getEntities(world, [ 'bullet', 'burning' ], 'added')  // works as expected, r.length === 1

This worked as you might expect. Another example, less obvious:

const a = ECS.createEntity(world)
ECS.addComponentToEntity(world, a, 'bullet')

const r = ECS.getEntities(world, [ 'bullet', 'burning' ], 'added')  // works as expected, r.length === 0

⋮
// 2 game frames later:
ECS.addComponentToEntity(world, a, 'burning')
const r = ECS.getEntities(world, [ 'bullet', 'burning' ], 'added')  // r.length === 1

This last example is confusing, because people might assume 'added' means the entity was added to the world, but the way it actually works is 'added' means it started matching the provided query filter ([bullet, burning]). So even though the entity has been in the game world for a few frames and isn't added, it gets marked as added as soon as it picks up the burning class.

The same confusion exists with removed events too.

My gut feeling is this behavior should change, to only track entities that were added/removed from the game, rather than attempting to track when they match the provided filter. I suspect most people just want to know when entities are added/removed rather than when their components match/unmatch the filter.

[kozak-codes]

Perhaps you could add system hooks for entityAdded/entityRemoved/componentAdded/componentRemoved

[mreinstein]

yeah, not a bad idea

[mreinstein]

I probably should have split this issue into several because there are several problems in here all related to the add/remove events. I think probably the most serious one is that remove and add events could get missed depending on system order.

[mreinstein]

I've split part of this out into #35

[mreinstein]

The solution I've come up with is to do ECS.cleanup(world) after each call to fixedStep:

function gameLoop () {

    ⋮

    while (accumulator >= FIXED_STEP_MS) {
        accumulator -= FIXED_STEP_MS
        ECS.fixedUpdate(world, FIXED_STEP_MS)
        ECS.cleanup(world)
    }

    ECS.update(world, frameTime)
    ECS.postUpdate(world, frameTime)

    requestAnimationFrame(gameLoop)
}

And I handle the add/remove event only within the fixedUpdate calls.

I don't know of a way around this presently. Perhaps this should go into a guidance section in the docs?