LayMines: fixed occasional incorrect mine position when using BeginMinefield order

[michaeldgg2]

When laying mine with PreLayDelay > 0, end activity's tick immediately. That means don't try to immediately move to next cell.

This change unifies the behavior with scenario when a mine is laid without any PreLayDelay.

[PunkPun]

Sorry, I don't understand the fix. It has nothing to do with PreLayDelay

[PunkPun]

Please also add a comment in-code explaining the fix

[michaeldgg2]

Sorry, I don't understand the fix. It has nothing to do with PreLayDelay

It does. There are two scenarios:

• PreLayDelay == 0
• PreLayDelay > 0

In the first one, the mine is laid immediately (on line 107):

if (minelayer.Info.PreLayDelay == 0)
{
    if (LayMine(self) && minelayer.Info.AfterLayingDelay > 0)
        QueueChild(new Wait(minelayer.Info.AfterLayingDelay));
}

In the second one, the mine laying is delayed by queuing Wait activity:

else
{
    layingMine = true;
    QueueChild(new Wait(minelayer.Info.PreLayDelay));
}

These both scenarios end the tick by returning false:

if (minelayer.Info.PreLayDelay == 0)
{
    //
}
else
{
    //
}

return false;

(Thus the code for picking next cell (after the main if in Tick) is not run in this tick, but in the next one.)

Now when the delay (in the second scenario) finishes, the mine is laid. In bleed version, the cause of the bug is that the tick is not ended, when the mine is laid:

if (layingMine)
{
    layingMine = false;
    if (LayMine(self))
    {
        if (minelayer.Info.AfterLayingDelay > 0)
        {
            QueueChild(new Wait(minelayer.Info.AfterLayingDelay));
            return false;
        }
    }
}

By "laying a mine" I mean the stuff that LayMine does - i.e. it schedules FrameEndTask, when the actual mine actor is created. And here lies the problem: after the task is scheduled, the execution drops back to Tick, where it picks next cell and queues Move activity:

var nextCell = NextValidCell(self);
if (nextCell != null)
{
    QueueChild(movement.MoveTo(nextCell.Value, 0));
    return false;
}

Move immediately (i.e. within the same tick) sets minelayer's location to the next cell.

if (!mobile.Info.TurnsWhileMoving && firstFacing != mobile.Facing)
{
    path.Add(nextCell.Value.Cell);
    QueueChild(new Turn(self, firstFacing));
    mobile.TurnToMove = true;
    return false;
}

mobile.SetLocation(mobile.FromCell, mobile.FromSubCell, nextCell.Value.Cell, nextCell.Value.SubCell);

As a result the FrameEndTask, which creates mine actor, works with incorrect location of the minelayer:

self.World.AddFrameEndTask(w =>
{
    var mine = w.CreateActor(minelayer.Info.Mine, new TypeDictionary
    {
        new LocationInit(self.Location),
        new OwnerInit(self.Owner),
    });
    // ...
});

Since Move activity changes actor's location only if the facing to the next cell in path does not match actor's current facing, this bug (with mine's incorrect location) happens only if the minelayer doesn't have to turn to move to the next cell.

Another possible fix could be to store current minelayer's location (to a temporary variable that will be captured for the lambda) before scheduling FrameEndTask (and then use it to create the mine actor), but this feels like a workaround. I believe the LayMine's tick should end when a mine is laid. Even the original code before all my changes does this:

https://github.com/OpenRA/OpenRA/blob/bd2b3d97938e8507e29b0bb1a24e53c430b2a7b9/OpenRA.Mods.Cnc/Activities/LayMines.cs#L87C1-L90C18

LayMine(self);
QueueChild(new Wait(20)); // A little wait after placing each mine, for show
minefield.Remove(self.Location);
return false;

[PunkPun]

Tbh the situation feels highly improbable. How can move activity change location without even running? frameEndActions should be executed at the end of the tick, meaning that this actor's activities have no chance to run a second time before the mine is placed.

[PunkPun]

Move activity only sets mobile.ToCell in Tick function

[PunkPun]

I see, Activity has a way to immediately tick child activity

// Avoid a single tick delay if the childactivity was just queued.

In that case I'd prefer this fix

Another possible fix could be to store current minelayer's location (to a temporary variable that will be captured for the lambda) before scheduling FrameEndTask (and then use it to create the mine actor), but this feels like a workaround.

[michaeldgg2]

Are you sure? To me it seems something like "corrupted state": when FrameEndTask for creating mine (in LayMine()) runs, it has to deal simultaneously with the previous state (i.e. tick where the mine laying was scheduled) and current state (where it should create the mine actor while the minelayer is already moving to the next cell).

[PunkPun]

You could create the actor on Tick, but only add it to world on FrameEndTask. As you might have noticed .CreateActor has an override specifically for this reason.

Though giving it more thought, the current solution is fine. An improvement on it would be adding a check on FrameEndTask to see if mine still can be placed

[PunkPun]

Changelog