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

Refactor low power/manual power-down actor disabling #13998

Merged
merged 10 commits into from Nov 13, 2017

Conversation

Projects
None yet
4 participants
@reaperrr
Contributor

reaperrr commented Sep 7, 2017

This aims to remove the biggest remaining roadblock towards getting rid of Actor.Disabled and the IDisable interface.

The idea is to no longer disable the actor as such, but only the traits that need to be disabled, via a condition (or multiple conditions) granted either at specific power levels (usually anything below Normal) or manual power down.

@reaperrr reaperrr added this to the Next + 1 milestone Sep 7, 2017

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Sep 7, 2017

Contributor

Note: Currently not working as intended (despite some first fixes suggested by @pchote), as affected actors are considered low power/disabled all the time, probably a bug somewhere in the PowerManager changes.

Only TS mod adapted on the yaml side so far, I first want to get the code debugged and the basic yaml approach approved before I adapt the other mods (this will also need an upgrade rule).

Contributor

reaperrr commented Sep 7, 2017

Note: Currently not working as intended (despite some first fixes suggested by @pchote), as affected actors are considered low power/disabled all the time, probably a bug somewhere in the PowerManager changes.

Only TS mod adapted on the yaml side so far, I first want to get the code debugged and the basic yaml approach approved before I adapt the other mods (this will also need an upgrade rule).

@reaperrr reaperrr requested review from pchote and atlimit8 Sep 7, 2017

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Sep 8, 2017

Contributor

Updated, but the lint errors suggest there's still something wrong with how conditions are granted, maybe the error actually lies in the Update methods. I haven't found the cause yet, though.

Contributor

reaperrr commented Sep 8, 2017

Updated, but the lint errors suggest there's still something wrong with how conditions are granted, maybe the error actually lies in the Update methods. I haven't found the cause yet, though.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Sep 8, 2017

Member

The lint errors look correct: actors that inherit ^DisabledOverlay but not ^CanPowerDown will consume powerdown which is not granted. The GrantCondition@DISABLED should be specifically overridden to add powerdown only on the actors that need it.

Member

pchote commented Sep 8, 2017

The lint errors look correct: actors that inherit ^DisabledOverlay but not ^CanPowerDown will consume powerdown which is not granted. The GrantCondition@DISABLED should be specifically overridden to add powerdown only on the actors that need it.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Sep 10, 2017

Contributor

Rebased and updated.

The TS yaml setup is fixed now, the only remaining issue is that buildings don't recover from disabled state when power becomes positive again, so either GrantConditionOnPowerState or PowerManager doesn't notify/update as intended.

Contributor

reaperrr commented Sep 10, 2017

Rebased and updated.

The TS yaml setup is fixed now, the only remaining issue is that buildings don't recover from disabled state when power becomes positive again, so either GrantConditionOnPowerState or PowerManager doesn't notify/update as intended.

Show outdated Hide outdated OpenRA.Game/Player.cs Outdated
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Sep 12, 2017

Contributor

Fixed the bug and adapted all mods.

This is ready for fully-fledged reviews now.

Contributor

reaperrr commented Sep 12, 2017

Fixed the bug and adapted all mods.

This is ready for fully-fledged reviews now.

Show outdated Hide outdated mods/ra/rules/structures.yaml Outdated
@pchote

Looking good overall, just a couple of nits on my first pass. Also needs a rebase unfortunately.

Show outdated Hide outdated OpenRA.Mods.Common/Traits/Power/CanPowerDown.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/Power/Player/PowerManager.cs Outdated
Show outdated Hide outdated mods/cnc/rules/structures.yaml Outdated
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Oct 6, 2017

Contributor

Rebased and updated.

Contributor

reaperrr commented Oct 6, 2017

Rebased and updated.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Oct 7, 2017

Contributor

Updated.

Contributor

reaperrr commented Oct 7, 2017

Updated.

@pchote

I also noticed the following issues with EMP in TS:

  • EMPed radar doesn't disable radar
  • EMPed laser fence nodes don't disable their connected segments

There may be other regressions in the other mods, but i'm sure they will be found and fixed in due time, so we probably shouldn't be too paranoid about that.

Show outdated Hide outdated OpenRA.Mods.Common/Traits/Conditions/GrantConditionOnPowerState.cs Outdated
Show outdated Hide outdated mods/d2k/rules/defaults.yaml Outdated

@pchote pchote removed the PR: Rebase me! label Nov 12, 2017

@pchote

With some more digging I found that the real problem wasn't with ValidPowerStates after all, but with buildings that were placed while the player was already in low power. Adding an Update call to Created fixed the problem.

I have fixed that, removed that // TODO:, rebased, and updated the upgrade rule date.

The code looks good and I was not able to spot any further regressions in any of the default mods.

@penev92

Some small nits to fix.

Show outdated Hide outdated OpenRA.Mods.Common/Traits/Power/CanPowerDown.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/Conditions/GrantConditionOnPowerState.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/Power/Player/PowerManager.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/Power/Player/PowerManager.cs Outdated
public override object Create(ActorInitializer init) { return new GrantConditionOnPowerState(init.Self, this); }
}
public class GrantConditionOnPowerState : ConditionalTrait<GrantConditionOnPowerStateInfo>, INotifyOwnerChanged, INotifyPowerLevelChanged

This comment has been minimized.

@penev92

penev92 Nov 13, 2017

Member

GrantConditionOnPowerStateChange, perhaps?

@penev92

penev92 Nov 13, 2017

Member

GrantConditionOnPowerStateChange, perhaps?

This comment has been minimized.

@reaperrr

reaperrr Nov 13, 2017

Contributor

The condition is provided as long as the power state is maintained, so I'd prefer to keep that name.
Edit: It's also consistent with blablaOnDamageState.

@reaperrr

reaperrr Nov 13, 2017

Contributor

The condition is provided as long as the power state is maintained, so I'd prefer to keep that name.
Edit: It's also consistent with blablaOnDamageState.

Show outdated Hide outdated mods/d2k/rules/structures.yaml Outdated

reaperrr added some commits Aug 4, 2017

Refactor PowerManager and RequiresPower to use conditions
Instead of Actor.IsDisabled.
Added INotifyPowerLevelChanged interface to do so as efficiently as possible.
Make WithSpriteBody a pausable trait
Allowing to drop the PauseAnimationWhenDisabled property (in favor of using PausOnCondition).
Adapt TD to low power/power-down refactor
Removed CanPowerDown from TD buildings.

Manual power-down is intentionally not supported by the in-game UI, the remaining CanPowerDown entries were effectively bit-rot.
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Nov 13, 2017

Contributor

Updated.

Contributor

reaperrr commented Nov 13, 2017

Updated.

@penev92

👍

@penev92 penev92 merged commit a7620c9 into OpenRA:bleed Nov 13, 2017

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Nov 13, 2017

Member

This needs a changelog entry!

Member

pchote commented Nov 13, 2017

This needs a changelog entry!

reaperrr added a commit to reaperrr/OpenRA that referenced this pull request Nov 15, 2017

reaperrr added a commit to reaperrr/OpenRA that referenced this pull request Nov 16, 2017

reaperrr added a commit to reaperrr/OpenRA that referenced this pull request Nov 18, 2017

reaperrr added a commit to reaperrr/OpenRA that referenced this pull request Nov 18, 2017

penev92 added a commit that referenced this pull request Nov 25, 2017

@reaperrr reaperrr deleted the reaperrr:refactor-PowerDisable branch Feb 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment