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

Improve ResupplyAircraft #16321

Merged
merged 3 commits into from Mar 30, 2019

Conversation

@reaperrr
Copy link
Contributor

commented Mar 17, 2019

This resolves two dependencies to make ResupplyAircraft fully compatible with use as ChildActivity (particularly in ReturnToBase).

  1. Replaced the work-around for #15055, as it causes complications in the above use-case.
    ResupplyAircraft is now immediately queued again from OnBecomingIdle if the actor hasn't finished rearming/repairing yet.

  2. On bleed, ResupplyAircraft (or rather its child activity WaitFor) continues to run until the actor is given an order or forced to yield reservation.
    This makes it hard to use it as-is as ChildActivity (for example in ReturnToBase), because it makes its parent implicitly continue until the same conditions are met.
    By instead preventing auto-TakeOff in Aircraft.OnBecomingIdle when TakeOffOnResupply is set to false, ResupplyAircraft itself no longer needs to take care of that, allowing it to finish properly as soon as the actor finishes resupply.

Split from #16241 and therefore dependency for it.

@tovl

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2019

Works as advertised. Just an idea though: I'm thinking that maybe simply letting OnBecomingIdle queue ResupplyAircraft when the aircraft is positioned on an appropriate resupply building would solve #15055 in just a few extra lines.

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2019

Both issues that I mentioned in #16241 are fixed here. This also changes the behavior described in #16320: Airplanes will now always continue their attack-move after reloading, regardless of the number of airfields. When they do not find a target, they RTB but immediately take off again and continue their attack-move. See https://imgur.com/a/9opZWcJ.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2019

Both issues that I mentioned in #16241 are fixed here.

The issues were caused by the changes in ReturnToBase, not in ResupplyAircraft, so they're not expected to show up here.

When they do not find a target, they RTB but immediately take off again and continue their attack-move.

As long as AbortOnResupply is set to false, that's basically the expected behaviour, I think. At least changing that behavior further is out of scope for this PR.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

Updated.

Just an idea though: I'm thinking that maybe simply letting OnBecomingIdle queue ResupplyAircraft when the aircraft is positioned on an appropriate resupply building would solve #15055 in just a few extra lines.

Done, though it took more than just a few extra lines to (hopefully) get it right...

@reaperrr reaperrr force-pushed the reaperrr:improve-ResupplyAircraft branch from 2d2c50e to cd64c3b Mar 18, 2019

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

The issues were caused by the changes in ReturnToBase, not in ResupplyAircraft, so they're not expected to show up here.

Stupid me.

As long as AbortOnResupply is set to false, that's basically the expected behaviour, I think. At least changing that behavior further is out of scope for this PR.

No idea, it was just something I noticed.

@@ -144,15 +144,19 @@ public Activity()
IsInterruptible = true;
}

// HACK: Ensure that OnFirstRun always triggers before the first Tick(self), even if Actor.CurrentActivity was updated in a 'dirty' way
// that would otherwise skip OnFirstRun (for example through Actor.QueueActivity).

This comment has been minimized.

Copy link
@obrakmann

obrakmann Mar 18, 2019

Contributor

Wait, what? How does Actor.QueueActivity cause skipping OnFirstRun?

This comment has been minimized.

Copy link
@reaperrr

reaperrr Mar 18, 2019

Author Contributor

Well, maybe I misinterpreted the cause, but

self.CancelActivity();
self.QueueActivity(new ResupplyAircraft(self));

results in OnFirstRun not triggering (it seems to go right to Tick instead, at least on bleed/without this commit).

This comment has been minimized.

Copy link
@obrakmann

obrakmann Mar 22, 2019

Contributor

So I just tested on bleed, and I can't reproduce:

Internal mods:
	all: All mods (git-16f1750252)
	cnc: Tiberian Dawn (git-16f1750252)
	d2k: Dune 2000 (git-16f1750252)
	modcontent: Mod Content Manager (git-16f1750252)
	ra: Red Alert (git-16f1750252)
	ts: Tiberian Sun (git-16f1750252)
External mods:
	cnc-git-fd5eaf1f49: Tiberian Dawn (git-fd5eaf1f49)
	d2k-git-09512f0987: Dune 2000 (git-09512f0987)
	ra-playtest-20180729: Red Alert (playtest-20180729)
	ts-git-c1b9df439e: Tiberian Sun (git-c1b9df439e)
	ra-git-16f1750252: Red Alert (git-16f1750252)
