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 HeliReturnToBase into ReturnToBase #16241

Merged
merged 6 commits into from Apr 13, 2019

Conversation

@reaperrr
Copy link
Contributor

commented Feb 24, 2019

Most other aircraft activities will have to be merged in a single PR and/or after the idle aircraft refactor, since they're too intertwined to feasibly split them across multiple PRs.
But the RTB activities were comparatively self-contained, so I decided to pull this in before the other aircraft refactorings to make the later PRs a bit smaller.

Depends on #16206.
Sort of split from #15803 (with some improvements, and some changes due to other activities not being merged yet).

Fixes #16377.

@reaperrr reaperrr added this to the Next + 1 milestone Feb 24, 2019

@reaperrr reaperrr force-pushed the reaperrr:merge-RTBs branch from ce8ab07 to d44ddae Mar 10, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2019

Rebased & updated.

@tovl Could you help reviewing this, especially the queueing part? I'm not 100% confident in that myself.

OpenRA.Mods.Common/Activities/Air/ReturnToBase.cs Outdated Show resolved Hide resolved
}
else if (nearestResupplier == null && aircraft.Info.VTOL && aircraft.Info.LandWhenIdle)
{
// Using Queue instead of QueueChild here is intentional, as we want VTOLs with LandWhenIdle to land and stay there in this situation

This comment has been minimized.

Copy link
@tovl

tovl Mar 10, 2019

Contributor

It might be nicer to have them return to a fallback position (such as the player spawn point or construction yard) instead of landing on the spot as that is usually the exact opposite of what you want when ordering return to base in such a situation. This is probably slightly out of scope though.

This comment has been minimized.

Copy link
@reaperrr

reaperrr Mar 10, 2019

Author Contributor

I agree about the idea, but that's what #15873 (or a follow-up to that) will take care of, so I'd like to leave this as-is in this PR.

OpenRA.Mods.Common/Activities/Air/ReturnToBase.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Activities/Air/ReturnToBase.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Air/Aircraft.cs Outdated Show resolved Hide resolved

@reaperrr reaperrr force-pushed the reaperrr:merge-RTBs branch from d44ddae to 1d6900d Mar 10, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2019

Rebased & updated.

@tovl
Copy link
Contributor

left a comment

Looks good. I just spotted a few more small things.

OpenRA.Mods.Common/Activities/Air/ReturnToBase.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Activities/Air/ReturnToBase.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Air/Aircraft.cs Outdated Show resolved Hide resolved

@reaperrr reaperrr force-pushed the reaperrr:merge-RTBs branch from 1d6900d to fabe33f Mar 11, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

Updated.

@abcdefg30
Copy link
Member

left a comment

You can no longer abort a resupply.

@tovl

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

I suspect that the problem is with the fact that ResupplyAircraft requeues itself after being cancelled without a queued activity. Because ReturnToBase queues it as a childactivity, at the end of the queue, this means ReturnTobase keeps going because it's childactivity is never null.

Curiously, Line 803-807 in Aircraft.cs already seems to take care of rerunning ResupplyAircraft in this case. Yet another instance of two bits of code trying to do the same thing. I think it would be best to remove the offending code from ResupplyAircraft and not let that activity bother with this eventuality at all.

@reaperrr reaperrr force-pushed the reaperrr:merge-RTBs branch from fabe33f to c086170 Mar 14, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

Updated.

I think it would be best to remove the offending code from ResupplyAircraft and not let that activity bother with this eventuality at all.

I think that work-around was done to effectively prevent players from accidentally canceling ResupplyAircraft by pressing 'Stop' while the actor resupplies.

I have an idea for a better, unhacky fix for that, but that's out of scope for this PR so I just removed the work-around for now.

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2019

Do you want to reopen #15055 for the meantime?

@matjaeck
Copy link
Contributor

left a comment

Noticed the following while testing:

  • When you order a helicopter or yak to RTB and then give a move order, the unit goes idle and the move order is ignored.
  • Build one airfield / one helipad and let 2 aircraft force attack somewhere. When they RTB, only one will land to rearm while the other stays idle. This also happens when you build a second airstrip / helipad while they RTB - units won't land there.

Can't reproduce these issues on bleed.

@tovl tovl referenced this pull request Mar 16, 2019
@tovl

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2019

It should still check for NextActivity if ticks is set to -1 though.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2019

Why? It doesn't on bleed, and I don't see were we would need that.

@reaperrr reaperrr force-pushed the reaperrr:merge-RTBs branch from a7b7bc5 to 40d41f9 Mar 31, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2019

Updated with what I have, the two issues @obrakmann pointed out are fixed with this.

Also added a commit that makes the current child activity visible with ActorTags checked.
Will come in its own PR.

@reaperrr reaperrr force-pushed the reaperrr:merge-RTBs branch from 40d41f9 to 5a44d40 Mar 31, 2019

@reaperrr reaperrr force-pushed the reaperrr:merge-RTBs branch from 5a44d40 to 0f5e0c2 Mar 31, 2019

@matjaeck
Copy link
Contributor

left a comment

Can confirm the fix for the mentioned issues. Another issue I encountered is that planes that are idle in air seem to ignore queued orders, which doesn't happen on current bleed.

Improve ReturnToBase Activity.cs adherance
And make it use child activities for queueability.

@reaperrr reaperrr force-pushed the reaperrr:merge-RTBs branch from 0f5e0c2 to 1b3893b Mar 31, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2019

It should still check for NextActivity if ticks is set to -1 though.

You were right, of course, this is what caused the issue @matjaeck pointed out.

Updated, should be fixed without regressions (fingers crossed...).

@matjaeck
Copy link
Contributor

left a comment

Tested in RA.

Make aircraft without TakeOffOnResupply always land
on resupplier, even if ammo is full, when given a
"ReturnToBase" order via deploy key.
In other words, in that case treat the RTB order like an
explicit "Repair" or "Enter" order.
@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2019

Minor addition: Aircraft with TakeOffOnResupply: false (for example RA MiG and Yak) and full ammo now land on a free resupplier when given "ReturnToBase" order via command bar / hotkey, instead of circling over some free resupplier just because they don't need to reload.
Basically, for aircraft with TakeOffOnResupply: false, "ReturnToBase" now behaves more like "Enter".

Fix ReturnToBase restarting on each hotkey press
Now that the RTB process is a single activity with childs,
it's relatively easy to prevent the activity from restarting
every time the deploy/RTB hotkey is pressed.
@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2019

Updated, added another small fix: the ReturnToBase activity is no longer restarted on "ReturnToBase" order/hotkey if the activity is already running, preventing additional key presses from delaying the arrival.

@matjaeck
Copy link
Contributor

left a comment

Both changes look good to me.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

Since it's another easy one, I added a fix for #16377 as well. Should be the last scope-creep though, anything else should be deferred to a follow-up.

@tovl
tovl approved these changes Apr 13, 2019
Copy link
Contributor

left a comment

Looks good to me.

@reaperrr reaperrr merged commit 840eb70 into OpenRA:bleed Apr 13, 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
6 participants
You can’t perform that action at this time.