Issue #796 - Domestic advisor details #853
Conversation
New Player.cs struct to handle empire-wide gold inflows/outflows, to be implemented with domestic advisor
Wired up all sources of incoming and outgoing GPT to Player object and domestic advisor screen. Set up compatibility with multi turn deals when implemented. Set up interest mechanic. See TODO in City.cs line 625.
TomWerner
left a comment
TomWerner
left a comment
There was a problem hiding this comment.
Looks pretty good! I haven't playtested this yet but just a couple initial comments.
Interest mechanic is now attached to buildings and pulled from ImportCiv3, not a player object boolean flag. Updated interest constants to exist in Rules.cs. Updated save .json files for Wall Street to include treasury interest flag, but I'm not sure where that's read in? Note that unit tests asserting save .jsons match developer json fail now. Fixed misc small hygiene tweaks as well.
|
@TomWerner @stavrosfa I THINK I covered all your comments with this last commit, but let me know if I missed anything or if you spot any other problems. Thanks! |
Needed to update the rules as well as the Wall Street flag! Should be good now.
|
@esbudylin I think you are the Lua expert here, should the flags for the building be added here too? Prototype/C7Engine/C7GameData/ImportCiv3.cs Line 1307 in 6419b0f and |
No, I don't think so. As far as I can see, this PR doesn't actually implement any behavior for this flag, so changes on the Lua side aren't needed. |
|
|
||
| result.unitSupport = TotalUnitsAllowedUnitsAndSupportCost().Item3; | ||
|
|
||
| if (interestBuildings > 0) result.interest = interestBuildings * Math.Min((int)(gold * rules.TreasuryInterestRate), rules.MaxInterest); |
There was a problem hiding this comment.
@esbudylin That's what cought my eye, and saw some similarities with the lua files
… gate. Fixed JSON indents.
…ary/Prototype into domesticAdvisorDetails
…coding-actuary/Prototype into domesticAdvisorDetails
TomWerner
left a comment
TomWerner
left a comment
There was a problem hiding this comment.
Very nice, thanks for the PR! Just a couple of notes.
I checked a couple of saves in civ3 vs openciv3 and the commerce numbers didn't line up exactly.
In one early game civ it looks like we aren't calculating the capital city's income properly, so that's unrelated to this PR (probably something related to despotism penalty/being on a river/civ trait/etc).
In a late game save I turned the sliders to zero and still saw negative numbers for happiness and science - I wonder if we're incorrectly counting the gold from specialists (entertainers/scientists) in our per-city accumulation. Might be worth checking.
Either way I think this PR is good to go, feel free to leave a TODO on the happiness/science thing and merge if you want. The corruption numbers matched up perfectly which was cool!
| "maxRankOfWorkableTiles": 2, | ||
| "maxRankOfBarbarianCampTiles": 2, | ||
| "defaultDealDuration": 20 | ||
| "defaultDealDuration": 20, | ||
| "treasuryInterestRate": 0.05, | ||
| "maxInterest": 50 |
There was a problem hiding this comment.
There's some sort of weird whitespace change here, could you fix that?
C7/Text/c7-static-map-save.json
Outdated
| "startUnitType2": "Worker", | ||
| "scoutUnitType": "Scout", | ||
| "maxRankOfWorkableTiles": 2, | ||
| "maxRankOfWorkableTiles": 2, |
4 participants
This one was a bit of a doozy! Lots of updates going on. Created a new struct under Player.cs that can handle all the different revenue and expense sources to put directly into the domestic advisor screen, as well as refactoring the end-turn gold reconciliation just a touch so we're not doubling calculations.
Additionally, this update should be compatible with Wall Street's interest income as well as GPT deals with other players (once implemented).
I know I messed with a lot here so lmk if any changes should be made or rolled back. Thanks!