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

Revert FlightDynamics and add CanSlide #16768

Merged
merged 4 commits into from Jul 18, 2019

Conversation

@reaperrr
Copy link
Contributor

commented Jul 12, 2019

This reverts most of #16685, but keeps the hover/slide separation and ties the latter to a new CanSlide boolean for now.

Additionally, some manual improvements on D2k and TS were kept, therefore at least these two should be tested for regressions (making sure RA/TD still work fine is recommended ayway, of course).

It would be good to get this merged quickly.

@reaperrr reaperrr added this to the Next Release milestone Jul 12, 2019

mods/ts/rules/aircraft.yaml Outdated Show resolved Hide resolved
@teinarss

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2019

Did not see any regressions in TD

@reaperrr reaperrr force-pushed the reaperrr:partial-FD-revert branch from 41f910e to 1a240d6 Jul 13, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2019

Updated.

@tovl

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2019

Orca Bombers and Banshees should probably not have TakeOffOnResupply (not sure about Orca Fighters and Harpies).

Also noticed that Banshees and Orca Bombers will hover over the helipad when ordered to resupply with full ammo. Fixing that would be beyond the scope of this PR I think, but without TakeOffOnResupply the issue would be circumvented anyway.

Noticed no issues in D2K.

@reaperrr reaperrr force-pushed the reaperrr:partial-FD-revert branch from 1a240d6 to 6d77fd8 Jul 14, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jul 14, 2019

Updated.

Removed the TakeOffOnResupply: true from default and only added it to Orca and Harpy.

@tovl
Copy link
Contributor

left a comment

Ok. All good. 👍

@pchote

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

Needs a rebase now.

@reaperrr reaperrr force-pushed the reaperrr:partial-FD-revert branch 2 times, most recently from b43a35f to 56f0a53 Jul 16, 2019

OpenRA.Mods.Common/Activities/Air/Fly.cs Outdated Show resolved Hide resolved
@@ -161,22 +161,22 @@ public override bool Tick(Actor self)
if (insideMaxRange && !insideMinRange)
return true;

var isSlider = aircraft.Info.FlightDynamics.HasFlag(FlightDynamic.Slide);
var isSlider = aircraft.Info.CanSlide;

This comment has been minimized.

Copy link
@pchote

pchote Jul 16, 2019

Member

Probably here too, but this is used in several places so could go either way.

This comment has been minimized.

Copy link
@reaperrr

reaperrr Jul 16, 2019

Author Contributor

I've left this as-is for now.

@@ -91,9 +69,33 @@ public class AircraftInfo : ITraitInfo, IPositionableInfo, IFacingInfo, IMoveInf
[Desc("The condition to grant to self while at cruise altitude.")]
public readonly string CruisingCondition = null;

[Desc("Can the actor hover in place mid-air? If not, then the actor will have to remain in motion (circle around).")]
public readonly bool CanHover = false;

This comment has been minimized.

Copy link
@pchote

pchote Jul 16, 2019

Member

Would Hovers and Slides be more consistent with the other parameter names?

This comment has been minimized.

Copy link
@reaperrr

reaperrr Jul 16, 2019

Author Contributor

Maybe, but since both will go away for good next dev cycle, I feel like renaming CanHover now after all these years would just cause unnecessary work for no real benefit, so IMO let's just leave them be until they get replaced by IdleSpeed/SlideRadius (or whatever we'll end up calling their replacements).

This comment has been minimized.

Copy link
@pchote

pchote Jul 16, 2019

Member

Ok, thats fine by me.

reaperrr added 3 commits Jul 12, 2019
Revert FlightDynamics
This needs more thought and most parts might get superseded
by other approaches.

Kept CanSlide separation from CanHover.
Revert FlightDynamics yaml changes
- TD and RA were straight up reverted.

- D2k was manually reverted with following changes:
-- Frigate staying VTOL.
-- Carryall staying CanSlide: true but CanHover: false.
-- bogus VTOL/CanHover flags on Ornithopter staying removed.

- TS was manually reverted to keep the behavior
improvements of the original FlightDynamics PR.

@reaperrr reaperrr force-pushed the reaperrr:partial-FD-revert branch from 56f0a53 to 36236a8 Jul 16, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2019

Updated.

@abcdefg30 abcdefg30 merged commit 4bf659c into OpenRA:bleed Jul 18, 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 18, 2019

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.