Openra: Replace mod upgrade rules with a fit for purpose alternative.

[pchote]

WIP code at https://github.com/pchote/OpenRA/tree/upgrade-rules.

This won't be ready for the first playtest, but if we want the Mod SDK to succeed then we need to get this done before the final release.

[pchote]

A key goal of the new system is to document (if not automate) all the changes that are required to keep a mod functional. Accounting for new mod-level features in the base mods is an explicit antifeature, so things like #14368 or #13276 can't reasonably be included.

I have gone through the mods/cnc history and compiled a list of PRs that will need to cover. Many of these will already have rules, but some don't.

Added in #15010:

• [ ] Replace Mobile.OnRails [#13635] → RemoveMobileOnRails
• [ ] Added Aircraft TakeOffOnResupply / VTOL [#13865] → AircraftCanHoverGeneralization
• [ ] WithNukeLaunchAnimation [#13874] → AddNukeLaunchAnimation
• [ ] WithEmbeddedTurretSpriteBody [#14023] → RenameWithTurreted
• [ ] Capturable stances and Type → Types [#14230] → CapturableChanges
• [ ] AmmoPool changes [#13538] → DecoupleSelfReloading / RemoveOutOfAmmo
• [ ] Power changes part 1 [#13998] → ReplaceRequiresPower
• [ ] Gives/Requires BuildableArea [#13641] → ChangeBuildableArea
• [ ] CustomRenderSize / AutoRenderSize [#13899] → (obsolete)
• [ ] Removing CustomRenderSize / AutoRenderSize [#14482] → MoveVisualBounds
• [ ] Health / damage scaling [#14448] → ScaleDefaultModHealth
• [ ] Support power time scaling [#14366] → ScaleSupportPowerSecondsToTicks

Missing:

• [ ] Remove TargetWhenIdle/TargetWhenDamaged from AutoTarget [#13695]
• [ ] Refactor BurstDelay to BurstDelays [#13684]
• [ ] Types: support for InfiltrateFor* traits [#14311]

Add update notes for:

• [x] Add debug visualization commands [#13328]
• [x] *Add EnableDebugCommandsInReplays option [#13813]
• [x] *Add chat tab to multiplayer/replays ingame menu [#13408]
• [x] Hotkey changes
• [x] *Dynamic map option UI [#14209,#14359]
• [x] *MP UI overhaul
• [x] *Debug menu ScreenMap checkbox [#14473]
• [x] More hotkey stuff [#14379]

Before we start on this we need to make a decision on how to handle upgrade rules that are superseded by later ones. For example #13899 and all the hotkey ones. If we want to support arbitrary bleed-bleed updates then we are forced to have separate definitions for each pr, and some of them require manual steps. My current idea on this is to write them as they would have been at the time and then add a note to the manual description that the rule is now defunct and can be deferred until future step X.Y. These useless manual steps will add a level of annoyance (the utility needs to be restarted after each manual step) but I don't see any other reasonable solutions.

UI additions are another annoying problem. Things like #14473 could be safely omitted because the game will run normally if they are missing, but things like #13813 will crash the game when opening the UI. We then have all the changes associated with the server list and lobby - it is unreasonable to bloat the upgrade rules with 20 near identical manual steps explaining "more stuff changed in the UI".

[pchote]

While researching and thinking about the comment above I have come to the conclusion that it is completely impractical to maintain an upgrade pathway between arbitrary bleed commits. I don't have the time or interest in spending the amount of work that would be required to maintain that, and i'm pretty sure that the overheads in running the updater would put everybody off actually using them.

Instead, I think the only reasonable course of action will be to support (for downstream) only tag-to-tag updates plus the bleed-tip updates we need for our default mods. Downstream mods that target bleed will have to manage their own fixes if they end up on an unsupported intermediate commit where the upgrade rules are no longer able to update them (note: this isn't that different to the status quo).

This gives us the ability to rewrite and amend the upgrade rules as we go, accounting for and smoothing over any partial or missteps that we work through with the default mods. The key requirement would be that we can use the upgrader + reported manual steps to update an SDK-based mod to a new candidate playtest or release before we ship it.

This changes the design requirements for the utility and rule files, and my prototype will need to be adapted to suit.

[GraionDilach]

I'm wholeheartedly supporting the option presented in this last comment and agreeing with all the reasons mentioned. The status quo is part of the pay for the bleed modders for their decision to mod on bleed, but we're getting a handful of modders these days who will need a better option as themselves will not be able to deal with the upgrade stuff.

[penev92]

The tag-to-tag option sounds very good for modders like @KOYK and @ABrandau, who are unable to follow bleed on their own, and for those mods that have the manpower to follow bleed (and try to do it) like AS and CD, the option to upgrade to latest bleed is there, so 👍 from me. (The option to upgrade to latest bleed is good enough to sacrifice the option to update to a random older version of bleed)

[jrb0001]

-1 because you get stuck on a specific bleed commit as soon as you leave a tag and there is no supported way back.

[penev92]

I would like to clarify that last comment:

[20:04] <+penev> which means you can't reliably upgrade from last week's bleed to current bleed :/
[20:04] <+penev> for free, at least

You can upgrade from a tag to a tag, or from tag to latest bleed. Once on bleed, you will need to put some effort into upgrading to a newer version of bleed, like not running the now-outdated-for-you upgrade rules.
This is a reasonable trade-off by all accounts, because the proposed system makes upgrades easier for non-experienced users, and those modding on bleed should be fully aware from the start that it would be harder, and for the most part they have enough knowledge/experience to handle themselves and live with the trade-off of having to put in effort.

[pchote]

The way I see this working is to separate out the whats from the whens.

Each update rule is written as a feature-oriented module, e.g. ReplaceMobileOnRails or ExplainUIChanges, and may in principle contain changes from several PRs if work was spread out.

We then get rid of update dates and api versions, and simply list the set of rules to run for a given starting point (e.g. --upgrade-mod release-20171014 or --upgrade-mod playtest-20180102). For our own mods and anyone else working from bleed the rules can be ran individually (e.g. --upgrade-mod ReplaceMobileOnRails or --upgrade-mod ExplainUIChanges). This change has a useful side effect of solving our endless rebase and stale date problems.

When I say that "we only support tag-to-tag updates" I mean that we cannot and will not test every arbitrary combination of starting and ending commits. This is how things have always worked and I expect that it won't change anything in the vast majority of cases. There are two cases that I can think of, and I expect that most people will agree that we shouldn't be forced to support these on our unstable development branch:

• An upgrade rule spans multiple PRs, but the starting commit is somewhere in the middle. In most of these cases I expect that the side effects will be harmless (e.g. reporting a manual step that has already been done) because we have always written upgrade rules to be reasonably robust and we will need to support this update path for our own bleed development. It should work, but you may need to unstage some unwanted changes or work out which manual steps you should ignore.
• We revert a PR that had an upgrade rule that was run before the current bleed commit. Supporting this case would mean maintaining both the original rule plus a new one that undoes it.

[pchote]

I took another stab at this, and have now finished all the major plumbing.

Here are some samples:

paul@Ragnarok:OpenRA+upgrade-rules$ mono --debug OpenRA.Utility.exe cnc2 --update-mod bogus
Unknown source: bogus
Valid sources are:
   Update Paths:
      release-20171014
      playtest-20180102
   Individual Rules:
      ScaleDefaultModHealth
      AircraftCanHoverGeneralization
      RemoveMobileOnRails

paul@Ragnarok:OpenRA+upgrade-rules$ mono --debug OpenRA.Utility.exe cnc2 --update-mod release-20171014 --detailed
Found 3 API changes:
  * RemoveMobileOnRails: Notify Mobile.OnRails removal
     The OnRails parameter on Mobile has been removed.
     Actors that want to duplicate the left-right movement of the original Tiberian Dawn gunboat
     should replace the Mobile and AttackTurreted traits with TDGunboat and AttackTDGunboatTurreted.

  * AircraftCanHoverGeneralization: Split Aircraft.CanHover into multiple parameters
     Aircraft VTOL behaviour has been moved from CanHover to a new VTOL parameter.
     Aircraft taking off automatically after reloading has been moved from CanHover to a new TakeOffOnResupply parameter.
     Actors that set CanHover: true are updated with appropriate defaults for these parameters.

  * ScaleDefaultModHealth: Scale health and damage in the default OpenRA mods
     All health and damage values are increased by a factor of 100 (ra, cnc, ts) or 10 (d2k)
     in order to reduce numerical inaccuracies in damage calculations.

Run this command with the --apply flag to apply the update rules.



md5-011097f575d5b4821233e099d9866f18



paul@Ragnarok:OpenRA+upgrade-rules$ mono --debug OpenRA.Utility.exe cnc2 --update-mod release-20171014 --apply
WARNING: This command will automatically rewrite your mod rules.
Side effects of this command may include changing the whitespace to 
match the default conventions, and any yaml comments will be removed.

We strongly recommend that you have a backup of your mod rules, and 
for best results, to use a Git client to review the line-by-line 
changes and discard any unwanted side effects.

Press y to continue, or any other key to cancel: y

RemoveMobileOnRails: Notify Mobile.OnRails removal
   Updating mod... COMPLETE
   Updating system maps... COMPLETE
   Manual changes are required to complete this update:
    * Mobile.OnRails is no longer supported for actor type BOAT.
      If you want to duplicate the left-right movement of the original Tiberian Dawn gunboat
      you must manually replace Mobile with the new TDGunboat trait, and AttackTurreted with AttackTDGunboatTurreted.

AircraftCanHoverGeneralization: Split Aircraft.CanHover into multiple parameters
   Updating mod... COMPLETE
   Updating system maps... COMPLETE

ScaleDefaultModHealth: Scale health and damage in the default OpenRA mods
   Updating mod... COMPLETE
   Updating system maps... COMPLETE
   Manual changes are required to complete this update:
    * Health and damage values have been muliplied by a factor of 100.
      The increased calculation precision will affect game balance and may need to be manually adjusted.

    * Map: /Users/paul/src/OpenRA/mods/cnc2/maps/funpark01:
       * Health and damage values have been muliplied by a factor of 100.
         The increased calculation precision will affect game balance and may need to be manually adjusted.

    * Map: /Users/paul/src/OpenRA/mods/cnc2/maps/gdi01:
       * Health and damage values have been muliplied by a factor of 100.
         The increased calculation precision will affect game balance and may need to be manually adjusted.

    * Map: /Users/paul/src/OpenRA/mods/cnc2/maps/gdi04a:
       * Health and damage values have been muliplied by a factor of 100.
         The increased calculation precision will affect game balance and may need to be manually adjusted.

    * Map: /Users/paul/src/OpenRA/mods/cnc2/maps/gdi04b:
       * Health and damage values have been muliplied by a factor of 100.
         The increased calculation precision will affect game balance and may need to be manually adjusted.

    * Map: /Users/paul/src/OpenRA/mods/cnc2/maps/gdi04c:
       * Health and damage values have been muliplied by a factor of 100.
         The increased calculation precision will affect game balance and may need to be manually adjusted.

    * Map: /Users/paul/src/OpenRA/mods/cnc2/maps/gdi05a:
       * Health and damage values have been muliplied by a factor of 100.
         The increased calculation precision will affect game balance and may need to be manually adjusted.

    * Map: /Users/paul/src/OpenRA/mods/cnc2/maps/gdi06:
       * Health and damage values have been muliplied by a factor of 100.
         The increased calculation precision will affect game balance and may need to be manually adjusted.

    * Map: /Users/paul/src/OpenRA/mods/cnc2/maps/nod07c:
       * Health and damage values have been muliplied by a factor of 100.
         The increased calculation precision will affect game balance and may need to be manually adjusted.

    * Map: /Users/paul/src/OpenRA/mods/cnc2/maps/nod08a:
       * Health and damage values have been muliplied by a factor of 100.
         The increased calculation precision will affect game balance and may need to be manually adjusted.

    * Map: /Users/paul/src/OpenRA/mods/cnc2/maps/nod08b:
       * Health and damage values have been muliplied by a factor of 100.
         The increased calculation precision will affect game balance and may need to be manually adjusted.

    * Map: /Users/paul/src/OpenRA/mods/cnc2/maps/nod09:
       * Health and damage values have been muliplied by a factor of 100.
         The increased calculation precision will affect game balance and may need to be manually adjusted.

    * Map: /Users/paul/src/OpenRA/mods/cnc2/maps/the-hot-box:
       * Health and damage values have been muliplied by a factor of 100.
         The increased calculation precision will affect game balance and may need to be manually adjusted.


Semi-automated update complete.
Please review the messages above for any manual actions that must be applied.



md5-011097f575d5b4821233e099d9866f18



paul@Ragnarok:OpenRA+upgrade-rules *$ mono --debug OpenRA.Utility.exe ra --update-mod ScaleDefaultModHealth --detailed --apply
WARNING: This command will automatically rewrite your mod rules.
Side effects of this command may include changing the whitespace to 
match the default conventions, and any yaml comments will be removed.

We strongly recommend that you have a backup of your mod rules, and 
for best results, to use a Git client to review the line-by-line 
changes and discard any unwanted side effects.

Press y to continue, or any other key to cancel: y

ScaleDefaultModHealth: Scale health and damage in the default OpenRA mods
   Updating mod... COMPLETE
   Updating system maps... FAILED

   The automated changes for this rule were not applied because of an error.
   After the issue reported below is resolved you should run the updater
   with SOURCE set to ScaleDefaultModHealth to retry these changes

   The map that caused the error was:
     /Users/paul/src/OpenRA/mods/ra/maps/foobar

   The exception reported was:
     System.ArgumentNullException: Value cannot be null.
     Parameter name: stream
       at System.IO.StreamReader..ctor (System.IO.Stream stream, System.Text.Encoding encoding, System.Boolean detectEncodingFromByteOrderMarks, System.Int32 bufferSize, System.Boolean leaveOpen) [0x0000c] in /Users/builder/data/lanes/5533/mono-mac-sdk/external/bockbuild/builds/mono-x64/mcs/class/referencesource/mscorlib/system/io/streamreader.cs:166 
       at System.IO.StreamReader..ctor (System.IO.Stream stream, System.Boolean detectEncodingFromByteOrderMarks) [0x00000] in /Users/builder/data/lanes/5533/mono-mac-sdk/external/bockbuild/builds/mono-x64/mcs/class/referencesource/mscorlib/system/io/streamreader.cs:138 
       at System.IO.StreamReader..ctor (System.IO.Stream stream) [0x00000] in /Users/builder/data/lanes/5533/mono-mac-sdk/external/bockbuild/builds/mono-x64/mcs/class/referencesource/mscorlib/system/io/streamreader.cs:134 
       at (wrapper remoting-invoke-with-check) System.IO.StreamReader..ctor(System.IO.Stream)
       at OpenRA.MiniYaml.FromStream (System.IO.Stream s, System.String fileName) [0x00002] in /Users/paul/src/OpenRA/OpenRA.Game//MiniYaml.cs:246 
       at OpenRA.Mods.Common.UtilityCommands.UpdateModCommand.UpdateMap (OpenRA.ModData modData, OpenRA.FileSystem.IReadWritePackage mapPackage, OpenRA.Mods.Common.UpdateRules.UpdateRule rule, System.Collections.Generic.List`1[System.Tuple`3[OpenRA.FileSystem.IReadWritePackage,System.String,System.Collections.Generic.List`1[OpenRA.MiniYamlNode]]]& files) [0x00026] in /Users/paul/src/OpenRA/OpenRA.Mods.Common//UtilityCommands/UpdateModCommand.cs:116 
       at OpenRA.Mods.Common.UtilityCommands.UpdateModCommand.ApplyRules (OpenRA.ModData modData, System.Collections.Generic.IEnumerable`1[T] rules) [0x001c9] in /Users/paul/src/OpenRA/OpenRA.Mods.Common//UtilityCommands/UpdateModCommand.cs:199 
Semi-automated update complete.
Please review the messages above for any manual actions that must be applied.

Note that in this last case that no files are updated - changes are only saved if the rule successfully runs on all files (including system maps).

[pchote]

14964 has been merged, but i would like to keep this open until the updates referenced above have been ported over.

[pchote]

I have split the PR list in my comment above into groups so we can clearly see the ones implemented in #15010, the ones that will be listed on the ModSDK update notes wiki page, and the ones we are still missing rules for.

[pchote]

15010 has been merged.