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 aircraft occupy cells when landed. #16315

Merged
merged 2 commits into from Apr 22, 2019

Conversation

@tovl
Copy link
Contributor

commented Mar 16, 2019

This makes aircraft occupy space on the ActorMap when landed. This prevents other ground units from moving into the same space.

Fixes #4605.
Fixes #2018.
Fixes #15131
Fixes #14114
Fixes #14867

This makes fixed-wing aircraft check for blocking actors as well. If they find the repair depot (or any other landing spot) blocked, they will circle and wait for it to be unblocked. Aircraft will try to reserve the required cells when landing is initiated.

@pchote

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

Can you comment on how this interacts with crates and mines?

mods/cnc/rules/aircraft.yaml Outdated Show resolved Hide resolved
@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

Can you comment on how this interacts with crates and mines?

Helicopters cannot land on mines or crates. This is true in bleed as well.

@pchote

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

Is there any prospect for fixing that here? Aircraft not interacting with the ActorMap was always the main blocker for supporting those.

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

I think that should be handled in a follow-up PR.

mods/cnc/rules/aircraft.yaml Outdated Show resolved Hide resolved
mods/ra/rules/aircraft.yaml Outdated Show resolved Hide resolved
@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2019

So, I've decided to add the crushing logic to aircraft anyway. This will allow transport helicopters to interact with crates and mines. It will also allow them to squash infantry unlucky enough to stand right under them when landing.

@tovl tovl force-pushed the tovl:air-occupy branch from 2bdce29 to e21076d Mar 17, 2019

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2019

Is this intended to fix helicopters, too? For example 2 helis on one pad or vehicle and heli on fix.

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2019

It should work for all types of aircraft.

@matjaeck
Copy link
Contributor

left a comment

Crate collection with Chinooks works well (but not all crate effects seem to work for aircraft - iron curtain crate did not have any effect). Crushing infantry with Chinooks is fun, too. For planes everything seems to work like intended.

Findings from a quick test:

helicopters and vehicles can still repair simultaneously on service depots

helitankfix

multiple helicopters at one pad

If you send 2 helicopters to rearm at a helipad, and then hit deploy when the first landed, the second heli will land on top of it. Rearming of the first heli will be stopped and it will only take off and land again when the 2nd heli has finished rearming.

2xhelipad

It can happen that 2 helicopters try to land at the same spot at the same time and then land on top of the other one. It's an edge case though that is unlikely to happen in normal games I think.

@tovl tovl force-pushed the tovl:air-occupy branch from e21076d to d33d2cd Mar 17, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2019

Helicopters should be fixed now.

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2019

Great, I'll also test it against these issues, but not tonight: #14114, #14867, #15131

@tovl tovl force-pushed the tovl:air-occupy branch from d33d2cd to eae2466 Mar 19, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

Update:

  • Fixed chinook being unable to unload cargo due to occupying space.
  • Added missing CrushDamageTypes yaml parameter.
  • aircraft can now nudge friendly units out of the way when landing.

@tovl tovl force-pushed the tovl:air-occupy branch from eae2466 to a0cfd07 Mar 27, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

Rebased.

@matjaeck
Copy link
Contributor

left a comment

#15131 looks fixed, haven't watched the replays of #14114 and #14867 but couldn't reproduce the issue when testing in RA.

@tovl tovl force-pushed the tovl:air-occupy branch from a0cfd07 to d0216ed Mar 30, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2019

Rebased.

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2019

👍

@tovl tovl referenced this pull request Apr 12, 2019
0 of 1 task complete
@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2019

Transport heli constantly lands and takes off again.

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2019

@tovl I guess you overlooked it (understandable since it's quite a bit up in the comments):

Regarding the LandableTerrain of RA/TD helicopters (and nearly all TS aircraft):

Well, 'objectionable' sounds a bit too harsh, we just try to avoid keeping lines around that won't have any effect on that actor.
That being said, as a compromise you could move this to the ^Helicopter default instead.

@tovl tovl force-pushed the tovl:air-occupy branch from f00d1ed to bd2214c Apr 14, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2019

Yes, I missed that. I've removed these lines for now, because as you said they do not have any effect currently.

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2019

[18:44] <+reaperrr> pchote: what was the technical reason again for preferably not using writeable public booleans, ints and so on?
[18:49] <+pchote> in what context?
[18:52] <+reaperrr> https://github.com/OpenRA/OpenRA/pull/16315/commits/30e77af6049e92d34adf22f7c4ed7a5e2ac47c57#diff-e8b231bb90082b00e18a95d9ae6a7ef4R187
[18:54] <+pchote> basically want to expose as small a public surface area as possible
[18:56] <+pchote> that LandingInitiated doesn't seem to be used at all?
@pchote

This comment has been minimized.

Copy link
Member

commented Apr 14, 2019

LandingCell could perhaps be kept private by passing the cell as an argument to AddInfluence.

@tovl tovl force-pushed the tovl:air-occupy branch from bd2214c to 58962cb Apr 14, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2019

Fixed.

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

In addition to #16417, the changes here - in particular to Land/HeliLand - are also a dependency for updating #16365, as well as a few of my local PRs, so it would be good to get this merged soon (as long as there are no remaining issues, of course).

@reaperrr
Copy link
Contributor

left a comment

Thinking about it, the aircraft should never block itself from landing.

OpenRA.Mods.Common/Traits/Air/Aircraft.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Air/Aircraft.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Cargo.cs Outdated Show resolved Hide resolved
@pchote
Copy link
Member

left a comment

Looking good, but a few comments:

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

@tovl tovl force-pushed the tovl:air-occupy branch from 58962cb to 59d3f4d Apr 21, 2019

@tovl tovl force-pushed the tovl:air-occupy branch from 59d3f4d to b120a50 Apr 21, 2019

@matjaeck
Copy link
Contributor

left a comment

LGTM

@reaperrr reaperrr merged commit 560f8c2 into OpenRA:bleed Apr 22, 2019

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.