Loading mod: ra
System.Net.Sockets.SocketOptionName 0x17 is not supported at IP level
[2019-03-22T21:06:48] Game started
ResupplyAircraft.OnFirstRun()
ResupplyAircraft.Tick()
ResupplyAircraft.Tick()
ResupplyAircraft.Tick()

Do I have to follow any special sequence of events, or is ordering an aircraft to an airfield enough?

This comment has been minimized.

Copy link
@reaperrr

reaperrr Mar 22, 2019

Author Contributor

It may not be reproducable on bleed because on bleed, it uses 'normal' queueing instead of ChildActivity, so unless you give the order when already on the pad/field, ResupplyAircraft has long finished before the aircraft arrives to reload.

To reproduce it on this PR:
Revert the last two commits, compile, then in RA let some aircraft deplete some of its ammo, order it to resupply, and as soon as it touches down on the pad/afld and you see the resupply anim starting, press Stop, so the activity is canceled.
Now press Stop again, and ResupplyAircraft.OnFirstRun() should not trigger, even though the activity was queued after CancelActivity.

This comment has been minimized.

Copy link
@tovl

tovl Mar 23, 2019

Contributor

This is really weird. I poked around a bit and found that ResupplyActivity for some arcane reason goes into the canceling state before it even gets the chance to tick for the first time. However, the moment the activity is queued it is in the queued state as it should. This happens even when there is no previous activity. So somehow something is doing weird things with the activity state somewhere in between those moments.

This comment has been minimized.

Copy link
@obrakmann

obrakmann Mar 24, 2019

Contributor

Why is AttackBase allowed to cancel non-attack orders?

AFAIK we don't check the type of Actor.CurrentActivity anywhere where the Stop order gets resolved, we just call Actor.CancelActivity without regard to that (which is why it gets called multiple times every time you press S, once from each applicable ResolveOrder method). It's only never - until recently - been a problem because we never automatically queued new activities on Stop.

But yes, as tovl mentioned, the FrameEndTask fix is hypothetical since the last commit removes the offending code. But the second-to-last commit should be removed.

This comment has been minimized.

Copy link
@reaperrr

reaperrr Mar 24, 2019

Author Contributor

You can just drop the second to last commit and everything still works fine.

Will do that.

About a future proper fix, I think there's several activities we don't want to cancel on "Stop", but we do want them to cancel on pretty much anything else, so maybe we should chain a bool cancelAttemptByStopOrder as optional parameter from CancelActivity all the way through to Activity, to allow activities to refuse cancellation by "Stop" orders.

We could also pass through the order string to make things more configurable, but that would be a bit more complicated, and I'm not sure we really need that.

This comment has been minimized.

Copy link
@obrakmann

obrakmann Mar 24, 2019

Contributor

About a future proper fix, I think there's several activities we don't want to cancel on "Stop", but we do want them to cancel on pretty much anything else

What other examples are you thinking of besides ResupplyAircraft?

I think the issue Paul is seeing is that traits were stopping an activity unrelated to that trait, not that the activity could be stopped in the first place.

This comment has been minimized.

Copy link
@pchote

pchote Mar 24, 2019

Member

I think the issue Paul is seeing is that traits were stopping an activity unrelated to that trait, not that the activity could be stopped in the first place.

Yeah, exactly.

Re customizing activity stop behaviour: we could implement a Stop method on Activity with a default implementation that cancels the activity. Special cases can then override this to do other things.

This comment has been minimized.

Copy link
@reaperrr

reaperrr Mar 24, 2019

Author Contributor

What other examples are you thinking of besides ResupplyAircraft?

Resupply (once that PR is merged) for ground units and (Heli)Land, at least. There may be one or two more, but these came to mind first.

@reaperrr reaperrr referenced this pull request Mar 22, 2019
0 of 1 task complete

@reaperrr reaperrr force-pushed the reaperrr:improve-ResupplyAircraft branch 2 times, most recently from b7ef76a to 1c468ca Mar 22, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

Rebased & updated.

@reaperrr reaperrr force-pushed the reaperrr:improve-ResupplyAircraft branch from 1c468ca to 8df4e74 Mar 24, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 24, 2019

Updated.

@reaperrr reaperrr force-pushed the reaperrr:improve-ResupplyAircraft branch from 8df4e74 to 3eada55 Mar 24, 2019

