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.
Follow-up from 34ff23d030, the previous `-Remap:` line was not working and has since been removed. Without the remap being unapplied, there are rendering artefacts introduced as a result. Fix this by explicitly setting the remap to something else instead.
- Previously the Inherits syntax was only resolved when used for top-level nodes. Now it is also resolved for nested nodes as well.
- Previously the MiniYAML Merge feature supported the ability to remove nodes, but this only worked within the context of inherited nodes. Now, we allow node removal to work outside of the inheritance context.
The player state and teams are fixed after the game starts, so it is safe to cache the resulting teams in the observer logic rather than re-evaluating them each time.
Updating squads is the most expensive part of SquadManagerBotModule. It involves ticking the current squad state. This usually involves finding nearby enemies and evaluating the fuzzy state machine to decide whether to interact with those enemies. Since all the AI squads for a player get ordered on the same tick, this can result in a lag spike.
To reduce the impact, we'll spread out the updates over multiple ticks. This means overall all the AI squads will still be refreshed every interval, but it'll be a rolling update rather than all at once. By spreading out the updates we avoid a lag spike from the cumulative updates of all the squads. Now the lag spike is reduced to the worst any single squad update can incur.
The StateMachine offered a feature to remember the previous state and allow reverting to it. However this feature is unused. Remove it to allow the previous states to be reclaimed by the GC earlier.
If a long path was being visualized with the path-debug command it would generate renderables for everything on the path, even for parts of the path that would be offscreen. Add some simplistic culling so the performance impact is reduced.
Several activities that queue child Move activities can get into a bad scenario where the actor is pathfinding and then gets stuck because the destination is unreachable. When the Move activity then completes, then parent activity sees it has yet to reach the destination and tries to move again. However, the actor is still blocked in the same spot as before and thus the movment finishes immediately. This causes a performance death spiral where the actor attempts to pathfind every tick. The pathfinding attempt can also be very expensive if it must exhaustively check the whole map to determine no route is possible.
In order to prevent blocked actors from running into this scenario, we introduce MoveCooldownHelper. In its default setup it allows the parent activity to bail out if the actor was blocked during a pathfinding attempt. This means the activity will be dropped rather than trying to move endlessly. It also has an option to allow retrying if pathfinding was blocked, but applies a cooldown to avoid the performance penalty. For activities such as Enter, this means the actors will still try and enter their target if it is unreachable, but will only attempt once a second now rather than every tick.
MoveAdjacentTo will now cancel if it fails to reach the destination. This fixes MoveOntoAndTurn to skip the Turn if the move didn't reach the intended destination. Any other derived classes will similarly benefit from skipping follow-up actions.
