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

Merge HeliAttack into FlyAttack #16400

Merged
merged 1 commit into from Jun 8, 2019

Conversation

@reaperrr
Copy link
Contributor

commented Apr 11, 2019

Split from #15803.

Depends on #16369.

@reaperrr reaperrr referenced this pull request Apr 11, 2019
0 of 1 task complete
@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

Waiting for #16483 to be merged first would likely save me from some serious rebase conflicts due to how HeliAttack currently handles movement, so I see little point in rebasing & updating this until #16483 is in.

@reaperrr reaperrr force-pushed the reaperrr:merge-air-atks branch from 14dc483 to d5b96bd Jun 8, 2019

@reaperrr reaperrr added this to the Next Release milestone Jun 8, 2019

@reaperrr reaperrr force-pushed the reaperrr:merge-air-atks branch from d5b96bd to 50989b7 Jun 8, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2019

Updated and rebased.

Note: In light of the experience (or should I say trauma...?) with the HeliFly merge, please lets stick to merging and - apart from the 2nd commit, which is limited in scope and therefore relatively easy to handle - fixing regressions, if there are any.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2019

Updated. I had the new commit almost ready anyway (though originally for a follow-up...).

If this regresses anything major/hard-to-fix, I'd rather drop it and defer those changes back to that follow-up again, though.

@pchote
Copy link
Member

left a comment

LGTM, but IMO it would be be better if the commits were squashed before merging.

@pchote pchote added the PR: Needs +2 label Jun 8, 2019

Merge HeliAttack into FlyAttack
And polish CanHover FlyAttack behavior:

- Get rid of direct TickFacing usage
- Fix that the CanHover facing/altitude update would override
  TakeOff child of Fly
- Streamline the queueing of child activities
- Get rid of a direct FlyTick in favor of relying on Fly activity
- Pull queueing of TakeOff out of the if-else

@reaperrr reaperrr force-pushed the reaperrr:merge-air-atks branch from 3e161b1 to 6aeece4 Jun 8, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2019

but IMO it would be be better if the commits were squashed before merging.

Done.

@tovl
tovl approved these changes Jun 8, 2019
Copy link
Contributor

left a comment

LGTM 👍

@pchote
pchote approved these changes Jun 8, 2019

@pchote pchote merged commit 979ed1b into OpenRA:bleed Jun 8, 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
3 participants
You can’t perform that action at this time.