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.
| scienceStatus.Text = player.SummarizeScience(gameData); | ||
| treasury.Text = $"Treasury: {player.gold}"; | ||
|
|
||
| // TODO: fill these in. |
There was a problem hiding this comment.
Looks like the TODO can be removed now?
|
|
||
| public struct PlayerCommerceBreakdown { | ||
| public int corrupted; | ||
| public int taxes; |
There was a problem hiding this comment.
Could you document a couple of these? In particular, taxes vs taxmenTaxes would be helpful.
| return corrupted + beakers + happiness + toOtherCivs + maintenance + unitSupport; | ||
| } | ||
|
|
||
| public int Netflows() { |
There was a problem hiding this comment.
Is this Inflows - Outflow?
| } | ||
| } | ||
|
|
||
| public bool hasWallStreet = false; |
There was a problem hiding this comment.
I think this is fine for now, but could you leave a TODO to track this in the Building class instead?
Prototype/C7Engine/C7GameData/Building.cs
Line 33 in 25be85a
That might make a good followup PR if you wanted to try doing something simple with ImportCiv3 - the relevant field in the BIQ struct is this one, and there are several other examples of the building flags here.
|
|
||
| int currentTurn = EngineStorage.gameData.turn; | ||
|
|
||
| // If current turn < 10 and player has no cities (hasn't planted yet), apply no expenses or income. |
There was a problem hiding this comment.
Where does this come from? Is it what the game does right now?
| result.maintenance += city.MaintenanceCosts(); | ||
|
|
||
| // TODO: Hook this up to .biq or .sav specifications for tax collector income. Using default Conquests value as placeholder. | ||
| const int TAXMAN_VALUE = 2; |
There was a problem hiding this comment.
You can actually get this from the CitizenType class here, so you shouldn't need to look at the specialist index at all.
|
|
||
| result.unitSupport = TotalUnitsAllowedUnitsAndSupportCost().Item3; | ||
|
|
||
| if (hasWallStreet) result.interest = (int)Math.Floor(gold * 0.05); |
There was a problem hiding this comment.
Optional, maybe add a TODO here to load this constant from the rules. It doesn't seem to be in the BIQ file that I can see, but could be an interesting thing to be able to change.
| } | ||
| } | ||
|
|
||
| public bool hasWallStreet = false; |
There was a problem hiding this comment.
I am not sure I like the idea of the Player knowing whether they have a Wall street building. It is too verbose, and there are certainly scenarios that this is completely irrelevant.
A method that checks for it would be more appropriate I think, or an inline check where it's used if it's just the one time, and also we don't have to store the state and having to update it.
Check this here
Prototype/C7Engine/C7GameData/Player.cs
Line 830 in 25be85a
Also have in mind that a public field will end up in the json save files too
|
|
||
| // Update player flag if Wall Street is built to accrue interest in gold calculations | ||
| // TODO: Add logic to make owner.hasWallStreet false if the city holding Wall Street is captured | ||
| if (building.name == "Wall Street") owner.hasWallStreet = true; |
There was a problem hiding this comment.
Checking if the building name is "Wall Street" is again too verbose. If you think about it, we don't care what the building is, rather, what building (a small wonder) gives us the bonuses that Wall Street does, namely 5% on our treasury.
If in my mod tomorrow I decide that I have a new building called InterestMaker, I have to change the source code, and that's not good.
Have a look at
Prototype/QueryCiv3/BiqSections/Bldg.cs
Line 89 in 25be85a
and the file in general. It basically stores the traits of a building among other info.
Then, the way to make this usable would be to add it as a flag to the building itself, that it "does" something.
So then my InterestMaker building can have a "TreasuryEarnsInterest" flag and the code stays intact.
Hope this makes sense
| } | ||
| } | ||
|
|
||
| if (hasWallStreet) result.interest = (int)Math.Floor(gold * 0.05); |
There was a problem hiding this comment.
From what I read in the Civilopedia, it is 5% "with a maximum cap of 50 gold per turn", so the amount would need to be added here too.
But as I was going to say about the 0.05 which is a magic number, these values should probably go either in the Rules class here
or somehow in the building itself, and of course in the save files
3 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!