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 HeliFly into Fly and make TakeOff self-contained #16584

Merged
merged 7 commits into from Jun 7, 2019

Conversation

@reaperrr
Copy link
Contributor

commented May 23, 2019

Split from #16483.

Please let's leave any further non-trivial refactors to #16483, which will effectively become the follow-up to this.
The aim of this PR is to get the activity merge into bleed without regressions, so we have a stable base for further improvements (and less time pressure).

OpenRA.Mods.Common/Activities/Air/Fly.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Activities/Air/Fly.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Activities/Air/Fly.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Activities/Air/TakeOff.cs Outdated Show resolved Hide resolved

@reaperrr reaperrr force-pushed the reaperrr:merge-fly-pt1 branch 2 times, most recently from 9d24416 to 0e55eaf May 23, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

Updated.

@reaperrr reaperrr referenced this pull request May 23, 2019

@pchote pchote dismissed their stale review May 24, 2019

misclick

@pchote

This comment has been minimized.

Copy link
Member

commented May 24, 2019

The code changes look broadly fine, but I found two regressions while testing:

  • If you move a helicopter over a terrain height change in TS (from bottom to top of a cliff works best) and then stop it as soon as it reaches the top it will stay hovering at its current altitude, which is much lower than the standard hovering altitude. This is less bad than the current behaviour on bleed (stopping until it reaches the right height), so I would be okay with merging without fixing this if the fix is going to be complicated
  • Issuing a fly order while a helicopter is part way through landing will make it take off diagonally, much like the issues I noted in #16483.
mods/d2k/rules/aircraft.yaml Outdated Show resolved Hide resolved

@reaperrr reaperrr force-pushed the reaperrr:merge-fly-pt1 branch from 0e55eaf to df06a4a May 24, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

Updated.

If you move a helicopter over a terrain height change in TS (from bottom to top of a cliff works best) and then stop it as soon as it reaches the top it will stay hovering at its current altitude, which is much lower than the standard hovering altitude. This is less bad than the current behaviour on bleed (stopping until it reaches the right height), so I would be okay with merging without fixing this if the fix is going to be complicated

Fixed via OnBecomingIdle in Aircraft, didn't notice any unwanted side-effects, either.

Issuing a fly order while a helicopter is part way through landing will make it take off diagonally, much like the issues I noted in #16483.

Fixed by making Land uninterruptible.

Additionally,

  • made TakeOff uninterruptible as well (except on ForceLanding)
  • clarified descriptions of MaximumPitch and AltitudeVelocity
  • fixed MaximumPitch values in TS to make all aircraft (not only Orca bomber and Banshee) adjust to cliffs naturally and not in slow motion.
@tovl

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

Fixed by making Land uninterruptible.

We already discussed this in #16509 and found that the loss in responsiveness was undesirable. Furthermore, the implementation here is incompatible with making Land self-contained because it would mean that the entire approach would be uninterruptible. A better solution would be to queue TakeOff as a childactivity whenever the altitude is lower than cruise-altitude on the first run.

@reaperrr reaperrr force-pushed the reaperrr:merge-fly-pt1 branch from df06a4a to 4971f29 May 25, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented May 25, 2019

Updated.

Reverted uninterruptability and instead queue a TakeOff child activity in Land if NextActivity != null. The case of NextActivity == null is already handled fully by OnBecomingIdle now, thanks to e980a46.

Additionally, added a speculative fix for #16579 (last commit).

@tovl

This comment has been minimized.

Copy link
Contributor

commented May 25, 2019

Actually, I meant to queue it as a childactivity of Fly instead of Land, but I guess this works just as well.

EDIT: On second thought, wouldn't this also trigger when the next activity is another Land activity? If so spam clicking would mean the heli would rise to CruiseAltitude before going back to the ground anyway. Doing it in Fly might therefore be cleaner after all. (This could then also replace the TakeOff queued around line 122 as well)

Speculative fix looks good at first glance.

@pchote

This comment has been minimized.

Copy link
Member

commented May 25, 2019

