search

hmans / miniplex

A 👩‍💻 developer-friendly entity management system for 🕹 games and similarly demanding applications, based on 🛠 ECS architecture.
MIT License
827 stars 38 forks source link

Query evaluation against outdated version of entity in nested component removal requests #310

Open evstinik opened 7 months ago

evstinik commented 7 months ago

First of all, thanks for your library. I found it useful.

Now, to the problem:

Imagine that you have a following effect: When component A is removed, remove also component B. At the time we remove component B all queries are evaluated against a version of entity that doesn't have B (correct), but still has A (incorrect).

abraini-Nascent commented 7 months ago

I'm having a similar issue, it seems to be a race condition with removing and reindexing: When removing an entity from the world, some onRemoved subscribers will try to do some cleanup and run a world.removeComponent. This will reindex the removed component, adding it back to any buckets it was just removed from. Then world.remove will finish and the entity will no longer be in the world bucket. so each loop of a system, when I pull something out of a query I have to check if the entity is still in the world and if not I remove it from the bucket.

There should be a way to skip reindexing entities that are being removed from the root world bucket

Otherwise the library has been very helpful =D

hmans commented 7 months ago

I'm assuming these are two separate (even if similar sounding) issues -- would you be willing to provide a (failing) test case? I'd be happy to fix and improve these situations, but I need to better understand what's going on.

In the meantime, it sounds like a potential workaround for these issues would be to queue the follow-up work (like removing the second component) using queueMicrotask.

joelmalone commented 2 months ago

Hello! Great project, @hmans! I really enjoy the simplicity, yet still having strong type safety! 🚀

I have the same issue as reported by @abraini-Nascent.

I would guess it's the same issue as reported by @evstinik, although that one looks like it might be triggered by a world.removeComponent(), and I haven't investigated that situation.

I'll present some diffs (rather than a PR) because I'm not sure of the accuracy of the proposed solutions. The reorder change drastically changes the behaviour of things and might break downstream code / listeners, and my gut tells me the isRemoving "feels" a bit band-aid-ey 🩹

Here's a diff that adds a failing unit test demonstrating the bug when triggered by a world.remove(entity):

https://github.com/hmans/miniplex/compare/main...joelmalone:miniplex:add-failing-test

Here's a diff showing a solution that fixes it by reordering the some code in remove():

https://github.com/joelmalone/miniplex/compare/add-failing-test...joelmalone:miniplex:fix-bug-by-reordering

Here a diff showing a solution that fixes the bug by refusing to re-add a removed entity during index evaluation:

https://github.com/joelmalone/miniplex/compare/add-failing-test...joelmalone:miniplex:fix-bug-by-isRemoving