Undeploy on move order for TS units

[tovl]

Fixes #13039.
Fixes #16600.

• Deployed artillery, juggernaut, mobile sensor array and tick tanks now undeploy when given a move order using the force-move modifier.
• Also fixed a minor glitch where EMPed units could not deploy but could still undeploy. They can now do neither.

This only applies to units that use the GrantConditionOnDeploy trait, not units that use the Transforms trait, so this does not yet work for MCVs.

[PunkPun]

This feature is nice but has also has some negatives, force move is mostly used for crushing infantry and I select my tick tanks with 'select units by type'. What if I want some of my tanks deployed and shooting while others go for crushing? There is no way to select all undeployed tick tanks for this issue to be avoided.

[tovl]

What if I want some of my tanks deployed and shooting while others go for crushing?

Ha, I hadn't considered that. Thanks for pointing this out!

There is no way to select all undeployed tick tanks for this issue to be avoided.

Well, now there is. 😉

[TheChosenEvilOne]

IMO This functionality should be enabled by setting something like "UndeployOnMove" to true

[tovl]

Do you mean setting it per unit type in yaml? Sure, that can be arranged, but I need a bit more context for that:

• What is the use case?
• What would the alternate behavior need to be?

[pchote]

I don't like the idea of hardcoding GrantConditionOnDeploy in the move activities. I think a better approach here would be to

1. Fix Mobile to properly use our pause/disable conventions. The current disabled state should be pause, and then disabled state should remove the cursors etc too.
2. Have the deploy state disable Mobile, EMP etc pause it.
3. Let GrantConditionOnDeploy handle the move cursors, and then issue the move order itself queued after the undeploy.
4. Do the same thing in Transforms, using a new MoveLocationInit to pass the requested move target to th enew actor.

[tovl]

I'm busy rewriting this now. However, I'm stuck at step 4. I can pass the target location to the new actor, but how do I make it actually perform a move activity?

[pchote]

If you didn't already find it, use ITransformActorInitModifier to insert the target location into the initializer dictionary for the new actor. Then in the Mobile and Aircraft constructors pull it out and store it in a field. Pull the contents of the if (order.OrderString == "Move") blocks in ResolveOrder out into their own methods, and then call this with the stashed target location from the Created method - note that you can't do this directly from the constructor because other traits on the actor may not be created yet.

[tovl]

Thanks! Yeah, I tried starting the activity from the constructor, but that didn't work. It sounds like the Created method is what I was missing.

[tovl]

Reimplemented according to the approach suggested by pchote. Additionaly:

• Undeploy on move order now also works with the Transforms trait
• Added a yaml parameter to GrantConditionOnDeploy and Transforms to undeploy-on-move either:
  • Always with or without force-move
  • Only on force-move
  • Never
• Deploying/undeploying with GrantConditionOnDeploy is now completely compatible with order queues. Everything can be queued before or after a deploy/undeploy order including other deploy/undeploy orders. Furthermore, queued orders are not ignored when in the middle of deploying/undeploying (unqueued orders are still ignored). In order to do so I have merged UndeployForGrantedCondition into DeployForGrantedCondition.
• Fixed a minor visual glitch on the Nod Artillery where the barrel temporarily disappeared when deploying.
• In case my first comment was a bit cryptic: Deployed and undeployed units are now selected separately when using the "W" hotkey or when double clicking on a unit.

The code does not yet work for aircraft because the OrderTargeters rely on knowing LocomotorInfo.MoveIntoShroud and LocomotorInfo.MovementCostForCell. I guess this should go into an interface? However, I'm not sure it's a good idea bothering with this now, when the whole Aircraft trait is in the middle of revision and there are no flying units who need this in the default mods anyway.

[PunkPun]

Nice work! Will it also work on construction yards? Actually It would be really nice if you could click on a con yard, force move it, queue a deploy. It would require less attention and would be more efficient overall

[tovl]

It would be really nice if you could click on a con yard, force move it, queue a deploy.

That was actually what I was thinking as well. Unfortunately, MCVs work with Transforms, not GrantConditionOnDeploy, so this is not yet possible. Making this possible for Transforms would be much more complicated, I fear. But, seeing that there are more people interested in this than just myself, I'll see how far I can get.

[tovl]

This should now fully work for aircraft as well, although I cannot test it since there are no relevant units in the default mods.

[pchote]

I'd prefer if the "separate selection criteria" commit could be handled in a separate PR, and then implemented using conditions so that it could fix #15370. As-is, its a bit much of a scope creep and not really needed for the main purpose of this PR.

[pchote]

Can you then please squash/rework the remaining commits so the history only contains the final implementation?

[tovl]

I guess that means Selectable would need to be moved to OpenRA.Mods.Common?

[pchote]

The idea from #4783 was to move it onto Mods.Common and merge it with SelectionDecorations as a prereq for implementing the isometric selection boxes for TS/RA2 buildings.