EDIT: On second thought, wouldn't this also trigger when the next activity is another Land activity? If so spam clicking would mean the heli would rise to CruiseAltitude before going back to the ground anyway. Doing it in Fly might therefore be cleaner after all. (This could then also replace the TakeOff queued around line 122 as well)

I like the idea of handling it in Land (with my suggested tweak) because it follows the rule that an activity should always leave the actor in a valid state when it completes/cancels. The cliff-height edge case can be resolved the same way in the Fly* activities, allowing us to drop the Aircraft.OnBecomingIdle special case.

Spam clicking is a more general problem that applies to many other activities (#16569) and that is going to be best handled with a more general fix on the ResolveOrder side - probably introducing a new standard code pattern to check if the current activity matches the requested order, then skipping the re-queue if they match.

@tovl

This comment has been minimized.

Copy link
Contributor

commented May 25, 2019

I like the idea of catching spam clicks on the ResolveOrder side instead of the activity side. However, this would still leave edge-cases where Land is a child of another activity. E.g. for chinooks, when a force-land order is directly followed by an unload order.

@pchote

This comment has been minimized.

Copy link
Member

commented May 25, 2019

That could perhaps be solved in two steps:

  1. Add a new Land.Cancel overload that includes a skipAltitudeFix flag
  2. Add a new IActivityAircraftAltitudeAware interface (maybe with a better name), and have the parent activities cancel call Land.Cancel with skipAltitudeFix: NextActivity is IActivityAircraftAltitudeAware.

My concern is that if we don't handle this at all in Land that we will be opening up potentially endless edge cases when other activities that are not explicitly aircraft related are queued after a cancelled Land.

This is probably best left to its own followup PR provided we can cover the points I raised in my comment above here.

@reaperrr reaperrr force-pushed the reaperrr:merge-fly-pt1 branch 2 times, most recently from f708417 to d26cce2 May 26, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2019

Updated.

Only 0524c3e changed.

@@ -48,6 +48,17 @@ public override Activity Tick(Actor self)

if (IsCanceling || target.Type == TargetType.Invalid)
{
// Make aircraft climb back to CruiseAltitude before moving on to NextActivity or going idle.
// TODO: Try to fix that even if NextActivity is not null, VTOLs will not turn towards the target while climbing,

This comment has been minimized.

Copy link
@pchote

pchote May 26, 2019

Member

I think this will be best handled by having those activities opt out of the altitude fixup here and handle the taking off themselves (which they are almost certainly already doing).

This comment has been minimized.

Copy link
@tovl

tovl May 26, 2019

Contributor

Generally, they don't. They would if we queue TakeOff in Fly.OnFirstRun whenever the altitude is lower than Aircraft.CruiseAltitude, as I already suggested.

The only aircraft related activity that takes care of this itself, as far as I can see, is HeliAttack and it really shouldn't. All these activities should just queue Fly instead of calling Fly.FlyTick or any other internal Fly method directly. Fly would then queue TakeOff as necessary.

This comment has been minimized.

Copy link
@pchote

pchote May 26, 2019

Member

Makes sense. Replacing FlyTick with Fly children is going to be important for the acceleration and cliff height prediction logic too.

The activities that queue Fly directly or use move.Move* will then be able to tag themselves with IActivityAircraftAltitudeAware (or whatever we want to call it) to opt out of the internal Land handling.

This comment has been minimized.

Copy link
@pchote

pchote May 26, 2019

Member

In hindsight, my request to allow facing changes while taking off has caused more trouble than it's worth. In practice it also looks a bit odd to have units turn before landing on a helipad but then turn while taking off when reloading is complete.

How about we abandon this idea and keep it simple?

This comment has been minimized.

Copy link
@tovl

tovl May 26, 2019

Contributor

I think units should be able to turn while landing as well.

Since we will probably want to replace FlyTick with Fly children anyway and Fly is already queueing TakeOff (but in the wrong place), this is keeping it simple.

Also, since any activity that cares about positioning at all will be queueing either Fly directly or using the IMove interface, we will be better off making the "TakeOff on cancelling Land" thing an opt-in instead of an opt-out.

(In fact, I've got the sneaking suspicion we will be left with exactly zero activities that would actually benefit from this, so we can just leave it for the idling case, making it even more simple.)

This comment has been minimized.

Copy link
@pchote

pchote May 26, 2019

Member

I think units should be able to turn while landing as well.

Turning when landing is more complicated because we need to make sure that the facing is correct before it touches down, which may mean turning part way before starting to descend.

Also, since any activity that cares about positioning at all will be queueing either Fly directly or using the IMove interface, we will be better off making the "TakeOff on cancelling Land" thing an opt-in instead of an opt-out.

Pragmatically, I expect you're correct. I'm just a bit annoyed that we still have to hardcode exceptions for aircraft in places like Transforms.

This comment has been minimized.

Copy link
@pchote

pchote May 26, 2019

Member

My main motivation for suggesting we drop the facing changes during takeoff is that the fly code is too broken to tackle in one PR, and each time through review we seem to find another direction or set of issues to scope creep it on. This is an area where we can at least keep the previous behaviour without introducing anything new to complain about, then the behaviour on both takeoff and land can be addressed together in a later PR.

This comment has been minimized.

Copy link
@tovl

tovl May 26, 2019

Contributor

I agree that endlessly debating this isn't getting us anywhere. We should probably leave this as is and get this and #16509 merged quickly, so we can tackle these loose ends in one PR. Throwing away the facing changes during take-off would be a mistake though. Better to keep what we have here, so we have something to work from and polish later.

This comment has been minimized.

Copy link
@pchote

pchote May 26, 2019

Member

I've been assuming that turning off the facing adjustment can be done by removing the var desiredFacing = delta.HorizontalLengthSquared != 0 ? delta.Yaw.Facing : aircraft.Facing; line and calling VerticalTakeOffOrLandTick with aircraft.Facing instead. This can be trivially restored in the followup.

This comment has been minimized.

Copy link
@tovl

tovl May 26, 2019

Contributor

You're right. If it is just the one line, then that isn't a big deal.

OpenRA.Mods.Common/Activities/Air/Land.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Activities/Air/Fly.cs Outdated Show resolved Hide resolved

@reaperrr reaperrr force-pushed the reaperrr:merge-fly-pt1 branch 2 times, most recently from fbcc130 to 0ba8359 May 26, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2019

Updated with the two suggested code parts by @pchote.

Not sure what else I should change right now.

@reaperrr reaperrr force-pushed the reaperrr:merge-fly-pt1 branch from 40ab65e to dfe24aa Jun 3, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

For some reason Apaches now stack onto each other.

from IRC:

the problem is that 2dddc03#diff-558171f0691cfeefaa697d6b9dd0734bR174 doesn't make sure that delta contains no non-zero Z component

Fixed.

@abcdefg30
Copy link
Member

left a comment

I think this regresses airfield reservations. Testcase: Start the first Soviet mission and keep the planes in the air. Spend a bit of ammo on two of them and send the first to rearm. After that plane is rearmed, let it take off and send the second plane to the same airfield. It can't land.

@pchote

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

I'm not able to repro the issue. Soviet01 disables the airfield rally point, so the plane will not take off automatically if ordered to return. It takes off automatically if you queue attack orders or force-attack the ground and wait for it to run out of ammo, but correctly releases the reservation in this case.

@pchote

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

21:25 <+pchote> now i can't get the planes to land at all because they are being repulsed out of the map
21:25 <+abcdefg30> are you sure
21:25 <+abcdefg30> that sounds what I was experiencing
21:25 <+abcdefg30> move them back inside
21:26 <+abcdefg30> and try to let them land
21:26 <+abcdefg30> (again from inside)
21:26 <+abcdefg30> because the airfields are at the border, the planes idle outside of the map
21:26 <+pchote> oh
21:26 <+pchote> more english fail
21:26 <+pchote> i thought you were saying that the reservation was blocked
21:26 <+pchote> so you don't get the land cursor
21:26 <+abcdefg30> oh fail
21:26 <+abcdefg30> sorry
21:27 <+abcdefg30> yes you get the land cursor but they don't land
21:27 <+abcdefg30> unless you let the original yak land on another airfield iirc
21:27 <+abcdefg30> (which led me to believe reservations are borked)
@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

I think this regresses airfield reservations. Testcase: Start the first Soviet mission and keep the planes in the air. Spend a bit of ammo on two of them and send the first to rearm. After that plane is rearmed, let it take off and send the second plane to the same airfield. It can't land.

See reproduction below.

takeoffpr

I clicked on the airfield with the "blocked" cursor before the plane landed.

@pchote

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

The second yak holds the Reservable reservation on the airfield, but is unable to land. I'm pretty sure this means that the first yak did not remove its cell reservation in the ActorMap.

@pchote

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

Ok, this is an interesting one. Something cancels the TakeOff activity before it can first tick. I haven't debugged deep enough to find out exactly what, yet.

Canceling the activity changes its .State from Queued to Canceling, which causes Activity.TickOuter to completely skip OnFirstRun! This is a fairly serious activity plumbing bug that we'll need to fix - maybe in #16481, or maybe in its own PR.

TakeOff.Tick doesn't check for IsCanceling, so one option could be to set IsInterruptable = false. The problem here is that this would also make the attack-move-to-rallypoint uninterruptable, which may be just as bad?

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

The problem here is that this would also make the attack-move-to-rallypoint uninterruptable, which may be just as bad?

Yes, this won't work from a gameplay perspective. Could we move the attack-move-to-rallypoint part out of the takeoff activity?

@pchote

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

This is another double-takeoff issue, the same as the rally point one earlier.

Adding skipTakeOff: true to the Unreserve calls in Aircraft's Move/Enter/Repair/ReturnToBase order handling and Aircraft.Nudge should fix this and the other issues that we didn't discover during ingame testing.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

Canceling the activity changes its .State from Queued to Canceling, which causes Activity.TickOuter to completely skip OnFirstRun! This is a fairly serious activity plumbing bug that we'll need to fix - maybe in #16481, or maybe in its own PR.

FWIW, that was a known issue to me and mentioned by @obrakmann before as well, because that very bug was the reason we moved the re-queueing of unfinished Resupply from ResolveOrder to OnBecomingIdle. I thought we had a ticket for this, but I guess with all the changes around the activity plumbing I lost track of what bugs were known, what bugs were already fixed etc.

@pchote

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

Interesting. I either had no idea or completely forgot about it.

With the activity rework pushing more important things into OnFirstRun this has become a critical issue to fix before the playtest, but certainly not here.

@pchote

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

The fix (5b15387) is simple, so I can file a PR with this and commits to undo the skipTakeOff: true changes here and maybe the Resupply change after this one is merged.

@reaperrr reaperrr force-pushed the reaperrr:merge-fly-pt1 branch from dfe24aa to 2953436 Jun 5, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

Adding skipTakeOff: true to the Unreserve calls in Aircraft's Move/Enter/Repair/ReturnToBase order handling and Aircraft.Nudge should fix this and the other issues that we didn't discover during ingame testing.

Updated, except I approached it the other way round: I turned the take-off into an opt-in.
There are only 2 situations where we actually want that automatic take-off:

  • if MayYieldReservation is true and some other aircraft requests reservation
  • if the reserved actor becomes invalid (destroyed, sold etc.) while we're on it

So making it opt-in considerably reduced the diff, as there are just 2 places in Reservable that need it set to true now.

@matjaeck
Copy link
Contributor

left a comment

LGTM.

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

@pchote
pchote approved these changes Jun 7, 2019
Copy link
Member

left a comment

This has been blocking progress for too long, so lets push any further regression fixes to followup PRs.

@pchote pchote merged commit 8589e26 into OpenRA:bleed Jun 7, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@matjaeck matjaeck referenced this pull request Jun 9, 2019
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.