Prevent movement pausing at invalid position.

[tovl]

Fixes #17209

Reverts #17189 and provides a proper fix for #17043 instead that still allows units to finish moving to a valid position before being halted by the thief/hijacker.

This also prevents a potential glitch if the carryall would be too quick to allow the unit to properly finish moving after locking. This same logic would also be needed for #17211, so this PR already brings us halfway on that.

[ghost]

The unit still slides back when you give move orders during pickup. I also noticed that due to the unit stopping later, the carryall often passes the harvester and has to turn around.

[tovl]

Fixed the sliding when given orders during pickup. The carryall passing the harvester probably has to do with the FlyIdle activity been queued when it has to wait. I reduced the number of ticks there, which should solve that.

[ghost]

Fix works for D2K carryall/harvester and RA thieves without obvious issues.

[pchote]

This looks generally like a good direction forward, and I may follow this up with a fix for #17211 (if only because it simplifies what I plan to write in the playtest news post).

[pchote]

Can we include the // HACK comment above here too?

[tovl]

Even when we can trust CurrentMovementTypes we would probably want to check FromCell and ToCell anyway, because in that way we are also checking if we are actually on the grid.

[pchote]

Can we put that in the comment then? 😄

[tovl]

I've created a new IsMovingBetweenCells property to make this more self-documenting and separated from the internal implementation of Mobile.

[pchote]

Good idea. Can we please cache mobile here from Created, though?

[pchote]

It turns out that we can't just queue Land at the start because we don't know the landing facing until after the carryable has been locked.

I have tried to fix this with 257b4b0 but have hit a roadblock with ChildActivity issues (Fly does not cancel, leaving the unit hanging mid-air; TakeOff not running, so carryall flies diagonally when moving to deliver) and have run out of time to debug further.

[tovl]

Those issues should be fixed now.

[tovl]

Okay. Final attempt, different approach: Now Land can deal with moving targets itself, so we don't have to requeue Land/Fly/FlyIdle orders in PickupUnit all the time.

[pchote]

Adding mobile-docking functionality to Land IMO doesn't feel right here, and would likely have to be replaced as part of the docking implementation anyway.

What was your objection to the approach I presented above? It turns out that the two issues were both simple oversights, and I fixed them for comparison in f291727

[pchote]

The new version removes the LockResponse enum that #17221 relies on, so I'd prefer it if we could go with my updated version of your original approach.

[tovl]

The reason for adding support for moving targets to Land is so that this logic can be easily reused by other Traits/Activities that need it (such as for carriers and the like). It was one of the main goals of #16509 that all the logic for how to approach a target for landing be included inside Land. In fact, Land already includes logic for dealing with moving targets, it just doesn't work very well. i.e., it directs the plane to the position the target was when the activity first ran, then when it finds the target is not there, redirects to where the target is at that point, etc. It also neglects the fact that moving targets may change their facing while moving.

So, as far as I'm concerned this fixes the issue at hand while simultaneously dealing with an oversight from #16509. The LockResponse enum isn't really necessary for #17221. We will just need to replace the reference to Mobile inside Land to reference all traits with an appropriate interface instead. This interface can then be implemented by Mobile and GrantConditionOnDeploy (and Aircraft if we want to enable things like flying carriers, mid-air refueling planes and the like). It might also be cleaner conceptually if we do not apply the condition and check for it's effect in the same method anyway.

That said, your fixed version of the initial approach works just as well for now and I understand if you are reluctant to redo #17221 on short notice. So the question for me is if you want to aim #17221 for stable? (It is unclear to me if that is your intention). If that is the case, I would agree to go with your solution for stable and aim the Land fixes at next+1. Practically speaking, it might make sense then to push the version from #17221 directly there and deal with this PR later under a different name.

[pchote]

So the question for me is if you want to aim #17221 for stable? (It is unclear to me if that is your intention).

I wasn't planning to since it doesn't affect any of the shipped mods, but on the other hand the changes for #17211 are relatively simple and it would be a nice feature for modders. I could get behind aiming that for prep and retargeting this for Next + 1.

My main motivation wrt the prep branch is to not merge another big refactor that may regress other things (in this case aircraft in general).

[pchote]

Swapping with #17221 on the milestone.

[tovl]

Ok. Closing for now to avoid confusion. Will open a new PR specifically for the land changes after things have calmed down with respect to the playtest.