When the Land activity is run, the aircraft adds influence to the cell so it cannot be used by other actors. When the TakeOff activity runs, it removes the influence so the cell can be used by other actors.
However, when a Carryall picks up a unit, it is told to Land with a vertical offset - it never reaches ground level. When the TakeOff activity runs, it saw the aircraft was above ground level and bailed out. The means the influence is never removed. The cell is now unusable despite the fact the Carryall has left.
To fix this, TakeOff now checks if influence was applied instead of checking if the aircraft is above ground level. If so, we know the Land activity had decided that influence was required, even if the aircraft has not made it to ground level. When TakeOff runs, it will treat it as a proper take off event even though the aircraft is already above ground level. This means influence will be removed and the cell will become accessible as intended.
In ActorMap, we also fix a design flaw where disposed actors where excluded from queries. This caused cache inconsistencies with clients using ActorMap.CellUpdated event to rely on updates. This event will not get called when the actor was disposed, so the downsteam client may have cached the actors at that location, only for them to "change" when the actor is later disposed. This could cause the Locomotor and HierarchicalPathFInder to have inconsistent views of the actors on the map, causing crashes if the inconsistent state broken some internal invariants. The only reason to exclude disposed actors would be to cover up for the actors not being removed properly from the map, which is fixed now aircraft are handled correctly. If ever an actor isn't removed from the actor map, then the caller needs fixing rather than having the actor map exclude it.
By tracking updates on the ActorMap the HierarchicalPathFinder can be aware of actors moving around the map. We track a subset of immovable actors that always block. These actors can be treated as impassable obstacles just like terrain. When a path needs to be found the abstract path will guide the search around this subset of immovable actors just like it can guide the search around impassable terrain. For path searches that were previously imperformant because some immovable actors created a bottleneck that needed to be routed around, these will now be performant instead. Path searches with bottlenecks created by items such as trees, walls and buildings should see a performance improvement. Bottlenecks created by other units will not benefit.
We now maintain two sets of HPFs. One is aware of immovable actors and will be used for path searches that request BlockedByActor.Immovable, BlockedByActor.Stationary and BlockedByActor.All to guide that around the immovable obstacles. The other is aware of terrain only and will be used for searches that request BlockedByActor.None, or if an ignoreActor is provided. A new UI dropdown when using the `/hpf` command will allow switching between the visuals of the two sets.
When the UpdateCellBlocking encountered a transit-only cell (the bibs around a building) it would bail from the loop. This would leave the cellCrushablePlayers set to all players. It would update the cell cache and mark that cell as a crushable location.
When CanMoveFreelyInto would later evaluate a cell, it would consider it passable because the crushable check would pass (cellCache.Crushable.Overlaps(actor.Owner.PlayerMask)) rather than because the transit check (otherActor.OccupiesSpace is Building building && building.TransitOnlyCells().Contains(cell)) would pass.
Although this meant the cell was treated as passable in either scenario, it means the cache contained incorrect data. The cell does not contain any crushable actors but the cache would indicate it did. Correcting this means we can rely on the crushability information stored in the cache to be accurate.
During the refactoring to introduce HierarchicalPathFinder, custom costs were no longer applied to source locations. The logic being that if we are already in the source location, then there should be no cost added to it to get there - we are already there!
Path searches support the ability to go from multiple sources to a single target, but the reverse is not supported. Some callers that require a search from a single source to one of multiple targets perform their search in reverse, swapping the source and targets so they can run the search, then reversing the path they are given so things are the correct way around again. For callers that also use a custom cost like the harvester code that do this in order to find free refineries, they might want the custom cost to be applied to the source location. The harvester code uses this to filter out overloaded refineries. It wants to search from a harvester to multiple refineries, and thus reverses this in order to perform the path search. Without the custom cost being applied to the "source" locations, this filtering logic never gets applied.
To fix this, we now apply the custom cost to source locations. If the custom cost provides an invalid path, then the source location is excluded entirely. Although this seems unintuitive on its own, this allows searches done "in reverse" to work again.
Activated with the '/path-debug' chat command, this displays the explored search space and costs when searching for paths. It supports custom movement layers, bi-directional searches as well as visualizing searches over the abstract graph of the HierarchicalPathFinder. The most recent search among selected units is shown.
Teach HierarchicalPathFinder to keep a cache of domain indices, refreshing them only on demand and when invalidated by terrain changes. This provides an accurate and quick determination for checking if paths exist between given locations.
By exposing PathExistsForLocomotor on the IPathFinder interface, we can remove the DomainIndex trait entirely.
Replaces the existing bi-directional search between points used by the pathfinder with a guided hierarchical search. The old search was a standard A* search with a heuristic of advancing in straight line towards the target. This heuristic performs well if a mostly direct path to the target exists, it performs poorly it the path has to navigate around blockages in the terrain. The hierarchical path finder maintains a simplified, abstract graph. When a path search is performed it uses this abstract graph to inform the heuristic. Instead of moving blindly towards the target, it will instead steer around major obstacles, almost as if it had been provided a map which ensures it can move in roughly the right direction. This allows it to explore less of the area overall, improving performance.
When a path needs to steer around terrain on the map, the hierarchical path finder is able to greatly improve on the previous performance. When a path is able to proceed in a straight line, no performance benefit will be seen. If the path needs to steer around actors on the map instead of terrain (e.g. trees, buildings, units) then the same poor pathfinding performance as before will be observed.
Prior to ef44c31a72eab61a597cf539ee4b138e94b254fe, Locomotor would be earlier in the trait initialization sequence than SpawnStartingUnits. After this commit, the initialization sequence was perturbed and SpawnStartingUnits would initialize first. When SpawnStartingUnits would query CanEnterCell this would generate a null reference as Locomotor had not yet initialized.
SpawnStartingUnitsInfo is made to initialize NotBefore LocomotorInfo to enforce the required trait ordering.
The restores the previous behaviour before FindUnitPathToTargetCell was introduced. This prevents callers such as the harvester code crashing when a harvester tries to route home to a refinery, but there are no refineries.
Prior to ef44c31a72eab61a597cf539ee4b138e94b254fe, Locomotor would be earlier in the trait initialization sequence than SpawnMapActors. Locomotor would assume no actors on the map, and register to update blocked cells when new ones were added. When SpawnMapActors created actors, Locomotor was made aware and kept up-to-date.
After this commit, the initialization sequence was perturbed and SpawnMapActors would initialize first. Locomotor would assume no actors on the map and thus be unaware of these starting units, meaning those starting units would not cause blocking, allowing units to pass through them.
There are two possible fixes. SpawnMapActorsInfo can initialize NotBefore<LocomotorInfo>, enforcing that actors are spawned after locomotor is ready. Or we can remove the assumption in Locomotor that the map starts empty, and have it update blocked cells on startup. The latter seems cleaner, so any other traits that may want to spawn actors don't have to be aware sequencing their initialization with the Locomotor trait, instead things would "just work".
Some path searches, using PathSearch, were created directly at the callsite rather than using the pathfinder trait. This means some searches did not not benefit from the performance checks done in the pathfinder trait. It also means the pathfinder trait was not responsible for all pathing done in the game. Fix this with the following changes:
- Create a sensible shape for the IPathFinder interface and promote it to a trait interface, allowing theoretical replacements of the implementation. Ensure none of the concrete classes in OpenRA.Mods.Common.Pathfinder are exposed in the interface to ensure this is possible.
- Update the PathFinder class to implement the interface, and update several callsites manually running pathfinding code to instead call the IPathFinder interface.
- Overall, this allows any implementation of the IPathFinder interface to intercept and control all path searching performed by the game. Previously some searches would not have used it, and no alternate implementations were possible as the existing implementation was hardcoded into the interface shape.
Additionally:
- Move the responsibility of finding paths on completed path searches from pathfinder to path search, which is a more sensible location.
- Clean up the pathfinder pre-search optimizations.
Requires<T> means that trait of type T will be initialized first, and asserts that at least one exists. The new NotBefore<T> means that trait of type T will be initialized first, but allows no traits.
This allows traits to control initialization order for optional dependencies. They want to be initialized second so they can rely on the dependencies having been initialized. But if the dependencies are optional then to not throw if none are present.
We apply this to Locomotor which was previously using AddFrameEndTask to work around trait order initialization. This improves the user experience as the initialization is applied whilst the loading screen is still visible, rather than the game starting and creating jank by performing initialization on the first tick.
The existing APIs surfaces for pathfinding are in a wonky shape. We rearrange various responsibilities to better locations and simplify some abstractions that aren't providing value.
- IPathSearch, BasePathSearch and PathSearch are combined into only PathSearch. Its role is now to run a search space over a graph, maintaining the open queue and evaluating the provided heuristic function. The builder-like methods (WithHeuristic, Reverse, FromPoint, etc) are removed in favour of optional parameters in static creation methods. This removes confusion between the builder-aspect and the search function itself. It also becomes responsible for applying the heuristic weight to the heuristic. This fixes an issue where an externally provided heuristic ignored the weighting adjustment, as previously the weight was baked into the default heuristic only.
- Reduce the IGraph interface to the concepts of nodes and edges. Make it non-generic as it is specifically for pathfinding, and rename to IPathGraph accordingly. This is sufficient for a PathSearch to perform a search over any given IGraph. The various customization options are concrete properties of PathGraph only.
- PathFinder does not need to deal with disposal of the search/graph, that is the caller's responsibility.
- Remove CustomBlock from PathGraph as it was unused.
- Remove FindUnitPathToRange as it was unused.
- Use PathFinder.NoPath as the single helper to represent no/empty paths.
- Split control groups management to its own interface
- Add hotkeys for selecting, creating, adding to and combining with control groups
- Add a ControlGroups widget to manage the player interaction
The path cache was originally a moderate benefit, but over time a couple of things have conspired against it:
- Only paths with BlockedByActor.None are cached. Originally all paths regardless of blocking were cached but this was deemed unacceptable due to potentially returning outdated paths as actors move about. Paths with BlockedByActor.None are only invalidated if terrain conditions change, which are rarer.
- Move will try and find a path 4 times, trying with a different BlockedByActor check each time. BlockedByActor.None is the last check and only reached if the other searches fail. This is a rare scenario.
Overall, this means the hit rate for the cache is almost non-existent. Given the constraints on path validity it seems unlikely that the hit rate could be improved significantly, therefore it seems reasonable to remove the cache entirely to remove the overhead of cache management.
- Make Status the first field.
- Rename EstimatedTotal to EstimatedTotalCost to make it clearer it has the same unit as the CostSoFar field.
- Rename PreviousPos to PreviousNode as node terminology is a better match for usage.
As there are few custom movement layers, using an array is good for improving lookup speed. Additionally, we can simplify some code by reserving index 0 of the array for the ground layer. Code that needs to maintain a state for the ground layer and every custom movement layer can now maintain a flat array of state using index 0 for the ground layer, and the the ICustomMovementLayer.Index for the custom movement layer. This removes a lot of ternary statements checking for the ground layer special case.
- Rename CostForInvalidCell to PathCostForInvalidPath
- Add MovementCostForUnreachableCell
- Update usages of int.MaxValue and short.Maxvalue to use named constants where relevant.
- Update costs on ICustomMovementLayer to return short, for consistency with costs from Locomotor.
- Rename some methods to distinguish between path/movement cost.
