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 more IsDisabled checks #14202

Merged
merged 8 commits into from Nov 25, 2017

Conversation

Projects
None yet
5 participants
@reaperrr
Contributor

reaperrr commented Oct 17, 2017

Depends on #13998.

This removes most of the remaining Actor.IsDisabled look-ups.
After this, only Gate, ProductionQueue and AI AirStates remain (might be worth investigating whether those are easy enough to migrate to add them to this PR, too).

@reaperrr reaperrr added this to the Next release milestone Oct 17, 2017

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Oct 17, 2017

Contributor

Wouldn't c7b7d8f conflict with the ammo refactors?

Contributor

GraionDilach commented Oct 17, 2017

Wouldn't c7b7d8f conflict with the ammo refactors?

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Oct 18, 2017

Member

OpenRA.Mods.Common/Traits/AffectsShroud.cs(70,19): error CS0103: The name `IsDisabled' does not exist in the current context

OpenRA.Mods.Common/Traits/AffectsShroud.cs(73,87): error CS0103: The name `cachedDisabled' does not exist in the current context

OpenRA.Mods.Common/Traits/AffectsShroud.cs(105,38): error CS0103: The name `cachedDisabled' does not exist in the current context

OpenRA.Mods.Common/Traits/Turreted.cs(99,13): error CS0103: The name `self' does not exist in the current context

Compilation failed: 4 error(s), 0 warnings

Member

pchote commented Oct 18, 2017

OpenRA.Mods.Common/Traits/AffectsShroud.cs(70,19): error CS0103: The name `IsDisabled' does not exist in the current context

OpenRA.Mods.Common/Traits/AffectsShroud.cs(73,87): error CS0103: The name `cachedDisabled' does not exist in the current context

OpenRA.Mods.Common/Traits/AffectsShroud.cs(105,38): error CS0103: The name `cachedDisabled' does not exist in the current context

OpenRA.Mods.Common/Traits/Turreted.cs(99,13): error CS0103: The name `self' does not exist in the current context

Compilation failed: 4 error(s), 0 warnings

reaperrr added some commits Sep 12, 2017

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Nov 13, 2017

Contributor

Updated and rebased.

Contributor

reaperrr commented Nov 13, 2017

Updated and rebased.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Nov 14, 2017

Contributor

Updated.

Contributor

reaperrr commented Nov 14, 2017

Updated.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Nov 15, 2017

Member

Regression: The RA gap generator does not disable on low power.

Member

pchote commented Nov 15, 2017

Regression: The RA gap generator does not disable on low power.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Nov 15, 2017

Member

Regression: RA tesla coils (and other powered defenses) lose the ability to target and their range circles when powered down. Something still seems to be treating them as disabled rather than paused.

Member

pchote commented Nov 15, 2017

Regression: RA tesla coils (and other powered defenses) lose the ability to target and their range circles when powered down. Something still seems to be treating them as disabled rather than paused.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Nov 15, 2017

Contributor

Regression: The RA gap generator does not disable on low power.
Regression: RA tesla coils (and other powered defenses) lose the ability to target and their range circles when powered down. Something still seems to be treating them as disabled rather than paused.

Fixed. These were introduced by #13998, but at least some of them needed Attack* traits to be pausable to fix them, so this is the right place to fix them anyway.

Contributor

reaperrr commented Nov 15, 2017

Regression: The RA gap generator does not disable on low power.
Regression: RA tesla coils (and other powered defenses) lose the ability to target and their range circles when powered down. Something still seems to be treating them as disabled rather than paused.

Fixed. These were introduced by #13998, but at least some of them needed Attack* traits to be pausable to fix them, so this is the right place to fix them anyway.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Nov 15, 2017

Member

I'm not sure if this is a new regression or and old bug, but if you build an AA gun (or another turreted defense), order it to force attack a Mig (or another unit) and then immediately power it down it then the turret will continue to track while disabled, without firing.

Edit: This is indeed a regression vs the last release.

Member

pchote commented Nov 15, 2017

I'm not sure if this is a new regression or and old bug, but if you build an AA gun (or another turreted defense), order it to force attack a Mig (or another unit) and then immediately power it down it then the turret will continue to track while disabled, without firing.

Edit: This is indeed a regression vs the last release.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Nov 15, 2017

Member

Something else that i'm not sure about: in some cases disabling an Attack* seems to cancel the target, and in others not.

Edit: this is also a regression vs the last release.

