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 Land activity self-contained and allow explicit landing orders #16509

Merged
merged 7 commits into from Jun 30, 2019

Conversation

@tovl
Copy link
Contributor

commented May 6, 2019

Depends on #16417 and #16365.

Implements features requested in #16417 (review).
Implements refactor requested in #16365 (comment).
Fixes #16331.
Fixes #12464.
Fixes #12201.
Fixes #4642.
Fixes #12081.
Fixes #11748.
Fixes #16572.
Fixes #16646.

This PR moves all the approach before landing logic into the Land activity itself. This includes flying and turning for VTOL aircraft. It also includes and generalizes the approach logic for non-VTOL aircraft found in ReturnToBase and the logic from DeliverUnit that looks for suitable landing locations. This makes the Land activity completely self-contained and flexible. No other activity should bother with the specifics of landing, but just queue a Land activity with a desired target, offset and facing (all optional parameters) instead.

Furthermore, this implements explicit land orders and reworks the carryall and transport logic to support transport craft remaining airborne when idle.

Changes in behaviour:

  • Orca carryalls, orca transports and chinooks remain airborn when idle.
  • All aircraft with LandableTerraintypes defined can be ordered to land at a location using force-move.
  • All VTOL aircraft in the default mods have LandableTerrainTypes defined and can therefore be ordered to land.
  • When ordered to land at a cell (not a docking actor) occupied by an actor that cannot be crushed or nudged, aircraft will land at a nearby cell instead. (This was generalized from DeliverUnit)
  • When infantry try to enter an aerial transport, it will automatically land and the infantry will wait for it to do so.
  • Carryalls will automatically lift off after picking up or delivering a unit.
  • Scripted unload orders may optionally include a destination and UnloadCargo will then take care of getting close to that destination.
  • Non-VTOL carryalls are fully supported. (Replace Inherits: ^Helicopter with Inherits: ^Aircraft in the TS carryall to test)

@pchote pchote added this to the Next Release milestone May 6, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented May 6, 2019

This will be a very nice way to cap off the aircraft changes for the playtest, so adding to the milestone.

@pchote

This comment has been minimized.

Copy link
Member

commented May 6, 2019

I few issues I noticed while quickly testing:

  • Please add "- Helicopters will land at the target location" to the Force Move command bar button tooltip in RA/TD/TS.
  • Force move no longer works at all for Yak/Mig - they should fall back to a regular move instead.
  • IMO it feels inconsistent to have transport helicopters stay on the ground after anything enters them. By default they should take off again once all reserved passengers have entered them, and after unloading - but care will be needed to not break scripted helicopters in missions.
  • The RA shellmap breaks with a lua error:
    Fatal Lua Error: [string "BindingSupport.lua"]:30: Argument 'Nullable`1 destination' of 'void UnloadPassengers(Nullable`1 destination)' is not optional.
      at OpenRA.Scripting.ScriptContext.FatalError (System.String message) [0x00000] in <1794848e7ea34e06a09d113c2a6c7693>:0 
      at OpenRA.Mods.Common.Scripting.ScriptTriggers.OpenRA.Mods.Common.Traits.INotifyDamage.Damaged (OpenRA.Actor self, OpenRA.Traits.AttackInfo e) [0x00069] in <501f99702a4548b3b3aace44ed353658>:0 
      at OpenRA.Mods.Common.Traits.Health.InflictDamage (OpenRA.Actor self, OpenRA.Actor attacker, OpenRA.Traits.Damage damage, System.Boolean ignoreModifiers) [0x000e9] in <501f99702a4548b3b3aace44ed353658>:0 
      at OpenRA.Actor.InflictDamage (OpenRA.Actor attacker, OpenRA.Traits.Damage damage) [0x00011] in <1794848e7ea34e06a09d113c2a6c7693>:0 
      at OpenRA.Mods.Common.Warheads.SpreadDamageWarhead.DoImpact (OpenRA.WPos pos, OpenRA.Actor firedBy, System.Collections.Generic.IEnumerable`1[T] damageModifiers) [0x00160] in <501f99702a4548b3b3aace44ed353658>:0 
      at OpenRA.Mods.Common.Warheads.DamageWarhead.DoImpact (OpenRA.Traits.Target target, OpenRA.Actor firedBy, System.Collections.Generic.IEnumerable`1[T] damageModifiers) [0x000b1] in <501f99702a4548b3b3aace44ed353658>:0 
      at OpenRA.GameRules.WeaponInfo.Impact (OpenRA.Traits.Target target, OpenRA.Actor firedBy, System.Collections.Generic.IEnumerable`1[T] damageModifiers) [0x00076] in <1794848e7ea34e06a09d113c2a6c7693>:0 
      at OpenRA.Mods.Common.Traits.Explodes.OpenRA.Mods.Common.Traits.INotifyKilled.Killed (OpenRA.Actor self, OpenRA.Traits.AttackInfo e) [0x00138] in <501f99702a4548b3b3aace44ed353658>:0 
      at OpenRA.Mods.Common.Traits.Health.InflictDamage (OpenRA.Actor self, OpenRA.Actor attacker, OpenRA.Traits.Damage damage, System.Boolean ignoreModifiers) [0x00249] in <501f99702a4548b3b3aace44ed353658>:0 
      at OpenRA.Actor.InflictDamage (OpenRA.Actor attacker, OpenRA.Traits.Damage damage) [0x00011] in <1794848e7ea34e06a09d113c2a6c7693>:0 
      at OpenRA.Mods.Common.Warheads.SpreadDamageWarhead.DoImpact (OpenRA.WPos pos, OpenRA.Actor firedBy, System.Collections.Generic.IEnumerable`1[T] damageModifiers) [0x00160] in <501f99702a4548b3b3aace44ed353658>:0 
      at OpenRA.Mods.Common.Warheads.DamageWarhead.DoImpact (OpenRA.Traits.Target target, OpenRA.Actor firedBy, System.Collections.Generic.IEnumerable`1[T] damageModifiers) [0x000b1] in <501f99702a4548b3b3aace44ed353658>:0 
      at OpenRA.GameRules.WeaponInfo.Impact (OpenRA.Traits.Target target, OpenRA.Actor firedBy, System.Collections.Generic.IEnumerable`1[T] damageModifiers) [0x00076] in <1794848e7ea34e06a09d113c2a6c7693>:0 
      at OpenRA.Mods.Common.Projectiles.Bullet.Explode (OpenRA.World world) [0x00032] in <501f99702a4548b3b3aace44ed353658>:0 
      at OpenRA.Mods.Common.Projectiles.Bullet.Tick (OpenRA.World world) [0x002e7] in <501f99702a4548b3b3aace44ed353658>:0 
      at OpenRA.World.<Tick>b__103_1 (OpenRA.Effects.IEffect e) [0x00000] in <1794848e7ea34e06a09d113c2a6c7693>:0 
      at OpenRA.WorldUtils.DoTimed[T] (System.Collections.Generic.IEnumerable`1[T] e, System.Action`1[T] a, System.String text) [0x00015] in <1794848e7ea34e06a09d113c2a6c7693>:0 
      at OpenRA.World.Tick () [0x00180] in <1794848e7ea34e06a09d113c2a6c7693>:0 
      at OpenRA.Game.InnerLogicTick (OpenRA.Network.OrderManager orderManager) [0x00206] in <1794848e7ea34e06a09d113c2a6c7693>:0 
      at OpenRA.Game.LogicTick () [0x0004a] in <1794848e7ea34e06a09d113c2a6c7693>:0 
      at OpenRA.Game.Loop () [0x000e0] in <1794848e7ea34e06a09d113c2a6c7693>:0 
      at OpenRA.Game.Run () [0x0003c] in <1794848e7ea34e06a09d113c2a6c7693>:0 
      at OpenRA.Game.InitializeAndRun (System.String[] args) [0x00010] in <1794848e7ea34e06a09d113c2a6c7693>:0 
      at OpenRA.Program.Main (System.String[] args) [0x00044] in <1794848e7ea34e06a09d113c2a6c7693>:0 
    
    This appears to be a breaking API change, which we try hard to avoid for the Lua API.

@tovl tovl force-pushed the tovl:orderlanding branch from 5912940 to 10166cd May 6, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

Applied all fixups.

