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

Allow explicit landing orders to be disabled in yaml. #16764

Merged
merged 1 commit into from Jul 19, 2019

Conversation

@tovl
Copy link
Contributor

commented Jul 10, 2019

Fixes #16762.

Adds a boolean parameter to the Aircraft trait to disable or enable the force landing logic.

@pchote pchote added this to the Next Release milestone Jul 10, 2019

@@ -97,6 +97,9 @@ public class AircraftInfo : ITraitInfo, IPositionableInfo, IFacingInfo, IMoveInf
[Desc("Does this actor cancel its previous activity after resupplying?")]
public readonly bool AbortOnResupply = true;

[Desc("Can this actor be given an explicit land order using the force-move modifier?")]
public readonly bool ForceLanding = true;

This comment has been minimized.

Copy link
@pchote

pchote Jul 10, 2019

Member

Would this make sense as a FlightDynamics flag?

This comment has been minimized.

Copy link
@tovl

tovl Jul 10, 2019

Author Contributor

That was my first instinct, until halfway through writing it I realized that if you have an exception unit you can't just inherit the FlightDynamics from the default and change just the one parameter. You have to repeat all the completely unrelated parts of FlightDynamics as well.
Furthermore, you can't have a parameter default to true, so unless I reword this into something awkward every unit in every mod will have to be adapted to explicitly allow it. Combined with the first point, there is lot's of potential for bugs due to oversights with units that already have some exceptions.

To be honest, I'm not sure if FlightDynamics was actually a good idea in the first place.

This comment has been minimized.

Copy link
@abcdefg30

abcdefg30 Jul 10, 2019

Member

Wouldn't something like DisableForceLanding work for inverting the default?

This comment has been minimized.

Copy link
@pchote

pchote Jul 10, 2019

Member

This raises a good point that, now thinking about it, I am also not completely happy with.

Merging the flags that define the actual movement characteristics was IMO a good thing. The potential problem is that we included flags that aren't strictly related - MoveIntoShroud, maybe also TakeOffOnResupply and TakeOffOnCreation.

So I think it makes sense to keep ForceLanding separate (but maybe rename to EnableForceLanding?), but also move MoveIntoShroud and maybe TakeOffOn* back to their own flags for consistency. What do you think @reaperrr?

This comment has been minimized.

Copy link
@reaperrr

reaperrr Jul 12, 2019

Contributor

I'm fine with moving MoveIntoShroud back. I'm a bit divided on moving back the TakeOff*s, because strictly speaking then it would be more consistent to move the TurnTo*s back as well, and possibly even VTOL as that's als take-off/land-related. At which point on of the main purposes of that PR - reducing the amount of fields - would be more than half-way defeated...

Could we maybe compromise like this?

  • Leave the order-related fields as booleans (move back MoveIntoShroud and let this new DisableForceLanding stay)
  • split the take-off/landing-related dynamics to a separate TakeOffAndLandDynamics (I'd file a PR for that myself)
    That would at least reduce the number of flags you have to re-list every time you change a flag and make the respective *Dynamics field name more accurate concerning what it does.

This comment has been minimized.

Copy link
@pchote

pchote Jul 12, 2019

Member

My thought wrt splitting the two TakeOffOn* flags is that they relate to when the aircraft should take off rather than how it takes off. IMO the VTOL and TurnToLand/TurnToDock flags are just as much a part of the dynamics as Slide or Hover, so I don't like the idea of moving them into their own field.

We could have a TakeOffOn enum instead (opening up options for Damage, Capture, etc), but if this is going to be controversial then it may be easier to keep them on FlightDynamics...

This comment has been minimized.

Copy link
@tovl

tovl Jul 12, 2019

Author Contributor

Changed the name to EnableForceLanding.

Revising the other flags and booleans is out of scope for this PR. However, to add my perspective:

As far as I'm concerned only VTOL, Slide and Hover (and maybe TurnToLand) belong in FlightDynamics.

TurnToDock specifically relates to whether there is a preferred orientation for docking at an airfield or helipad and might be better of as a characteristic of those actors. For example, what might be an appropriate orientation for landing at an airfield might not be for landing at a service depot. Such a parameter would be relevant for ground units as well.

TurnToLand is currently not used by any unit at all and seems to exist only for legacy behaviour.

I can get behind a TakeOffOn enum if we indeed plan to make more variants. I'm just wondering if it would be feasible to make a TakeOffOnCondition parameter instead and to do everything with conditions to make it completely generic.

Of the three flags left, two might in the future be replaced by integer parameters (IdleSpeed and SlideSpeed). So that leaves us with just VTOL...

This comment has been minimized.

Copy link
@reaperrr

reaperrr Jul 13, 2019

Contributor

TurnToLand

Even if we keep the logic, we can replace the boolean with a LandFacing int, as using InitialFacing is a bit counter-intuitive anyway.

This comment has been minimized.

Copy link
@pchote

pchote Jul 13, 2019

Member

InitialFacing really should be removed anyway - it should be up to the thing creating the actor to define its initial facing.

@tovl tovl force-pushed the tovl:disable-forcelanding branch from 2630930 to ca723a5 Jul 12, 2019

@pchote
Copy link
Member

left a comment

LGTM. Just one comment / question

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

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

@tovl tovl force-pushed the tovl:disable-forcelanding branch from ca723a5 to df9c7f5 Jul 17, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2019

Rebased.

@tovl tovl force-pushed the tovl:disable-forcelanding branch from df9c7f5 to 8531002 Jul 18, 2019

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

left a comment

Confirming my 👍 still stands

@abcdefg30 abcdefg30 merged commit ed18ecf into OpenRA:bleed Jul 19, 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 19, 2019

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