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

Add more update rules #15010

Merged
merged 9 commits into from Jul 22, 2018

Conversation

Projects
None yet
4 participants
@abcdefg30
Copy link
Member

abcdefg30 commented Mar 30, 2018

Another step towards completing #14600.

@abcdefg30 abcdefg30 force-pushed the abcdefg30:upgrades branch from e0211e4 to 01429cf Mar 30, 2018

{
public class AddNukeLaunchAnimation : UpdateRule
{
public override string Name { get { return "Add 'WithNukeLaunchAnimation' and remove 'ActivationSequence'"; } }

This comment has been minimized.

@pchote

pchote Mar 30, 2018

Member

These should mention NukePower.ActivationSequence so users know what trait you are talking about.

@@ -34,6 +34,19 @@ public class UpdatePath
Justification = "Extracting update lists to temporary variables obfuscates the definitions.")]
static readonly UpdatePath[] Paths =
{
new UpdatePath("release-20171014", "release-20180218", new UpdateRule[]

This comment has been minimized.

@pchote

pchote Mar 30, 2018

Member

Can we please comment out this block or maybe rename it to incomplete-release-20171014 to make it clear that this is not ready yet?

{
foreach (var nuke in actorNode.ChildrenMatching("NukePower"))
{
var sequence = nuke.LastChildMatching("ActivationSequence").Value.Value;

This comment has been minimized.

@pchote

pchote Mar 30, 2018

Member

.NodeValue<string>();

var allowAllies = node.LastChildMatching("AllowAllies");
if (allowAllies != null)
{
if (FieldLoader.GetValue<bool>("AllowAllies", allowAllies.Value.Value))

This comment has been minimized.

@pchote

pchote Mar 30, 2018

Member

if (allowAllies.NodeValue<bool>()) and so on.

}

if (stance != (Stance.Neutral | Stance.Enemy))
actorNode.AddNode("ValidStances", stance.ToString());

This comment has been minimized.

@pchote

pchote Mar 30, 2018

Member

actorNode.AddNode("ValidStances", stance); (the ToString isn't needed).

var reloadCount = pool.LastChildMatching("ReloadCount");
if (reloadCount != null)
{
var rc = reloadCount;

This comment has been minimized.

@pchote

pchote Mar 30, 2018

Member

This appears to be redundant.

var selfReloadDelay = pool.LastChildMatching("SelfReloadDelay");
if (selfReloadDelay != null)
{
var rd = selfReloadDelay;

This comment has been minimized.

@pchote

pchote Mar 30, 2018

Member

This appears to be redundant.

var rearmSound = pool.LastChildMatching("RearmSound");
if (rearmSound != null)
{
var rs = rearmSound;

This comment has been minimized.

@pchote

pchote Mar 30, 2018

Member

This appears to be redundant.

ppfct.AddNode("Tileset", "");
ppfct.RenameKeyPreservingSuffix("PaletteFromFile");
yield return ppfct.Location + ": The trait 'PlayerPaletteFromCurrentTileset'\n" +
"has been removed. Use 'PaletteFromFile' with a Tileset filter.";

This comment has been minimized.

@pchote

pchote Mar 30, 2018

Member

"has been replaced by 'PaletteFromFile'. The trait has been renamed for you, but you will need to update the definition to specify the correct filename and tileset filters"


namespace OpenRA.Mods.Common.UpdateRules.Rules
{
public class ScaleDefaultModHealth : UpdateRule

This comment has been minimized.

@pchote

pchote Mar 30, 2018

Member

I'm not sure how useful this is as it is currently written - we do need to have rules keyed on mod ID for the resource center, but this won't run at all for other mods. It would be better if we could have a base ScaleModHealth superclass that takes the scaling factor in the ctor, a pair of ScaleModHealthBy10 and ScaleModHealthBy100 subclasses that can be called manually by any mod, and then a ScaleDefaultModHealth subclass that then uses ModScales to pick the scaling value or skips if non-default.

@abcdefg30 abcdefg30 force-pushed the abcdefg30:upgrades branch 2 times, most recently from 46b87ff to 222b874 Mar 30, 2018

@abcdefg30

This comment has been minimized.

Copy link
Member Author

abcdefg30 commented Mar 30, 2018

Updated. Didn't test the rules yet, though.

{
{ "Health", new[] { Pair.New("HP", string.Empty) } },
{ "SelfHealing", new[] { Pair.New("Step", "500") } },
{ "RepairsUnits", new[] { Pair.New("HP", "1000") } },

This comment has been minimized.

@MustaphaTR

MustaphaTR Apr 23, 2018

Member

This should be HpPerStep.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Apr 28, 2018

Can you please take a look at #15090 and amend these to match?

@pchote pchote added this to the Next release milestone May 4, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented May 20, 2018

Ping @abcdefg30

new CapturableChanges(),
new DecoupleSelfReloading(),
new RemovePlayerPaletteTileset(),
new RemoveWithReloadingSpriteTurret(),

This comment has been minimized.

@reaperrr

reaperrr May 20, 2018

Contributor

This must be a copy-paste error, this rule shouldn't be in this update path (it's part of the release-20180307 -> upcoming release path).

@reaperrr reaperrr force-pushed the abcdefg30:upgrades branch from 222b874 to 9747653 May 21, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented May 21, 2018

I was a little bored and working on update rules anyway, so I

  • rebased
  • fixed the issues me and @MustaphaTR pointed out
  • added a relatively simple update rule for the super weapon seconds-to-ticks conversion (and yes, I tested it)
get
{
return "'SelfReloads', 'SelfReloadDelay', 'ReloadCount', 'ResetOnFire'\n" +
"and 'RearmSound' were renamed and moved to a new 'ReloadAmmoPool' trait.";

This comment has been minimized.

@reaperrr

reaperrr May 21, 2018

Contributor

Neither Name above, nor this explicitly mentions they were moved from AmmoPool. Maybe just insert from 'AmmoPool' between the "move" and "to" here.

This comment has been minimized.

@abcdefg30

abcdefg30 May 22, 2018

Author Member

Added.

foreach (var pool in ammoPools)
{
var selfReloads = pool.LastChildMatching("SelfReloads");
if (selfReloads == null || selfReloads.NodeValue<bool>())

This comment has been minimized.

@reaperrr

reaperrr May 21, 2018

Contributor

If .NodeValue<bool>() simply returns the value, isn't this missing an ! in front, as we only want to continue here if the the pool is not self-reloading?

This comment has been minimized.

@abcdefg30

abcdefg30 May 22, 2018

Author Member

Ops, nice catch. Thanks.

{
selfReloadDelay.RenameKeyPreservingSuffix("Delay");
reloadOnCond.AddNode(selfReloadDelay);
pool.RemoveNodes("SelfReloads");

This comment has been minimized.

@reaperrr

reaperrr May 21, 2018

Contributor

Why do you only remove "SelfReloads" if selfReloadDelay != null? It might have been using the internal default. I'd put this right after the 'continue' in the beginning of the loop, as we don't need it afterwards.

This comment has been minimized.

@abcdefg30

abcdefg30 May 22, 2018

Author Member

I copied that from the original rule. Moved it out, though.

@abcdefg30 abcdefg30 force-pushed the abcdefg30:upgrades branch 4 times, most recently from 686be20 to 17cb7b2 May 22, 2018

@abcdefg30

This comment has been minimized.

Copy link
Member Author

abcdefg30 commented May 22, 2018

Updated, all rules are ported now.

@abcdefg30 abcdefg30 force-pushed the abcdefg30:upgrades branch from 17cb7b2 to 2aa63b6 May 22, 2018


namespace OpenRA.Mods.Common.UpdateRules.Rules
{
public class SplitSelectionAndRenderSize : UpdateRule

This comment has been minimized.

@reaperrr

reaperrr May 22, 2018

Contributor

We dropped/replaced these traits entirely with @pchote's bounds refactor, the old update rule was only valid if run in that specific order.
You should merge this into the MoveVisualBounds update rule which depends on this, we definitely want this as a single rule to avoid confusion and yaml conflicts.

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented May 22, 2018

I haven't really looked at them yet, but iirc the power refactor, IDisable removal etc. kind of depend on each other, so we have to make sure their rules don't screw up anything when run in arbitrary order.

@reaperrr reaperrr force-pushed the abcdefg30:upgrades branch from 4147784 to ee23230 Jul 4, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Jul 4, 2018

Updated, also moved legacy rules into 20171014 subfolder as discussed on IRC.

@pchote
Copy link
Member

pchote left a comment

Almost there IMO.

I noticed a few issues when printing the rule details and running them on the bleed version.

get
{
return "'RequiresPower' has been replaced with 'GrantConditionOnPowerState' which\n" +
"toggles a condition depending on the power state.\n Possible PowerStates are: Normal " +

This comment has been minimized.

@pchote

pchote Jul 4, 2018

Member

Stray space between the newline and "Possible" looks odd:

  * ReplaceRequiresPower: Replace 'RequiresPower' with 'GrantConditionOnPowerState'
     'RequiresPower' has been replaced with 'GrantConditionOnPowerState' which
     toggles a condition depending on the power state.
      Possible PowerStates are: Normal (0 or positive), Low (negative but higher than 50% of required power)
     and Critical (below Low).
{
return "'RequiresPower' has been replaced with 'GrantConditionOnPowerState' which\n" +
"toggles a condition depending on the power state.\n Possible PowerStates are: Normal " +
"(0 or positive), Low (negative but higher than 50% of required power)\nand Critical (below Low).";

This comment has been minimized.

@pchote

pchote Jul 4, 2018

Member

Would look better if the newline was moved from before the "and" to before the "50%"


public override IEnumerable<string> AfterUpdate(ModData modData)
{
var message = "Due to time constraints and the legacy status of the included rules,\n" +

This comment has been minimized.

@pchote

pchote Jul 4, 2018

Member

This message is currently repeated for all maps too!
Please add a flag to only display it the first time.

if (anyMatchingArmament)
{
pool.AddNode(ammoNoAmmo);
yield return "Aircraft returning to base is now triggered when all armaments are paused via condition.\n" +

This comment has been minimized.

@pchote

pchote Jul 4, 2018

Member

This message is repeated with identical messages for each match. Please either report it only once for all actors, or, better, push the actor name/file to a list that we then report together at the end.

@reaperrr reaperrr force-pushed the abcdefg30:upgrades branch from ee23230 to 653397f Jul 4, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Jul 4, 2018

Updated.

Regarding the Travis failure, I checked code style locally and it passed fine.

@abcdefg30

This comment has been minimized.

Copy link
Member Author

abcdefg30 commented Jul 5, 2018

Travis passes now.

if (pal != null)
canPowerDown.RemoveNodes("IndicatorPalette");

yield return "'CanPowerDown' now provides a condition. You might need to review your condition setup.";

This comment has been minimized.

@pchote

pchote Jul 5, 2018

Member

Repeated messages:

ChangeCanPowerDown: Provide a condition in 'CanPowerDown' instead of using 'Actor.Disabled'
   Updating mod... COMPLETE
   Updating system maps... COMPLETE
   Manual changes are required to complete this update:
    * 'CanPowerDown' now provides a condition. You might need to review your condition setup.
    * 'CanPowerDown' now provides a condition. You might need to review your condition setup.
    * 'CanPowerDown' now provides a condition. You might need to review your condition setup.
    * 'CanPowerDown' now provides a condition. You might need to review your condition setup.
    * 'CanPowerDown' now provides a condition. You might need to review your condition setup.
    * 'CanPowerDown' now provides a condition. You might need to review your condition setup.
    * 'CanPowerDown' now provides a condition. You might need to review your condition setup.
    * 'CanPowerDown' now provides a condition. You might need to review your condition setup.
requiresPower.AddNode("Condition", "lowpower");
requiresPower.AddNode("ValidPowerStates", "Low, Critical");

yield return "'RequiresPower' was renamed to 'GrantConditionOnPowerState'.\n" +

This comment has been minimized.

@pchote

pchote Jul 5, 2018

Member

Repeated messages:

ReplaceRequiresPower: Replace 'RequiresPower' with 'GrantConditionOnPowerState'
   Updating mod... COMPLETE
   Updating system maps... COMPLETE
   Manual changes are required to complete this update:
    * 'RequiresPower' was renamed to 'GrantConditionOnPowerState'.
      You might need to review your condition setup.
    * 'RequiresPower' was renamed to 'GrantConditionOnPowerState'.
      You might need to review your condition setup.
    * 'RequiresPower' was renamed to 'GrantConditionOnPowerState'.
      You might need to review your condition setup.
    * 'RequiresPower' was renamed to 'GrantConditionOnPowerState'.
      You might need to review your condition setup.
    * 'RequiresPower' was renamed to 'GrantConditionOnPowerState'.
      You might need to review your condition setup.
    * 'RequiresPower' was renamed to 'GrantConditionOnPowerState'.
      You might need to review your condition setup.
    * 'RequiresPower' was renamed to 'GrantConditionOnPowerState'.
      You might need to review your condition setup.
    * 'RequiresPower' was renamed to 'GrantConditionOnPowerState'.
      You might need to review your condition setup.
    * 'RequiresPower' was renamed to 'GrantConditionOnPowerState'.
      You might need to review your condition setup.
    * 'RequiresPower' was renamed to 'GrantConditionOnPowerState'.
      You might need to review your condition setup.
    * 'RequiresPower' was renamed to 'GrantConditionOnPowerState'.
      You might need to review your condition setup.
    * 'RequiresPower' was renamed to 'GrantConditionOnPowerState'.
      You might need to review your condition setup.
    * 'RequiresPower' was renamed to 'GrantConditionOnPowerState'.
      You might need to review your condition setup.

{
foreach (var doc in actorNode.ChildrenMatching("DisableOnCondition"))
{
yield return doc.Location + "Actor.IsDisabled has been removed in favor of pausing/disabling traits via conditions.\n" +

This comment has been minimized.

@pchote

pchote Jul 5, 2018

Member

This and the loop below both refer to specific line numbers (which we now know is bad practice) and have a spacing error:

RemoveIDisable: Remove 'IDisabled'
   Updating mod... COMPLETE
   Updating system maps... COMPLETE
   Manual changes are required to complete this update:
    * rules/defaults.yaml:1084Actor.IsDisabled has been removed in favor of pausing/disabling traits via conditions.
      GrantConditionOnDisabled was a stop-gap solution that has been removed along with it.
      You'll have to use RequiresCondition or PauseOnCondition on individual traits to 'disable' actors.
    * rules/defaults.yaml:1091Actor.IsDisabled has been removed in favor of pausing/disabling traits via conditions.
      DisableOnCondition was a stop-gap solution that has been removed along with it.
      You'll have to use RequiresCondition or PauseOnCondition on individual traits to 'disable' actors.
    * rules/structures.yaml:685Actor.IsDisabled has been removed in favor of pausing/disabling traits via conditions.
      GrantConditionOnDisabled was a stop-gap solution that has been removed along with it.
      You'll have to use RequiresCondition or PauseOnCondition on individual traits to 'disable' actors.

@reaperrr reaperrr force-pushed the abcdefg30:upgrades branch from 653397f to d3d7e0d Jul 6, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Jul 6, 2018

Updated.

@pchote
Copy link
Member

pchote left a comment

One last crash, then I think this will be okay enough to ship:

pool.RemoveNodes("RearmSound");
}

actorNode.AddNode(reloadOnCond);

This comment has been minimized.

@pchote

pchote Jul 7, 2018

Member
DecoupleSelfReloading: Replace 'SelfReloads' with 'ReloadAmmoPool'
   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 DecoupleSelfReloading to retry these changes

   The exception reported was:
     System.InvalidOperationException: Collection was modified; enumeration operation may not execute.
       at System.ThrowHelper.ThrowInvalidOperationException (System.ExceptionResource resource) [0x00000] in /Users/builder/jenkins/workspace/build-package-osx-mono/2017-12/external/bockbuild/builds/mono-x64/mcs/class/referencesource/mscorlib/system/throwhelper.cs:104 
       at System.Collections.Generic.List`1+Enumerator[T].MoveNextRare () [0x00013] in <bb7b695b8c6246b3ac1646577aea7650>:0 
       at System.Collections.Generic.List`1+Enumerator[T].MoveNext () [0x0004a] in <bb7b695b8c6246b3ac1646577aea7650>:0 
       at System.Linq.Enumerable+WhereListIterator`1[TSource].MoveNext () [0x0004e] in /Users/builder/jenkins/workspace/build-package-osx-mono/2017-12/external/bockbuild/builds/mono-x64/external/corefx/src/System.Linq/src/System/Linq/Where.cs:348 
       at OpenRA.Mods.Common.UpdateRules.Rules.DecoupleSelfReloading+<UpdateActorNode>c__Iterator0.MoveNext () [0x001ab] in /Users/paul/src/OpenRA/OpenRA.Mods.Common//UpdateRules/Rules/20171014/DecoupleSelfReloading.cs:32 
       at OpenRA.Mods.Common.UpdateRules.UpdateUtils+<ApplyTopLevelTransform>c__Iterator3.MoveNext () [0x00127] in /Users/paul/src/OpenRA/OpenRA.Mods.Common//UpdateRules/UpdateUtils.cs:220 
       at System.Collections.Generic.List`1[T].InsertRange (System.Int32 index, System.Collections.Generic.IEnumerable`1[T] collection) [0x000ea] in <bb7b695b8c6246b3ac1646577aea7650>:0 
       at System.Collections.Generic.List`1[T].AddRange (System.Collections.Generic.IEnumerable`1[T] collection) [0x00000] in <bb7b695b8c6246b3ac1646577aea7650>:0 
       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) [0x00265] in /Users/paul/src/OpenRA/OpenRA.Mods.Common//UpdateRules/UpdateUtils.cs:173 
       at OpenRA.Mods.Common.UtilityCommands.UpdateModCommand.ApplyRules (OpenRA.ModData modData, System.Collections.Generic.IEnumerable`1[T] rules, System.Boolean skipMaps) [0x00081] in /Users/paul/src/OpenRA/OpenRA.Mods.Common//UtilityCommands/UpdateModCommand.cs:152 

This either needs to move outside the loop, or ammoPools needs to be ToList()ed so that you aren't enumerating the same collection you're modifying.

This comment has been minimized.

@reaperrr

reaperrr Jul 8, 2018

Contributor

Fixed by going the ToList() route.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jul 7, 2018

@abcdefg30: Will you want to give this a last look over before we merge, or shall we merge immediately once the issue above is fixed?

@reaperrr reaperrr force-pushed the abcdefg30:upgrades branch 2 times, most recently from 5bdd6d5 to ad6b7c2 Jul 8, 2018


var sequence = activation.NodeValue<string>();
nuke.RemoveNode(activation);
actorNode.AddNode("WithNukeLaunchAnimation", "");

This comment has been minimized.

@pchote

pchote Jul 8, 2018

Member

Same issue as before. Discovered when running the update rules on D2K:

AddNukeLaunchAnimation: Add 'WithNukeLaunchAnimation' and remove 'NukePower.ActivationSequence'
   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 AddNukeLaunchAnimation to retry these changes

   The exception reported was:
     System.InvalidOperationException: Collection was modified; enumeration operation may not execute.
       at System.ThrowHelper.ThrowInvalidOperationException (System.ExceptionResource resource) [0x0000b] in <bb7b695b8c6246b3ac1646577aea7650>:0 
       at System.Collections.Generic.List`1+Enumerator[T].MoveNextRare () [0x00013] in <bb7b695b8c6246b3ac1646577aea7650>:0 
       at System.Collections.Generic.List`1+Enumerator[T].MoveNext () [0x0004a] in <bb7b695b8c6246b3ac1646577aea7650>:0 
       at System.Linq.Enumerable+WhereListIterator`1[TSource].MoveNext () [0x0004e] in <b29fa38c592649ee8d566c045a7d9307>:0 
       at OpenRA.Mods.Common.UpdateRules.Rules.AddNukeLaunchAnimation+<UpdateActorNode>c__Iterator0.MoveNext () [0x000a1] in <300dcefade434fd18783a65f41b5ec39>:0 
       at OpenRA.Mods.Common.UpdateRules.UpdateUtils+<ApplyTopLevelTransform>c__Iterator3.MoveNext () [0x00121] in <300dcefade434fd18783a65f41b5ec39>:0 
       at System.Collections.Generic.List`1[T].InsertRange (System.Int32 index, System.Collections.Generic.IEnumerable`1[T] collection) [0x000ea] in <bb7b695b8c6246b3ac1646577aea7650>:0 
       at System.Collections.Generic.List`1[T].AddRange (System.Collections.Generic.IEnumerable`1[T] collection) [0x00000] in <bb7b695b8c6246b3ac1646577aea7650>:0 
       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) [0x00265] in <300dcefade434fd18783a65f41b5ec39>:0 
       at OpenRA.Mods.Common.UtilityCommands.UpdateModCommand.ApplyRules (OpenRA.ModData modData, System.Collections.Generic.IEnumerable`1[T] rules, System.Boolean skipMaps) [0x0007a] in <300dcefade434fd18783a65f41b5ec39>:0 

This comment has been minimized.

@pchote

pchote Jul 8, 2018

Member

I've checked for all other uses of AddNode in this PR and it looks like this is the last case to fix.

This comment has been minimized.

@reaperrr

reaperrr Jul 8, 2018

Contributor

Fixed.

@reaperrr reaperrr force-pushed the abcdefg30:upgrades branch from ad6b7c2 to d91d491 Jul 8, 2018

@pchote

pchote approved these changes Jul 8, 2018

@pchote pchote added the PR: Needs +2 label Jul 8, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jul 22, 2018

Taking this without a second 👍 so that we can move ahead with a playtest.

@pchote pchote merged commit c434a38 into OpenRA:bleed Jul 22, 2018

2 checks passed

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

@abcdefg30 abcdefg30 deleted the abcdefg30:upgrades branch Jul 22, 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.