Removing a component changes entity reference within a System update method

[ghost]

Within an AEntitySystem update method, if a component is removed from the current entity reference, such that the entity no longer matches the system's EntitySet filter, then the entity reference will actually jump to a different entity matching the EntitySet filter. In other words, the Update method is now suddenly operating on a different entity. This would obviously lead to unexpected behavior within the rest of the update method.

Given this behavior, this really should be documented somewhere.

[ghost]

On this topic, it seems that basically this is because removing components can update the underlying array in the ReadOnlySpan of entities while it is being iterated through. That is very problematic behavior and should be clearly documented and warned against if that is a behavior of the system that can only be prevented by not using it in that manner.

[Doraku]

ah I thought I had written something in the readme but it is just for multi-threading, not the simpler case >_<
If you need to do operations on entities in an EntitySet which may change the composition of said EntitySet, you can either:

• copy entities before doing your modification

// if you are using an AEntitySystem<>
protected override void Update(float elaspedTime, ReadOnlySpan<Entity> entities)
{
    Span<Entity> copy = stackalloc Entity[entities.Length]; // be carefull if you can actually allocate on the stack
    entities.CopyTo(copy);
    base.Update(elaspedTime, copy);
}

• use an EntityCommandRecorder to delay your changes

I will add some info in the readme, thanks for bringing it up, sometimes I forget that obvious things are only obvious because I am the one who made it...

Special case where it will accidentally work:

// let's say the observed EntitySet want a Dummy component
protected override void Update(float elaspedTime, in Entity entity)
{
    entity.Remove<Dummy>();
    // since the ReadOnlySpan<Entity> currently enumerated can't change sized, what will happen here as you said is that the last Entity of the EntitySet will take the place of the current entity, but your current entity parameter will not change.
    // Also the ReadOnlySpan will not change and will still be fully enumerated even though the EntitySet is being cleared, but it is probably not a good idea to use this behavior
}

[ghost]

Might I suggest changing the API to deal with this as part of your library?

Removing a component seems a very typical usage pattern, and mutating the Span under enumeration as a result is definitely problematic behavior. Relying on the user to understand this and use workarounds like copying the array (slow for performance intensive cases) or using Recorder API (a bit unwieldy) is not ideal - we want the typical expected usage to behave as expected (the "Pit of Success").

One way to solve this is to write a wrapper struct around Entity which you enumerate from EntitySet. This struct would record mutation changes that would otherwise change your underlying array. Let's say it's called EntityMutator for the purposes of this argument. Then you could write a custom version of ReadOnlySpan which your EntitySet would return. This Span would be simple, it would only differ from ReadOnlySpan in returning an enumerator that would wrap each Entity in an EntityMutator as it is returned in the Current method, then apply those changes after enumeration is finished or by explicitly calling a method.

This would make the default usage the correct one, with minimal performance overhead (essentially just wrapping an entity in another struct), and require only one iteration through the array still.

[Doraku]

What you are desbribing as EntityMutator is basically the EntityCommandRecorder and EntityRecord. The main focus of current implementation of EntitySet and AEntitySystem was to have a default iteration as fast as possible with no garbage generated for what I thought was the main usage: updating component values without changing the composition of entities.
Making composition changes to entity require some extra setup because garding against such action is not free (you need memory to record those modification, or to copy your entities) and the user need to make those choices. Always putting those rails (for what I consider default usage maybe wrongly so) would impact negatively the program in my opinion.
The way I see it it is a little like IEnumerable in general in dotnet, if you modify a collection while enumerating it, you will get a CollectionChangedexception. Here you are doing modification on entity which will make them disapear of the EntitySet you are enumerating, so while it is not throwing an exception (and maybe this is what is really missing), things going wrong would not be that surprising. I still want to make a roslyn analyser to detect such bad usage and warn users to help prevent those undesirable effects but sadly I had not much time to invest into it.

