[PATCH] Units garrisoned inside a ship should not always be killed when the ship is destroyed

[mimo]

When the ship is destroyed near the shoreline, we may expect that some of the garrisoned units would not be killed when the ship is destroyed. Here is a patch to do that. For the time being, female and infantry will survive if near the shoreline.

This patch includes the fix to #2238 as it was needed to work properly.

[Josh]

As for a quick review, code looks great but the help text for "EjectSwimmingEntitiesOnDestroy" doesn't really make any sense.

[mimo]

added missing word in the help text of EjectSwimmingEntitiesOnDestroy

[sanderd17]

See IRC comments from 16:44 onwards: ​http://irclogs.wildfiregames.com/2013-10-28-QuakeNet-%230ad-dev.log

[mimo]

I've uploaded a new version of the patch, following the previous comments. The EjectEntitiesOnDestroy works as usual : when true, all entities are ejected on destroy (or when the health goes below a given threshold). But when false, there is a new EjectClassesOnDestroy which list the classes which can nonetheless be ejected when the garrisonholder is destroyed: typical use case is a ship not far from the shoreline.

[sanderd17]

I don't like the tag duplication. Just a tag to decide by class is enough. As you can put "Unit" if you mean everything.

Also, did you duplicate the PickSpawnPoint method? Won't it be possible/better to overload it with some argument?

[mimo]

Replying to sanderd17:

I don't like the tag duplication. Just a tag to decide by class is enough. As you can put "Unit" if you mean everything.

I agree. If we go with this EjectClassesOnDestroy, it can fulfil the role of the old EjectEntitiesOnDestroy is and replace it.

Also, did you duplicate the PickSpawnPoint method? Won't it be possible/better to overload it with some argument?

Part of the needed initialisations are the same, but the logic to find the spawning point is quite different (even if they look the same). So there is not much to gain by trying to combine, and it will be less readable.

[mimo]

with EjectEntitiesOnDestroy removed and replaced by the new tag

[mimo]

In 14550:

add more flexibility to eject units when garrisonHolder is destroyed, fixes #2242