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 #14387

Merged
merged 5 commits into from Dec 17, 2017

Conversation

Projects
None yet
4 participants
@reaperrr
Contributor

reaperrr commented Nov 19, 2017

This removes the remaining uses of Actor.IsDisabled and IDisable.

Closes #7807.
Closes #10237.
Closes #12480.
Closes #14177.

@reaperrr reaperrr added this to the Next + 1 milestone Nov 19, 2017

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Nov 19, 2017

Contributor

OpenRA.Utility(1,1): Error: Missing Type: DisableOnConditionInfo

Where is this hiding?! Can't find it anywhere...
Edit: Nevermind.

Contributor

reaperrr commented Nov 19, 2017

OpenRA.Utility(1,1): Error: Missing Type: DisableOnConditionInfo

Where is this hiding?! Can't find it anywhere...
Edit: Nevermind.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Nov 19, 2017

Contributor

Updated.

Contributor

reaperrr commented Nov 19, 2017

Updated.

@reaperrr reaperrr modified the milestones: Next + 1, Next release Nov 19, 2017

@penev92

This comment has been minimized.

Show comment
Hide comment
@penev92

penev92 Nov 25, 2017

Member

The dependency was merged.

Member

penev92 commented Nov 25, 2017

The dependency was merged.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Nov 25, 2017

Contributor

Rebased and updated.

Contributor

reaperrr commented Nov 25, 2017

Rebased and updated.

@pchote

Looking good! Just a couple of minor code nits that i'd like you to address before I dig into ingame testing:

Show outdated Hide outdated OpenRA.Mods.Common/AI/States/AirStates.cs Outdated
}
public class Production : INotifyCreated
public class Production : PausableConditionalTrait<ProductionInfo>, INotifyCreated

This comment has been minimized.

@pchote

pchote Nov 25, 2017

Member

The Produce method below should check and return false if paused.

@pchote

pchote Nov 25, 2017

Member

The Produce method below should check and return false if paused.

This comment has been minimized.

@reaperrr

reaperrr Nov 26, 2017

Contributor

Done.

@reaperrr

reaperrr Nov 26, 2017

Contributor

Done.

Show outdated Hide outdated OpenRA.Mods.Common/Traits/Buildings/Gate.cs Outdated
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Nov 26, 2017

Contributor

Updated.

Contributor

reaperrr commented Nov 26, 2017

Updated.

PR updated

@penev92

Leaving a bunch of change requests. Some of them may need to be discussed first, like the LINQ vs foreach one.

Show outdated Hide outdated OpenRA.Mods.Common/AI/States/AirStates.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/Production.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/Player/ProductionQueue.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/Player/ProductionQueue.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/Player/ProductionQueue.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/Buildings/Gate.cs Outdated
@@ -109,15 +112,14 @@ void INotifyBlockingMove.OnNotifyBlockingMove(Actor self, Actor blocking)
desiredPosition = OpenPosition;
}
bool CanRemoveBlockage(Actor self, Actor blocking)
void INotifyAddedToWorld.AddedToWorld(Actor self)

This comment has been minimized.

@penev92

penev92 Nov 26, 2017

Member

This also needs

		void INotifyRemovedFromWorld.RemovedFromWorld(Actor self)
		{
			blockedPositions = Enumerable.Empty<CPos>();
		}

to fully address @pchote's earlier comment:

blockedPositions still needs to be updated if the actor is manually removed and readded to the world (e.g. using Lua).

@penev92

penev92 Nov 26, 2017

Member

This also needs

		void INotifyRemovedFromWorld.RemovedFromWorld(Actor self)
		{
			blockedPositions = Enumerable.Empty<CPos>();
		}

to fully address @pchote's earlier comment:

blockedPositions still needs to be updated if the actor is manually removed and readded to the world (e.g. using Lua).

This comment has been minimized.

@penev92

penev92 Nov 27, 2017

Member

Done.

@penev92

penev92 Nov 27, 2017

Member

Done.

Show outdated Hide outdated mods/ts/rules/defaults.yaml Outdated
Show outdated Hide outdated mods/ts/rules/defaults.yaml Outdated
Show outdated Hide outdated mods/ts/rules/defaults.yaml Outdated
@penev92

This comment has been minimized.

Show comment
Hide comment
@penev92

penev92 Nov 26, 2017

Member

Also here are some comments that I couldn't fit in the review because of GitHub being limiting about where you can put comments in the diffs:

  • ProductionQueue.MostLikelyProducer seems like it should start respecting the fact that Production is now Pausable/Disablable.
  • Same goes for ClassicProductionQueue's Tick, MostLikelyProducer and GetBuildTime, since they all have references to Production.
  • I have not checked the RA rule definitions for the gates, but the TS ones needed some fixing, so I'm going to assume those do as well.
