Add/Remove entities from systems when when its component bitmask changes

[waldnercharles]

Figured I'd open this in case its helpful. I shamelessly stole the tests from #11. Feel free to close if you had something else in mind, or if there's something that's better for performance.

[empyreanx]

For some reason the tests.yml workflow is crashing on the mingw32/mingw64 jobs. It is difficult to see which test is actually failing so I'm now printing the pwd in the job commands. Please commit the revised tests.yml so that we can deduce the problem.

Otherwise I have no qualms with your PR, it's a simple solution to the problem. It's possible that some optimizations can be made using some bitset magic. I will give it some thought, but I think this is a great place to start.

[empyreanx]

When I have a moment I will try the mingw32/mingw64 tests in MSYS2.

[waldnercharles]

I'm running msys2 ucrt. I'll try running mingw64 to see what's going on.

Edit: It appears to run successfully on my machine so far. Here's a screenshot of the steps I took.

cat /proc/version returns MINGW64_NT-10.0-26100 version 3.5.4-0bc1222b.x86_64 (runneradmin@fv-az521-583) (gcc version 13.3.0 (GCC) ) 2024-09-04 18:28 UTC

My gcc version is 13.3.0
ld version is 2.43.1

[waldnercharles]

Just some findings so far:
It appears "windows-latest" is running on github's windows server 2022, which includes 12.2.0 of gcc. The latest version of gcc is 14.2 as of August 2024. I switched the "runs-on" in tests.yaml to point to windows-2025 in a separate branch and tested again, but, it appears only mingw64 stopped segfaulting.

Still not entirely sure why but, figured it was interesting and might help with troubleshooting.

[empyreanx]

I tried to replicate the problem in MSYS2 without success. I'll try a few more things tomorrow. The fact that this is happening is very peculiar since the ECS tests for the other platforms run successfully, and all of the other MinGW tests run successfully across all platforms. If it comes down to it, I'll create a new project and do some brute force debugging.

[empyreanx]

One thing to consider, it could be a problem with the tests.

[waldnercharles]

As a sanity check, I'll try running main with no changes tonight, and then the tests without my new test changes.

[empyreanx]

Valgrind reveals this:

==63165== 1 errors in context 1 of 2:
==63165== Use of uninitialised value of size 8
==63165==    at 0x10B85B: ecs_sparse_set_find (pico_ecs.h:1329)
==63165==    by 0x10B8DB: ecs_sparse_set_remove (pico_ecs.h:1339)
==63165==    by 0x10AA9E: ecs_add (pico_ecs.h:927)
==63165==    by 0x10DC81: test_queue_remove_system (main.c:606)
==63165==    by 0x109418: pu_run_test (pico_unit.h:302)
==63165==    by 0x10E240: suite_ecs (main.c:732)
==63165==    by 0x1095C0: pu_run_suite (pico_unit.h:369)
==63165==    by 0x10E2B9: main (main.c:741)
==63165== 
==63165== 
==63165== 1 errors in context 2 of 2:
==63165== Conditional jump or move depends on uninitialised value(s)
==63165==    at 0x10B835: ecs_sparse_set_find (pico_ecs.h:1329)
==63165==    by 0x10B8DB: ecs_sparse_set_remove (pico_ecs.h:1339)
==63165==    by 0x10AA9E: ecs_add (pico_ecs.h:927)
==63165==    by 0x10DC81: test_queue_remove_system (main.c:606)
==63165==    by 0x109418: pu_run_test (pico_unit.h:302)
==63165==    by 0x10E240: suite_ecs (main.c:732)
==63165==    by 0x1095C0: pu_run_suite (pico_unit.h:369)
==63165==    by 0x10E2B9: main (main.c:741)

[waldnercharles]

Just looking from my phone, but, that's likely my doing. I assumed ecs_sparse_set_remove was safe to call even if the id didn't exist in the sparse set.

From a cursory look, my guess is we probably need to add a bounds check on the sparse set to make sure id is less than capacity.

[empyreanx]

ecs_sparse_set_remove calls ecs_sparse_set_find, so there is no need to do a bounds check.

I think something very subtle is going on.

[empyreanx]

Actually, perhaps I'm wrong. Maybe there is a need to do a bounds check in ecs_sparse_set_find. Anyway I'm fading fast. I'll have another look at this tomorrow.

[waldnercharles]

No worries. I added the bounds check and it seems to have completed successfully in my fork. (Still using windows-latest for good measure, not windows-2025)

Thanks for running valgrind; I didn't even think of it.

[empyreanx]

I was able to do a bit of optimization. Naturally, the changes to ecs_add and ecs_remove will incur some performance hit. The three systems benchmark saw run time increase of 22%. I've been been able to get it down to around 6% by checking if the system has any exclusions. I'll push a commit later on this evening.

[empyreanx]

I think everything is about ready to go. I would, however, appreciate any comments you have regarding the optimization. It is designed to make the common case fast (i.e. no exclusions). If you agree with the change, I'll merge the PR right away.

[waldnercharles]

Looks great! All of the negative logic was hurting my brain, but, I think it makes sense. ie. When removing a component, we only need to add an entity to a system if that system could have been excluding the component we just removed.

We might get an optimization for the exclude-case by doing something similar: maybe a bitset test on the exclude bits for the component we just removed/added? I'd need to benchmark.

[empyreanx]

It might be helpful to think of the ecs_remove case in these terms: if a system has no exclusions, then there is no need to perform any dynamic operations at all, and thus, no need to add the entity to the system.