When a move is made where the source and target locations are the same and no actual moving is required, a path of length 0 is returned. When a move cannot be made as there is no valid route, a path of length 0 is also returned. This means Move is unable to tell the difference between no movement required, and no path is possible. Currently it will hit the `hadNoPath` case and report CompleteDestinationBlocked.
To fix the scenario where the source and target location match, track a alreadyAtDestination field. When this scenario triggers, report CompleteDestinationReached instead.
This fixes activities that were using this result to inform their next actions. e.g. MoveOntoAndTurn would previously cancel the Turn portion of the activity, believing that the destination could not be reached. Now, it knows the destination was reached (since we are already there!) and will perform the turn.
In 05ed9d9a73 we stopped caching the values with ToArray to resolve a desync. But even caching the enumerable can lead to a desync, so remove the caching entirely.
----
Let's explain how the code that cached values via ToArray could desync.
Usually, the cell given by `self.Location` matches with the cell given by `self.GetTargetablePositions()`. However if the unit is moving and close to the boundary between two cells, it is possible for the targetable position to be an adjacent cell instead.
Combined with the fact hovering over the unit will evaluate `CurrentAdjacentCells` only for the local player and not everybody, the following sequence becomes possible to induce a desync:
- As the APC is moving into the last cell before unloading, the local player hovers over it. `self.Location` is the last cell, but `self.GetTargetablePositions()` gives the *previous* cell (as the unit is close to the boundary between the cells)
- The local player then caches `CurrentAdjacentCells`. The cache key of `self.Location` is the final cell, but the values are calculated for `self.GetTargetablePositions()` of an *adjacent* cell.
- When the order to unload is resolved, the cache key of `CurrentAdjacentCells` is already `self.Location` and so `CurrentAdjacentCells` is *not* updated.
- The units unload into cells based on the *adjacent* cell.
Then, for other players in the game:
- The hover does nothing for these players.
- When the order is resolved, `CurrentAdjacentCells` is out of date and is re-evaluated.
- `self.Location` and `self.GetTargetablePositions()` are both the last cell, because the unit has finished moving.
- So the cache is updated with a key of `self.Location` and values from the *same* cell.
- The units unload into cells based on the *current* cell.
As the units unload into different cells, a desync occurs. Ultimately the cause here is that cache key is insufficient - `self.Location` can have the same value but the output can differ. The function isn't a pure function so memoizing the result via `ToArray()` isn't sound.
Reverting it to cache the enumerable, which is then lazily re-evaluated reduces the scope of possible desyncs but is NOT a full solve. The cached enumerable caches the result of `Actor.GetTargetablePositions()` which isn't a fully lazy sequence. A different result is returned depending on `EnabledTargetablePositions.Any()`. Therefore, if the traits were to enable/disable inbetween, then we can still end up with different results. Memoizing the enumerable isn't sound either!
Currently our only trait is `HitShape` which is enabled based on conditions. A condition that enables/disables it based on movement would be one way to trigger this scenario. Let's say you have a unit where you toggle between two hit shapes when it is moving and when it stops moving. That would allow you to replicate the above scenario once again.
Instead of trying to come up with a sound caching mechanism in the face of a series of complex inputs, we just give up on trying to cache this information at all.
Several activities that queue child Move activities can get into a bad scenario where the actor is pathfinding and then gets stuck because the destination is unreachable. When the Move activity then completes, then parent activity sees it has yet to reach the destination and tries to move again. However, the actor is still blocked in the same spot as before and thus the movment finishes immediately. This causes a performance death spiral where the actor attempts to pathfind every tick. The pathfinding attempt can also be very expensive if it must exhaustively check the whole map to determine no route is possible.
In order to prevent blocked actors from running into this scenario, we introduce MoveCooldownHelper. In its default setup it allows the parent activity to bail out if the actor was blocked during a pathfinding attempt. This means the activity will be dropped rather than trying to move endlessly. It also has an option to allow retrying if pathfinding was blocked, but applies a cooldown to avoid the performance penalty. For activities such as Enter, this means the actors will still try and enter their target if it is unreachable, but will only attempt once a second now rather than every tick.
MoveAdjacentTo will now cancel if it fails to reach the destination. This fixes MoveOntoAndTurn to skip the Turn if the move didn't reach the intended destination. Any other derived classes will similarly benefit from skipping follow-up actions.
This allows the LINQ spelling to be used, but benefits from the performance improvement of the specific methods for these classes that provide the same result.
This rule no longer appears to be buggy, so enforce it. Some of the automated fixes are adjusted in order to improve the result. #pragma directives have no option to control indentation, so remove them where possible.
Previously, the ClosestTo and PositionClosestTo existed to perform a simple distance based check to choose the closest location from a choice of locations to a single other location. For some functions this is sufficient, but for many functions we want to then move between the locations. If the location selected is in fact unreachable (e.g. on another island) then we would not want to consider it.
We now introduce ClosestToIgnoringPath for checks where we don't care about a path existing, e.g. weapons hitting nearby targets. When we do care about paths, we introduce ClosestToWithPathFrom and ClosestToWithPathTo which will check that a path exists. The PathFrom check will make sure one of the actors from the list can make it to the single target location. The PathTo check will make sure the single actor can make it to one of the target locations. This difference allows us to specify which actor will be doing the moving. This is important as a path might exists for one actor, but not another. Consider two islands with a hovercraft on one and a tank on the other. The hovercraft can path to the tank, but the tank cannot path to the hovercraft.
We also introduce WithPathFrom and WithPathTo. These will perform filtering by checking for valid paths, but won't select the closest location.
By employing the new methods that filter for paths, we fix various behaviour that would cause actors to get confused. Imagine an islands map, by checking for paths we ensure logic will locate reachable locations on the island, rather than considering a location on a nearby island that is physically closer but unreachable. This fixes AI squad automation, and other automated behaviours such as rearming.
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.
