Updated.

We are missing the target lines when ordering repairing. Is this intended and will be fixed with #16549?

That wasn't intended, no.

Updated.

Helicopters in RA don't take off at all if you queue a reload and repair activity.

Not this PR's fault and intentional, see discussion in #16660. We can still change that back after the first playtest if the feedback on that is negative.

As far as this PR is concerned, manually set AbortOnResupply: false if you need a test case for aircraft (works fine according to my testing).

Edit: Ah wait, nevermind, they should at least take off, right.

As discussed on IRC, fixing that in a non-work-around way requires #16509 to be finished and merged first.

#16509 has been merged.

23:24 <+pchote> re the comments in #16721, I have a sinking feeling that we may end up having to rework AbortOnResupply before the end of the playtest series
23:24 <@orabot> Pull request #16721 (open) by reaperrr: Make repair/rearm of ground units queueable | http://bugs.openra.net/16721
23:25 <+reaperrr> ugh but yeah, am not that surprised
23:25 <+pchote> if you have a damaged aircraft and want to queue a repair/reload and then move somewhere that isn't the rallypoint then this really should be possible
23:25 <+pchote> and this is independent from whether they break off after a single attack run
23:26 <+pchote> tovl reworks the flag a bit in #16481
23:26 <@orabot> Pull request #16481 (open) by tovl: Move ChildActivity handling into base Activity class | http://bugs.openra.net/16481
23:27 <+pchote> I expect that after that has been merged that we'll need to take it further and probably move it to AttackAircraft
23:27 <+pchote> have it abort FlyAttack only, not the whole quue
23:27 <+pchote> *queue
23:28 <+reaperrr> yeah

Updated. Fingers crossed that the last commit might already be enough.

Needs a rebase after #16481 was merged.

The fix works. However, aircraft with TakeOffOnResupply == false will not move to the rally point when taking off from the repair bay.

I also noticed that when an aircraft with TakeOffOnResupply == true is made to resupply using the force-enter command and the stop command is given it will forget to supress the TakeOff. This is due to the Resupply activity being reqeued without the stayOnResupplier parameter.

The fix works. However, aircraft with TakeOffOnResupply == false will not move to the rally point when taking off from the repair bay.

Aircraft.GetActorBelow() only looks for Reservable actors, and Reservable in its current form won't mesh with repair-only hosts. I'd like to leave this as "known issue, but not serious enough to be a release blocker" and adress it properly during the next release cycle.

I also noticed that when an aircraft with TakeOffOnResupply == true is made to resupply using the force-enter command and the stop command is given it will forget to supress the TakeOff. This is due to the Resupply activity being reqeued without the stayOnResupplier parameter.

Fixing this properly would likely require to remove the hacky re-queueing of Resupply, which in turn would require Attack* traits to only cancel their own activities but not the entire queue on "Stop" orders, which would require us to refactor Actor.CancelActivity().
tl;dr: Another issue I'd like to defer to Next+1.

I agree with deferring these issues to Next+1.

As far as refactoring Actor.CancelActivity() is concerned, I already have a potential solution in mind that would not require us touching stop orders or requeueing Resupply but would require reworking Actor.QueueActivity(). I will try that out as soon as this is merged.

Found another bug: In D2K, orders queued after the repair order get ignored when the unit has been carried by a carryall.

I'm pretty sure it was that way before this PR already.
I could pass a sequence of Resupply, NextActivity instead of only Resupply through RequestTransport, not sure if that also implicitly passes any activities queued after NextActivity, though.

Any activity in the queue carries a reference to the next activity in the queue. So passing NextActivity would also implicitly pass NextActivity.NextActivity, NextActivity.NextActivity.Nextactivity, etc..., so I suspect this will work.

OK, not that easy, unfortunately. Resupply triggers the request right on its first tick, so any activity queued after the transport was requested is ignored. I think this might require refactoring how RequestTransport works, but that would be out of scope for this PR.

I think this might require refactoring how RequestTransport works, but that would be out of scope for this PR.

When the time comes, this can be done the same way I was arguing the transport locking for passengers should be done. Right now the carryall makes the requesting actor cancel its activity and queues a WaitFor until it has picked it up. Instead, Carryable should grant a condition that pauses Mobile so that the activity queue doesn't need to be touched at all.

Can we at least leave that to a follow-up? I'm literally getting a headache right now so I won't get that done today, and as long as this PR drags on I won't make progress on #16695's update rule, either.

"When the time comes" are the magic words 😄 IMO that is a job for Next + 1.

I was referring to the "right now" as well ;)

Fixed.

https://github.com/OpenRA/OpenRA/wiki/Changelog-(bleed)/_compare/bf379b06d37d013355fb28563aaa4ce4263b801b