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

Make FlyAttack use FlyFollow as a child. #16945

Merged
merged 1 commit into from Aug 18, 2019

Conversation

@tovl
Copy link
Contributor

commented Aug 13, 2019

Fixes #16922.

FlyTimed is unneccesary because Fly itself already calculates the correct trajectory based on the turn radius.

@pchote pchote added this to the Next Release milestone Aug 13, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

Removing the AttackTurnDelay should entail an update rule that removes it from mods that set custom values (and ideally notifies the modder about the places it was removed from, like we do in some other update rules already).

@tovl tovl force-pushed the tovl:flyby branch from 0f9c793 to 6cd5458 Aug 15, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

Added updaterule and applied fixup.

@tovl tovl force-pushed the tovl:flyby branch from 6cd5458 to 5060a78 Aug 15, 2019

@tovl tovl force-pushed the tovl:flyby branch 2 times, most recently from b65f44f to a65abd0 Aug 15, 2019

@tovl tovl force-pushed the tovl:flyby branch from a65abd0 to 2050f09 Aug 15, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

Removing AttackTurnDelay completely means that aircraft will always start turning immediately after the attack once they reach a distance suitable to be able to intersect with the target, correct?

There are cases where we (and i'm sure modders) do not want that - for example the proper D2k airstrike behaviour: https://www.youtube.com/watch?v=dmflM70vtZI&feature=youtu.be&t=37

We should keep the parameter, but we can override it to 0 for Yaks (and Migs?) in the RA yaml.

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

Removing AttackTurnDelay completely means that aircraft will always start turning immediately after the attack, correct?

Incorrect. Fly (called by FlyFollow) will always make sure to first make enough distance (flying straight) before turning in order to be able to reach the destination taking into account the turn radius. I have not played the original D2k but, judging from that video, that pattern can easily be replicated by setting the TurnSpeed for ornihopters low enough.

We should keep the parameter, but we can override it to 0 for Yaks (and Migs?) in the RA yaml.

Keeping the parameter means keeping FlyTimed here and that is exactly where the problem comes from. We would need to completely rewrite either FlyTimed or FlyAttack in order to accommodate that and still solve the regression. I'm not going to do that here and now and I don't think this parameter is useful anyway.

@pchote

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

make sure to first make enough distance (flying straight) before turning in order to be able to reach the destination taking into account the turn radius.

This is what I meant above, sorry.

My point is that mods can use AttackTurnDelay to specify an additional delay for the aircraft to keep moving in a straight line, longer than otherwise needed for it to turn back to the target. We are removing this, and instead hardcoding the same flight dynamics for all cases.

I don't see why we can't keep this flexibility (by queueing FlyTimed before FlyFollow if the delay is > 0), but just set the value to 0 for the cases that don't want it.

@pchote

This comment has been minimized.

Copy link
Member

commented Aug 17, 2019

The AttackTurnDelay issue is largely theoretical at this point, so I'm okay with letting that go if needed to fix the bug.

Unfortunately, we have a much larger practical problem here: The aircraft doesn't break out of the FlyFollow when it needs to reload, so ends up circling empty above the target.

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2019

Unfortunately, we have a much larger practical problem here: The aircraft doesn't break out of the FlyFollow when it needs to reload, so ends up circling empty above the target.

Hmm, this means that, in order to solve the issue, FlyAttack would need to be completely refactored in any case (to be able to take priority over it's child activities). That invalidates my main objection to leaving AttackTurnDelay in, but it does mean that this needs a lot more work and an increased risk of new regressions.

@pchote

This comment has been minimized.

Copy link
Member

commented Aug 17, 2019

A simpler option could be to set ChildHasPriority = false in FlyAttack, and have that cancel the FlyFollow if out of ammo. Unless that's what you mean - in which case I may be missing why it needs to be completely refactored to do this.

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2019

Maybe I'm being too pessimistic about it, but it is not as simple as setting ChildHasPriority = false and everything else being the same as it was. The entire activity assumes that it doesn't tick when a child ticks, so at the very least we would need to set a couple of flags to prevent an endless amount of childactivities being queued every tick. I guess we'll soon find out.

@tovl tovl force-pushed the tovl:flyby branch from 2050f09 to 3136cf9 Aug 17, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2019

Solved the issues. I went with a completely different solution now that also pulls in a bit of the refactoring from #16739 and keeps AttackTurnDelay in.

@tovl tovl force-pushed the tovl:flyby branch from 3136cf9 to 4d29c39 Aug 17, 2019

@pchote
pchote approved these changes Aug 17, 2019

@pchote pchote added the PR: Needs +2 label Aug 17, 2019

@abcdefg30 abcdefg30 merged commit 70459b3 into OpenRA:bleed Aug 18, 2019

2 checks passed

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

This comment has been minimized.

Copy link
Member

commented Aug 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.