[PATCH] Garrison units inside allied buildings

[mimo]

It would be nice to be able to garrison units inside allied buildings. Here is a patch to allow that.

[mimo]

new version after changes in Command.js following leper's worries

[mimo]

There is a problem with the new AI Aegis in rel 13 which seems to ungarrison all its garrisonHolders at each turn, so making the previous patch unusable with it. Here is a slightly modified of the patch so that the AI only ungarrison the units it owns.

[leper]

Most of the patch is looking good, but I don't like the duplication of logic in Commands.js and GarrisonHolder.js.

You should probably add some more Unload* methods to GarrisonHolder (and factor out the common logic (extracting an array of units)) or change some of the already present methods to take another parameter that tells the owner of the units to be extracted, or all of them.

Small nitpick: The "than" in the comment in GarrisonHolder should be "as".

[mimo]

I agree with you than GarrisonHolder.js can be improved. I'll have a look at it. For the dupplication in Commands.js, can you be more specific ?

But, if it's agreed that this functionnality is wanted and that the patch provides it and works fine although it can be improved a bit, wouldn't it be better to check it in already now ? It modifies already 10 files and is quite big with possibly some problems when merging it to the trunk if we wait too long. It would be much simpler for me to provide an improved version, and for you to review it, if we would work on a small patch involving only one or two files.

[leper]

Replying to mimo:

I agree with you than GarrisonHolder.js can be improved. I'll have a look at it. For the dupplication in Commands.js, can you be more specific ?

All of the unload logic is handled in GarrisonHolder, but unload-template is now done mostly by Commands.js. (I think that should be moved to GarrisonHolder too)

It modifies already 10 files and is quite big with possibly some problems when merging it to the trunk if we wait too long. It would be much simpler for me to provide an improved version, and for you to review it, if we would work on a small patch involving only one or two files.

If you can fix the above (at least move the logic for unload-template to GarrisonHolder) and change the one GuiInterfaceCall you moved in unit_commands.js to GetSimState() and update the patch, we should be able to get this in by the end of the week (if not earlier).

[mimo]

Here is a new version of the patch which implements most of your suggestions (the "most" is because I've not understood your point related to GetSimState).

In addition, when testing it, I encountered a problem I had not spotted before : when both allies play the same civ, the garrisoned units from both appear grouped together because they share the same templates, and all the logic in EntityGroups is based on the template name. I had then to introduce an extended template name used for garrisoned units (see changes linked to extended in selection.js and GuiInterface.js).

[leper]

In 13357:

Allow mutual allies to garrison in each others buildings. Patch by mimo. Fixes #1807.

[leper]

Thanks for the patch. The special handling for templates is probably the best solution apart from rewriting EntityGroups (which would cause other problems) or rewriting a lot of the gui code.

A possible improvement would be to allow the ally to unload all of his units at once (by using unload-all-own and rewriting that to take the player as an argument).

[leper]

In 13358:

Fix tests. Refs #1807.

[Imarok]

In 19202:

Properly sort selected entities by owner

Fixes #4425. Refs #1807, #4167. Fixes rP18979

Reviewed by: mimo

Differential Revision: ​https://code.wildfiregames.com/D123