Hotseat production queue tweak

[Kerael]

Part of issue #271. This is not a complete fix to all hotseat issues; I agree with the author that the code requires further refactoring. The changes were made for a game with my partner in a fairly short time (an hour and a half starting from no knowledge of how civ modding works, but with acceptable experience with lua), it was bugging me too much that the production queue was broken.

In summary the production queue is no longer a table with city numbers as keys, but instead a table of keys that were derived from the city number and the player (city owner).

Testing done: So far I had a single hotseat session for about 10 hours with lots of cities, and a bunch of small sessions that lasted no longer than 5 minutes to test that the changes are functional. The changes seem stable, I had a single crash (in 10 hours) and I have not investigated the cause of the crash but I doubt it was due to these changes.

[SpaceOgre]

Did a code review only since I can't test atm, and I think it looks good.
I find it highly unlikely that a player would even reach 100+ cities so it should not be a problem with a limit of 1000. If someone actually breaks the limit it is easy to increase ;)

[JHCD]

Great work, first view looks good.
I would prefer a human-readable combined key instead a calculated one.
e.g.: productionQueueTableKey = localPlayerID + "_" + cityID

[Kerael]

I would prefer a human-readable combined key instead a calculated one.
e.g.: productionQueueTableKey = localPlayerID + "_" + cityID

I can't see why not, lua is fairly lax as far as keys go - string or number. Might as well make it a string. However I extracted all of this key derivation into a function so that it was easier for me to refactor and experiment, any reader of the code does not have to care how it is done.
localPlayerID that is given by the game is the same for all active players in hotseat, it was my first iteration. Owner of the city does not change though and provides id of the player that owns the city.

[JHCD]

I meen human-readable for debugging (catching the used key).
For reading the lua files using this function is great.

[chaorace]

I'm feeling pretty neutral on the current key implementation. I'm leaning towards not using string indices since the PP is already a memory leak ridden mess, but the savings would be relatively minor either way, so I leave this decision up to @Kerael.

Either way, I don't plan to merge this change until tomorrow because of the breaking potential. I'd rather get the workshop version pushed out tonight, then let the nightly users experiment with this for a week before we let it out into the wild

[Kerael]

I would stick with an existing changeset; while string concatenation might be neat (achieved with .. not +) it is something that might take more CPU. It is a rare action that happens every now and then, but more often than policy changes and that lag annoys me greatly. I don't have much time to test out the change and see if it affects performance on mine (really high spec) let alone a bunch of other PCs... unlikely, but it is a drop in the bucket.
On debugging front the keys would still look x..x001 x..x010, so it is not that different from x..x_1 x..x_10.
I don't think it would affect memory much, lua's problems with memory leaking are likely to do with garbage collection that is just not designed to be real time - I would expect that Civ engine handle this every now and then (perhaps run it every few frames) - if it does not, likely might be the root of some of the issues. That said strings would require more memory to do the same job.

Testing: after another 5 hours (and finishing the session with my partner) I noticed that the queues might behave a bit funny when doing quick save and quickly going back to the city. Nothing game breaking, exiting out of the window and back in restores back the queue as it was. Next time when I play some more civ, I might try to replicate it again.

(Allow edits from maintainers is set for a reason, if you can make the change and give it a few rounds to check that it works - you are welcome =) )

[JHCD]

Okay... I was thinking to difficult... I whould say, that the localPlayerID will never be more than two digits... Key = (CityID x 100) + localPlayerID; So the first digits are the City with no limitations and the last two the localPlayerID.

[chaorace]

@JHCD Yes, that seems much better, I've implemented your suggested change. Final formula is CityID * 1000 + localPlayerID because I hate edge cases

@Kerael Alright, time to release this one into the wild and see if the nightly users can catch any issues. Just give me the word and I'll merge it

[Kerael]

CityID * 1000 + localPlayerID
It does come with an assumption that localPlayerID is a few digits at best. I would honestly expect it to be a random 4-6 digit number. That might even change on every new/loaded game.

I think I should have time today to give it a go and use some debug messages for instances where I thought I have seen some weirdness in play. With these changes on loading the game from a save file the production queue is confused for the second player until it is reset and requeued, as the first player my wife did not seem to notice anything untoward. While looking at this if I notice that the player ids are indeed only 2 digits (or in a nice order 1,2,3,4,5) I will try the suggested formula. Unless you already loaded up the game and tried some changes.

[Kerael]

Found an hour to check the change and look into cityID/playerID. There are good and bad news.

I can't seem to find a way to debug multiplayer (firetuner does not work in multiplayer). In single player though I got some data for playerID and cityID just to get some idea of how they look like.
CityID: 0x010000 (first city), 0x020001 (second city), 0x030002 (third city)...
PlayerID: 0

So I am dead certain that changes made by this tweak would have had no effect on single player.

Now what happens in multiplayer with playerID is unknown (due to challenges in debugging). Perhaps for networking reasons it would be random number, perhaps it would be 0,1,2,3,4,5... Might as well go with the suggested key, it makes more sense especially now that I see how cityID looks like and looks a lot nicer. New formula still works fine.

[JHCD]

@Kerael You are sure that you use Chris changes how to calculate the key?

[Kerael]

@JHCD used the ones from his commit for a quick check on duel map in hotseat, and a quick singleplayer check with 3 cities to get debugging info.
Reverted afterwards though as I am still playing a hotseat game with lots of cities that started with the original formula.