Dynamically add/remove entity from system(s) on ecs_remove/ecs_add (based on exclude mask)

[empyreanx]

See PR: #11

[vq]

I took another crack at this. I extended the unit test to use two systems in order reproduce the issue you described. I updated ecs_add() and ecs_remove() to check both the before and after versions of the component bit set when removing system associations.

https://github.com/empyreanx/pico_headers/compare/main...vq:pico_headers:ecs_exclude_fix

It's just a rough draft. The bit set is calculated twice and neither the implementation nor the unit test is very tidy.

[empyreanx]

I really appreciate your work. After meditating for a while, I thought this might work:

// Create bit mask with comp bit flipped on
ecs_bitset_t comp_bit = 0;
ecs_bitset_flip(&comp_bit, comp_id, true);

for (ecs_id_t sys_id = 0; sys_id < ecs->system_count; sys_id++)
{
    ecs_sys_t* sys = &ecs->systems[sys_id];

    // If the component is excluded by the system, remove the entity from the
    // system if it exists
    if (ecs_bitset_and(&sys->exclude_bits, &comp_bit))
    {
        if (ecs_sparse_set_remove(&sys->entity_ids, entity_id))
        {
            if (sys->remove_cb)
                sys->remove_cb(ecs, entity_id, sys->udata);
        }
    }
}

[empyreanx]

Does this look correct? The logic is a bit subtle. The code assumes that the entity in question will have the comp_id bit flipped during the course of the function call. It searches the systems that will exclude the entity (once the comp_id bit is flipped), and removes the entity from them.

[vq]

It looks correct and it passes my version of the test_exclude unit test. It fails to compile for PICO_ECS_MAX_COMPONENTS>64 but that is easy to fix.

You probably get more calls to ecs_sparse_set_remove() for entities that isn't part of the set but on the other hand you don't make the extra calls to ecs_entity_system_test() that my version does. I'm not sure what that means performance wise but your proposal looks simpler.

[empyreanx]

Both calls are pretty cheap, that is, until you call them millions of times lol! It's hard to say in advance how the compiler will optimize the code, only benchmarks will reveal that. That said, I'm inclined to use the simple solution for now and iterate from there. We can benchmark your solution once we have everything working properly again.

One other thing I've notice through the course of our work on this is that ecs_remove just removes the entity from all systems it's a member of. We encountered this before. There's probably a solution similar to the one I proposed for ecs_add (probably involving ecs_entity_system_test). I'm busy getting ready to go on vacation, so I won't have a lot of time over the next few days. Feel free to take a stab at it.

[vq]

There's no urgency here. Enjoy your vacation!

I'll try to update the unit test to cover the situation you describe where ecs_remove() removes more systems than it should and maybe take another crack at a fix.

[empyreanx]

I'm back from vacation and ready to work! Below is my fix to the second bug we were discussing. It's very similar to my solution above for the dynamic exclude bug. Please let me know what you think, and if you can find a more efficient way of doing this.

// Create bit mask with comp bit flipped on
ecs_bitset_t comp_bit = 0;
ecs_bitset_flip(&comp_bit, comp_id, true);

for (ecs_id_t sys_id = 0; sys_id < ecs->system_count; sys_id++)
{
    ecs_sys_t* sys = &ecs->systems[sys_id];

    if (ecs_entity_system_test(&sys->require_bits, &sys->exclude_bits, &comp_bit))
    {
        if (ecs_sparse_set_remove(&sys->entity_ids, entity_id))
        {
            if (sys->remove_cb)
                sys->remove_cb(ecs, entity_id, sys->udata);
        }
    }
}

This code will not compile if there are more than 64 components, as you observed in the above solution.

[TheWagi]

Hey,

What I'm understanding is that the functions to add or remove components to and from an entity remove or add the entity from and in the system matching the new component signature, which is indeed something that needs to be included in the ECS.

Could you show both entire functions (ecs_add() and ecs_remove()) with the addition you wish to make to fix the issue ? Checking vq's proposed solution above, I think it could be improved even further, but I'd prefer to see yours (empyreanx) in context before to claim anything and end up with what you wrote.

Thanks

Wagi

[empyreanx]

Hey,

Sorry, for neglecting this. I've been taking a break from programming, studying mathematics instead. I have been meaning to get to this sooner rather than later because these are bug fixes after all. I think I can devote some time to implementing these fixes and updating the tests this weekend.

Best, Empy

[empyreanx]

I pushed a fix and test case for ecs_remove. I'll try to integrate @vq 's dynamic exclusion code in the coming week.