Opened 7 years ago

Closed 6 years ago

Last modified 3 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 7 years ago.
garrison-v2.diff (16.7 KB) - added by mimo 7 years ago.
new version after changes in Command.js following leper's worries
garrison-v3.diff (18.6 KB) - added by mimo 6 years ago.
garrison-v4.diff (23.0 KB) - added by mimo 6 years ago.

Download all attachments as: .zip

Change History (16)

Changed 7 years ago by mimo

Attachment: garrison.patch added

comment:1 Changed 7 years ago by Kieran P

Milestone: BacklogAlpha 13
Priority: Should HaveNice to Have

comment:2 Changed 7 years ago by historic_bruno

Keywords: patch review added
Type: defectenhancement

Changed 7 years ago by mimo

Attachment: garrison-v2.diff added

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

comment:3 Changed 7 years ago by Kieran P

Milestone: Alpha 13Alpha 14

Changed 6 years ago by mimo

Attachment: garrison-v3.diff added

comment:4 Changed 6 years ago by 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.

comment:5 Changed 6 years ago by 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".

comment:6 Changed 6 years ago by 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.

comment:7 in reply to:  6 Changed 6 years ago by 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).

comment:8 Changed 6 years ago by 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).

Changed 6 years ago by mimo

Attachment: garrison-v4.diff added

comment:9 Changed 6 years ago by leper

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 Changed 6 years ago by leper

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 Changed 6 years ago by leper

In 13358:

Fix tests. Refs #1807.

comment:12 Changed 3 years ago by 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

Note: See TracTickets for help on using tickets.