#1391 closed enhancement (fixed)
[PATCH] Move ship to shoreline when tasking units to garrison inside
Reported by: | rogue-spectre | Owned by: | mimo |
---|---|---|---|
Priority: | Should Have | Milestone: | Alpha 15 |
Component: | UI & Simulation | Keywords: | patch |
Cc: | Patch: |
Description (last modified by )
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.
Attachments (6)
Change History (31)
comment:1 by , 12 years ago
Description: | modified (diff) |
---|---|
Priority: | Nice to Have → Should Have |
Summary: | ships don't approach the cost to get citizen and warriors → Move ship to shoreline when tasking units to garrison inside |
comment:2 by , 12 years ago
Type: | defect → enhancement |
---|
comment:3 by , 12 years ago
Milestone: | Alpha 11 → Backlog |
---|
comment:4 by , 11 years ago
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.
comment:5 by , 11 years ago
Keywords: | patch review garrison ship added |
---|---|
Summary: | Move ship to shoreline when tasking units to garrison inside → [PATCH] Move ship to shoreline when tasking units to garrison inside |
by , 11 years ago
Attachment: | shipgarrison.diff added |
---|
comment:6 by , 11 years ago
Milestone: | Backlog → Alpha 14 |
---|
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.
comment:7 by , 11 years ago
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.
comment:8 by , 11 years ago
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.
comment:9 by , 11 years ago
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 ?
comment:10 by , 11 years ago
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.
comment:11 by , 11 years ago
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.
by , 11 years ago
Attachment: | shipgarrison-v2.diff added |
---|
comment:12 by , 11 years ago
Added a test on the number of garrisoned units, to avoid waiting for loading units when the capacity is exceeded.
by , 11 years ago
Attachment: | shipgarrison-v3.diff added |
---|
comment:13 by , 11 years ago
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).
by , 11 years ago
Attachment: | shipgarrison-v4.diff added |
---|
change the name of the method as suggested in comment #13
follow-up: 16 comment:14 by , 11 years ago
Keywords: | garrison ship removed |
---|
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.
comment:15 by , 11 years ago
Keywords: | review removed |
---|
comment:16 by , 11 years ago
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:93the 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.
comment:17 by , 11 years ago
Thanks to revision #13505, the implementation with messages is much easier than I anticipated. So here is an update of the patch.
comment:18 by , 11 years ago
Keywords: | review added |
---|
follow-up: 20 comment:19 by , 11 years ago
Component: | Core engine → UI & Simulation |
---|
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)
andUnitAI.CancelPickup()
- Maybe
MT_PickupRequired
should be renamed toMT_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.
comment:20 by , 11 years ago
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)
andUnitAI.CancelPickup()
Not sure. I've the impression that would make the code more difficult to read ?
- Maybe
MT_PickupRequired
should be renamed toMT_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.
by , 11 years ago
Attachment: | shipgarrison-v5.diff added |
---|
comment:21 by , 11 years ago
Owner: | set to |
---|
comment:22 by , 11 years ago
Milestone: | Alpha 14 → Alpha 15 |
---|
by , 11 years ago
Attachment: | shipgarrison-v6.diff added |
---|
comment:25 by , 11 years ago
Keywords: | review removed |
---|
(have altered the title and description to be clearer about what is required)