Show whether city has a garrison in city overview

[WhoIsJohannes]

Smaller improvement I'd like to work on, just wanted to get quick feedback. I haven't found any good way to see which cities have a garrison. This is relevant for some culture improvements and technologies (?). Would it be okay to add another column for this in the city table? Maybe it could show the icon of the unit that is garrisoned there.

[SomeTroglodyte]

Btw, the Units page shows whether the Unit is in or only near a City in that closest City column - by colour. Not the same, so just fyi. Biggest Problem I see is - wasn't the City overview one of the very few tables allowing sorting? That would then be expected to work for the new column too, and would be a few extra lines.

[WhoIsJohannes]

Ah, damn it. I just finished this and only realized when trying to create the pull request that you had already done this.

https://github.com/yairm210/Unciv/pull/8945/commits/9450f9860410de1ce2a378c0d71efe28aaf5293a

Anyways, I think there's a bug in your code. You should use this:

private fun City.getGarrison(): MapUnit? { return this.getCenterTile().getUnits() .firstOrNull() { it.civ == this.civ && it.canGarrison() } }

Otherwise there will also be planes showing up I guess.

[SomeTroglodyte]

Not planes, but ships. I'm not sure whether they would count as garrison - wanted to check and forgot. And once I found out how exactly Unciv does garrison boni, I feared I'd feel the need to double-check Civ5 behaviour....

But if that canGarrison function exists, then it should be trusted. Good catch. Go ahead and PR that ifyawanna - four places to use, easily found. And getUnits() explicitly takes civilians and planes - think a mod could make them garrisoning capable? If not, a militaryUnit?.takeIf{} might do.

...Maybe that // Only military land units can truly "garrison" comment there should have been a Kdoc instead...

[WhoIsJohannes]

Do you mind doing it if you already identified 4 places? I'd only know the one in the city overview table?

Is there anything else you're currently working on, so we don't do the same thing twice again? 😅

Il ven 17 mar 2023, 23:03 SomeTroglodyte @.***> ha scritto:

Not planes, but ships. I'm not sure whether they would count as garrison - wanted to check and forgot. And once I found out how exactly Unciv does garrison boni, I feared I'd feel the need to double-check Civ5 behaviour....

But if that canGarrison function exists, then it should be trusted. Good catch. Go ahead and PR that ifyawanna - four places to use, easily found. And getUnits() explicitly takes civilians and planes - think a mod could make them garrisoning capable? If not, a militaryUnit?.takeIf{} might do.

— Reply to this email directly, view it on GitHub https://github.com/yairm210/Unciv/issues/8877#issuecomment-1474444887, or unsubscribe https://github.com/notifications/unsubscribe-auth/A6CETILOLRYXG2OYYTEO6VLW4TNSFANCNFSM6AAAAAAVYJUQ4Q . You are receiving this because you authored the thread.Message ID: @.***>

[SomeTroglodyte]

I meant 4 lines in the 1 file...

anything else

Keyboard bindings 3 or 4 branches, taking apart 8795, doing worksheet-like sortable tables with a generic widget class that runs on a columns provider model (compiles and runs but has header display glitches), an interesting experiment making Stats immutable to prevent bugs (this one mostly functional but meant for the garbage bin due to a quirk how kotlin handles field overrides - but I wanted to present the idea and fix the one single bug it found)...

And a few backburner ideas. How the Civilopedia is opened / gets its ruleset, Tutorials reorg and rendering, the thousand todo's in map editor, evaluate and fix startBias (mods use Mountain and that does nothing, plus I had a project long ago to allow natural wonders as start bias - think themed mods - and somewhere there is a stash of once-working proof of concept for that).

If you want details on any of these, take it over, double-check designs, or something, just say so

[SomeTroglodyte]

At least the Stats breakdown of the city screen says the Garrison bonus from Military Caste is applied even when there's only a Privateer in the city -> so either our current Garrison column in Cities Overview is "correct" as in matching reality or it's lying the same as the other available "am I garrisoned" info... Another little project - that popup could do with little improvements too, like fixed header or a proper tree with individually toggle-able details. Update: The fixed header thing was discussed when created (#8180), and the removal of individual toggles toted as improvement.

[SomeTroglodyte]

Do you mind doing it if you already identified 4 places

OK, in the works. Fixing the actual calculation and the city stats popup header, too.

[SomeTroglodyte]

...Done...