[proposal] Dispatch events upon adding and removing components

[player-03]

Events are an important part of any game, and it's only natural to want them for ECS programming. There are plenty of situations where you'll want to know about new components being added and old ones being removed. foreachEntity is good, but can't do everything.

My suggestion is to add two new functions to EntityGroup: onEntityMatched (calls a function when an entity begins to match an archetype) and onEntityUnmatched (calls a function when an entity stops matching an archetype). Here's an example of how they might look in action:

entities.onEntityMatched( function (entity:{ display:DisplayObject }) {
    stage.addChild(entity.display);
} );
entities.onEntityUnmatched( function (entity:{ display:DisplayObject }) {
    stage.removeChild(entity.display);
} );

var entity = {};
entities.createEntity(entity);
entity.display = Images.get("BeachBall.png"); //The image is automatically added to the stage.
entity.display = null; //The image is automatically removed from the stage.

Note that both systems need to know the value of display. Thus onEntityMatched has to run after setting the new value, but onEntityUnmatched has to run before.

[dpomier]

I think we need something like this. We could later add some sort of priority level or whatnot.

I wonder if assigning components in onEntityUnmatched should affect the entity, because this would complicate things.

[player-03]

I wonder if assigning components in onEntityUnmatched should affect the entity, because this would complicate things.

Can confirm. I've attempted to do the same in Echoes, and it causes one specific error due to order of operations. Before getting to that error, I'm going to try making some examples.

//Test case 1
entities.onEntityUnmatched( function (entity:{ component:Int }) {
    trace(entity.component); //should not be null yet
    entity.component = null;
} );

For a very simple case, consider this potential infinite loop. Because component still has to exist during this function, setting it to null will remove it and call this function again... at least if Esco isn't careful.

In Echoes, this isn't a problem. Each archetype stores a list of matched entities plus a set of match/unmatch listeners. And they have a removeIfMatched() function. This function removes an entity from the list and calls the unmatch listener, in that order. If the unmatch listener removes the component again, then removeIfMatched() will be called again but won't do anything. After all the removeIfMatched() functions return, the component is finally removed from the entity (because components are stored elsewhere). So that's one solution.

With all that said, this example is pretty unrealistic. Here's one that might realistically come up:

//Test case 2
entities.onEntityUnmatched( function (entity:{ yin:Yin, yang:Yang }) {
    entity.yin = null;
    entity.yang = null;
} );

Say you have two components that must exist as a pair, so whenever one is removed, you'd want to remove the other. Without a solution to the infinite loop problem, both yin = null and yang = null will trigger such a loop. (Though of course it'll never get to the latter.)

If using Echoes's solution, this setup would end up calling removeIfMatched() three times, but the second and third times would have no effect. A small waste of processing time, but no big deal.

