search

LoneGazebo / Community-Patch-DLL

Community Patch for Civilization V - Brave New World
Other
285 stars 157 forks source link

City governor does not want to assign laborers to tiles #8378

Closed bsesar closed 2 years ago

bsesar commented 2 years ago

  1. Mod version (X.Y.Z, e.g. 1.2.0): 1.1.3

  2. Mod list (if using Vox Populi only, leave blank):

  3. Error description: I just picked Tradition opener and got 2 pop. Instead of assigning this population to work the two Hill tiles, the governor leaves them as laborers, even though that is not optimal and even though I have Production focus selected.

with_laborers Clicking on the city tile (to reassign the population) does nothing and changing the focus does nothing either.

By manually assigning laborers to Hill tiles the production changes from +13.2 (with laborers) to +15.6, which is obviously better.

manual_placement

  1. Steps to reproduce (optional): Load the attached save and check out the Thebes city view.

Supporting information: Please note that you can attach .zip files by dragging-and-dropping them. If possible, zip up all supporting data and post that way.

  1. Log files (always attach your Logs folder, located at My Documents/My Games/Sid Meier's Civilization 5. For instructions, go to the repository's main page, under "To enable logging for bug reports"):

  2. Save game (always attach a save that was made a turn before the error; located at My Documents/My Games/Sid Meier's Civilization 5/ModdedSaves; you can change autosave frequency in the game's Options menu): bad_governor.zip

  3. CvMiniDump.dmp file (attach if experiencing a game crash. Located at Program Files/Steam/steamapps/common/Sid Meier's Civilization V):

  4. Screenshots (optional):

ilteroi commented 2 years ago

cannot load the save, cannot reproduce it either (but i have slightly different terrain)

bsesar commented 2 years ago

That's odd. I am certain that I am using v1.1.3 and no mods. @ilteroi, would you please try to load this save?

bad_governor2.zip

It's one turn later and now only one laborer is not assigned (the other one got assigned to the coffee tile).

ilteroi commented 2 years ago

it's a well known issue, the saves are in general not portable between machines even if the version matches.

bsesar commented 2 years ago

Well, if there is anything else I can provide (e.g., logs), please let me know.

IanE9 commented 2 years ago

@ilteroi The incompatibility of the save was almost certainly introduced by this commit: https://github.com/LoneGazebo/Community-Patch-DLL/commit/8f62a31d26375e55a0163a3ce99dda0d958e29f7

I was able to load the save provided in bad_governor.zip by commenting out this line: https://github.com/LoneGazebo/Community-Patch-DLL/blob/1f5c9d79c4b0a94d5bbe9dd9e0d9ff82165535c1/CvGameCoreDLL_Expansion2/CvGame.cpp#L11846

ilteroi commented 2 years ago

nope, i don't have that, i'm not on the bleeding edge :)

RecursiveVision commented 2 years ago

@ilteroi Could you upload the debug DLL you are using please? :)

Ian's attempting to debug why saves are incompatible.

IanE9 commented 2 years ago

nope, i don't have that, i'm not on the bleeding edge :)

It would be helpful to know what branch or commit you're building from in that case

ilteroi commented 2 years ago

04717ae6b973095566b27693255026632c5e0670

would be great if you could fix that problem. i have some suspicions but i never got far in my investigations

IanE9 commented 2 years ago

04717ae

would be great if you could fix that problem. i have some suspicions but i never got far in my investigations

* it might be due to different windows versions ... there was a crash once that only happened on windows 7 because different memory management apparently.

* it crashes while processing this here: visitor(game.m_strScriptData); might be a coincidence but it's a bit suspicious that it would be one of the few strings.

* finally i'm using a debug build and the save is probably from a release build

I've built from your branch and was able to load the save no problem. Your theory about something being platform related is likely correct. I'd like to investigate this further, but it's hard to know where the error occurs since I do not suffer any issues loading the save. I've been able to reproduce the issue though so I'll take a look at what might be causing it.

Having stepped through the process of deserializing m_strScriptData (we don't have any source code for it so I've just reviewed the disassembly) it looks like the bulk of the work occurs in zlib in order to decompress the save data. Otherwise it just checks the endianness of the saved data against the machine endianness, flips it if necessary, clears the string and then tries to assign the string from the decompressed buffer assuming the string length is non-zero. Knowing where in this process the crash occurs may shed some light on what's going on.

IanE9 commented 2 years ago

@ilteroi

I discovered what is causing this issue. CvCityCitizens::GetSpecialistValue() and CvCityCitizens::GetPlotValue() determine what yields need emphasis in different manners and it is causing the governor to value specialists vs plots irrationally.

Specifically what is happening in the save is that the governor chooses to emphasize food when calculating the value of the hills plot but does not choose to emphasize food when calculating the value of a laborer. The emphasis is occurring for the hills plot because bNeedFood is evaluated to true in CvCityCitizens::GetPlotValue() while CvCityCitizens::GetSpecialistValue() never decides to emphasize food under any circumstances.

The difference in how the values are determined is causing the hills plot to be valued at 600 (value is reduced due to food emphasis) and the laborer to be valued at 700 (value remains unchanged due to lack of food emphasis). This means the laborer is picked despite the hills plot being objectively more valuable.

ilteroi commented 2 years ago

thanks for the detective work, should be an easy fix then

edit: fix is pushed, don't have time to test it though. anyway found another bug as a side effect, so the change would need to introduce three new bugs to become net negative, what are the odds for that :)