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 1 #12955

Merged
merged 5 commits into from Apr 30, 2017

Conversation

Projects
None yet
4 participants
@atlimit8
Member

atlimit8 commented Mar 12, 2017

First part of a series of PRs to remove IDisable. This removes DisabledOverlay using WithColoredOverlay and GrantConditionOnDisabled as a shim as well as converting ProximityExternalCondition, RepairsUnits, and Production. This PR also halts repairs and production when they are disabled. I noticed that the EMP effect does not stop repairs or production in the ts mod as is.

@atlimit8 atlimit8 changed the title from Remove IDdisable - part 1 to Remove IDisable - part 1 Mar 12, 2017

@pchote

Looks sensible overall, but I have a spanner to throw in the works:

@atlimit8 atlimit8 added this to the Tiberian Sun Public Alpha milestone Mar 12, 2017

@atlimit8

This comment has been minimized.

Show comment
Hide comment
@atlimit8

atlimit8 Mar 12, 2017

Member

Applied requested changes using a cherry-picked commit I created to update affected condition expressions of multiple supported by a trait and adding PausableConditionalTrait abstract base class with PauseOnCondition as a replacement to replace PauseOnLowPower for other IDisable affected traits (ex: With*Overlay).

Member

atlimit8 commented Mar 12, 2017

Applied requested changes using a cherry-picked commit I created to update affected condition expressions of multiple supported by a trait and adding PausableConditionalTrait abstract base class with PauseOnCondition as a replacement to replace PauseOnLowPower for other IDisable affected traits (ex: With*Overlay).

Requested changes made.

@atlimit8

This comment has been minimized.

Show comment
Hide comment
@atlimit8

atlimit8 Mar 19, 2017

Member

Rebased after #12747 merge - Prevent duplicate upgrades (plugs) to GDI Upgrade Center (Pluggable Requirements).

Member

atlimit8 commented Mar 19, 2017

Rebased after #12747 merge - Prevent duplicate upgrades (plugs) to GDI Upgrade Center (Pluggable Requirements).

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Mar 19, 2017

Contributor

Manually powering down Radar (and presumably any other building with CanPowerDown) also uncloaks it, even if there's an active Cloak Gen nearby. In my opinion the building needs to stay hidden in that situation.

Contributor

reaperrr commented Mar 19, 2017

Manually powering down Radar (and presumably any other building with CanPowerDown) also uncloaks it, even if there's an active Cloak Gen nearby. In my opinion the building needs to stay hidden in that situation.

@atlimit8

This comment has been minimized.

Show comment
Hide comment
@atlimit8

atlimit8 Mar 19, 2017

Member

That is because Cloak still checks self.IsDisabled. I plan on changing that after part 2. I could stuff that in this PR though.

Member

atlimit8 commented Mar 19, 2017

That is because Cloak still checks self.IsDisabled. I plan on changing that after part 2. I could stuff that in this PR though.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Mar 19, 2017

Contributor

I think this PR is large enough as-is, knowing it's a known issue that will be adressed in a follow-up is enough for me.

Contributor

reaperrr commented Mar 19, 2017

I think this PR is large enough as-is, knowing it's a known issue that will be adressed in a follow-up is enough for me.

@reaperrr

Looks good to me and works as advertised.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Apr 9, 2017

Member

Depends on #13074.

Member

pchote commented Apr 9, 2017

Depends on #13074.

Want to give this another look after rebase

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Apr 22, 2017

Contributor

#13074 was merged, you should be able to rebase now.

Contributor

reaperrr commented Apr 22, 2017

#13074 was merged, you should be able to rebase now.

@atlimit8

This comment has been minimized.

Show comment
Hide comment
@atlimit8

atlimit8 Apr 22, 2017

Member

Rebased.

Member

atlimit8 commented Apr 22, 2017

Rebased.

@pchote

The overall approach looks good to me now, but I found a few more specific bugs/nits:

Show outdated Hide outdated OpenRA.Mods.Common/Traits/Conditions/ProximityExternalCondition.cs
@@ -196,7 +196,10 @@ NASTLH:
PowerupSpeech: EnablePower
PowerdownSpeech: DisablePower
IndicatorPalette: mouse
GrantConditionOnDisabled:

This comment has been minimized.

@pchote

pchote Apr 23, 2017

Member

Why not Inherits@IDISABLE: ^DisabledOverlay?

@pchote

pchote Apr 23, 2017

Member

Why not Inherits@IDISABLE: ^DisabledOverlay?

This comment has been minimized.

@atlimit8

atlimit8 Apr 23, 2017

Member

It didn't have DisabledOverlay before, so I added just this to avoid making functional changes such as adding an overlay.

@atlimit8

atlimit8 Apr 23, 2017

Member

It didn't have DisabledOverlay before, so I added just this to avoid making functional changes such as adding an overlay.

