Fixed a desync in Cargo.CurrentAdjacentCells

[Mailaender]

Closes #21506

[RoosterDragon]

I could do with some help understanding how CurrentAdjacentCells comes to be out of sync between the clients, and why this is sufficient to fix it. I should inspect a replay when I can but wondering if somebody else is able to tell by inspection.

I also feel like the cache is wrong?: Util.AdjacentCells calls into target.Positions which calls Actor.GetTargetablePositions() which may return enabledTargetableWorldPositions and thus the results of this call can change if targetable position traits become enabled or disabled, even if the actor hasn't moved. However I'm not sure if this would be relevant to the desync or just an existing bug (if it is even in fact a bug)

[pchote]

I strongly suspect that it is due to the same enumerable being cached and returned for multiple callers. The CanUnload method is called from order generation (on the owner's client only), and will change the Current pointer of the shared enumerable.

[abcdefg30]

Changelog

[RoosterDragon]

I looked into the root cause, and it's the same as described in #17863:

It seems like it doesn't/can't calculate the CurrentAdjacentCells as the unit dies, because actor's CenterPosition is null. Causes creation of an Invalid Target and ends up with an empty CurrentAdjacentCells which kills the units instead of unloading. But, if the CurrentAdjacentCells was already calculated and cached for another reason (hovering over the building calculates that to get Unload cursor). CurrentAdjacentCells will have values in it and the units will unload. But in a MP game this may be cached for a player and not for the other. If that's the case game tries to kill the units on one side and unload them on other, desyncing the game.

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.

[pchote]

A solution could be to add a INotifyTargetablePositionsChanged interface, and to use this to invalidate the cached value when the unit moves or enables/disables them.