Member

penev92 commented Nov 26, 2017

Also here are some comments that I couldn't fit in the review because of GitHub being limiting about where you can put comments in the diffs:

  • ProductionQueue.MostLikelyProducer seems like it should start respecting the fact that Production is now Pausable/Disablable.
  • Same goes for ClassicProductionQueue's Tick, MostLikelyProducer and GetBuildTime, since they all have references to Production.
  • I have not checked the RA rule definitions for the gates, but the TS ones needed some fixing, so I'm going to assume those do as well.
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Nov 27, 2017

Contributor

Updated.

Contributor

reaperrr commented Nov 27, 2017

Updated.

@penev92

This comment has been minimized.

Show comment
Hide comment
@penev92

penev92 Nov 27, 2017

Member

Code looks mostly good (nothing was changed to the AirStates code), with a few notes:

  • ProductionQueue.Tick may or may not want to check IsTraitDisabled/IsTraitPaused.
  • ClassicProductionQueue.GetBuildTime more than likely wants to check those.
  • Those two lead to the question "does ProductionQueue.GetBuildTime also want to check that?"
  • I haven't checked RA gate rule definitions yet, but the TS rules look good now.
  • Have not yet tested anything ingame.
Member

penev92 commented Nov 27, 2017

Code looks mostly good (nothing was changed to the AirStates code), with a few notes:

  • ProductionQueue.Tick may or may not want to check IsTraitDisabled/IsTraitPaused.
  • ClassicProductionQueue.GetBuildTime more than likely wants to check those.
  • Those two lead to the question "does ProductionQueue.GetBuildTime also want to check that?"
  • I haven't checked RA gate rule definitions yet, but the TS rules look good now.
  • Have not yet tested anything ingame.
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Dec 1, 2017

Contributor

Those two lead to the question "does ProductionQueue.GetBuildTime also want to check that?"

In my opinion no, because it only calculates the time for that item once before its production would be started ( you can tell by the fact that cheats or low power have no impact on build speed of actors already in production).

Contributor

reaperrr commented Dec 1, 2017

Those two lead to the question "does ProductionQueue.GetBuildTime also want to check that?"

In my opinion no, because it only calculates the time for that item once before its production would be started ( you can tell by the fact that cheats or low power have no impact on build speed of actors already in production).

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Dec 1, 2017

Contributor

Updated.

Contributor

reaperrr commented Dec 1, 2017

Updated.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Dec 13, 2017

Contributor

Rebased and updated.

  • Fixed broken ClassicProductionQueue
  • Fixed that even after above fix, ClassicProductionQueue would continue ticking when it should be paused
  • Avoid LINQ in AirStates
Contributor

reaperrr commented Dec 13, 2017

Rebased and updated.

  • Fixed broken ClassicProductionQueue
  • Fixed that even after above fix, ClassicProductionQueue would continue ticking when it should be paused
  • Avoid LINQ in AirStates
@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Dec 13, 2017

Contributor

Needs another rebase now.

Contributor

GraionDilach commented Dec 13, 2017

Needs another rebase now.

@penev92

Disabling all production structures for a production tab has a pretty crap interaction with the UI, but I'm not sure what we want to do there, and is probably not for this PR.
Also EMP-ing a ConYard in TS doesn't pause production. Is that intentional?

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Dec 13, 2017

Contributor

Rebased and updated.

Disabling all production structures for a production tab has a pretty crap interaction with the UI, but I'm not sure what we want to do there, and is probably not for this PR.

Agree on both accounts (unless it's easy to fix and someone tells me how).

Contributor

reaperrr commented Dec 13, 2017

Rebased and updated.

Disabling all production structures for a production tab has a pretty crap interaction with the UI, but I'm not sure what we want to do there, and is probably not for this PR.

Agree on both accounts (unless it's easy to fix and someone tells me how).

@pchote

I'm sorry... I want to get this merged as much as you do, but:

reaperrr added some commits Nov 19, 2017

Make Gate more independent from Building and pausable-conditional
Replace Gate IsDisabled checks with IsTraitDisabled/Paused checks
Replace IsDisabled checks in production with IsTraitPaused/Disabled c…
…hecks

Note: We might want to separate IsTraitDisabled checks later (possibly make the latter cancel the currently produced item), but that can be done in a follow-up.
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Dec 17, 2017

Contributor

Updated. I hope this is it...

Contributor

reaperrr commented Dec 17, 2017

Updated. I hope this is it...

@pchote

pchote approved these changes Dec 17, 2017

There are a couple of potential nits, but it is not worth delaying this further on them. Any further issues can be dealt with in followup PRs.

@pchote pchote merged commit 85c54e0 into OpenRA:bleed Dec 17, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment