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

Add IdleBehavior to Aircraft #16695

Merged
merged 4 commits into from Jul 21, 2019

Conversation

@reaperrr
Copy link
Contributor

commented Jun 14, 2019

Replacing the LandWhenIdle boolean, ReturnOnIdle and LeaveOnIdle traits and the "hover or fly in circles on idle" part of the former CanHover by an IdleBehavior field.

Note 1: In my opinion it's best to allow only one default idle behavior and fallback to hover/circle (depending on whether the aircraft has the Hover FlightDynamic) if the idle action is impossible (which, out of the current options, in practice can only happen with Land and ReturnToBase).

Note 2/'fun' fact: ReturnOnIdle is horribly hardcoded and non-functional on bleed: its legacy code is only designed for non-hovering actors (there was no HeliFly support even before that got merged into Fly), yet it never works because idle non-hover aircraft immediately start to FlyCircle, so TickIdle never triggers. Hence why I removed it from RA Mig/Yak without setting the RTB idle flag.

Depends on #16685.

Closes #16338.

@pchote

This comment has been minimized.

Copy link
Member

commented Jun 29, 2019

Can we defer this to Next + 1, or do you think its important for wrapping up the aircraft work in Next?

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2019

I'm a bit divided between "it's strictly speaking not that important" and "if #16685 gets merged soon, it will be only a single commit of acceptable size/complexity, so it would be nice to get this into the pt if no noticable regressions or conceptual issues are found during review".

@reaperrr reaperrr force-pushed the reaperrr:air-IdleBehavior branch from 92d9024 to 02223f7 Jul 4, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

Rebased & updated.

@reaperrr reaperrr force-pushed the reaperrr:air-IdleBehavior branch from 02223f7 to 16bdc4d Jul 4, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

Rebased.

OpenRA.Mods.Common/Traits/Air/Aircraft.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Activities/Air/Fly.cs Outdated Show resolved Hide resolved
@pchote

This comment has been minimized.

Copy link
Member

commented Jul 7, 2019

it never works because idle non-hover aircraft immediately start to FlyCircle, so TickIdle never triggers.

Isn't this #16197? Does anything change wrt these PR changes when that is fixed?

@pchote
Copy link
Member

left a comment

Shouldn't we have an update rule here? It could be combined with MoveAbortOnResupply from #16721...

OpenRA.Mods.Common/Traits/Air/Aircraft.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Air/Aircraft.cs Outdated Show resolved Hide resolved
}
else if (Info.IdleBehavior == IdleBehaviorType.ReturnToBase)
{
// If we're already on a resupplier, do nothing

This comment has been minimized.

Copy link
@pchote

pchote Jul 7, 2019

Member

Should we care about FlightDynamic.TakeOffOnResupply here?

This comment has been minimized.

Copy link
@reaperrr

reaperrr Jul 18, 2019

Author Contributor

Not sure what you mean...?

OpenRA.Mods.Common/Traits/Air/Aircraft.cs Outdated Show resolved Hide resolved
@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2019

Isn't this #16197? Does anything change wrt these PR changes when that is fixed?

Not as far as I can tell.
ReturnOnIdle is removed by this PR, and Aircraft uses OnBecomingIdle instead of the first TickIdle tick to trigger those activities/behaviors.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2019

Waiting with rebase/update until #16768 and #16721 are merged to minimize rebases.

Remove AbortOnResupply from Aircraft again
This was accidentally re-added during a rebase in
a previous PR.

@reaperrr reaperrr force-pushed the reaperrr:air-IdleBehavior branch from 16bdc4d to d015c00 Jul 18, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

Rebased and updated.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

Updated, now also removes the re-queueing of Resupply hack, instead Attack* traits and TransformsIntoMobile no longer cancel a running Resupply on "Stop" order.

reaperrr added 3 commits Jun 10, 2019
Remove Resupply re-queueing hack from Aircraft
By preventing that other traits can remotely cancel Resupply
or ReturnToBase.
Set InitialFacing for TS aircraft to 224
North-East looks better as starting and landing facing.

@reaperrr reaperrr force-pushed the reaperrr:air-IdleBehavior branch from c24795b to a35b7fa Jul 19, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2019

Updated.

reaperrr added a commit to reaperrr/OpenRA that referenced this pull request Jul 19, 2019
Change Resupply closEnough check to return true on negative
...values, instead of 'zero'.

Returning 'true' at a distance of zero was a legacy left-over
that isn't used anymore once OpenRA#16695 is merged.
@pchote

This comment has been minimized.

Copy link
Member

commented Jul 20, 2019

Typo in the last commit message closEnough

@pchote
pchote approved these changes Jul 20, 2019

@pchote pchote added the PR: Needs +2 label Jul 20, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2019

Typo in the last commit message closEnough

That's in #16789, actually :-)

reaperrr added a commit to reaperrr/OpenRA that referenced this pull request Jul 20, 2019
Change Resupply closeEnough 'infinite' to negative
...instead of 'zero'.

Returning 'true' at a distance of zero was a legacy left-over
that isn't used anymore once OpenRA#16695 is merged.
@abcdefg30
Copy link
Member

left a comment

Imho IdleBehavior: Land is a bit confusing since it does not land on idle, only when force moving or (un)loading.

abcdefg30 added a commit that referenced this pull request Jul 21, 2019
Change Resupply closeEnough 'infinite' to negative
...instead of 'zero'.

Returning 'true' at a distance of zero was a legacy left-over
that isn't used anymore once #16695 is merged.
@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2019

Imho IdleBehavior: Land is a bit confusing since it does not land on idle, only when force moving or (un)loading.

Right now, only the TS Drop Pod (whose rules are bogus anyway, we should remove this as the superweapon will likely be implemented in a different way) and Dropship use it. I tested it with the Orca, and it works fine when enabled.

@abcdefg30

This comment has been minimized.

Copy link
Member

commented Jul 21, 2019

My bad, messed the testing up.

@abcdefg30 abcdefg30 merged commit f25449a into OpenRA:bleed Jul 21, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abcdefg30

This comment has been minimized.

Copy link
Member

commented Jul 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.