Skip to content
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

Fix attack behaviour of disabled units #15651

Merged
merged 1 commit into from Apr 20, 2019

Conversation

@extmind
Copy link
Contributor

commented Sep 27, 2018

This moves the disabled/paused check in AttackBase from AttackTarget to DoAttack. Adding it to DoAttack pauses (and unpauses) an already attacking unit. Removing it from AttackTarget allows for retargeting while the unit is disabled.

Fixes #13355.

@pchote

This comment has been minimized.

Copy link
Member

commented Sep 27, 2018

Removing it from AttackTarget allows for retargeting while the unit is disabled.

This sounds correct when paused, but not when disabled. The distinction between paused and disabled is that disabled should act as if the unit doesn't have the trait at all.

@reaperrr reaperrr force-pushed the extmind:issue-13355 branch from bab20a1 to 5c449ac Feb 22, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2019

Took the liberty to rebase and apply the fixup.
👍 from me.

@reaperrr reaperrr force-pushed the extmind:issue-13355 branch from 3fb3023 to 3ca0a52 Feb 24, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2019

Fixed. Not retested yet, so removing my own +1 for now.

@reaperrr reaperrr removed the PR: Needs +2 label Feb 24, 2019

@tovl

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2019

For me this makes the EMP canon not work at all. This change seems to somehow interfere with the EMPs own attack.

@reaperrr reaperrr force-pushed the extmind:issue-13355 branch 2 times, most recently from 32abeda to a6da7fb Mar 3, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

Updated.

@tovl
Copy link
Contributor

left a comment

LGTM now. 👍

@reaperrr reaperrr force-pushed the extmind:issue-13355 branch 2 times, most recently from e218117 to 0461a58 Mar 10, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2019

Updated.

@reaperrr reaperrr force-pushed the extmind:issue-13355 branch from 0461a58 to e795892 Mar 10, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2019

Rebased.

@@ -144,6 +144,9 @@ protected virtual AttackStatus TickAttack(Actor self, AttackFrontal attack)
if (!target.IsValidFor(self))
return AttackStatus.UnableToAttack;

if (attack.IsTraitPaused)

This comment has been minimized.

Copy link
@pchote

pchote Mar 10, 2019

Member

Unfortunately this isn't quite right either 😢

Returning AttackStatus.UnableToAttack makes the actor drop the target completely, which takes us from one extreme to the other. Ignoring attack.IsTraitPaused completely leaves us back at square one, with the bug that this PR originally set out to solve.

I believe the best workaround here is to put a if (!attack.IsTraitPaused) around the CheckFire calls below, and I will force-push over this PR to add this.

@pchote pchote force-pushed the extmind:issue-13355 branch from e795892 to 6b8acbd Mar 10, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented Mar 10, 2019

Updated, but would like to get some final testing and a 👍 for the last change before we merge.

@abcdefg30

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

I guess we somehow need to call DoAttack after all, this is what currently happens with worms in D2k:
wormLoop

@pchote pchote force-pushed the extmind:issue-13355 branch from 6b8acbd to 522dad3 Mar 30, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented Mar 30, 2019

Pushed over a fix for the sandworms.

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

Pausing an AttackFrontal actor (in my case TS Wolverine) does not prevent it from turning when given an attack order to a different direction.

No other issues found, so 👍 after that.

@pchote

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

IMO that is best left to a followup PR, since it is a bug in Turn (which still pauses only when Mobile is disabled rather than AttackFrontal itself) and risks conflicting with the other activity PRs.

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

OK.

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

@abcdefg30 @matjaeck Can I get one of you to give this another (hopefully final) test?

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

I'm testing it now.

@matjaeck
Copy link
Contributor

left a comment

Can confirm the fix and didn't encounter any related issues in ts.

@reaperrr reaperrr merged commit e6750bf into OpenRA:bleed Apr 20, 2019

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
Projects
None yet
6 participants
You can’t perform that action at this time.