[PATCH] Move ship to shoreline when tasking units to garrison inside

[rogue-spectre]

When you garrison units inside a ship, the ship should make it's way toward the nearest coastline between it and the units trying to garrison into it. Currently, the units move to the shoreline, but the ship remains in the middle of the ocean, unreachable.

[Kieran P]

(have altered the title and description to be clearer about what is required)

[mimo]

I've made a patch which does that. In the present implementation, the ship will pick up the units and then continue with its previous orders.

For example, if we have a merchant ship on a trade route between two docks, and we select a trader and then ctrl-click on the ship, the ship will come and pick-up the trader, and then starts again its trade route.

[leper]

I think you shouldn't use PushOrderFront if the current order is forced. You should also register the message type (in interfaces/UnitAI.js). Apart from that the code does look good.

[mimo]

In that case, the force is not used (it's just a copy-and-paste). This argument can be safely removed. For the interface, I just forgot it in the diff. I've updated the patch just afterwards.

[leper]

I'm talking about not using PushOrderFront when the ship being garrisoned to is executing a forced order. Maybe just use PushOrder in that case.

[mimo]

Ah, I see what you mean. But may-be we should test on the "forced" of the garrison order ? if it was forced, then PushOrderFront, otherwise only PushFront ?

[leper]

I don't see how a garrison order can be non-forced. There is no code that garrisons units in UnitAI (and isn't called from the outside) so checking for that is a bit pointless (though it wouldn't hurt in case some code gets added in the future that does garrison from within UnitAI).

I think adding the pickup order after the last forced order should work, but probably just adding it at the end would be better.

[mimo]

My point was that, as the garrison order is a forced order and that the PickUp order is just a consequence of it, we may consider the PickUp order to be forced, and then the PushOrderFront is legitimate. But ok, here is a slightly modified version with the PickUp order inserted after the last forced order. Using a simple PushFront as you suggested would not work because of some never ending orders like Trade.

[mimo]

Added a test on the number of garrisoned units, to avoid waiting for loading units when the capacity is exceeded.

[wraitii]

I dislike the name of the InsertOrder function. It should be slightly more explicit (I'm not necessarily against long function names, but PushOrderAfterForced or something would work better, imo, particularly as UnitAI is already messy in that respect).

[mimo]

change the name of the method as suggested in comment #13

[historic_bruno]

After briefly testing the patch, I have some comments:

• I think this should be made more generic, not limited to ships, because I could see similar behavior being desired for other transporting entities. Maybe it could be added as a flag to CmpGarrisonHolder? (eventually there should be a separate transport component)
• The logic in PICKUP.LOADING.Timer is somewhat buggy, for example when a unit fills the ship to capacity, it will throw errors like:

  ERROR: FinishOrder called for entity 11 (units/athen_ship_fishing) when order queue is empty ()@simulation/components/UnitAI.js:2835 ([object Object])@simulation/components/UnitAI.js:2381 ([object Object],[object Object])@simulation/helpers/FSM.js:274 ([object Object],200)@simulation/components/UnitAI.js:3029 ([object Object])@simulation/components/Timer.js:93
  

  the easiest way to test that is with a fishing boat and a single infantryman.
• Is it desired that if I order one unit to garrison in the ship, it moves toward that unit exclusively, but ignores further garrison orders until it's finished? This seems to break the typical order handling where the most recent order takes precedence.
• A buggy case I found: take a formation of units on the opposite side of a decent sized lake from a waiting ship, tell them to garrison in it, both the units and ship start moving, the ship certainly meets the formation, but the formation continues walking far out of its way before turning back and re-approaching the ship. This is unnecessary and can be a waste of precious time in the game.
• It seems weird that the transporter ends up testing UnitAI orders for the loading units, could they send a message instead when/if they leave the garrison approaching state and no longer need transport? Presumably the transporter would continue following the target(s) until it received this message?
• As a convention, don't access member variables of another component directly, use getter/setter functions if you need to do that.

[mimo]

Replying to historic_bruno:

After briefly testing the patch, I have some comments:

• I think this should be made more generic, not limited to ships, because I could see similar behavior being desired for other transporting entities. Maybe it could be added as a flag to CmpGarrisonHolder? (eventually there should be a separate transport component)

done

• The logic in PICKUP.LOADING.Timer is somewhat buggy, for example when a unit fills the ship to capacity, it will throw errors like:

  ERROR: FinishOrder called for entity 11 (units/athen_ship_fishing) when order queue is empty ()@simulation/components/UnitAI.js:2835 ([object Object])@simulation/components/UnitAI.js:2381 ([object Object],[object Object])@simulation/helpers/FSM.js:274 ([object Object],200)@simulation/components/UnitAI.js:3029 ([object Object])@simulation/components/Timer.js:93
  

  the easiest way to test that is with a fishing boat and a single infantryman.

fixed. Sorry, there was a missing return in the patch.

• Is it desired that if I order one unit to garrison in the ship, it moves toward that unit exclusively, but ignores further garrison orders until it's finished? This seems to break the typical order handling where the most recent order takes precedence.

that is a deliberate choice, because otherwise the ship would start moving towards the first unit, and then, when you order a second unit to garrison, it would switch towards this second unit, thus loosing its approach to the first unit. But this can be easily changed by removing lines 220, 234 and 235 of the patch.

• A buggy case I found: take a formation of units on the opposite side of a decent sized lake from a waiting ship, tell them to garrison in it, both the units and ship start moving, the ship certainly meets the formation, but the formation continues walking far out of its way before turning back and re-approaching the ship. This is unnecessary and can be a waste of precious time in the game.

yes, I also had sometimes such weird behaviour. The problem is due to limitations of MoveToTarget when the target is moving. May-be we should have a MoveToMovingTarget which would follow more closely the target movement. But this would be for a future improvement, may-be after an upgrade of the path finding.

• It seems weird that the transporter ends up testing UnitAI orders for the loading units, could they send a message instead when/if they leave the garrison approaching state and no longer need transport? Presumably the transporter would continue following the target(s) until it received this message?

changed. I've used a function here. Using messages could be better, but would need much more changes. For a future improvement.

• As a convention, don't access member variables of another component directly, use getter/setter functions if you need to do that.

changed.

[mimo]

Thanks to revision r13505, the implementation with messages is much easier than I anticipated. So here is an update of the patch.

[historic_bruno]

I've just spotted a serious issue with the latest patch. If a ship is full and you try to garrison units inside it, but they are out of range, the ship won't pick them up (as expected) but they will apparently continue trying to approach it and requesting pickup. It causes the game to lag and is definitely a regression :(

Other changes I was going to make before committing, but I'll suggest them here:

• I don't think you need the empty "leave" event handlers. At least in my testing it seems to work OK without them.
• There is a lot of duplicate logic for sending cancel/require messages and determining if the target allows pickup, that would be best moved into two member functions like UnitAI.RequirePickup(target) and UnitAI.CancelPickup()
• Maybe MT_PickupRequired should be renamed to MT_PickupRequested? Because it's not technically requiring pickup, the unit is requesting it but the target can ignore it, it's only a little misleading.

[mimo]

Replying to historic_bruno:

I've just spotted a serious issue with the latest patch. If a ship is full and you try to garrison units inside it, but they are out of range, the ship won't pick them up (as expected) but they will apparently continue trying to approach it and requesting pickup. It causes the game to lag and is definitely a regression :(

Yes, you are right ! I didn't spot it before; But in fact, the problem is already there in the present version when the target is not reachable and the unit continues trying to approach (but without sending the message). I'm not aware of a way to know if a point is reachable by a unit (independantly of the fact that other units are presently blocking it). Without that information, I've not found a clean solution, but a "trial method" : we try several times to reach the point, increasing progressively the delay between trials, and eventually stop. Not fully satisfactory, but that works.

Other changes I was going to make before committing, but I'll suggest them here:

• I don't think you need the empty "leave" event handlers. At least in my testing it seems to work OK without them.

removed

• There is a lot of duplicate logic for sending cancel/require messages and determining if the target allows pickup, that would be best moved into two member functions like UnitAI.RequirePickup(target) and UnitAI.CancelPickup()

Not sure. I've the impression that would make the code more difficult to read ?

• Maybe MT_PickupRequired should be renamed to MT_PickupRequested? Because it's not technically requiring pickup, the unit is requesting it but the target can ignore it, it's only a little misleading.

changed

I've also added a member function MoveToGarrisonRange with the same structure as the existing CheckGarrisonRange to answer some TODO in the code.

[mimo]

New cleaned version

[mimo]

In 14115:

Move ship to shoreline when tasking units to garrison inside, fixes #1391