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

Upgrade maps without parsing ruleset #14948

Merged
merged 1 commit into from Mar 21, 2018

Conversation

Projects
None yet
3 participants
@pchote
Copy link
Member

pchote commented Mar 19, 2018

Creating a Map object throws an exception if the mod's default Ruleset is not valid. This is a good thing in most cases, but is a major blocker for #14600 - even an innocuous error (from a yaml syntax perspective) like a missing required key is enough to completely break the upgrade process.

I expect that @reaperrr, @abcdefg30, and others will be familiar with this frustration, but for completeness I have included a testcase that demonstrates the issue when cherry-picked to bleed.

This sidesteps the crash by applying the upgrade rules directly to the map yaml without first parsing a Map. This has an extra side-effect of significantly improving the upgrade rule run time.

@pchote pchote force-pushed the pchote:upgrade-maps-without-parsing-ruleset branch from a028608 to 0c3f49c Mar 19, 2018

@abcdefg30
Copy link
Member

abcdefg30 left a comment

Looks good to me otherwise.

UpgradeRules.UpgradePlayers(modData, engineDate, ref map.PlayerDefinitions, null, 0);
UpgradeRules.UpgradeActors(modData, engineDate, ref map.ActorDefinitions, null, 0);
map.Save(package);
var yaml = new MiniYaml(null, MiniYaml.FromStream(package.GetStream("map.yaml"), package.Name));

This comment has been minimized.

@abcdefg30

abcdefg30 Mar 19, 2018

Member

This will crash with an NRE later on if there is no map.yaml file present in the directory. Can we get a proper error message here instead? (Or ignore such maps/folders.)

Apply map upgrade rules without instantiating the Map object.
This avoids a crash that stops the maps from being updated
when the base mod ruleset is not valid (e.g. a trait is missing a
FieldLoader.Required property).  This also significantly improves the
upgrade performance.

@pchote pchote force-pushed the pchote:upgrade-maps-without-parsing-ruleset branch from 0c3f49c to dbe7d67 Mar 20, 2018

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Mar 20, 2018

Good catch, fixed.

processYaml(modData, engineDate, ref fileNodes, null, 0);

// HACK: Obtain the writable save path using knowledge of the underlying filesystem workings
var packagePath = filename;
var package = map.Package;
var p = package;

This comment has been minimized.

@MunWolf

MunWolf Mar 20, 2018

Contributor

Any reason why you don't use the package parameter directly here instead of the p variable?

This comment has been minimized.

@pchote

pchote Mar 20, 2018

Author Member

This is inside a loop, and each invokation needs access to the original package value.

@pchote pchote force-pushed the pchote:upgrade-maps-without-parsing-ruleset branch from dbe7d67 to 54ee9c4 Mar 21, 2018

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Mar 21, 2018

Rebased and removed test case. I count 2 👍's counting my own implicit one, but I would prefer @abcdefg30 or another @OpenRA/engine-hackers to re-approve instead of overriding the merge approval.

@abcdefg30 abcdefg30 merged commit 8ea1da1 into OpenRA:bleed Mar 21, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Mar 21, 2018

@pchote pchote deleted the pchote:upgrade-maps-without-parsing-ruleset branch Apr 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.