I really get your "pit of success", I am starting to think of "free" way (memory wise) to freeze an EntitySet while operating on it (maybe disable its subscriptions so the content won't change, but then either you would have to record all messages to replay them after the processing so not memory free, or recheck every entities individually if it still need to stay there which would be expensive), or maybe some extension methods to help the setup and usage of EntityRecord, this probably won't satisfy you as an answer and I hope I (or someone else) will come up with something better but I really don't want to make those cases the default one for now.

[ghost]

I understand that you want to keep the default case very fast and efficient. And I think you've done that very well. It's the best ECS in C# that I saw as far as efficiency, memory locality, and performance (I profiled it too). That's why I'm using it for a game :)

To your point about it being like IEnumerable, I completely agree and that is exactly my point. To modify an IEnumerable's underlying collection while iterating is a blatant user error. But the interface doesn't easily let you do so. To to be able to do so, you would have to go outside the interface. Also, it's the collection itself that would have to be modified, but modifying the state of the items is safe. Here in the ECS, modifying the state of any item through the ECS is unstable and leads to the collection being modified while iterating, which I think is very unintuitive behavior and easy for a user to cause, leading to a difficult debugging exercise (without an exception).

I do think that there are a range of types of logic needed for a game, and the system can robustly acomodate more than one. For example, here are two use cases. 1. Go through a large set of entities with a position and a velocity, and update the position based on velocity. This is a clear use case for your ECS system and fits well with the type of logic you have in mind. It is safe and easy to write with your system. 2. A status effect is added/removed on a target unit from a source depending on some skill checks, defense checks, etc. This type of code is really different, and does not take advantage of or need the memory locality or non-allocation that your code provides. What is needed is clarity and expressiveness of code. I contend that in the course of writing a typical game, there is far more code of the second type written than the first. So I do think it is important to consider supporting the second type of logic.

I think your system already has the solution at hand. You already have the EntityCommandRecorder and recommend using it for these situations. I think you can just build your system to help the user and make that usage easy. For example, you can have two types of virtual methods for Entity Systems. Keep the one you have for the first type of code - high N cache-sensitive "functional" style code. Provide a second type of virtual method, passing in an EntityMutator to the user code. You can create an EntityCommandRecorder before iterating and calling user code, and the EntityMutator can just be a simple wrapper that forwards Set/Remove calls to the EntityCommandRecorder, and then after the loop you can execute the commands. The user can operate on the entity with pretty similar logic and the stateful mutations are handled. That's pretty simple; it uses your existing code systems, and helps the user follow the pit of success.

[Doraku]

I tried to make the api open so it should be possible to define your own base system types but you are right, those use cases are common enough that there should probably be already offered here.
It will probably be its own type AEntityDeferredSystem since the use of a EntityCommandRecorder is completely different from the current base usage.
I am also thinking of a AEntityBufferedSystem to handle the copy of the EntitySet so you can do modification on its entities (single threaded obviously) even if it removes them from the set, or maybe make it a constructor parameter of the existing AEntitySystem.

[Doraku]

3009d77 added AEntityBufferedSystem
still something bothering me with AEntityDeferredSystem

[ghost]

Another way to look at it is that the ECS approach is treating game state akin to a database. The data is laid out contiguously and efficiently, and a query is constructed with an EntitySet to iterate over the contents.

It would be very problematic in a DB if a query that modified items would mutate the selection, causing references to jump, skipping over elements, etc. A DB query therefore pulls an immutable selection. What if this code followed that model and didn't try to propagate component changes immediately? It doesn't need to be updated so reactively perhaps.

Maybe instead of recording the actions and replaying them, the part of the system that is mutating the collection should be reconsidered.

[Doraku]

It would be very problematic in a DB if a query that modified items would mutate the selection, causing references to jump, skipping over elements, etc. A DB query therefore pulls an immutable selection. What if this code followed that model and didn't try to propagate component changes immediately?

To return something immutable, it means you need to copy something somewhere, which you probably don't need nor want (for performance) in every cases. I believe the AEntityBufferedSystem fills this need.

Maybe instead of recording the actions and replaying them, the part of the system that is mutating the collection should be reconsidered.

I think there is only two ways to handle mutations, either synchronously like it is done for now (which is not thread safe), or by queuing the changes to process them later (which require something to store the changes, so it would cost memory and might stress the gc, and a method to "commit" them safely). Honestly I choose the synchronous way because I am lazy, and didn't want to be responsible for gc garbage generation and incurs a not needed cost in all cases.

Currently this is what I am leaning to do:

• introduce a ReadOnlyEntity type with as the name imply only read methods. The catch is that while the Get<T> is a "read" method you can still modify the component returned because it is only for the concerned ReadOnlyEntity (except if you share a component with SetSameAs but I think it would be ok to expect the user to know how he sets up his components).
• EntitySet would now give you those ReadOnlyEntity thus only allowing you thread safe operations (no need to change AEntitySystem except for the type change).
• AEntityBufferedSystem would copy the ReadOnlyEntity as Entity (same representation in memory) to give back the full modification api, since it will be executed on a single thread
• EntityCommandRecorder could take a ReadOnlyEntity to return a EntityRecord
• a EntityCommandRecorder property could then be added in AEntitySystem in case you want to make component change, no need for a specific AEntityDeferredSystem (my grip with this one is that you always need to get an EntityRecord of your entities, even if you end up not doing any modification on them to hide the api, which cost room in the recorder, introducing the ReadOnlyEntity type would solve this problem)

Things that still bother me:

• while EntitySet is now safe from bad usage, nothing stop you from creating entities and setting components from a World instance in multithread
• obviously big breaking change
• what about the user who knows what he is doing? (already use the EntitySet in a single thread to set components different from the one observed by it which is perfectly safe. Now he would have to copy the EntitySet to a buffer or use a EntityCommandRecorder because the api prevents it)
• two Entity types would add some confusion

I believe there is some balance to find in there, for example maybe I will only introduce the ReadOnlyEntity type for the AEntityDeferredSystem, still thinking about this.

[Doraku]

so I finally got around working on an analyzer, the first version is available with some basic validation which I feel is much better than what I proposed previously. You would be warned from improper usage inside AEntitySystem.Update method and decide from there if you know what you are doing or if it could cause problems.
Again this is some basic check, but the analyzer can only grow from there.