Member

pchote commented Nov 15, 2017

Something else that i'm not sure about: in some cases disabling an Attack* seems to cancel the target, and in others not.

Edit: this is also a regression vs the last release.

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Nov 16, 2017

Contributor

The turrets-stuck-in-attack mode bug (which the last two instances have showcased) have been already filed as #12856.

Contributor

GraionDilach commented Nov 16, 2017

The turrets-stuck-in-attack mode bug (which the last two instances have showcased) have been already filed as #12856.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Nov 16, 2017

Contributor

Something else that i'm not sure about: in some cases disabling an Attack* seems to cancel the target, and in others not.

I assume disabling should always cancel it?

Contributor

reaperrr commented Nov 16, 2017

Something else that i'm not sure about: in some cases disabling an Attack* seems to cancel the target, and in others not.

I assume disabling should always cancel it?

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Nov 16, 2017

Member

Sorry, I meant disabling in the sense of EMP or power-down ingame. i.e. Pausing.

Member

pchote commented Nov 16, 2017

Sorry, I meant disabling in the sense of EMP or power-down ingame. i.e. Pausing.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Nov 16, 2017

Contributor

Edit: This is indeed a regression vs the last release.
Edit: this is also a regression vs the last release.

This doesn't make any sense to me. Going by code changes, I see no way how PauseOnCondition should behave any different to Actor.IsDisabled on last release.

Both 'regressions' should already be present on last release, going by the code changes in this PR alone. (Unless I'm routine-blinded, of course)

Contributor

reaperrr commented Nov 16, 2017

Edit: This is indeed a regression vs the last release.
Edit: this is also a regression vs the last release.

This doesn't make any sense to me. Going by code changes, I see no way how PauseOnCondition should behave any different to Actor.IsDisabled on last release.

Both 'regressions' should already be present on last release, going by the code changes in this PR alone. (Unless I'm routine-blinded, of course)

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Nov 16, 2017

Contributor

Ok, managed to find the cause of the first one. Still investigating the 2nd one.
Edit: 2nd was a yaml issue in TS (RequiresCondition instead of PauseOnCondition), couldn't reproduce the issue with TD Obelisk.

Contributor

reaperrr commented Nov 16, 2017

Ok, managed to find the cause of the first one. Still investigating the 2nd one.
Edit: 2nd was a yaml issue in TS (RequiresCondition instead of PauseOnCondition), couldn't reproduce the issue with TD Obelisk.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Nov 16, 2017

Contributor

Updated.

Contributor

reaperrr commented Nov 16, 2017

Updated.

@pchote

The latest version seems to have fixed all the regressions in the classic mods, great job!

I found a couple of possible issues in TS:

  • Deployed Tick Tanks and Juggernauts can't attack.
  • Inline comments below
Show outdated Hide outdated mods/ts/rules/gdi-vehicles.yaml
Show outdated Hide outdated mods/ts/rules/nod-vehicles.yaml
Show outdated Hide outdated mods/ts/rules/nod-vehicles.yaml
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Nov 18, 2017

Contributor

Updated.

Deployed Tick Tanks and Juggernauts can't attack.

Found and fixed the problem. AttackTurreted's new Turrets property defaults to "primary", while the turrets of those 2 + arty had their name changed to deployed.

Contributor

reaperrr commented Nov 18, 2017

Updated.

Deployed Tick Tanks and Juggernauts can't attack.

Found and fixed the problem. AttackTurreted's new Turrets property defaults to "primary", while the turrets of those 2 + arty had their name changed to deployed.

@pchote

pchote approved these changes Nov 19, 2017

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Nov 19, 2017

Contributor

@abcdefg30 @penev92 There's one last follow-up waiting that will remove the remaining 3 uses of Actor.IsDisabled(), so this is also one of the PRs that should get merged soon so we can finally wrap this up.

Contributor

reaperrr commented Nov 19, 2017

@abcdefg30 @penev92 There's one last follow-up waiting that will remove the remaining 3 uses of Actor.IsDisabled(), so this is also one of the PRs that should get merged soon so we can finally wrap this up.

@reaperrr reaperrr referenced this pull request Nov 19, 2017

Merged

Remove IDisable #14387

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

@penev92 penev92 merged commit 5ec3ad0 into OpenRA:bleed Nov 25, 2017

2 checks passed

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

@reaperrr reaperrr deleted the reaperrr:less-idisable1 branch Feb 22, 2018

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