Also added an optional delay for unloading transport units, because it kinda looks silly that they speed away in an instant while the passengers are still disembarking. This already became noticeable with the addition of queued unload orders but with chinooks taking off immediately after unloading it's difficult not to notice. I think the default delay already works reasonably well for both chinooks and apc's, but can be tweaked of course.

@Punsho
Copy link
Contributor

left a comment

So far I really enjoy the changes. Bugs:

  • When the chinook is ordered to land with alt-move infantry scatter away from the landing spot. When infantry order the chinook to land, it doesn't do that and just hovers in air.
  • Alt-load doesn't work.
  • If a chinook in air is sharing a cell with another aircraft it will not land when infantry order it to

These bugs most likely also apply to #16417

@tovl tovl force-pushed the tovl:orderlanding branch from 10166cd to 0b2cb68 May 8, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

When the chinook is ordered to land with alt-move infantry scatter away from the landing spot. When infantry order the chinook to land, it doesn't do that and just hovers in air.

Since the infantry is not really idle in that case, they cannot be nudged by the chinook. I've made the Land activity consider non-idle allied units blocking so they will now land adjacent to them instead.

Alt-load doesn't work.

Fixed.

If a chinook in air is sharing a cell with another aircraft it will not land when infantry order it to

This is actually just one expression of a very nasty bug in the aircraft repulsion logic. Aircraft can only be repulsed by other aircraft that were build after them. They will not budge otherwise, no matter which one is moving. This is probably the underlying cause of most other issues with aircraft blocking each-other. Fixing this is beyond the scope of this PR (and I haven't been successful at finding the exact cause anyway).

@Punsho
Copy link
Contributor

left a comment

It feels much better, the mid cell thingie is really annoying though

  • If a chinook is issued a stop command when landing/ascending it will hover in that height (all helicopters do this)
  • If a chinook is ordered to land by infantry and then immediately the order is cancelled the chinook will stay landed
  • I think the alt-load distance needs to be increased, it's low for such big units
@reaperrr

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

If a chinook is issued a stop command when landing/ascending it will hover in that height (all helicopters do this)

I think that 'Land' should either be non-interruptible, or aircraft should rise back to CruiseAltitude if given a Stop command while not on ground.

Edit: That being said, it doesn't necessarily have to be adressed in this PR if it turns out to be tricky to get right, as it's not a regression vs bleed. We should fix it before release though, otherwise the aircraft refactor can't really be called complete.

@tovl tovl force-pushed the tovl:orderlanding branch from 0b2cb68 to a851165 May 9, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

think that 'Land' should either be non-interruptible, or aircraft should rise back to CruiseAltitude if given a Stop command while not on ground.

Land is now uninterruptible from the moment the landing cell is reserved.

If a chinook is ordered to land by infantry and then immediately the order is cancelled the chinook will stay landed

Fixed.

I think the alt-load distance needs to be increased, it's low for such big units

This is a matter of preference, but I tend to agree. The way I see it, if you need more control over which transports the units go to you can always use shift click. I've doubled the default range.

@Punsho
Copy link
Contributor

left a comment

  • Landing now is uninterruptible but it shouldn't be the case, if an order of any kind is given that doesn't require it to continue landing it should come back to the cruise altitude and do the action
  • Ascending can still be stopped midway

Even though the range of alt-load was increased it seems as it is the same as it was previously, I guess the logic behind it has to be reworked. Also I never considered queueing up loading but I guess since all behaviours are now queueable I should start thinking with them.

@tovl tovl force-pushed the tovl:orderlanding branch from a851165 to 6c182be May 10, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

Helis that are not on the ground and do not have the LandOnIdle flag will now always rise to CruiseAltitude when idle.

I've found that the alt-load behaviour isn't even consistent so this indicates a bigger problem. This isn't directly related to this PR so I'll defer a fix to future PRs.

@Punsho
Copy link
Contributor

left a comment

Works almost perfectly, the last complaint I have is when I select a chinook that is over ground based units the units below are going to be prioritised

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

This is true for all aircraft and is pretty out of scope for this PR.

OpenRA.Mods.Common/Activities/Air/Land.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Carryall.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Activities/UnloadCargo.cs Outdated Show resolved Hide resolved
@pchote

This comment has been minimized.

Copy link
Member

commented May 17, 2019

Dependencies have been merged.

@matjaeck
Copy link
Contributor

left a comment

Looks overall very good. Some suggestions, findings, requests, notes:

  • Only allow transport / carryalls to force land
    It otherwise conflicts IMO with the decision that helicopters take off after resupply. I find it weird that they can stay landed anywhere but on the helipad.

  • Fixes #11748, but stop order can prevent take off
    You have to issue the stop order at the exact right moment before it takes off again. This leads to the issue that the helipad is permanently blocked. Would be nice if you could fix this while at it.

  • RTB polish
    When you use the RTB hotkey on a helicopter with full ammo, it returns but bounces away from the helipad. On bleed it stays above the pad. Do you have an idea how to polish this?

    bounce

  • Take-off / Landing conflict
    It seems that the helipad is already freed for other helicopters before the occupying heli has fully taken off. If you spam land orders while a heli is trying to take off (easy to reproduce with fully armed helis), weird things can happen. The behavior on bleed is different and the issues that

    • the take off can be interrupted
    • another heli lands while the first one has not taken off completely
      should be fixed if possible.

    landspam

  • Interruptible rearming
    When you spam click-land orders on a helipad or airfield, rearming will be interrupted. This is likely unrelated to this PR but related to #16492.

General notes:

  • The aircraft repulsion behavior unfortunately prevents that the usability improvements here can fully unfold. I expect that there will be complaints some things wouldn't work, but the real issue is the aircraft pathfinding and repulsion behavior (helicopters blocking each other)

  • There is the unfortunate side effect that the new force landing feature makes it impossible to hover over buildings when using left click commands. This is not a problem of the changes here though. A workaround for the player is to give a move order and stop order above the structure.

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2019

OpenRA.Utility(1,1): Error: ts|rules/aircraft.yaml:219 refers to a trait field WaitAfterUnload that does not exist on Cargo.
OpenRA.Utility(1,1): Error: ts|rules/aircraft.yaml:248 refers to a trait field LoadingDelay that does not exist on Carryall.
OpenRA.Utility(1,1): Error: ts|rules/aircraft.yaml:249 refers to a trait field UnloadingDelay that does not exist on Carryall.

This will also want an update rule.

@tovl tovl force-pushed the tovl:orderlanding branch 3 times, most recently from ec841e8 to e54148f Jun 29, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2019

Added update rule.

@reaperrr reaperrr force-pushed the tovl:orderlanding branch from e54148f to e120143 Jun 30, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2019

Works well in-game, as far as I can tell.

On the code side, the only things that stood out to me were the int/WDist issue @pchote pointed out and that the update rule only applied to Carryall, but not Cargo.
I updated the update rule myself to save some time, so +1 from me once @pchote has added his commit on the int/WDist issue.

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2019

Cargo didn't have these parameters before, so there is nothing to update.

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2019

Cargo didn't have these parameters before, so there is nothing to update.

Ah damn, that's the disadvantage of looking at commits individually (overlooked their introduction and only saw their rename in the last commit). Will revert that myself.

@pchote

This comment has been minimized.

Copy link
Member

commented Jun 30, 2019

As part of fixing the int/WDist issue I am also merging some of the later fixups into the original commits that introduced them. I will comment with the updated branch once I am happy with the result.

I am already part way through this, so please don't force push over this branch!

tovl added 6 commits Jun 30, 2019
Overhaul Land activity:
- Landing on an actor is no longer blocked by the underlying terrain
- Land in a nearby cell if the requested location is blocked
- Internally manages the fixed-wing landing sequence
- ProductionAirdrop transport waits until the exit is free before landing
@pchote

This comment has been minimized.

Copy link
Member

commented Jun 30, 2019

I've massaged this into: https://github.com/OpenRA/OpenRA/compare/bleed...pchote:orderlanding?expand=1

The main changes were reworking the nearEnough code and moving/merging commits so that only the final changes are implemented. There are some other tweaks such as changing GetLandableTerrain to actually return terrain (it was a bool) but these should not significantly change the behaviour here.

👍 from me after resetting over to these commits.

@tovl tovl force-pushed the tovl:orderlanding branch from e120143 to 87ece49 Jun 30, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2019

Took over pchote's changes without further adaptation.

@reaperrr reaperrr merged commit da0b24e into OpenRA:bleed Jun 30, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tovl tovl referenced this pull request Aug 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.