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

Replace pseudo-childactivities in AttackMoveActivity. #16382

Merged
merged 2 commits into from May 12, 2019

Conversation

@tovl
Copy link
Contributor

commented Apr 3, 2019

This PR refactors AttackMoveActivity to get rid of the pseudo-childactivities and replace them with an approach based on ChildActivity. Behaviour should be the same as #16105.

Depends on #16369 just to be sure no weird regressions are happening due to SequenceActivities.

This is the last activity that still rolled its own pseudo-childactivities.

@matjaeck
Copy link
Contributor

left a comment

There seem to be problems with the facing of non-turreted units:

facings

The issue does not occur on bleed.
@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

This must be an issue with #16369. I deduplicated some of the AttackFrontal code there.

@pchote

This comment has been minimized.

Copy link
Member

commented May 5, 2019

#16369 has been merged.

@tovl tovl force-pushed the tovl:refactor-attackmove branch from 50819fc to d728c2e May 5, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

Rebased.

@pchote

This comment has been minimized.

Copy link
Member

commented May 8, 2019

Can we please now move the condition granting/revoking logic from AttackMove.Tick into the activity OnFirstRun / OnLastRun? This finally fixes #14010 (review).

@tovl tovl force-pushed the tovl:refactor-attackmove branch from d728c2e to 44264c6 May 9, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

Moved condition management into the activity.

}
if (!isAssaultMove && !string.IsNullOrEmpty(attackMove.Info.AttackMoveScanCondition))
token = conditionManager.GrantCondition(self, attackMove.Info.AttackMoveScanCondition);
else if (!string.IsNullOrEmpty(attackMove.Info.AssaultMoveScanCondition))

This comment has been minimized.

Copy link
@pchote

pchote May 9, 2019

Member

This also needs to check isAssaultMove to prevent it granting the assault move condition for everything when AttackMoveScanCondition isn't defined (which is the case for the default mods).

This comment has been minimized.

Copy link
@pchote

pchote May 9, 2019

Member

The regression can be seen by ordering an attack-move past an enemy structure. Expected behaviour is for the units to ignore the structure during a regular attack move, but attack for an assault move.

@pchote

This comment has been minimized.

Copy link
Member

commented May 9, 2019

Can you please also cherry pick 0c59adc into this PR to wrap up the condition refactoring?

@tovl tovl force-pushed the tovl:refactor-attackmove branch from 44264c6 to a45b2bf May 10, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

Done.

@pchote
pchote approved these changes May 10, 2019

@pchote pchote added the PR: Needs +2 label May 10, 2019

@reaperrr
Copy link
Contributor

left a comment

Didn't spot any regressions, lgtm

@reaperrr reaperrr merged commit 6248326 into OpenRA:bleed May 12, 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
5 participants
You can’t perform that action at this time.