Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Overhaul chrome.yaml and panel rendering #17489

Merged
merged 4 commits into from Dec 28, 2019
Merged

Conversation

@pchote
Copy link
Member

pchote commented Dec 21, 2019

This PR tackles a major refactoring of ChromeProvider and the chrome.yaml definitions with two goals:

  • Allowing non-region metadata to be defined on image collections
  • Removing excessive duplication using Inherits and the new PanelRegion syntax.

This also fixes a bug with our MiniYaml comment parsing that impacts the new chrome.yaml update rule. The yaml changes in the second commit are from the update rule, with further manual cleanups in the remaining commits.

This prepares for #10382 by extracting defaults with the Image definitions for each collection - future steps add Image2x and Image3x definitions next to these for the higher resolution assets.

Once this and #16885 have been merged we will be able to switch ChromeProvider over to ISpriteLoader and introduce Palette and Frame fields to enable paletted and other custom mod formats (e.g. cps). This should cover @evgeniysergeev's requirements from #17379.

Supersedes #16774.

I plan to create a tool to visually inspect the chrome.yaml mappings, and will update this PR when its ready. I don't expect anybody to waste their time trying to manually verify the updated mapped regions, but this doesn't need to hold up review of the rest of the changes here.

@pchote pchote added this to the Next+1 milestone Dec 21, 2019
@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Dec 21, 2019

The chrome.yaml mappings can now be easily tested by cherry picking f549e79 and running the --debug-chrome-regions utility command.

Run e.g. OpenRA.Utility.exe cnc --debug-chrome-regions chrome.png 4 and open the generated chrome.html page in a web browser. The image regions are shown in yellow, and, if you enable your browser's web console, mousing over a region will print its name to the console log.

The code behind this command is a bit awful, so I'm not sure its worth the effort to try and merge it upstream.

Testing this PR revealed a couple of long-standing bugs in d2k, which I have fixed in the latest commit here.

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Dec 24, 2019

Otherwise looks good as far as I can tell.
I guess ideally some other @OpenRA/engine-hackers or @OpenRA/core should take a look, too (at least at the code), but given the sheer size of this PR it may be more effective to just merge it soon and hope we catch anything we may have overlooked in one of the follow-ups (or while working on unrelated PRs).

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Dec 24, 2019

To be fair, this PR is at its core actually quite small: most of the lines of actual lines of code changes are tied up in the new update rule, and (after verifying that the format is sensible) the details of the chrome.yaml changes can be visually inspected using the utility command 😄

@pchote pchote force-pushed the pchote:render-chromeprovider branch from 1406066 to 7dd983b Dec 24, 2019
@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Dec 24, 2019

Fixed and rebased.

@reaperrr reaperrr merged commit 7ccc63b into OpenRA:bleed Dec 28, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pchote pchote deleted the pchote:render-chromeprovider branch Jan 12, 2020
@Mailaender

This comment has been minimized.

Copy link
Member

Mailaender commented Jan 26, 2020

Just noticed the update rule crashing:

ReformatChromeProvider: Reformat UI image definitions.
   Updating mod... 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 ReformatChromeProvider to retry these changes

   The exception reported was:
     OpenRA.YamlException: FieldLoader: Cannot parse `chrome.png` into `Image.System.Int32` 
       at OpenRA.FieldLoader+<>c.<.cctor>b__27_0 (System.String s, System.Type t, System.String f) [0x00000] in /home/matthias/Entwicklung/OpenHV/engine/OpenRA.Game/FieldLoader.cs:60 
       at OpenRA.FieldLoader.GetValue (System.String fieldName, System.Type fieldType, OpenRA.MiniYaml yaml, System.Reflection.MemberInfo field) [0x00034] in /home/matthias/Entwicklung/OpenHV/engine/OpenRA.Game/FieldLoader.cs:192 
       at OpenRA.FieldLoader.GetValue (System.String fieldName, System.Type fieldType, System.String value, System.Reflection.MemberInfo field) [0x00000] in /home/matthias/Entwicklung/OpenHV/engine/OpenRA.Game/FieldLoader.cs:179 
       at OpenRA.FieldLoader.GetValue (System.String fieldName, System.Type fieldType, OpenRA.MiniYaml yaml, System.Reflection.MemberInfo field) [0x0083a] in /home/matthias/Entwicklung/OpenHV/engine/OpenRA.Game/FieldLoader.cs:453 
       at OpenRA.FieldLoader.GetValue (System.String fieldName, System.Type fieldType, System.String value, System.Reflection.MemberInfo field) [0x00000] in /home/matthias/Entwicklung/OpenHV/engine/OpenRA.Game/FieldLoader.cs:179 
       at OpenRA.FieldLoader.GetValue[T] (System.String field, System.String value) [0x00000] in /home/matthias/Entwicklung/OpenHV/engine/OpenRA.Game/FieldLoader.cs:169 
       at OpenRA.Mods.Common.UpdateRules.UpdateExtensions.NodeValue[T] (OpenRA.MiniYamlNode node) [0x00000] in /home/matthias/Entwicklung/OpenHV/engine/OpenRA.Mods.Common/UpdateRules/UpdateUtils.cs:262 
       at OpenRA.Mods.Common.UpdateRules.Rules.ReformatChromeProvider+<UpdateChromeProviderNode>d__9.MoveNext () [0x00062] in /home/matthias/Entwicklung/OpenHV/engine/OpenRA.Mods.Common/UpdateRules/Rules/20191117/ReformatChromeProvider.cs:194 
       at OpenRA.Mods.Common.UpdateRules.UpdateUtils+<ApplyTopLevelTransform>d__8.MoveNext () [0x000d6] in /home/matthias/Entwicklung/OpenHV/engine/OpenRA.Mods.Common/UpdateRules/UpdateUtils.cs:223 
       at System.Collections.Generic.List`1[T].AddEnumerable (System.Collections.Generic.IEnumerable`1[T] enumerable) [0x00059] in /home/abuild/rpmbuild/BUILD/mono-6.4.0.198/external/corefx/src/Common/src/CoreLib/System/Collections/Generic/List.cs:1108 
       at System.Collections.Generic.List`1[T].InsertRange (System.Int32 index, System.Collections.Generic.IEnumerable`1[T] collection) [0x000f4] in /home/abuild/rpmbuild/BUILD/mono-6.4.0.198/external/corefx/src/Common/src/CoreLib/System/Collections/Generic/List.cs:771 
       at System.Collections.Generic.List`1[T].AddRange (System.Collections.Generic.IEnumerable`1[T] collection) [0x00000] in /home/abuild/rpmbuild/BUILD/mono-6.4.0.198/external/corefx/src/Common/src/CoreLib/System/Collections/Generic/List.cs:257 
       at OpenRA.Mods.Common.UpdateRules.UpdateUtils.UpdateMod (OpenRA.ModData modData, 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, System.Collections.Generic.HashSet`1[T] externalFilenames) [0x002a2] in /home/matthias/Entwicklung/OpenHV/engine/OpenRA.Mods.Common/UpdateRules/UpdateUtils.cs:178 
       at OpenRA.Mods.Common.UtilityCommands.UpdateModCommand.ApplyRules (OpenRA.ModData modData, System.Collections.Generic.IEnumerable`1[T] rules, System.Boolean skipMaps) [0x00076] in /home/matthias/Entwicklung/OpenHV/engine/OpenRA.Mods.Common/UtilityCommands/UpdateModCommand.cs:177
@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jan 26, 2020

That will only happen if the yaml has a syntax error, where one image definition is incorrectly nested inside another image definition.

@Mailaender

This comment has been minimized.

Copy link
Member

Mailaender commented Jan 26, 2020

My https://github.com/Mailaender/OpenHV/blob/master/mods/hv/chrome.yaml is mostly RA minus factions and with changed offsets. No obvious errors in it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.