@obrakmann
Copy link
Contributor

left a comment

Not sure how we missed that crash the first time around.

{
ChildActivity = ActivityUtils.SequenceActivities(self, aircraft.GetResupplyActivities(host).ToArray());
QueueChild(self, new AllowYieldingReservation(self));
ChildActivity = ActivityUtils.SequenceActivities(self, aircraft.GetResupplyActivities(host).ToArray());

This comment has been minimized.

Copy link
@obrakmann

obrakmann Mar 26, 2019

Contributor

This crashes when there's nothing to resupply:

Exception of type `System.InvalidOperationException`: Sequence contains no elements
  at System.Linq.Enumerable.Aggregate[TSource] (System.Collections.Generic.IEnumerable`1[T] source, System.Func`3[T1,T2,TResult] func) [0x00030] in <fb28f09769054726b681ecb6df11a20b>:0 
  at OpenRA.Traits.ActivityUtils.SequenceActivities (OpenRA.Actor self, OpenRA.Activities.Activity[] acts) [0x00020] in /home/oliver/devel/openra/git/OpenRA.Game/Traits/ActivityUtils.cs:56 
  at OpenRA.Mods.Common.Activities.ResupplyAircraft.OnFirstRun (OpenRA.Actor self) [0x00028] in /home/oliver/devel/openra/git/OpenRA.Mods.Common/Activities/Air/ResupplyAircraft.cs:31 

This comment has been minimized.

Copy link
@reaperrr

reaperrr Mar 26, 2019

Author Contributor

Hm, ok. This should already be an issue on bleed and would be fixed by #16352 anyway, but I guess I can throw in a temporary fix.

This comment has been minimized.

Copy link
@obrakmann

obrakmann Mar 26, 2019

Contributor

No, bleed is fine. The reason this crashes is that you've changed GetResupplyActivities to evaluate whether anything needs to be done. Bleed just returns all the activities.

This comment has been minimized.

Copy link
@reaperrr

reaperrr Mar 26, 2019

Author Contributor

Ah, I see. Well, should be fixed now.

self.QueueActivity(new HeliFlyCircle(self, Info.IdleTurnSpeed > -1 ? Info.IdleTurnSpeed : TurnSpeed));
}

This comment has been minimized.

Copy link
@obrakmann

obrakmann Mar 26, 2019

Contributor

I'm really looking forward to #15873, this whole chain of conditionals is so horrible 🤣

This comment has been minimized.

Copy link
@reaperrr

reaperrr Mar 26, 2019

Author Contributor

Yeah. I'm actually thinking about pulling in #15803 first because less activities + properly untangled CanHover/VTOL would make #15873 a bit simpler than it would currently be.

@reaperrr reaperrr force-pushed the reaperrr:improve-ResupplyAircraft branch from 3eada55 to d8d10e4 Mar 26, 2019

@reaperrr reaperrr requested a review from obrakmann Mar 26, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

Updated, crash should be fixed now.

@reaperrr reaperrr force-pushed the reaperrr:improve-ResupplyAircraft branch from d8d10e4 to f9b37d6 Mar 26, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

Updated.

@obrakmann
Copy link
Contributor

left a comment

Looks fine to me now, 👍

@tovl
Copy link
Contributor

left a comment

This fixes #15055 and found no regressions. 👍

Simplify ResupplyAircraft
By moving part of the take-off prevention (when TakeOffOnResupply
is set to false) to Aircraft.

Main reason & advantage is that dropping the 'WaitFor' child
makes this activity always end when resupplies are done,
which makes it more compatible with being queued as ChildActivity
itself (for example by ReturnToBase).
reaperrr added 2 commits Mar 17, 2019
Fix ResupplyAircraft being cancelable by Stop command
It is now immediately queued again, as long as the actor
has not finished rearming/repairing yet.

@reaperrr reaperrr dismissed stale reviews from tovl and obrakmann via 6c952eb Mar 30, 2019

@reaperrr reaperrr force-pushed the reaperrr:improve-ResupplyAircraft branch from f9b37d6 to 6c952eb Mar 30, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2019

Rebased.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2019

I count 2 approvals, so lgtm.

@reaperrr reaperrr merged commit 32ab822 into OpenRA:bleed Mar 30, 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
5 participants
You can’t perform that action at this time.