{
[ConsumedConditionReference]
[Desc("Boolean expression defining the condition to pause this trait.")]
public readonly BooleanExpression PauseOnCondition = null;

This comment has been minimized.

@pchote

pchote Apr 23, 2017

Member

Question: PauseOnCondition, or just PauseCondition?

@pchote

pchote Apr 23, 2017

Member

Question: PauseOnCondition, or just PauseCondition?

This comment has been minimized.

@atlimit8

atlimit8 Apr 23, 2017

Member

I just copied PauseOnLowPower. I don't really care myself, but PauseCondition would be shorter.

@atlimit8

atlimit8 Apr 23, 2017

Member

I just copied PauseOnLowPower. I don't really care myself, but PauseCondition would be shorter.

This comment has been minimized.

@pchote

pchote Apr 23, 2017

Member

PauseOnCondition is probably better, at least its consistent with RequiresCondition vs RequireCondition.

@pchote

pchote Apr 23, 2017

Member

PauseOnCondition is probably better, at least its consistent with RequiresCondition vs RequireCondition.

Show outdated Hide outdated OpenRA.Mods.Common/Activities/Repair.cs
Show outdated Hide outdated OpenRA.Mods.Common/Traits/Player/ClassicProductionQueue.cs
Show outdated Hide outdated OpenRA.Mods.Common/Traits/Player/ProductionQueue.cs
Show outdated Hide outdated OpenRA.Mods.Common/Traits/Player/ProductionQueue.cs
Show outdated Hide outdated OpenRA.Mods.Common/Traits/Production.cs
}
Game.Sound.PlayNotification(self.World.Map.Rules, self.Owner, "Speech", repairsUnits.FinishRepairingNotification, self.Owner.Faction.InternalName);
Game.Sound.PlayNotification(self.World.Map.Rules, self.Owner, "Speech", repairsUnits.Info.FinishRepairingNotification, self.Owner.Faction.InternalName);
return NextActivity;
}

This comment has been minimized.

@pchote

pchote Apr 23, 2017

Member

You'll need a if (repairsUnits.IsTraitPaused)\n\treturn this here to prevent units from being repaired when they first dock with a paused repair depot.

@pchote

pchote Apr 23, 2017

Member

You'll need a if (repairsUnits.IsTraitPaused)\n\treturn this here to prevent units from being repaired when they first dock with a paused repair depot.

This comment has been minimized.

@atlimit8

atlimit8 Apr 23, 2017

Member

See here

			if (repairsUnits == null)
				return paused ? this : NextActivity;
@atlimit8

atlimit8 Apr 23, 2017

Member

See here

			if (repairsUnits == null)
				return paused ? this : NextActivity;
@atlimit8

This comment has been minimized.

Show comment
Hide comment
@atlimit8

atlimit8 Apr 23, 2017

Member

I've made the production changes.

Member

atlimit8 commented Apr 23, 2017

I've made the production changes.

@atlimit8

This comment has been minimized.

Show comment
Hide comment
@atlimit8

atlimit8 Apr 23, 2017

Member

Made changes except for the production logic.

Member

atlimit8 commented Apr 23, 2017

Made changes except for the production logic.

@atlimit8

This comment has been minimized.

Show comment
Hide comment
@atlimit8

atlimit8 Apr 24, 2017

Member

Note: I have not test these latest changes for ProductionAvailable & ITick.Tick(Actor self) in the last commit; I plan to later.

Member

atlimit8 commented Apr 24, 2017

Note: I have not test these latest changes for ProductionAvailable & ITick.Tick(Actor self) in the last commit; I plan to later.

@atlimit8

This comment has been minimized.

Show comment
Hide comment
@atlimit8

atlimit8 Apr 25, 2017

Member

I tested the production code and did not find any regressions.
I think all issues have been addressed.

Member

atlimit8 commented Apr 25, 2017

I tested the production code and did not find any regressions.
I think all issues have been addressed.

All issues have been addressed.

@atlimit8

This comment has been minimized.

Show comment
Hide comment
@atlimit8

atlimit8 Apr 25, 2017

Member

Fixed canceling repair while paused.

Member

atlimit8 commented Apr 25, 2017

Fixed canceling repair while paused.

@pchote

The production queue stuff is tricky, but everything else looks good to go (minor nits aside). Would you mind splitting the last commit off to a new PR so that we can focus on it separately and merge the others?

}
}
if (repairsUnits == null)

This comment has been minimized.

@pchote

pchote Apr 30, 2017

Member

The IsCancelled check below (not the inner one) doesn't depend on repairsUnits or paused, so if you move that to the top of the method then this could simplify down to

if (repairsUnits == null)
    return paused ? this : NextActivity
@pchote

pchote Apr 30, 2017

Member

The IsCancelled check below (not the inner one) doesn't depend on repairsUnits or paused, so if you move that to the top of the method then this could simplify down to

if (repairsUnits == null)
    return paused ? this : NextActivity

This comment has been minimized.

@pchote

pchote Apr 30, 2017

Member

Note: this has been done now.

@pchote

pchote Apr 30, 2017

Member

Note: this has been done now.

Show outdated Hide outdated OpenRA.Mods.Common/Activities/Repair.cs
Show outdated Hide outdated OpenRA.Mods.Common/Scripting/Properties/ProductionProperties.cs
Show outdated Hide outdated OpenRA.Mods.Common/Traits/Player/ProductionQueue.cs
@atlimit8

This comment has been minimized.

Show comment
Hide comment
@atlimit8

atlimit8 Apr 30, 2017

Member

I have split off the production stuff and will look into making it more conditional.

Member

atlimit8 commented Apr 30, 2017

I have split off the production stuff and will look into making it more conditional.

@pchote

pchote approved these changes Apr 30, 2017

@pchote pchote added the PR: Needs +2 label Apr 30, 2017

@GraionDilach

👍

@reaperrr reaperrr merged commit 437f9a7 into OpenRA:bleed Apr 30, 2017

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@reaperrr
Contributor

reaperrr commented Apr 30, 2017

@atlimit8 atlimit8 deleted the atlimit8:RemoveIDisable-part1 branch May 1, 2017

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