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

Enable undeploy-and-<do something> orders for deployed actors #16607

Merged
merged 6 commits into from Jun 8, 2019

Conversation

@pchote
Copy link
Member

commented May 28, 2019

This PR takes @tovl's hard work and all the things we learned during #16142 and iterates it several more steps towards hopefully being mergeable.

Fixes #13039.
I don't think this covers #16600 anymore - a separate PR will be needed to fix the transforms code.

Abbreviated history of the evolution between #16142 and now:

Problem: (from #16142) handing activities between actors is going to cause maintenance issues.
Fix: Introduce IActivityTranferrableToTransformedActor interface as suggested in the previous PR, which also required introducing a new EnterTunnel activity.

Problem: Using alt to activate the undeploy orders clashes with Passenger's alternate transport mode.
Fix: Change all cursors except Move to remain enabled while deployed. This had a nice side-effect of making IWrapMove.CanWrapMoveOrder mostly redundant, so I removed this and reimplemented the remaining move case using a condition.

Problem: Implementing TransformIntoMobile using a dummy IMove regressed targeting of construction yards by AttackFollow based units.
Fix: Reimplement deploy-to-transform behaviour without using IMove. This meant introducing a separate trait for each order type, but this downside was outweighed by supporting EnterTunnel and Repairable without having to rewrite their standard traits and activities. IActivityTranferrableToTransformedActor became redundant, and was replaced by a single concrete IssueOrderAfterTransform activity.

Problem: Further discussion about the cursor problem in Discord and #13039 made a reasonable argument that the alternate transport mode has been partly superseded by the newly fixed ability to queue multiple enter orders, and on balance it is better to sacrifice that feature than to bodge this one.
Fix: Removed AlternateTransportMode and restored the ability to require force-move to show the tunnel/repair/transport cursors.

If there is a consensus that this is the wrong call wrt AlternateTransportMode I can drop the last two commits - but please test the behaviour on the 4th vs 6th commits before arguing for this. IMO the current behaviour feels much nicer.

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

@pchote pchote requested review from reaperrr and tovl May 28, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

commented May 28, 2019

We're going to want a TransformsIntoAircraft and conditional aircraft move cursors to cover mod cases like flying MCVs and the YR siege chopper.

@Punsho
Copy link
Contributor

left a comment

I did some testing in ra and ts, conyard undeploy is bugged. If tania places a c4 on a conyard and then it's undeployed using this method no animation will be played and conyard will instantly turn into an mcv. Same goes for chrono vortex.

@pchote

This comment has been minimized.

Copy link
Member Author

commented May 28, 2019

The TransformsInto* traits need to check for Transforms being paused, not just disabled.

@tovl

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

I don't think this covers #16600 anymore

While I'm glad you were willing to take this over, this is a bit of a bummer, because it's arguably the more pressing issue.

I will review in more detail when I have time, but this is quite a busy week for me.

@pchote pchote force-pushed the pchote:move-undeploy branch from 9512b79 to 30fba76 May 29, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

commented May 29, 2019

Indeed. However, with having to rewrite the transform-and-<do something> logic (twice) it was easier to drop back to the original Transform activity. Better to focus the fix in its own pr in any case, in case history repeats itself again here!

@pchote

This comment has been minimized.

Copy link
Member Author

commented May 29, 2019

Fixed and updated.

@Punsho
Copy link
Contributor

left a comment

Checked out all mcv activities and it seems to work ok. There's just one thing worth discussing. Should the mcv still undeploy when the path to its destination is blocked?

@abcdefg30
Copy link
Member

left a comment

Looks good to me.

OpenRA.Mods.Common/Traits/Attack/AttackBase.cs Outdated Show resolved Hide resolved
@pchote

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

Fixed and changed EnterAlliedActorTargeter to use the same approach as #16509.

@matjaeck
Copy link
Contributor

left a comment

Looks good, didn't spot any issues.

@tovl
Copy link
Contributor

left a comment

  • When queueing a move order after a deploy order on units with GrantConditionOnDeploy they will move in deployed state instead of undeploying:
    Peek 2019-06-07 23-09. This is handled correctly in #16142

  • Orders (queued or otherwise) given to MCVs during the undeploy animation are ignored. This is not the case in #16142.

@pchote pchote force-pushed the pchote:move-undeploy branch 2 times, most recently from b8252d3 to cc9a623 Jun 7, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

Fixed both issues.

  • Artillery issue was caused by a bogus premature optimization, which I remove and replaced with a comment so others aren't tempted to rebreak it.
  • MCV queued orders were broken by Transforms being paused while deploying. Updated the TransformsInto* traits to work with, and prioritise, a running Transform activity.

@pchote pchote force-pushed the pchote:move-undeploy branch from cc9a623 to 25505fb Jun 7, 2019

@matjaeck
Copy link
Contributor

left a comment

Can confirm the fixes for the queued orders for MCV and artillery.

@pchote pchote force-pushed the pchote:move-undeploy branch from 25505fb to 9bebe6e Jun 7, 2019

@reaperrr
Copy link
Contributor

left a comment

Looks good to me and didn't encounter any issues

@reaperrr reaperrr merged commit ebe37a4 into OpenRA:bleed Jun 8, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dragunoff

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2019

There is a problem with "attack ground" - if a deployed unit is ordered to attack ground outside of firing range it would undeploy, move into range and just stop.

I'm not sure what the behavior should be here. Either one of these:

  • undeploy, move into range, deploy, attack ground
  • undeploy, move into range, attack ground (if unit is able to attack in undeployd state)
  • do not undeploy, ignore order if target is out of range

The last one seems most useful because the player probably would not want to undeploy when target is out of range (imagine a group of deployed artilery units). This however goes against how the force move modifier works.

@pchote

This comment has been minimized.

Copy link
Member Author

commented Jun 9, 2019

An AttackDeploy to allow artillery to automatically deploy on attack orders has been discussed in other issues, and should come eventually in a later PR.

@dragunoff

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2019

Yep, but as I said -- I wonder what the behavior should be. And also what about units that can attack in both states - like the tick tank.

@pchote

This comment has been minimized.

Copy link
Member Author

commented Jun 9, 2019

This pr added a specific flag to disable attacking out of range targets without force attack on artillery and tick tanks - triggering this requires an explicit user action, so IMO 1 is the correct behaviour.

@tovl

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2019

The problem with MCV orders being ignored during the undeploy animation wasn't quite fixed. Queued deploy orders are still ignored and move orders are always treated as queued even when shift is not pressed.

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2019

Queued deploy orders are still ignored

It works when you release the ALT key after the transformation is complete and then press the deploy key.

@pchote pchote deleted the pchote:move-undeploy branch Aug 26, 2019

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.