Opened 8 years ago

Closed 7 years ago

Last modified 4 years ago

#1807 closed enhancement (fixed)

[PATCH] Garrison units inside allied buildings

Reported by: mimo Owned by: leper
Priority: Nice to Have Milestone: Alpha 14
Component: Core engine Keywords: patch
Cc: Patch:

Description

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

Attachments (4)

garrison.patch (24.1 KB ) - added by mimo 8 years ago.
garrison-v2.diff (16.7 KB ) - added by mimo 8 years ago.
new version after changes in Command.js following leper's worries
garrison-v3.diff (18.6 KB ) - added by mimo 7 years ago.
garrison-v4.diff (23.0 KB ) - added by mimo 7 years ago.

Download all attachments as: .zip

Change History (16)

by mimo, 8 years ago

Attachment: garrison.patch added

comment:1 by Kieran P, 8 years ago

Milestone: BacklogAlpha 13
Priority: Should HaveNice to Have

comment:2 by historic_bruno, 8 years ago

Keywords: patch review added
Type: defectenhancement

by mimo, 8 years ago

Attachment: garrison-v2.diff added

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

comment:3 by Kieran P, 8 years ago

Milestone: Alpha 13Alpha 14

by mimo, 7 years ago

Attachment: garrison-v3.diff added

comment:4 by mimo, 7 years ago

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.

comment:5 by leper, 7 years ago

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".

comment:6 by mimo, 7 years ago

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.

in reply to:  6 comment:7 by leper, 7 years ago

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).

comment:8 by mimo, 7 years ago

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).

by mimo, 7 years ago

Attachment: garrison-v4.diff added

comment:9 by leper, 7 years ago

Owner: set to leper
Resolution: fixed
Status: newclosed

In 13357:

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

comment:10 by leper, 7 years ago

Keywords: review removed

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).

comment:11 by leper, 7 years ago

In 13358:

Fix tests. Refs #1807.

comment:12 by Imarok, 4 years ago

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

Note: See TracTickets for help on using tickets.