Fix issues when transferring capitals

[SeventhM]

• Reverts 06377fe
• Reverts and reimplements 42a9c3a
• Fixes issues where you could move a different capital than the intended one if a mod allows you to have more than one capital

---

Admittedly, this PR is a complete shot in the dark, and I have no clue if anything in it actually fixes anything. The only fix I'm certain of is the fix for potentially moving the wrong capital. Currently I have no clue why 4.7.6 kept incorrectly moving the capital when this seems to not have been an issue beforehand, but addressing these previous commits seems like the first step, as this seems to have been when things started going wrong

Still, this should be a better and safer attempt to fix #9679. #9709 and #9731 should make any calls to removeBuilding safe if the player is lacking a capital. In fact, thanks to how recalculating city connections works, removeBuilding is actually better and safer for this despite us updating stats later anyways because there are potienal checks that rely on the city connections to be up to date. addBuilding has also been made safe when having no capital, so it's entirely viable to add the capital if they have only 1 city after everything else is done.

As for the fix regarding potentially moving the wrong capital: getCapital only returns the first capital, but this movement checks isCapital which doesn't matter if it's the first or the 5th. If that's not the capital being moved, it will end up removing a completely irrelevant capital. This has the bonus issue of not deleting the old capital properly. Whoops!

[SeventhM]

Unit test is bugged. Unit test implies we should have 2 capitals (theirCapital and ourCapital). This should actually be false. That also implies I actually did fix something... Huh

[SomeTroglodyte]

Unit test is bugged

Well, first of all it should differentiate its four asserts with messages. As is, the output is lacking, and you need to look up source to see theirCapital.isCapital() is what failed. And very very very YES - it should have been an assertFalse all this time, very very very obviously.

[SomeTroglodyte]

... unless moveToCiv was never meant to do all the necessary work, assuming that cleanup is done later by the caller. In which case - minus points for no Kdoc defining intentions or scope, so a code reviewer cannot know without digging far deeper.

I feel you are going in a good direction.

So *42a9c3a was wrong after all - like I said it can't be the culprit for a not-enough-capitals case, but it did indeed cause the multiple capitals we saw. My bad for debugging up to an explanation but then not switching brain on when reading the fix - and this insight also corroborates your decision that it's better to remove conquered Palaces first unconditionally and move all code to replace it when necessary to the end of the process. Heck, one could even go extreme and leave it to exist in only one place - getCapital ("Hey I'm supposed to tell where the Palace is, but I can't find any. Does the Ruleset say there always must be one? Yes - I'll just add one then") - if nobody circumvents that helper...

[SeventhM]

but it did indeed cause the multiple capitals we saw

Something bothers me about this though. The implication here is that there's still an old capital here when we're removing buildings. But that should be gone now by moveCapitalToNextLargest(). There should be no capital left over that slips through the cracks besides any we add manually. It's possible my fix of explicitly passing in the city we're moving from fixed something, not simply nuking the capital without a care alongside the other national wonders. But with the assumption that you only have 1 capital (not considering mods trying to break that assumption), that shouldn't be broken either... Right?

Funnily enough by the way. Because they is the cause of multiple capitals, if another civ comes and takes one of your new fake capitals you'll lose your original one too, thanks to moveCapitalToNextLargest() not actually explicitly knowing which city to remove the old capital from... Which as I write this... Huh, this could be the cause of the missing capitals

Heck, one could even go extreme and leave it to exist in only one place - getCapital

I don't like this. Again for some mods, allowing players to be capital-less is intentional, so having getCapital enforce anything is likely problematic. Honestly, I would be for moving towards allowing mods that don't have capitals altogether, even if it was a sort of novelty

On an irrelevant note, it does bother me that capialCityIndicator is in City and not Civilization. Feels to me on a city version should return a collection of buildings in the city that indicates a capital, not go look up in the ruleset which building is the capital

[SomeTroglodyte]

Something...

Well, your first paragraph just shows you analyzed more deeply than I am capable of these days. My comments are more from a "how should the architecture be fundamentally" perspective. I couldn't code any improvement to the Capitals situation - after max 15 minutes I would comment out all elements with "Capital" in their name and re-implement, wouldn't be able to resist. And that's at my current rate 3-6 weeks to get it running again, another 1-2 to debug a few turns of one vanilla game, and by then the branch would be so far off it wouldn't stand a chance as PR.

for some mods, allowing players to be capital-less is intentional

Exactly - so optimally, there would be just one place where we decide "G&K rules Mandatory Palace" - or not.

I would be for moving towards allowing mods that don't have capitals altogether

Been preaching that for a long time. Older Civs had that as a challenge - Fail to protect capital - massive malus without a Palace, so you need to build one ASAP or be overrun. Tweak until every decision takes the possibility into account - e.g. a City-State quest just wishing to place a barb camp - if there's no capital to suggest some geography, just use next best. After that, a clean "always Palace" rule reduces to just making sure your build-replacement code is run in time. And that may perhaps mean defer until someone is interested, no matter whether UI for a Star or AI for road building or updateStats...

does bother me that capialCityIndicator is in City and not Civilization

There's a comment from me somewhere (you already linked it) saying exactly the same thing. It's a civ's property (would be global but for the option of nation-unique variants) and should be cached there.

[SeventhM]

Reverted my dev environment and did some tests of my own to figure out what's going on

1. Looks like I was right. It wasn't moving the capital in moveCapitalToNextLargest. Read below for why
2. moveCapitalToNextLargest is failing to remove the old capital regardless of if the city is explicitly passed in or not. Something is definitely going wrong there, and that's why the previous assumptions wasn't working
3. Simply switching from "builtBuildings.remove" to "removeBuilding" does provide the intended behavior though, including the bug when not explicitly passing in the city with 2 capitals. Come to think of it, have we had a single issue before patch 2 with weird city counts? Because patch 2 was what switched it away from removeBuilding, and I don't remember seeing a single issue about weird capital shenanigans with patch 1. Patch 1 was just null pointers from removeBuilding

Btw, this might be that capital checks look at builtBuildingsObjects NOT builtBuildings. This is even more reason to use removeBuilding instead of trying anything tricky. But it also means that there's a disturbing potential for errors if not both of them are updated any time you do anything. I would need to double check stuff, but tbh I'm this close to wanting to just PR a refactor of every builtBuildings call to something of a getBuiltBuildings().name check. This also means that the base unit test function createCity (in tests/src/com/unciv/logic/civilization/CapitalConnectionsFinderTests.kt) needs to switch to addBuilding. We need to not be touching the building lists directly ever outside of cityConstruction

As for the rest of this PR besides reverting the actively detrimental 06377fe, the fix to explicitly pass in the city into moveCapitalToNextLargest is still imo generally something worth fixing for any mods that allows for move than 1 capital. As for the order at which to add/remove the capital when you only have 1 city, I still think this PR is more clear than what came before it. If I were to edit this PR, it's to do some cleaning up of the unit tests.

I will lazily pretend it's out of scope for this PR, but has anyone noticed that we only remove the first building that indicates the capital... not every building that does? Because, uh, that's an issue too.