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.
- EditorActorLayer now tracks previews on map with a SpatiallyPartitioned instead of a Dictionary. This allows the copy-paste logic to call an efficient PreviewsInCellRegion method, instead of asking for previews cell-by-cell.
- EditorActorPreview subscribes to the CellEntryChanged methods on the map. Previously the preview was refreshed regardless of which cell changed. Now the preview only regenerates if the preview's footprint has been affected.
This is a more natural representation than int that allows removal of casts in many places that require uint. Additionally, we can change the internal representation from long to uint, making the Color struct smaller. Since arrays of colors are common, this can save on memory.
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.
- Consuming methods cared only about the count and not the actual actors, so only counts the actors rather that creating lists.
- ProvidesPrerequisites implementations return cached objects rather then allocating new enumerables on each call.
