Changes between Version 3 and Version 7 of Ticket #3412


Ignore:
Timestamp:
Jan 14, 2016, 12:11:52 AM (8 years ago)
Author:
elexis
Comment:

Replying to smitec:

(not sure if I am supposed to add review back for the new patch, apologies if this isn't the right step, instructions didn't say)

Adding review is fine if you upload an updated patch.

Review: We discussed the patch and decided that adding a canGarrison flag to the templates is not ideal, since every unit might be garrisonable at some point and it also means having "canGarrison" logic in two complex components (UnitAI and GarrisonHolder). It is probably not nicely achievable to find out whether a given unit (ship, animal) is garrisonable in any of all buildings that can appear on a map. Hiding the button is likely not achievable. Sorry for changing the mind.

Todo: Updating the templates in those two places should still be done:

  • Since animals can't be garrisoned due to UnitAI, the GarrisonHolder entry should be removed as well (I'll do that now).
  • The worker elephant shouldn't be garrisonable in places that don't fit.

Legend:

Unmodified
Added
Removed
Modified

  • Ticket #3412

    • Property Summary [PATCH] Hide garrisson button for ungarrisonable units → [PATCH] Fix templates of non-garrisonable units

  • Ticket #3412 – Description

    v3 v7  
     1The original idea of this ticket was to hide the garrison-button for non-garrisonable units. But since that doesn't seem nicely achievable, we might want to fix those two related bugs identified in the course of this ticket:
     2
    13Some units can't be garrissoned anywhere and have incoherent garrisson usage:
    2 
    3 * '''Ships''' can't be garrissoned anywhere.
    4  Whether or not they should (for example in the carthaginian military dock) is a design question. In case we go with that, it should be done in another ticket.
    5 
    64* '''Sheep'''
    75 Currently `UnitAI` can't deal with garrisoning animals. Hence it has a hardcoded disabling of animals. The GUI still draws the garrison cursor for sheep, which should not happen.
    … …  
    97* The '''worker elephant''' can be garrisoned in temples, towers, outposts and other buildings that don't make  sense (see #3611). It should be coherent with '''war-elephants''', i.e. garrisonable only in buildings that make sense (#3135) or nowhere.
    108
    11 Due to these circumstances it seems appropriate to add a new boolean property `IsGarrisonable` to the '''templates''' which is true for all units except for those mentioned above.
    12 If that property is false, the garrison '''cursor and button should be hidden'''.
    139
    14 Besides making those three cases coherent and fixing the worker elephant bug, it will also ease future edits (prohibiting garrisoning of some units).
     10~~ Due to these circumstances it seems appropriate to add a new boolean property `IsGarrisonable` to the '''templates''' which is true for all units except for those mentioned above. ~~