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 repair/rearm of ground units queueable #16721

Merged
merged 9 commits into from Jul 15, 2019

Conversation

@reaperrr
Copy link
Contributor

commented Jun 22, 2019

Moves Repairable(Near) activity handling to Resupply activity, plus a few fixes.

Implements first step of #15023 (comment)

Fixes #16462.
Fixes #16463.
Fixes #16761.
Fixes #16492.

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

@reaperrr reaperrr force-pushed the reaperrr:fix-Resupply branch from 4edf277 to b18aa33 Jun 25, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2019

Updated.

@teinarss

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2019

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

@reaperrr reaperrr force-pushed the reaperrr:fix-Resupply branch from b18aa33 to 4be9abb Jun 28, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2019

That wasn't intended, no.

Updated.

@teinarss
Copy link
Contributor

left a comment

LGTM

@abcdefg30
Copy link
Member

left a comment

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

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2019

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.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2019

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

@pchote

This comment has been minimized.

Copy link
Member

commented Jun 30, 2019

#16509 has been merged.

@pchote

This comment has been minimized.

Copy link
Member

commented Jun 30, 2019

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

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

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

@pchote

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

Needs a rebase after #16481 was merged.

@reaperrr reaperrr force-pushed the reaperrr:fix-Resupply branch from 7ef0f5b to e248698 Jul 4, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2019

This does not seem to fix #16463.

Right, forgot that this would require more changes.
I've added a hacky short-term fix for the upcoming release, let's leave the proper fix (TakeOffOn flag list?) for a follow-up.

@tovl

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2019

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.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2019

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.

@tovl

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2019

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.

@tovl

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2019

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

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jul 14, 2019

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.

@tovl

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2019

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.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jul 14, 2019

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.

@pchote

This comment has been minimized.

Copy link
Member

commented Jul 14, 2019

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.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jul 14, 2019

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.

@pchote

This comment has been minimized.

Copy link
Member

commented Jul 14, 2019

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

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jul 14, 2019

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

@tovl
Copy link
Contributor

left a comment

With those known issues deferred to the future, I think this is good to go. 👍

@teinarss
Copy link
Contributor

left a comment

Otherwise it looks good. Did not test the update rules

OpenRA.Mods.Common/Activities/Resupply.cs Outdated Show resolved Hide resolved
Make aircraft always take off after repair
Reservable logic doesn't handle repairs, and we
don't want aircraft to block repair bays etc. until it does.

@reaperrr reaperrr force-pushed the reaperrr:fix-Resupply branch from 2a63d2d to 3fa2406 Jul 15, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

Fixed.

@pchote
pchote approved these changes Jul 15, 2019
Copy link
Member

left a comment

The important issues from earlier reviews appear to have been resolved.

The logic (both code and expected gameplay behaviours) here is complicated, so I expect that more regressions will probably be found in this code. The changes here are a needed step forward, so lets get these in as a first step to all the other fixes discussed above, and any other fixes that might be needed for Next.

Issue has been resolved.

@pchote pchote merged commit c51f2c0 into OpenRA:bleed Jul 15, 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.