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

Prevent bogus attackmove on take-off. #17017

Merged
merged 4 commits into from Sep 26, 2019

Conversation

@tovl
Copy link
Contributor

commented Aug 30, 2019

It was reported in the forums that aircraft will attack-move when taking off after being force-landed. This PR fixes that issue while untangling some of the TakeOff/unreserve/rallypoint mess.

Fixes #17054.

@tovl tovl changed the title Prevent bogus attackmove of take-off. Prevent bogus attackmove on take-off. Aug 30, 2019
@abcdefg30 abcdefg30 added this to the Next Release milestone Sep 7, 2019
@pchote

This comment has been minimized.

Copy link
Member

commented Sep 7, 2019

Crash when issuing a land order on an airfield that already has an idle aircraft:

Exception of type `System.NullReferenceException`: Object reference not set to an instance of an object
  at OpenRA.Mods.Common.Traits.Reservable.UnReserve (OpenRA.Actor self) [0x00016] in /Users/paul/src/OpenRA/OpenRA.Mods.Common/Traits/Buildings/Reservable.cs:81 
  at OpenRA.Mods.Common.Traits.Reservable.Reserve (OpenRA.Actor self, OpenRA.Actor forActor, OpenRA.Mods.Common.Traits.Aircraft forAircraft) [0x00030] in /Users/paul/src/OpenRA/OpenRA.Mods.Common/Traits/Buildings/Reservable.cs:45 
  at OpenRA.Mods.Common.Traits.Aircraft.MakeReservation (OpenRA.Actor target) [0x00010] in /Users/paul/src/OpenRA/OpenRA.Mods.Common/Traits/Air/Aircraft.cs:496 
  at OpenRA.Mods.Common.Activities.ReturnToBase.Tick (OpenRA.Actor self) [0x00203] in /Users/paul/src/OpenRA/OpenRA.Mods.Common/Activities/Air/ReturnToBase.cs:121 
  at OpenRA.Activities.Activity.TickOuter (OpenRA.Actor self) [0x0004a] in /Users/paul/src/OpenRA/OpenRA.Game/Activities/Activity.cs:81 
  at OpenRA.Traits.ActivityUtils.RunActivity (OpenRA.Actor self, OpenRA.Activities.Activity act) [0x00015] in /Users/paul/src/OpenRA/OpenRA.Game/Traits/ActivityUtils.cs:37 
  at OpenRA.Actor.Tick () [0x00007] in /Users/paul/src/OpenRA/OpenRA.Game/Actor.cs:144 
  at OpenRA.World.Tick () [0x00123] in /Users/paul/src/OpenRA/OpenRA.Game/World.cs:454 
  at OpenRA.Game.InnerLogicTick (OpenRA.Network.OrderManager orderManager) [0x001bc] in /Users/paul/src/OpenRA/OpenRA.Game/Game.cs:610 
  at OpenRA.Game.LogicTick () [0x0003e] in /Users/paul/src/OpenRA/OpenRA.Game/Game.cs:637 
  at OpenRA.Game.Loop () [0x000e3] in /Users/paul/src/OpenRA/OpenRA.Game/Game.cs:784 
  at OpenRA.Game.Run () [0x0003c] in /Users/paul/src/OpenRA/OpenRA.Game/Game.cs:824 
  at OpenRA.Game.InitializeAndRun (System.String[] args) [0x00010] in /Users/paul/src/OpenRA/OpenRA.Game/Game.cs:261 
  at OpenRA.Program.Main (System.String[] args) [0x00044] in /Users/paul/src/OpenRA/OpenRA.Game/Support/Program.cs:37 
@tovl tovl force-pushed the tovl:takeoff-fix branch from a46cea1 to 69dd49a Sep 7, 2019
@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2019

Fixed the crash.

Copy link
Member

left a comment

Changes look reasonable, and I couldn't find anything obviously wrong while testing.
Just a couple of minor code style requests:

OpenRA.Mods.Common/Traits/Buildings/Reservable.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Buildings/Reservable.cs Outdated Show resolved Hide resolved
@tovl tovl force-pushed the tovl:takeoff-fix branch from 69dd49a to bf67779 Sep 8, 2019
@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2019

Fixed the nits.

@pchote pchote added the PR: Needs +2 label Sep 8, 2019
Copy link
Member

left a comment

There is still an inconsistency here: Force land a helicopter on a helipad. Then build a new helicopter. The newly built helicopter will attack move to the rally point (as expected) but the force-landed helicopter will only move to the rally point.

This does fix the issue however for force landing helicopters on terrain.

Copy link
Contributor

left a comment

LGTM. For the record, since this touches the reservation code, changes here have no effect on #16891.

Copy link
Contributor

left a comment

I already had my suspicions while looking at the code (since I didn't see any special handling for that), and testing confirmed I was right: this reintroduces the issue that the move to the rally point isn't skipped if there's a queued order.
Example: queue a reload and a move, after the reload is finished the aircraft will first move to the rp before performing the queued move.

@tovl tovl dismissed stale reviews from matjaeck and pchote via bce07ab Sep 15, 2019
@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2019

Fixed.

@reaperrr reaperrr dismissed their stale review Sep 18, 2019

PR updated

Copy link
Contributor

left a comment

Fixed and code looks good to me

Copy link
Contributor

left a comment

Tested in RA.

@reaperrr reaperrr merged commit 48059e8 into OpenRA:bleed Sep 26, 2019
2 checks passed
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.