The Harvester trait and MoveAdjacentTo activity called the pathfinder but had a single source and multiple targets. The pathfinder interface only allows for the opposite: multiple sources and a single target. To work around this they would swap the inputs. This works in most cases but not all cases. One aspect of asymmetry is that an actor may move out of an inaccessible source cell, but not onto an inaccessible target cell.
Searches that involved an inaccessible source cell and that applied this swapping method would therefore fail to return a path, when a valid path was possible. Although a rare case, once good way to reproduce is to use a production building that spawns actors on inaccessible cells around it, such as the RA naval yard. A move order uses the pathfinder correctly and the unit will move out. Using a force attack causes the unit to use the broken "swapped" mechanism in MoveAdjacentTo and it will be stuck.
This asymmetry has been longstanding but the pathfinding infrastructure only sporadically accounted for it. It is now documented and applied consistently. Create a new overload on the pathfinder trait that allows a single source and multiple targets, so callers have an overload that does what they need and won't be tempted to swap the positions and run into this issue.
Internally, this requires us to teach Locomotor to ignore the self actor when performing movement cost checks for these "in reverse" searches so the unit doesn't consider the cell blocked by itself.
- Enforce SA1604 ElementDocumentationShouldHaveSummary.
- Enforce SA1629 DocumentationTextShouldEndWithAPeriod.
- Turn off some rules covered by IDExxxx rules.
- Remaining rules are treated as part of OpenRA style.
DeployForGrantedCondition is wrapped around the Move activity, so the AttackMoveActivity thinks that DeployForGrantedCondition is the Move activity.
All it means is that we need to forward the target line request to the Move activity
Occupied cells was defined by height yet we didn't update actor map on changing height. This in some scenarios could have caused the aircraft to forget to remove its influence from actor map
When crushables and crates change their Location/TopLeft, their crushability is cached, but when their CenterPosition is changed, their cached crushability is not refreshed. Since their CrushableBy functions depends on IsAtGroundLevel, which depends on the CenterPosition, this means that when the crushability is cached it will depend on the current height of the object. If the height of the object changes, the cache is not refreshed and now contains out of date information.
The Locomotor cache and the HPF both cache this same information, but at different times. HPF caches immediately, but Locomotor caches on demand which means there can be a delay. This means they can have inconsistent, differing views of the crushability information. This eventually surfaces in a "The abstract path should never be searched for an unreachable point." error from HPF when it detects the inconsistency.
The bug is that Locomotor was caching information without refreshing it when required. Fixing this to refresh the cache when the CenterPosition changes is likely to have negative performance impacts. As would removing crushability from the cache. These would both be fixes that address the underlying bug.
The high impacts of a proper fix lead us to a workaround instead. If we set the CenterPosition before setting the Location, then when the Location is set and the caches are refreshed, the new CenterPosition is available when caching the crushability information. This means logic depending on IsAtGroundLevel will get the new information and cache a more up-to-date view of things. This means when changing both the CenterPosition and Location together we now cache correct information. However calls that set only the CenterPosition and not the Location can still result in a bad cache state. Although this is imperfect it is an improvement over current affairs, and has less impact.
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.
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.
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.
The `Refinery` trait has a hardcoded usage of `SpriteHarvesterDockSequence`, which requires the harvester to have `WithDockingAnimation`, making it inconvenient-at-best to NOT have a docking/unloading animation.
Actor previously cached targetable locations for static actors as an optimization. As we can no longer reference the IPositionable interface, move this optimization to HitShape instead. Although we lose some of the efficiency of caching the final result on the actor, we gain some by allowing HitShape to cache the results as long as they have not changed. So instead of being limited to static actors, we can extend the caching to currently stationary actor.