Another possibility is to put off processing yin = null and yang = null. Add them to a list of pending operations, and once all listeners return, then remove the components. One of them will already be gone (because that's what triggered the unmatch listener in the first place), so it'd only have to remove the other.

Hmm. What about this?

//Test case 3
entities.onEntityUnmatched( function (entity:{ yin:Yin, ?yang:Yang }) {
    entity.yang = null;
} );
entities.onEntityUnmatched( function (entity:{ ?yin:Yin, yang:Yang }) {
    entity.yin = null;
} );

I was thinking that maybe adding restrictions could help. I thought the rule could be "an unmatch listener can't remove any of the required components, but can still remove optional ones." But no, these two listeners will trigger each other.

Both solutions I mentioned still work, but I don't think this restriction opens up anything new.

---

Now the other side of the coin. What if we add new components?

//Test case 4
entities.onEntityUnmatched( function (entity:{ caterpillar:Caterpillar, ?butterfly:Butterfly }) {
    if (entity.butterfly == null)
        entity.butterfly = new Butterfly( entity.caterpillar );
} );
entities.onEntityMatched( function (entity:{ ?caterpillar:Caterpillar, butterfly:Butterfly }) {
    trace(entity.caterpillar);
    entity.caterpillar = null;
} );

//Test case 4.1
var green = {};
entities.createEntity( green );
green.caterpillar = new Caterpillar( 0x00FF00 );
green.butterfly = new Butterfly( green.caterpillar );

//Test case 4.2
var yellow = {};
entities.createEntity( yellow );
yellow.caterpillar = new Caterpillar( 0xFFFF00 );
yellow.caterpillar = null;

So here we have a goal of replacing one component with another. We can either do this by adding the new component and having it automatically remove the old, or removing the old and having it automatically add the new. (I've found use cases for both.)

In case 4.1, we start by adding the butterfly component. The match listener reacts by removing caterpillar, which triggers the unmatch listener, which does nothing. No issues here.

In case 4.2, we start by removing the caterpillar component. The unmatch listener reacts by creating a butterfly component. This triggers the match listener, which prints the value of caterpillar before setting caterpillar = null again. This triggers the unmatch listener again, but this time it does nothing. (Note that without the if statement, it would overwrite butterfly unnecessarily, but this presumably wouldn't trigger any events.)

No infinite loops here, just slightly unexpected behavior. This all stems from the order of operations I mentioned in my first post: match listeners must be called after adding the component, while unmatch listeners must be called before removing it. When a match listener is called by an unmatch listener, the old component still hasn't been removed.

And this brings us to the case where Echoes fails.

//Test case 5
entities.onEntityUnmatched( function (entity:{ caterpillar:Caterpillar, ?butterfly:Butterfly }) {
    if (entity.butterfly == null)
        entity.butterfly = new Butterfly( entity.caterpillar );
} );

var timer = new haxe.Timer( 1000 );
timer.run = function():Void {
    entities.foreachEntity( function (entity:{ caterpillar:Caterpillar, butterfly:Butterfly }) {
        trace( 'Warning: forgot to clean up caterpillar ${entity.caterpillar.id}' );
        entity.caterpillar = null;
    } );
};

If I converted this code to run in Echoes, it would crash when attempting to retrieve caterpillar.id. This is the result of the details I mentioned earlier: archetypes store a list of matched entities, and this list is updated before calling any unmatch listeners. In this case:

1. The entity is initialized with a caterpillar component (not shown).
2. It gets added to the first archetype (caterpillar, ?butterfly) but not the second (caterpillar, butterfly).
3. entity.caterpillar is set to null.
4. Archetype 1 removes the entity from its list, then calls the listener.
5. The listener adds a butterfly component.
6. Echoes searches for archetypes that newly match. It searches only archetypes that require a butterfly component. (It used to search every archetype, but I've submitted a pull request to fix that.)
7. The entity currently has both a caterpillar and butterfly component, so Echoes adds it to archetype 2.
8. The unmatch listener returns, and caterpillar is set to null.
9. Now the entity is in archetype 2's list despite lacking a caterpillar component. entity.caterpillar.id crashes.

After over a year of use, this has been the only edge case I've found. Realistically, the conditions are unlikely to come to pass. You have to add one component in response to another's removal, implying the two don't coexist, and then call foreachEntity() taking both as arguments. I spent a lot of time trying to figure out a more practical example, and eventually gave up. So... Echoes's solution isn't perfect, but it's close.

I think the alternative I proposed (where you postpone component assignments until after all listeners return) covers every case. The only problem is implementing it.

[dpomier]

I think the difficulty with postponing component assignments is we want this code to work as one would expect:

entities.onMatchedEntity(function(entity:{ b:Int }) {
    entity.b *= 2;
});
entities.onMatchedEntity(function(entity:{ a:Int, ?b:Int }) {
    if (entity.b == null)
        entity.b = 5;
    trace(entity.b); // Should be 10
});
entities.createEntity({ a: 0 });

It should trace 10, but if we postpone b's assignment it would trace 5 instead, which seems wrong.

I gave some thought to this based on your feedback coming from Echos: If onEntityUnmatched is invoked after a given component is removed from an entity, it doesn't make sense to have this component in any following onEntityMatched when we add additional components. The particularity we seek for, though, is that onEntityUnmatched keeps the access to the removed component, even if internally ecso considers it is not owned by the entity anymore.

Let's run this through the different cases:

//Test case 1
entities.onEntityUnmatched( function (entity:{ component:Int }) {
    trace(entity.component); //should not be null yet
    entity.component = null;
} );

Setting component to null removes it from the entity, which triggers onEntityUnmatched with the particularity to have access to the removed component. Setting component to null in the callback will not remove the component from the entity because it has already been removed: therefore "onEntityUnmatched" is not triggered a second time.

//Test case 2
entities.onEntityUnmatched( function (entity:{ yin:Yin, yang:Yang }) {
    entity.yin = null;
    entity.yang = null;
} );

Same as above, both yin and yang are already removed from the entity when onEntityUnmatched runs, setting them to null doesn't trigger any callback.

//Test case 3
entities.onEntityUnmatched( function (entity:{ yin:Yin, ?yang:Yang }) {
    entity.yang = null;
} );
entities.onEntityUnmatched( function (entity:{ ?yin:Yin, yang:Yang }) {
    entity.yin = null;
} );

This one gets more interesting. Considering an entity with both components, setting yin to null first will trigger the first callback with yin's value and yang: when the latter is set to null in the first callback, the second callback is called but only with yang because yin is already "unmatched" by the entity.

//Test case 4
entities.onEntityUnmatched( function (entity:{ caterpillar:Caterpillar, ?butterfly:Butterfly }) {
    if (entity.butterfly == null)
        entity.butterfly = new Butterfly( entity.caterpillar );
} );
entities.onEntityMatched( function (entity:{ ?caterpillar:Caterpillar, butterfly:Butterfly }) {
    trace(entity.caterpillar);
    entity.caterpillar = null;
} );

Test case 4.1:

Once caterpillar is set, setting butterfly triggers onEntityMatched which set caterpillar to null, resulting in calling onEntityUnmatched with both components (caterpillar being now "unmatched"), and butterfly.

Test case 4.2:

Once caterpillar is set, setting it to null triggers onEntityUnmatched with caterpillar only, resulting in setting butterfly to the entity which now becomes internally { butterfly:Butterfly }, and then triggers onEntityMatched without caterpillar.

//Test case 5
entities.onEntityUnmatched( function (entity:{ caterpillar:Caterpillar, ?butterfly:Butterfly }) {
    if (entity.butterfly == null)
        entity.butterfly = new Butterfly( entity.caterpillar );
} );

var timer = new haxe.Timer( 1000 );
timer.run = function():Void {
    entities.foreachEntity( function (entity:{ caterpillar:Caterpillar, butterfly:Butterfly }) {
        trace( 'Warning: forgot to clean up caterpillar ${entity.caterpillar.id}' );
        entity.caterpillar = null;
    } );
};

Considering an entity { caterpillar:Caterpillar }: setting caterpillar to null triggers onEntityUnmatched (with caterpillar as "unmatched") which set butterfly. The interval function never matches the entity because caterpillar is considered "unmatched" before butterfly is added, therefore internally it never "has" both components at the same time, even though onEntityUnmatched has access to both.

Some more cases:

entities.onEntityMatched(function(entity:{ a:Int }) {
    trace("Matched " + entity.a);
});
entities.onEntityUnmatched(function(entity:{ a:Int }) {
    trace("Unmatched " + entity.a);
});
entities.createEntity({ a: 33 }); // traces "Matched 33"
entities.foreachEntity(function(entity:{ a:Int }) {
    entity.a = 99; // traces "Unmatched 33" then "Matched 99"
});

Re-assigning a component should trigger onEntityUnmatched before onEntityMatched.

---

This approach seems more technical to implement but I think it also feels more natural.

An issue I see with it, however, is when we want to invoke onEntityUnmatched on 2 or more components:

entities.onEntityUnmatched(function(entity:{ a:Int, b:Int }) {

});
entities.foreachEntity(function(entity:{ a:Int, b:Int }) {
    entity.a = null;
    entity.b = null;
});

1. At entity.a = null;, the entity becomes { b:Int }, onEntityUnmatched is triggered with both a which is not own by the entity anymore, and b, as described above.
2. Then at entity.b = null;, the entity becomes { }, onEntityUnmatched is not triggered.

The callback onEntityUnmatched will receive a mixture of "removed" and "still owned" components. Now is this really an issue? Postponing component assignments might allow onEntityUnmatched to be called once when both components have been removed, but I think this defies the concept of (un)matching.

[player-03]

If you think you can pull this off efficiently, it seems like it should work well.

My main concern is, it sounds like it would involve constructing a new anonymous structure, to pass to the listener. If you're already doing that, no big deal, but if not, make sure it performs well.

Postponing component assignments might allow onEntityUnmatched to be called once when both components have been removed, but I think this defies the concept of (un)matching.

Ok, I need to clarify a bit. You only need to postpone operations that happen within an unmatch listener*, and only until the listener returns. When you set entity.a = null, it'd call any listeners, then set a = null, then process any assignments that happened during the listener, and then return.

You'd probably track this using a static variable somewhere. unmatchListenerCount or something. Increment it just before calling the listener, decrement it afterwards. If it's 0, mutations get processed immediately. If it's > 0 mutations get added to a list, to be processed once it hits 0.

And I guess that leaves two problems. If the listener throws an error, the count might not decrement, and then the whole library breaks. Sometimes it's possible to keep going after an error, but not anymore. Second, if you're inside a listener, there's still the problem that you can't access variables you just set.

---

*It's specifically unmatch listeners because of the whole "removed component must still be accessible" thing. Since you put off removing the component, then if any other mutations happen in the meantime, they're out of order, and you get something akin to a race condition.

Since your solution updates the underlying representation before calling the listener, everything stays in order and there shouldn't be any issues. At least, not any that resemble a race condition. I say go for it.