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

Rework carryall drop-off queueing and ordering. #16417

Merged
merged 3 commits into from May 12, 2019

Conversation

@tovl
Copy link
Contributor

commented Apr 17, 2019

Fixes #16376
Fixes #16375

This PR revises the carryall drop-off behaviour to be fully compatible and consistent with order queueing and to be more consistent with other transport units.

New behaviour is as follows:

  • Normal move orders never release the cargo, queued or otherwise.
  • The deploy hotkey or cursor releases the cargo on the spot.
  • Force Move deploys the cargo at the ordered location, queued or otherwise.

This also includes a fix for the unreported issue of carryalls landing at the wrong altitude when not performing a drop-off maneuver, which also causes carryalls to 'land twice' when doing a pick-up maneuver.

This PR depends on #16315

@matjaeck
Copy link
Contributor

left a comment

This allows much easier queuing and control of pick up, move and deploy orders for carryalls. I haven't found any issues while testing and like this a lot.

I like the concept to introduce the deploy hotkey for carryalls and think the behavior here is easier to understand than the current setup. However, I think the Carryall should remain idle airborne on Force Move.

Force Move is with these changes essentially a shortcut. The feature to let aircraft remain airborne when idle has been requested before #12201 and would further extend control. I find it also more logical in the context of Force Move that is described as:

Selected units will move to the desired location
Default activity for the target is suppressed

Until now, the default activity for the carryall is (from a player perspective) to land.

@dragunoff

This comment has been minimized.

Copy link
Contributor

commented Apr 21, 2019

I also think that ForceMove would be better for ordering the carryall to stay airborne. Drop-off after reaching the destination can be ordered by queueing a deploy command after the move command.

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Apr 21, 2019

I also really like that you can select multiple Carryalls, then shift-click on waiting units and then queue move and deploy commands a lot 👍

@pchote

This comment has been minimized.

Copy link
Member

commented Apr 22, 2019

Needs a rebase now that #16315 has been merged.

@tovl tovl force-pushed the tovl:carryallorders branch from e9752ad to ce25572 Apr 22, 2019

@ltem ltem removed the PR: Rebase me! label Apr 22, 2019

@tovl tovl force-pushed the tovl:carryallorders branch from ce25572 to 188a970 Apr 22, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

I agree that alt-move is better left to determine the airborne/landing behaviour. I do like the idea of dropping off at a specific location instead of on the spot, but using alt-move for that is probably not the best implementation. so:

  • I've made the drop off at order location optional and disabled by default
  • I'll address ForceMove for remaining airborne in a follow-up PR
@matjaeck
Copy link
Contributor

left a comment

As stated before, I really like the general behavior here. Everything works as intended.

Some general thoughts for future PR's:

  • That Carryalls land after the drop is not optimal: When you select a Transport and a Carryall and order them to drop / unload somewhere, the landed Carryall will block the Transport and it won't unload.
  • If we change this to take-off-after-drop, we might want to change the idle behavior of Carryalls and Transports to stay idle airborne, too. This would also reduce their vulnerability to ground units. Then, deploy would make the Carryall / Transport land, drop or unload, an then take off again. Coupled with the improvements from #16315, this should solve the issue of blocked target zones (ground units will make room but a landed Carryall will not make room for a waiting Transport).
  • Force Move could then be utilized to let Carryalls and Transports land, regardless whether they carry something or not.
@pchote
Copy link
Member

left a comment

Ingame behavior works well - tested both manual carryalls in TS and the automated carryals in D2k.

A few comments/requests on the code:

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

@tovl tovl force-pushed the tovl:carryallorders branch from 95d2f00 to 36554c6 May 12, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented May 12, 2019

I'm wondering whether it would make sense to support WVec offsets instead of just altitude.
If LocalOffset has a horizontal component (e.g. LocalOffset: 512,0,-317) the carryall will have to take off and adjust its position before landing again and unloading. IMO it would be better if the carryall could adjust its landing position so that the cargo was always in the center of the cell.

LGTM otherwise

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2019

I kind of already did that in #16509 although I still kept vertical and horizontal offsets separate and it could use some more polish in general. Supporting that was a requirement for merging the approach behaviour into Land without having exceptions for carryalls.

So I'd say: Yes it makes sense, but let's do it in that PR instead.

@pchote
pchote approved these changes May 12, 2019

@pchote pchote added the PR: Needs +2 label May 12, 2019

@reaperrr
Copy link
Contributor

left a comment

Looks good as far as I can tell, D2k carryalls work perfectly, TS carryalls work well enough (with the follow-ups in mind), LGTM

@reaperrr reaperrr merged commit 6f1aaab into OpenRA:bleed May 12, 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
7 participants
You can’t perform that action at this time.