When HarvesterBotModule is ordering idle harvesters to nearby resources, it previously scanned from the harvester's current location. Instead, it now scans from the location of the nearest refinery. As the harvester will likely make many runs between the resource and the refinery, it is better to choose a location near the refinery. This will minimise overall distance travelled to harvest the resource patch.
The AI uses HarvesterBotModule to check for idle harvesters, and give them harvest orders. By default it scans every 50 ticks (2 seconds at normal speed), and for any idle harvesters locates an ore patch and issues a harvest order. This FindNextResource to scan for a suitable ore path is quite expensive. If the AI has to scan for ore patches for several harvesters, then this can produce a noticeable lag spike. Additionally, when there are no available ore patches, the scan will just keep repeating since the harvesters will always be idle - thus the lag spikes repeat every 50 ticks.
To reduce the impact, there already exists a randomization on the first scan interval so that multiple different AIs scan on different ticks. By ensuring the AI players scan at different times, we avoid a huge lag spike where they all operate on the same tick.
To reduce the impact even more, we make four additional changes:
- Scans continue to be done every 50 ticks to detect harvesters. But we spread out the searches for ore patches over multiple later ticks. We'll only perform one ore patch search per tick. This means instead of ordering e.g. 30 harvesters on a single tick and creating a spike, we order one on each tick over the next 30 ticks instead. This spreads out the performance impact.
- When a harvester fails to locate any suitable ore patch, we put it on a longer cooldown, by default 5x the regular cooldown. We don't need to scan as often for these harvesters, since it'll take time for new resources to appear.
- We change the path search in FindNextResource from FindPathToTargetCellByPredicate to FindPathToTargetCells. The format in an undirected path search that must flood fill from the start location. If ore is on the other side of the map, this entails searching the whole map which is very expensive. By maintaining a lookup of resource types per cell, we can instead give the target locations directly to the path search. This lookup requires a small overhead to maintain, but allows for a far more efficient path search to be carried out. The search can be directed towards the target locations, and the hierarchical path finder can be employed resulting in a path search that explores far fewer cells. A few tweaks are made to ResourceClaimLayer to avoid it creating empty list entries when this can be avoided.
- We adjust how the enemy avoidance cost is done. Previously, this search used world.FindActorsInCircle to check for nearby enemies, but this check was done for every cell that was searched, and is itself quite expensive. Now, we create a series of "bins" and cache the additional cost for that bin. This is a less fine grained approach but is sufficient for our intended goal of "avoid resource patches with too many enemies nearby". The customCost function is now less expensive so we can reuse the avoidance cost stored for each bin, rather than calculating fresh for every cell.
When this hits the case that "As both ends are accessible, we can freely swap them." - we must note that we are reversing the path search and pass the information into the HierarchicalPathFinder. When a normal path search occurs, the actor trying to pathfind will never check its own location - and thus never gets blocked by itself. When a search is reversed, the search will check the actors location. If we inform the search it is doing done in reverse, it will special case this scenario and avoid the actor blocking itself. But if it is not told about this scenario, then this special case is not applied and no path will be found when in fact a path is possible.
When spawning starting units, or spawning units when collecting a crate, nearby locations will be used. If a nearby location cannot be reached, e.g. it is on top of a cliff or blocked in by trees, then any unit spawned there will be isolated which is not ideal.
Use the PathMightExistForLocomotorBlockedByImmovable method to filter nearby locations to those where the spawned unit can path back to the original location. This ensures the spawned unit is not isolated.
The default Stream implementation of this method has to rent an array so it can call the overload that accepts an array, and then copy the output over. This is because the array overload is required and the span overload was only added more recently.
We can avoid the overhead of this by implementing the span overload and working with the destination span directly. Do so for all classes we have that derive from Stream, and redirect their array overload to the span overload for code reuse.
- AI places rally points at pathable locations. A pathable location isn't strictly required, but avoids the AI setting rally points at seemingly dumb locations. This is an addtional check on top of the existing buildability check.
- AI now evaluates rally points every AssignRallyPointsInterval (default 100 ticks). Invalid rally points aren't that harmful, so no need to check them every tick. Additionally we do a rolling update so rally points for multiple locations are spread across multiple ticks to reduce any potential lag spikes.
The ExtractEmmyLuaAPI utility command, invoked with `--emmy-lua-api`, produces a documentation file that is used by the [OpenRA Lua Language Extension](https://marketplace.visualstudio.com/items?itemName=openra.vscode-openra-lua) to provide documentation and type information is VSCode and VSCode compatible editors when editing the Lua scripts.
We improve the documentation and types produced by this utility in a few ways:
- Require descriptions to be provided for all items.
- Fix the type definitions of the base engine types (cpos, wpos, wangle, wdist, wvec, cvec) to match with the actual bindings on the C# side. Add some extra bindings for these types to increase their utility.
- Introduce ScriptEmmyTypeOverrideAttribute to allow the C# side of the bindings to provide a more specific type. The utility command now requires this to be used to avoid accidentally exporting poor type information.
- Fix a handful of scripts where the new type information revealed warnings.
The ability to ScriptEmmyTypeOverrideAttribute allows parameters and return types to provide a more specific type compared to the previous, weak, type definition. For example LuaValue mapped to `any`, LuaTable mapped to `table`, and LuaFunction mapped to `function`. These types are all non-specific. `any` can be anything, `table` is a table without known types for its keys or values, `function` is a function with an unknown signature.
Now, we can provide specific types. , e.g. instead of `table`, ReinforcementsGlobal.ReinforceWithTransport is able to specify `{ [1]: actor, [2]: actor[] }` - a table with keys 1 and 2, whose values are an actor, and a table of actors respectively. The callback functions in MapGlobal now have signatures, e.g. instead of `function` we have `fun(a: actor):boolean`. In UtilsGlobal, we also make use of generic types. These work in a similar fashion to generics in C#. These methods operate on collections, we can introduce a generic parameter named `T` for the type of the items in those collections. Now the return type and callback parameters can also use that generic type. This means the return type or callback functions operate on the same type as whatever type is in the collection you pass in. e.g. Utils.Do accepts a collection typed as `T[]` with a callback function invoked on each item typed as `fun(item: T)`. If you pass in actors, the callback operates on an actor. If you pass in strings, the callback operates on a string, etc.
Overall, these changes should result in an improved user experience for those editing OpenRA Lua scripts in a compatible IDE.
In 4312a4d3f4 MiniYaml merging was adjusted. One effect of this change was that duplicate keys in files that did not previously require merging was previously allowed, but was now an error. (Test case `TestMergeConflictsNoMerge`)
The installer files were relying on the previous behaviour to allow multiple `ContentPackage` keys. The above change caused a regression where attempting to manage mod content would crash due to now erroring on the duplicate keys.
We fix the issue by applying a unique ID suffix, as is a common pattern elsewhere in our yaml files, and teach InstallFromSourceLogic to recognise and strip it.
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.
Currently when linting translations, we check for any unused translation keys. This works fine for the default mods, which own the entire sets of translation files. For community mods, they often import the translation files from the common mod assets, but they may only use some of the translations provided. Currently, they would get warnings about not using translations from the common files they have imported.
Since the community mods don't own those translations, getting warnings about it is annoying. To solve this issue, introduce a AllowUnusedTranslationsInExternalPackages in the mod.yaml which defaults to true. This will prevent reporting of unused translation keys from external assets. Keys that are used for external assets will still be validated, and keys from the mod assets will be both validated and unused keys will be reported.
We default the new flag to true and don't provide an update rule. This means community mods will get the new behaviour. For the default mods, we do want to check the "external" assets, since we control those assets. So the default mods have their mod.yaml updated to disable the flag and retain the existing behaviour of checking everything.
The AI would often invoke this method inside of loops, searching for a different category of queue each time. This would result in multiple searches against the trait dictionary to locate matching queues. Now we alter the method to create a lookup of all the queues keyed by category. This allows a single trait search to be performed.
UnitBuilderBotModule and BaseBuilderBotModule are updated to fetch this lookup once when required, and pass the results along to avoid calling the method more times than necessary. This improves their performance.
The scenario is that an actor is on an unreachable tile, and would like to path. As long as it is immediately adjacent to some reachable tiles, it can still move onto them and path. Now imagine a map split into two by a one tile wide line of impassable cliffs. It is important which side it chooses to jump into, as once it has moved off the cliff it loses access to the other side. Jumping off the correct side will allow a valid path, jumping off the wrong side will prevent a path from being possible.
In d8ebb96077, handling was added to prevent a crash where the path search would simulate having the actor jump off the wrong side and then get confused that it could not find a path when one was expected. This fix works by remembering the `unpathableNodes` - the nodes where you jump onto the wrong side. If we encounter them later, we can ignore them.
In 5157bc375d we added domain checks - this allows the HPF to bail on impossible paths early by checking if they belong to different domains. For example islands on a water map will belong to different domains.
This caused a regression where `sourcesWithReachableNodes` was now badly named. Some reachable nodes because they were in the wrong domain. Later when `sourcesWithPathableNodes` and `unpathableNodes` are built - we don't populate the `unpathableNodes` correctly, because we already excluded the unpathable nodes earlier on! This means we don't ignore them any more, and we reintroduce the crash.
Now that we are checking the domain, we can simplify the code and resolve the crash at the same time. `sourcesWithReachableNodes` becomes `sourcesWithPathableNodes` because the domain check has been done. We can build `unpathableNodes` at the same time. This allows us to remove the later code that explored the abstract graph, as the domain check succinctly achieves the same end goal of determining whether the node has a path or not.
Using the glory of regex, we can scrape any Lua script files that a map includes and locate calls to the UserInterface.Translate method. We can then treat them in the same way as C# fields marked with a TranslationReferenceAttribute. This allows the lint check to validate the translation invoked in the .lua script has a matching entry in the translation .ftl files, with all the required arguments (if any).
We can also locate any calls to AddPrimaryObjective or AddSecondaryObjective defined by the utils.lua script, which also accept translation keys.
The are a couple of restrictions:
- When linting the map, we don't check for keys in the ftl file that are unused. This is because the linter doesn't load all the keys when checking maps.
- In order to validate translation arguments with the regex, we require the Lua script to pass the table of arguments inline at the callsite. If it does not, we raise a warning so the user can adjust the code.
