Added feature to save unitType promotion. #12894
Conversation
…of running away Settler unit will now settle on best tile in dangerous Tiles without escort instead of running away.
with "Founds a new puppet city" in "uniques" of an unit in Units.json. Making it so you can now settle a puppet city.
| @@ -113,6 +113,7 @@ Requires [PolicyOrNationalWonder] = | |||
| Cannot be purchased = | |||
| Can only be purchased = | |||
| See also = | |||
| Build units with saved UnitType promotion = | |||
There was a problem hiding this comment.
Edit: I noticed the checkbox is only shown when a default promotion was previously saved, so I think it's a good spot to place the checkbox!
Having the limited screen space on Android in mind, please keep the caption as short as possible. For example:
[ ] auto promote
|
|
||
| // adds the checkBoxs to choice to save unit promotion. | ||
| private fun saveUnitTypePromotionForCity() { | ||
| val checkBoxSaveUnitPromotion = "Save unitType promotions.".toCheckBox(saveUnitTypePromotion) {saveUnitTypePromotion = it} |
There was a problem hiding this comment.
The wording could be better. Short and descriptive and no period at the end. What about:
[ ] default for <Warrior>
in case you are promoting a Warrior. And in the city screen the checkbox would say
[ ] default promotions
There was a problem hiding this comment.
I like this but instead Warrior it could be Sword to not mislead the player into thinking it's only for the warrior unit😅
| @@ -113,6 +113,8 @@ Requires [PolicyOrNationalWonder] = | |||
| Cannot be purchased = | |||
| Can only be purchased = | |||
| See also = | |||
| Default promotions = | |||
| "Default promotions for [unitType] = | |||
| // To not have to redo it for every unit build afterword. | ||
| var canBuildUnitTypeWithSavedPromotion = HashMap<String, Boolean>() | ||
|
|
||
| // To stroe unitType to promotion. |
There was a problem hiding this comment.
Remove comment, call this "unitTypeToPromotion"
We already know it's for city since it's in the city class
| // Check if the player want to rebuild the unit the saved promotion | ||
| // and null check. | ||
| val savedPromotion = city.cityUnitTypePromotion[unit.baseUnit.unitType] | ||
| if (city.canBuildUnitTypeWithSavedPromotion[unit.baseUnit.unitType] != null && |
There was a problem hiding this comment.
Unnecessary check, "== true" is enough
| savedPromotion != null) { | ||
|
|
||
| // check if current unit has enough ep | ||
| if (unit.promotions.XP >= savedPromotion.XP) { |
| if (city.canBuildUnitTypeWithSavedPromotion[unitType] != null) { | ||
| row() | ||
| add(city.canBuildUnitTypeWithSavedPromotion[unitType]?.let { | ||
| "Default promotions".toCheckBox( |
There was a problem hiding this comment.
Do not use .let here, instead use regular assignment
If this is bull then you'd get add(null) which is not what we want, probably
|
|
||
| if (unitCurrentCity != null) { | ||
| // If you are clicked the save unitType promotion, you want the next unitType to have the same promotion. | ||
| unitCurrentCity.canBuildUnitTypeWithSavedPromotion.put(unit.baseUnit.unitType,true) |
There was a problem hiding this comment.
I'm unclear why we need this second hashmap
There was a problem hiding this comment.
This hashmap needed for the checkbox at ConstructionInfoTable.kt to turn on and off the auto promotion of the unit
There was a problem hiding this comment.
So the name should be like "should" etc, "can" usually means "is there nothing preventing us while "should" means "do we want to". "Can" is about ability, "Should" is about desire.
More philosophically, Can and Should sit on opposite sides of Hume's Guillotine.
|
|
||
| // adds the checkBoxs to choice to save unit promotion. | ||
| private fun saveUnitTypePromotionForCity() { | ||
| val checkBoxSaveUnitPromotion = "Default promotions for ${unit.baseUnit.unitType}".toCheckBox(saveUnitTypePromotion) {saveUnitTypePromotion = it} |
There was a problem hiding this comment.
Wrap parameter in square brackets
There was a problem hiding this comment.
Like this [saveUnitTypePromotion] ?
There was a problem hiding this comment.
More like val checkBoxSaveUnitPromotion = "Default promotions for [${unit.baseUnit.unitType}]".toCheckBox(saveUnitTypePromotion) {saveUnitTypePromotion = it} - the square brackets are part of the string and passed to the translation engine, which will then translate Default promotions for [placeholder] and the unit type string separately...
| if (construction is BaseUnit) { | ||
| val unitType = construction.unitType | ||
|
|
||
| if (city.unitTypeToPromotion[unitType] != null) { |
There was a problem hiding this comment.
Extract city.unitTypeToPromotion[unitType] into val - you use it 3 times here
There was a problem hiding this comment.
The {city.unitTypeToPromotion[unitType] = it} it's to avoid adding an another line just to assign it, in the updated version.
|
Is it normal that I still have the "changes requested" 😅 |
|
|
||
| if (buildUnitWithPromotions != null) { | ||
| row() | ||
| add("Default promotions".toCheckBox(buildUnitWithPromotions) {city.unitTypeToPromotion[unitType] = it}).colspan(2).center() |
There was a problem hiding this comment.
Why is this not "for unittype" like the other one?
There was a problem hiding this comment.
To save space on android-portrait. Because this text is in the info table of the unit, the context is set and it's clear that the checkbox counts for this unit type only.
"Default promotions" is very short and descriptive, but I could also live with
"use Default promotions" if that sounds better
There was a problem hiding this comment.
Done and i also added that clicking a promotion button when save promotion is on to save it too.
Just like promotion pick button.
|
|
||
| // going to reuse this bit of code 2 time so turn it into a funtion | ||
| private fun checkSaveUnitTypePrormotion() { | ||
| if (saveUnitTypePromotion) { |
There was a problem hiding this comment.
Reverse the if checks, e.g. if (!save) return
|
|
||
|
|
||
| var unitTypeToPromotion = HashMap<String, Boolean>() | ||
|
|
There was a problem hiding this comment.
Do you have a good name for this variable 😅
There was a problem hiding this comment.
What exactly are you saving here? unitType is the key, sure but the value isn't promotion, is it?
Similarly the bottom hashmap
Maps should almost always be named in such a way that you can understand what is being mapped to what - "unitTypeTo" is a great start, e.g. "unitTypeToShouldUseSavedPromotion" or something
There was a problem hiding this comment.
Shorter is better, but "long and understandable" is better than "short but not understandable"
There was a problem hiding this comment.
would 'unitTypeBuildWithSavedPromtion' be a good name ?
There was a problem hiding this comment.
If the boolen is true build unit with save promotion
There was a problem hiding this comment.
I don't understand - per city there are saved promotions per type, right?
And also you can decide to... Not use them?
There was a problem hiding this comment.
Yes like the player can build a unit without the saved promotion, like that with that unit they can change the save promotion to something else if needed
There was a problem hiding this comment.
But this config will stick to all other units of this type?
By type do you mean like "archery" or like "archer"?
|
I think it's good enough for now, can always change it later |
This is to save unitType promotion, to not have to redo it when you are building an another unit with the same unitType.
This can help if you are building units in bulk with the same unitType and promotions.