[tovl]

• Transforms now carries over the complete activity queue to the newly created actor, which makes it fully compatible with queued orders. The behavior of MCVs should now be identical to the behavior of other deployable units introduced earlier.
• The changes to selection criteria are removed and will soon be posted to a new PR.
• Squashed some commits.

[pchote]

Some more detailed thoughts, and it looks like this will want to depend on #16246 too.

[tovl]

Update:

• Refactored the code somewhat and addressed the above concerns as well as making sure the code works well with Childactivity fixes #16246 and Make Mobile a PausableConditionalTrait #16262.
• Added new methods to IMove interface.
• Squashed commits further to prevent redundant review work in the future (sorry about that).
• Transforms is no longer disabled by build-incomplete. This is to ensure deploy orders are still registered when transitioning and make the trait more consistent with DeployforGrantedCondition.
• In order to prevent the build animation from being interruptible I have used a Wait activity. I am not entirely happy with this, because that means the deployment length is not dependent on the animation length. (Of course you can interpret this as an advantage in some respects...) I'm not sure how else to do this without scope-creeping again. One way to handle this could be to make the build animation itself into an Activity that is not interruptible, which might also be useful for handling Defence build times are inconsistent #16250 as well and might open up lots of interesting possibilities. However, like I said, this would be way out of scope and I'd rather use a quick fix here.
• Added voice response to GrantConditionOnDeploy to be consistent with Transforms.

[pchote]

#16246 has been merged, so this needs a rebase now.

[pchote]

I'm still wrapping my head around this, but have a couple of initial questions/comments:

[pchote]

However, I'm still not sure how to actually solve it for Transforms

I checked vanilla TS, and it cops out by only supporting the move cursor on standard clear terrain. You get a move-blocked cursor when mousing over tunnels and even tiberium.

[tovl]

I've finally managed to get it working for Transforms in combination with a dummy Mobile trait as suggested. The only thing that still doesn't work is ordering a construction yard to a repair depot, but that has more to do with the Repairable trait than with Transforms (specifically AfterReachActivities needs to be a proper activity). With all the upcoming Resupply/Repair refactoring I don't feel particularly inclined to fix that here, so I've just disabled it for ConYards for now.

I checked vanilla TS, and it cops out by only supporting the move cursor on standard clear terrain. You get a move-blocked cursor when mousing over tunnels and even tiberium.

I can understand why; Having actors switch out like that gets pretty head-ache inducing 😛

[pchote]

Adding this to the release milestone, but I expect to wait until after the aircraft activities and #16481 are merged before looking at this in detail again.

[tovl]

Rebased.

[pchote]

Transferring orders created for one actor over to another actor is asking for a lot of trouble:

• Activities are very likely to regress in the future when someone changes them without realizing the non-conventional use here
• It forces us to remove performance optimizations (e.g. caching trait references) from any activities that may be transferred
• It breaks potentially many other assumptions, which will lead to difficult to debug issues

At the risk of falling into the "everything is a <X>" trap, I do think our newly introduced activity interface concept provides a clean solution that doesn't require another total rewrite here:

• Define an IActivityTranferrableToTransformedActor interface with a method Order IssueOrderForTransformedActor(Actor self, Actor newActor).
• Either:
  • Implement the interface on Move, EnterTransport, etc (allowing reuse of traits, but may still require workarounds at the activity level)
  • Create placeholder MoveAfterTransform, EnterTransportAfterTransform, etc activities (cleaner separation of concerns, but requires duplicated TransformToPassenger etc traits)
• Replace the init.Add(new ActivityInit(self.CurrentActivity)) with a loop over ActivitiesImplementing<IActivityTranferrableToTransformedActor> calling a.ResolveOrder() with each order on the newly created actor.

The workarounds in MoveAdjacentTo, Enter, etc can then go away, and the (IMO somewhat confusing) re-running of Transforms on the newly created actor to handle the ActivityInit.

[pchote]

If we go down the *AfterTransform activity route we could make things simpler by having a single IssueOrderAfterTransform activity that the TransformTo* trait(s) queue, and is special cased in Transform, instead of having an interface.

[pchote]

The default behavior for GrantConditionOnDeploy is to have nothing to do with movement, so the default here really should be UndeployOnMoveType.Never to match. Mods must explicitly opt-in to having the deploy affect movement, so it it logical for them to opt in to having movement affect the deploy at the same time.

This otherwise breaks usecases that have nothing to do with movement - e.g. deploy to activate gap generation or cloak.

[pchote]

I can take this over. This is real close to being mergable, so it would be a shame to leave it at the (hopefully) last hurdle.

[pchote]

16e0d55 implements my suggestion above, which seems to work well enough.

I'll open a new PR once I have a chance to go through in more detail and make sure I understand the rest of the changes, apply any other comments I might have, and maybe split the last commit into a few smaller ones.

Edit IMove is posing further problems... will require some further work.