Overhaul sequence filename definitions

[pchote]

This is the first in a series of PRs aiming to remove technical debt from the sequence code and add features needed to finish and upstream remastered asset support.

All the magic around sprite filenames is removed in favour of explicitly specifying the full sprite name (per tileset if needed). This allows mods that don't follow the existing C&C conventions (e.g. by putting each tilesets assets in its own directory) to use tileset-specific artwork. It also prepares for a future PR that will add new ways of specifying filenames (e.g. FilenamePattern: mytank-{:D4}.png).

The Defaults node was previously combined into each sequence using MiniYaml.Merge at sequence-parsing time. This prevented us from implementing recursive merging at the MiniYaml level (see e.g. this complaint) because the raw sequence yaml may be malformed (e.g. trying to remove keys that don't exist) before the defaults have been copied over. This has been replaced with a much simpler fallback convention: the parser first looks for the requested key in the current sequence, and if not found it will look again in Defaults. Sequences can override (or more correctly, mask) the Defaults value by defining a key with no value.

I suggest reviewing by commit, and reading the commit descriptions for more information.
The yaml changes in the third commit are fully automated, so it shouldn't be necessary to inspect every single line in detail. A small number of manual cleanups are specified in the fourth commit.

[PunkPun]

Since we are changing everything up we may want a more user friendly exception for missing values. It doesn't mention where something is missing nor what

I deleted the filename field from MiG and got this

Exception of type `System.IO.FileNotFoundException`: File not found: 
   at OpenRA.Mods.Common.Graphics.FileNotFoundSequence.OpenRA.Graphics.ISpriteSequence.get_Tick() in /Users/gustas/Documents/GitHub/OpenRA/OpenRA.Mods.Common/Graphics/DefaultSpriteSequence.cs:line 112
   at OpenRA.Graphics.Animation.PlaySequence(String sequenceName) in /Users/gustas/Documents/GitHub/OpenRA/OpenRA.Game/Graphics/Animation.cs:line 126

Completely unhelpful to modders

[PunkPun]

The CheckSequences lint test (or a new one) could also be used to pick up on missing values

[pchote]

Fixed.

[PunkPun]

This is much better 👍. However it would be more ideal if lint test didn't display the tracelogs

[pchote]

The next PR completely overhauls the way that the sprite loading and lint tests work, so can we leave that for now?

[PunkPun]

sure

[PunkPun]

I tried updating Treasure_Hunt.oramap.zip and the update rule crashed

Crashlog

System.Collections.Generic.KeyNotFoundException: The given key 'SNOW' was not present in the dictionary.
        at OpenRA.Mods.Common.UpdateRules.Rules.ExplicitSequenceFilenames.ProcessNode(ModData modData, MiniYamlNode sequenceNode, MiniYamlNode resolvedSequenceNode, String imageName) in /Users/gustas/Documents/GitHub/OpenRA/OpenRA.Mods.Common/UpdateRules/Rules/20221203/ExplicitSequenceFilenames.cs:line 281
        at OpenRA.Mods.Common.UpdateRules.Rules.ExplicitSequenceFilenames.UpdateSequenceNode(ModData modData, MiniYamlNode imageNode)+MoveNext() in /Users/gustas/Documents/GitHub/OpenRA/OpenRA.Mods.Common/UpdateRules/Rules/20221203/ExplicitSequenceFilenames.cs:line 136
        at OpenRA.Mods.Common.UpdateRules.UpdateUtils.ApplyTopLevelTransform(ModData modData, List`1 files, TopLevelNodeTransform transform)+MoveNext() in /Users/gustas/Documents/GitHub/OpenRA/OpenRA.Mods.Common/UpdateRules/UpdateUtils.cs:line 292
        at System.Collections.Generic.List`1.InsertRange(Int32 index, IEnumerable`1 collection)
        at System.Collections.Generic.List`1.AddRange(IEnumerable`1 collection)
        at OpenRA.Mods.Common.UpdateRules.UpdateUtils.UpdateMap(ModData modData, IReadWritePackage mapPackage, UpdateRule rule, List`1& files, HashSet`1 externalFilenames) in /Users/gustas/Documents/GitHub/OpenRA/OpenRA.Mods.Common/UpdateRules/UpdateUtils.cs:line 143
        at OpenRA.Mods.Common.UtilityCommands.UpdateMapCommand.ApplyRules(ModData modData, IReadWritePackage mapPackage, IEnumerable`1 rules) in /Users/gustas/Documents/GitHub/OpenRA/OpenRA.Mods.Common/UtilityCommands/UpdateMapCommand.cs:line 108

[pchote]

The update rule requires the legacy tileset metadata (which is removed in the last commit) to be defined in mod.yaml. The map updates fine if you add that back.

[PunkPun]

Seems less than ideal. Update rules are also meant for maps, I'd even say that they're used for maps more than for mods. We should handle the crashes

[pchote]

I can’t think of an acceptable way to do that - keeping unused junk data in mod.yaml isn’t acceptable, and hardcoding the upgrade rule with data for only the upstream mods isn’t really either.

[abcdefg30]

The update rule shouldn't crash but write a proper error message - explaining what is written in the comments above: That it needs the definitions in mod.yaml and that those can be removed (again) afterwards.

[pchote]

Updated.

ExplicitSequenceFilenames: Sequence filenames must be specified explicitly.
   Updating map... COMPLETE
   Manual changes are required to complete this update:
    * The ExplicitSequenceFilenames rule requires TilesetExtensions
      to be defined under the SpriteSequenceFormat definition in mod.yaml.
      Add these definitions back and run the update rule again.

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

[PunkPun]

In the future we should make it clearer when update rules fail / require manual changes. Currently that information is fairly hard to find within walls of text.

I did run this rule on some other fairly artwork heavy maps and it seems to function well

[PunkPun]

LGTM

[PunkPun]

Could there be anything done to the resource centre update function to account for the missing definitions?

[pchote]

Yes, the definitions can be added back to mod.yaml on the RC engines.

[PunkPun]

Needs a rebase / copyright header update

[pchote]

Rebased.

[penev92]

The naming may or may not just be me, but there is at least one crash to fix.

[PunkPun]

https://github.com/OpenRA/OpenRA/wiki/Sequences
will need to be updated after the merge

[pchote]

Updated.

[Mailaender]

--check-sprite-sequences clean. No problems in-game.

[pchote]

Updated.

[penev92]

👍

[penev92]

Changelog