Dunno if mimo is aware of the diff or is against the revert of his commit.
I personally would prefer the removal of this too, for both gameplay reasons (if the ship is near land, then I don't see why units shouldn't be ungarrisoned there) and code reasons (we have several diffs that want to change the Footprint getter and it's much nicer if we only have to change one place each).

In D1039#42141, @elexis wrote:

Dunno if mimo is aware of the diff or is against the revert of his commit.

I assumed mentioning the commit would give him an alert (same with Vlad with the game speed dropdown).

In D1039#42134, @wraitii wrote:

Also it seems to me if units can garrison they should be able to try to ungarrison too, so I don't like EjectUnitsOnDestroy. But again that's not included here.

Don't understand this?

For example, healers and cavalry can garrison in a bireme, but when it sinks they'll die rather than try to ungarrison:

<GarrisonHolder>
  <EjectClassesOnDestroy datatype="tokens">FemaleCitizen Infantry Dog</EjectClassesOnDestroy>
  <List datatype="tokens">Support Infantry Cavalry Dog</List>

I feel the lists should be the same (which means we can get rid of EjectClassesOnDestroy).

For example, healers and cavalry can garrison in a bireme, but when it sinks they'll die rather than try to ungarrison:

<GarrisonHolder>
  <EjectClassesOnDestroy datatype="tokens">FemaleCitizen Infantry Dog</EjectClassesOnDestroy>
  <List datatype="tokens">Support Infantry Cavalry Dog</List>

I feel the lists should be the same (which means we can get rid of EjectClassesOnDestroy).

That seems limiting. Imagine a case where a ship can and does garrison siege engines. Assuming sane people they would try to get to land, but nobody would care about dying just to save some heavy equipment.

In D1039#42284, @leper wrote:

For example, healers and cavalry can garrison in a bireme, but when it sinks they'll die rather than try to ungarrison:

<GarrisonHolder>
  <EjectClassesOnDestroy datatype="tokens">FemaleCitizen Infantry Dog</EjectClassesOnDestroy>
  <List datatype="tokens">Support Infantry Cavalry Dog</List>

I feel the lists should be the same (which means we can get rid of EjectClassesOnDestroy).

That seems limiting. Imagine a case where a ship can and does garrison siege engines. Assuming sane people they would try to get to land, but nobody would care about dying just to save some heavy equipment.

The logic is, if a unit can ungarrison when we click the ungarrison button then they should also ungarrison when the ship dies, the two behaviors should be exactly the same.

(In this particular case, presumably there's some humans included with the siege engine, sometimes visible but sometimes not -- how else would it move around? Why would those humans drown when they're right next to shore?)

That seems limiting. Imagine a case where a ship can and does garrison siege engines. Assuming sane people they would try to get to land, but nobody would care about dying just to save some heavy equipment.

The logic is, if a unit can ungarrison when we click the ungarrison button then they should also ungarrison when the ship dies, the two behaviors should be exactly the same.

Depends if we want to reward players for being lazy. Also it seems that the consensus was that we don't want to do that in this case at some point.

(In this particular case, presumably there's some humans included with the siege engine, sometimes visible but sometimes not -- how else would it move around? Why would those humans drown when they're right next to shore?)

The humans get to the shore, the siege engine doesn't. Since plain humans like that aren't something that is handled they just go home.

In D1039#42299, @leper wrote:

That seems limiting. Imagine a case where a ship can and does garrison siege engines. Assuming sane people they would try to get to land, but nobody would care about dying just to save some heavy equipment.

The logic is, if a unit can ungarrison when we click the ungarrison button then they should also ungarrison when the ship dies, the two behaviors should be exactly the same.

Depends if we want to reward players for being lazy. Also it seems that the consensus was that we don't want to do that in this case at some point.

It's not lazy, they still have to get their ships to shore. But now they won't have to annoyingly micromanage over when to ungarrison.

The behavior before rP14550 was to kill all units along with the ship, so the consensus at the time was that we should be saving units if they're near the shore.
The solution of adding PickSpawnPointBothPass and EjectClassesOnDestroy is overly complicated, in my opinion. It seems a lot cleaner to say "near the shore" = "if we can ungarrison".

I personally don't mind "EjectClassesOnDestroy", and I think it is potentially useful for mods.

I think it is important to keep EjectClassesOnDestroy: why would you recover your catapults or elephants if your ship is destroyed. It is not micromanagement to me, but forcing the player to do responsible actions with its ships: when it is heavily damaged, you have the choice between retreating or continuing the action with the risk of loosing everything.
Futhermore, before i added that some years ago, all units were lost when the ship was destroyed. That was a sensible possibility for me (although i preferred the EjectClassesOnDestroy one), but allowing all units to escape as is proposed here just does not make any sense for me: it is just allowing people to play without planning the consequences of their acts and without paying the price for their mistakes.

Concerning the PickSpawnPointBothPass, the main reason was to avoid some weird cases as units ungarrisoned in an isolated rock or any small area surrounded by impassable regions, or units ungarrisoned in a cliff without any access to any shoreline to get there. I agree the code is a bit complicated, but that's because we don't have any easy way (at least not that i know) to define if a tile is connected to a shore. Maybe using the hierarchical pathfinder, that could be done simply.

So in short, i really don't support the patch.

Concerning the PickSpawnPointBothPass, the main reason was to avoid some weird cases as units ungarrisoned in an isolated rock or any small area surrounded by impassable regions, or units ungarrisoned in a cliff without any access to any shoreline to get there. I agree the code is a bit complicated, but that's because we don't have any easy way (at least not that i know) to define if a tile is connected to a shore. Maybe using the hierarchical pathfinder, that could be done simply.

That sounds like it wouldn't be a problem if the ungarrison range is the same as the garrison range (i.e. the behaviour is symmetrical).

I'll leave this up for more discussion.

In D1039#43751, @wraitii wrote:

Concerning the PickSpawnPointBothPass, the main reason was to avoid some weird cases as units ungarrisoned in an isolated rock or any small area surrounded by impassable regions, or units ungarrisoned in a cliff without any access to any shoreline to get there. I agree the code is a bit complicated, but that's because we don't have any easy way (at least not that i know) to define if a tile is connected to a shore. Maybe using the hierarchical pathfinder, that could be done simply.

That sounds like it wouldn't be a problem if the ungarrison range is the same as the garrison range (i.e. the behaviour is symmetrical).

Take care that if you want to always be able to ungarrison 30 (with possibly several siege and elephant) units from a trireme, and all of them on the same side to be on land, you have to allow a quite large ungarrisoning range.

In D1039#43747, @mimo wrote:

I think it is important to keep EjectClassesOnDestroy: why would you recover your catapults or elephants if your ship is destroyed. It is not micromanagement to me, but forcing the player to do responsible actions with its ships: when it is heavily damaged, you have the choice between retreating or continuing the action with the risk of loosing everything.
Futhermore, before i added that some years ago, all units were lost when the ship was destroyed. That was a sensible possibility for me (although i preferred the EjectClassesOnDestroy one), but allowing all units to escape as is proposed here just does not make any sense for me: it is just allowing people to play without planning the consequences of their acts and without paying the price for their mistakes.

If you want them to pay the price for their mistake, then all the units should die. And I'd be fine with this, it would be consistent.
But currently, if they have a ship full of infantry they won't pay the price, all will be saved.
But if they have a ship full of cavalry, then they will pay the full price, all will be lost.
How does that make any sense?

Concerning the PickSpawnPointBothPass, the main reason was to avoid some weird cases as units ungarrisoned in an isolated rock or any small area surrounded by impassable regions, or units ungarrisoned in a cliff without any access to any shoreline to get there. I agree the code is a bit complicated, but that's because we don't have any easy way (at least not that i know) to define if a tile is connected to a shore. Maybe using the hierarchical pathfinder, that could be done simply.

But currently they can still manually ungarrison onto those areas, so what has been solved?

In D1039#43786, @temple wrote:

In D1039#43747, @mimo wrote:

I think it is important to keep EjectClassesOnDestroy: why would you recover your catapults or elephants if your ship is destroyed. It is not micromanagement to me, but forcing the player to do responsible actions with its ships: when it is heavily damaged, you have the choice between retreating or continuing the action with the risk of loosing everything.
Futhermore, before i added that some years ago, all units were lost when the ship was destroyed. That was a sensible possibility for me (although i preferred the EjectClassesOnDestroy one), but allowing all units to escape as is proposed here just does not make any sense for me: it is just allowing people to play without planning the consequences of their acts and without paying the price for their mistakes.

If you want them to pay the price for their mistake, then all the units should die. And I'd be fine with this, it would be consistent.
But currently, if they have a ship full of infantry they won't pay the price, all will be saved.
But if they have a ship full of cavalry, then they will pay the full price, all will be lost.
How does that make any sense?

The main point here is Siege and Elephant which should be killed as i said above. Then what is the detail of other rescued units (or even none) i don't really mind (we could even replace all cav by an infantry unit in that case). But whatever our choice in vanilla game, EjectClassesOnDestroy is certainly useful for mods and should be kept.

Concerning the PickSpawnPointBothPass, the main reason was to avoid some weird cases as units ungarrisoned in an isolated rock or any small area surrounded by impassable regions, or units ungarrisoned in a cliff without any access to any shoreline to get there. I agree the code is a bit complicated, but that's because we don't have any easy way (at least not that i know) to define if a tile is connected to a shore. Maybe using the hierarchical pathfinder, that could be done simply.

But currently they can still manually ungarrison onto those areas, so what has been solved?

Yes, and why breaking something which work just to unify two pieces of code. We should first fix the manual ungarrisoning, and only then unify the code.

In D1039#43804, @mimo wrote:

The main point here is Siege and Elephant which should be killed as i said above. Then what is the detail of other rescued units (or even none) i don't really mind (we could even replace all cav by an infantry unit in that case). But whatever our choice in vanilla game, EjectClassesOnDestroy is certainly useful for mods and should be kept.

I still don't agree with the reasoning, why would an elephant drown if infantry don't? Especially if they're being ungarrisoned in the shallows.
I guess my problem is I keep thinking of it as ungarrisoning, whereas you think of it as something slightly different (units emerging from the wreckage).
But mods can do crazy things, so okay.

Yes, and why breaking something which work just to unify two pieces of code. We should first fix the manual ungarrisoning, and only then unify the code.

Well, I didn't think it was "working". Plus it saves some duplication on D1040 and D1041. By the way, with the spawn direction patch I lower the ungarrison radius, so if you had a lot of units on a sinking ship, not all of them might find room to ungarrison, and the ones that didn't would die (especially bigger units like siege and elephants).

In D1039#43815, @temple wrote:

I still don't agree with the reasoning, why would an elephant drown if infantry don't? Especially if they're being ungarrisoned in the shallows.
I guess my problem is I keep thinking of it as ungarrisoning, whereas you think of it as something slightly different (units emerging from the wreckage).

Yes, emerging from the wreckage as you say is what this patch was done for: note that for ships, EjectHealth=0 and that there are no requirements on the ship position itself (it could be in deepwater). The only requirement is that the unit can be ungarrisoned in a surrounding shallow water (which abstracted the fact that it could swim up to the shore).

Well I still like this change for the reason that code that doesn't exist can't be technical debt. And this is a pretty big simplification.

The edge case described above is annoying but as I've said having a symmetric garrison/ungarrison range would fix this.

@elexis if you too still want this I'll get it committed (after commandeering to rebase).

@elexis if you too still want this I'll get it committed (after commandeering to rebase).

Mostly I'd prefer if the author of the commits is the author of the patch.

I don't want to take a part in the dispute, but two thoughts that weren't mentioned before:

• If the choice between the two behaviors should be kept, there is still duplication that might be better off refactored out. Perhaps one could serve both behaviors with one function and some arguments.
• What is likely to happen on a ship if it becomes sinking realistically and what is sane for a 0ad gameplay pov to implement mostly concern 0ad. But this code is part of the pyrogenesis engine that should be able to host a variety of RTS games that can include fantasy models. (Which doesn't mean that one has to serve any hypothetical use case on the other hand)