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

Split fixed-wing attack and strafe attack types. #17524

Merged
merged 2 commits into from Jan 12, 2020
Merged

Conversation

@pchote
Copy link
Member

pchote commented Dec 30, 2019

A lot of the problems we're having with strafing attacks seem to come down to the fact that it is trying to solve two mutually incompatible use cases:

  • Making fixed-wing aircraft attacks work like other units.
  • Implementing strafing attacks, which don't work like other units.

This PR aims to solve both usecases by

  • Renaming the current AirAttackType.Strafe to AirAttackType.Default and making sure that both #17413 and #16922 are fixed.
  • Adding a new AirAttackType.Strafe that performs a fixed-length strafing run on a target, fixing the issue where strafes stop mid-run if the targeted actor is killed.

The first commit is basically #17418, modified to remove the update rule (this is now in the second commit) and to use a wrapper activity to cancel the attack pass instead of changing Fly. This allows aircraft to continue moving to the previous target position if it is killed while they are en route (like all other units and later C&C games), and also allows them to break off immediately (again like all other units) if the target is killed while they are actively attacking/repositioning;

The second commit renames the fixed behaviour to Default and introduces the new actually-strafing behaviour. This works by targeting the ground rather than the actor, and keeps firing on the last known position of the target is killed. In the future I can see us expanding this to move the target position along the ground path instead of relying on FirstBurstTargetOffset / FollowingBurstTargetOffset - but this is out of scope of this issue. This also adds an amended update rule to migrate any old uses of Strafe over to the Default.

The third commit is a quick testcase that demonstrates the improved strafing logic.

Fixes #17413.
Supersedes #17418.

@pchote pchote added this to the Next Release milestone Dec 30, 2019
Copy link
Contributor

Punsho left a comment

It is pretty hard to test the testcase as yaks can't hit units at all, they instead shoot a short burst right in front of them. They also barely attack buildings. Other than that their logic seems to work as intended

Non-testcase planes seem to work as intended. T̶h̶o̶u̶g̶h̶ ̶I̶ ̶h̶a̶v̶e̶ ̶f̶o̶u̶n̶d̶ ̶o̶n̶e̶ ̶r̶e̶g̶r̶e̶s̶s̶i̶o̶n̶,̶ ̶a̶l̶l̶ ̶a̶i̶r̶c̶r̶a̶f̶t̶ ̶n̶o̶w̶ ̶r̶e̶m̶e̶m̶b̶e̶r̶ ̶t̶h̶e̶i̶r̶ ̶q̶u̶e̶u̶e̶d̶ ̶o̶r̶d̶e̶r̶s̶ ̶a̶f̶t̶e̶r̶ ̶a̶u̶t̶o̶-̶r̶e̶s̶u̶p̶p̶l̶y̶ ̶i̶n̶s̶t̶e̶a̶d̶ ̶o̶f̶ ̶f̶o̶r̶g̶e̶t̶t̶i̶n̶g̶ ̶t̶h̶e̶m̶.̶

@pchote pchote force-pushed the pchote:fix-flyby branch from 645b24a to 5f52863 Dec 30, 2019
@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Dec 30, 2019

Fixed the strafing issue and updated the testcase.

@pchote pchote force-pushed the pchote:fix-flyby branch from 5f52863 to 212a5ce Dec 31, 2019
@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Dec 31, 2019

Rebased to get us past the CI failures.

@pchote pchote force-pushed the pchote:fix-flyby branch from 212a5ce to 5cb9fdc Jan 2, 2020
@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jan 2, 2020

Fixed and removed testcase (available as b01a7b5 if anybody still wants to try it).

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jan 11, 2020

Feedback from my custom test builds have been positive. Is there anything else blocking this from being merged?

Copy link
Contributor

reaperrr left a comment

LGTM

@reaperrr reaperrr merged commit adb3c8e into OpenRA:bleed Jan 12, 2020
2 checks passed
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.