#Problem with Order of operations of EntityCommandBuffer or QueryMask

1 messages · Page 1 of 1 (latest)

native wadi
#

Hi 👋

I am following ECS tank tutorial video from DOTS package github in Unity2022.3.22 Entities 1.2. I was trying to use EntityCommandBuffer to set the player tank and to modify each tank color. Here is the code:

public void OnUpdate(ref SystemState state)
{
    state.Enabled = false;

    var config = SystemAPI.GetSingleton<Config>();

    //Unity.Mathematics random
    var random = new Random(123);

    //Only accepts objects with this component.
    var query = SystemAPI.QueryBuilder().WithAll<URPMaterialPropertyBaseColor>().Build();

    var ecb = new EntityCommandBuffer(Allocator.Temp);
    //Create a query mask to be used later
    var queryMask = query.GetEntityQueryMask();
    //Cretes an empty array that can store Entities and has specified size
    var tanks = new NativeArray<Entity>(config.TankCount, Allocator.Temp);
    // Fill in the array with new tank entities based on the TankPrefab
    ecb.Instantiate(config.TankPrefab, tanks);

    for (int i = 0; i < tanks.Length; i++)
    {
        var tankInstance = tanks[i];
        if (i == 0)
        {
            ecb.AddComponent<PlayerTankTag>(tankInstance);
            ecb.AddComponent<PlayerTankInputComponent>(tankInstance);
        }
        float4 color = RandomColor(ref random);
        ecb.SetComponentForLinkedEntityGroup(tankInstance, queryMask, new URPMaterialPropertyBaseColor { Value = color });


    }
    ecb.Playback(state.EntityManager);
}

The problem that I am experiencing is that if I call the if (i == 0) ... block first and next call ecb.SetComponentForLinkedEntityGroup(... line the tankInstance object with index [0] doesn't seem to receive the random color (so the player object).

IF i move the if(i==0) code AFTER the ecb.SetComponentForLinkedEntityGroup( it will work as expected setting the color correctly. I don't quite understand why. Is it because the order of operations / commands of the EntityCommandBuffer ?

image.png
native wadi
#

Just to clarify when I move the** if** block after the **ecb.SetComponentForLinkedEntityGroup( **line it all works as expected:

for (int i = 0; i < tanks.Length; i++)
{
    var tankInstance = tanks[i];
    float4 color = RandomColor(ref random);
    ecb.SetComponentForLinkedEntityGroup(tankInstance, queryMask, new URPMaterialPropertyBaseColor { Value = color });

    if (i == 0)
    {
        ecb.AddComponent<PlayerTankTag>(tankInstance);
        ecb.AddComponent<PlayerTankInputComponent>(tankInstance);
    }
}
ecb.Playback(state.EntityManager);
image.png
frank spindle
#

I expect you are running afoul of this annoying situation, noted in the API docs for SetComponentForLinkedEntityGroup():

New archetypes created by structural changes earlier in this command buffer may not be correctly matched by the mask, leading to this command targeting fewer entities than expected. This command should therefore be used only with extreme caution.
Adding the PlayerTankTag and PlayerTankInputComponent would move the player's tank into a new archetype, so it would no longer match the query mask. If the tags are added afterwards, then it's still in the "main" tank archetype when the color is changed, and it receives the color change as expected.

#

Frankly, the issue here is that EntityQueryMask has some serious design flaws that weren't discovered until they were already entrenched in the public API. We're stuck with them for now, but I'd avoid their usage in new code wherever possible until we have a chance to make the necessary (breaking) changes to either fix or remove them.

frank spindle
#

...having said that, after a bit more thought, I'm pretty sure we can fix this.

astral crater
frank spindle
#

They are very good at one particular thing: efficiently telling you whether an archetype matches a query.
For a while, early on, they were pretty good at telling you whether a chunk or an entity matched a query, but with query filtering they can't do that at all. But a lot of existing code still assumes that's what they do.

#

In the context of an ECB, the gory details for why OP's code didn't work as-is are that every query stores a list of the archetypes it matches, and we update that list after every structural change. But for efficiency, ECBs don't run this "after structural changes" logic after every single structural change, since they tend to make a bunch of them in a row, and it would be wasteful. Instead, we batch them all up and process all archetype-tracking changes at the end of playback.
One place this ends up mattering is these commands that take an EntityQueryMask. If ECB playback causes new archetypes to be created, those archetypes won't be added to their matching queries until the end-of-playback archetype-tracking code runs. Query results in the meantime will still use the stale list of matching archetypes, giving you incorrect results.

#

But again, it turns out there's a super-easy fix that I'm adding right now. Sorry for the trouble 🙂

#

(the fix became possible at some point after the warning was added to the API docs, and it seems we forgot to go apply the fix here)

native wadi
native wadi
#

@frank spindle I understand the problem (and that it should probably not happen) but I am wandering if this is also not a problem with my system design? Now maybe I am to attached to Single Responsibility principle 😀 but maybe my if (i == 0) ... should be in a separate "PlayerTankSpawn" system with its own query that just runs after the tanks are there (so we don't duplicate the spawn code) ?

I'm just trying to get a hang of code architecture in Data Oriented programming. My intuition tells me that many bugs can be avoided if we avoid combining disconnected logic into a single system? Or is it not the case based on your experience? 🤔

frank spindle
#

If I were to read this system with my code-reviewer hat on, my first suggestion would actually be that there's no reason it needs to use an EntityCommandBuffer at all. For a simple loop on the main thread, EntityManager is almost always simpler and faster.

(The thing to understand about EntityCommandBuffers is that they're always more work than the corresponding EntityManager operation. EM.DoTheThing() can just immediately do the thing; ECB.DoTheThing() has to serialize a command that captures the intent to do the thing, sort the command buffer contents for determinism, read the command back out of memory and deserialize it, and then do the thing. ECBs exist because there's otherwise no way to perform structural changes from jobs, and that's generally the only place they should be used IMO.)

#

As for the player-tank-specific initialization, I have no strong opinions. If we're talking micro-optimizations, then having an if (i == 0) branch inside the loop is certainly a bit wasteful; you know it's going to be true exactly once, but every loop iteration has to pay the (insignificant) price to test the branch. Probably better to just hoist that code out of the loop as:

AddComponent<PlayerTankTag>(tanks[0]);
AddComponent<PlayerTankInputComponent>(tanks[0]);

I personally wouldn't bother splitting that out into a separate system just for those two lines, but if you reached a point where player-tank init diverged significantly from NPC-tank init, or needed to run at a difference cadence (e.g. if tanks are respawning individually at runtime), then perhaps it would make sense.

#

Bottom line: aside from the bug with LinkedEntityGroup commands (which btw wouldn't be an issue if you used EntityManager instead of ECB here, and will be fixed shortly in any case), the code works perfectly as originally written, and nothing in my last two posts would significantly improve its real-world quality from a player's perspective. I'd stop worrying about principles, leave it as-is, and move on to something else until I had a good reason to revisit the spawning code.