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

Remove IDisable - part 2 #12996

Merged
merged 11 commits into from May 7, 2017

Conversation

Projects
None yet
4 participants
@atlimit8
Member

atlimit8 commented Mar 19, 2017

This PR converts render traits from having a PauseOnLowPower property and support powers to being [pausable-]conditional (a subclass of PasuableConditional) and converts AffectedByPowerOutage to granting a condition. This continues #12955. Fixes #12797.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Apr 30, 2017

Contributor

Can be rebased now.

Contributor

reaperrr commented Apr 30, 2017

Can be rebased now.

@atlimit8 atlimit8 removed the PR: Rebase me! label May 1, 2017

@atlimit8

This comment has been minimized.

Show comment
Hide comment
@atlimit8

atlimit8 May 1, 2017

Member

Rebased.

Member

atlimit8 commented May 1, 2017

Rebased.

@pchote

Looks good overall. The "poweroutage" crash is fairly major, otherwise only a couple minor requests:

Show outdated Hide outdated OpenRA.Mods.Common/UtilityCommands/UpgradeRules.cs
Show outdated Hide outdated OpenRA.Mods.Cnc/Traits/SupportPowers/GpsPower.cs
Show outdated Hide outdated OpenRA.Mods.Common/Commands/DevCommands.cs
@@ -25,6 +25,7 @@ NAPOWR:
Range: 4c0
MaxHeightDelta: 3
WithIdleOverlay@LIGHTS:
RequiresCondition: !disabled

This comment has been minimized.

@pchote

pchote May 1, 2017

Member

Most other structures have blinking lights or other animations that should also be paused when disabled. Why stop with just the power plants?

@pchote

pchote May 1, 2017

Member

Most other structures have blinking lights or other animations that should also be paused when disabled. Why stop with just the power plants?

This comment has been minimized.

@atlimit8

atlimit8 May 1, 2017

Member

I have touched the structures using RequiresPower, but RequiresPower will have to grant a condition before it can be used for the other structures that must maintain some level of functionality.

@atlimit8

atlimit8 May 1, 2017

Member

I have touched the structures using RequiresPower, but RequiresPower will have to grant a condition before it can be used for the other structures that must maintain some level of functionality.

This comment has been minimized.

@pchote

pchote May 6, 2017

Member

I was thinking more about EMP, which already grants disabled. But ok, this can be done in a future PR.

@pchote

pchote May 6, 2017

Member

I was thinking more about EMP, which already grants disabled. But ok, this can be done in a future PR.

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach May 1, 2017

Contributor

I'd say this should be considered as the fix of #12797.

Contributor

GraionDilach commented May 1, 2017

I'd say this should be considered as the fix of #12797.

@atlimit8

This comment has been minimized.

Show comment
Hide comment
@atlimit8

atlimit8 May 1, 2017

Member

Changed.

Member

atlimit8 commented May 1, 2017

Changed.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 4, 2017

Contributor

Looks good to me 👍

Contributor

reaperrr commented May 4, 2017

Looks good to me 👍

@pchote

One last request, then LGTM

@atlimit8

This comment has been minimized.

Show comment
Hide comment
@atlimit8

atlimit8 May 6, 2017

Member

Fixed.

Member

atlimit8 commented May 6, 2017

Fixed.

@pchote pchote merged commit d787429 into OpenRA:bleed May 7, 2017

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@atlimit8 atlimit8 deleted the atlimit8:RemoveIDisable-part2 branch May 7, 2017

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