Refactor CvCity::doYields()

[ShadesOT]

No description provided.

[devolution79]

Please do not carry out this refactoring without us agreeing on how to solve #87

[abmpicoli]

There is no description whatsoever of what this ticket is about... Refactor what, and to which purpose?

[abmpicoli]

Notice that these bugs are public to newcomers and the community: we should explain whenever possible what we intend to do...

[Nightinggale]

Changed milestone to 2.8 for multiple reasons. One is a suspicion that it breaks savegames and as such can't go in 2.7.x.

However we also need time to review the changes and test them. The amount of changed code grows and a proper review will take time. #196 doesn't help in finishing this quickly.

[ShadesOT]

Yeah, it is probably a good idea to not include this into a release until it is finished and tested. If time isn't our friend, ...

[ShadesOT]

The main refactoring comes along nicely.

But I spotted another possible bug in doYields() and it is one of those bugs, that stop me from progressing because I do not know what the game rules are.

The behavior in question is the rewarding of father trade points for auto-selling storage loss (part of the overflow) from a city to europe.
Auto-selling rewards father trade points only if the building, that is auto-selling, is the customs house or the central customs authority. Auto-selling because of warehouse expansien 1&2 does not reward father trade points. Despite the fact, that the father points are calculated based on the profit. The profit already incorporates

1. the percentage at which the storage loss is sold (and which is dependend on the building: warehouse expansion = 50%, warehouse expansion 2 = 60%, customs house = 80%, central customs authority = 90%)
2. the tax rate in europe

Is this the intended behavior?

I can not imagine it is. Getting father trade points for auto-selling is not documented to the players anywhere (is it?). But if a player assumes that auto-selling does reward father trade points (after all, it is selling to europe), then it should not depend on any particular building, but only if selling happened or not.
And code-wise it depends on, if the customs house trade settings have been unlocked by a building or not - which is not very generic.

I see 3 possibilities:

1. Keep it as it is.
   Pro: does not change current game mechanic.
2. Do not reward father points for autoselling at all.
   Pro: Players have to make more of an effort to get father trade points.
3. Reward father trade points for all auto-selling.
   Pro: Coherent game mechanics.

How should we move along?

[abmpicoli]

In my opinion, Option 3. Reward father trade points for all auto-selling. Always.

[LibSpit]

I quite like 2.

[Nightinggale]

I would go for 3. Looks like everybody says either 2 or 3, which makes me say the solution would be to go for 3, but add some kind of on/off switch. You can hardcode it to on right now, but make it trivial to change later if we decide otherwise. That way you aren't stalled even if we don't disagree on a solution anytime soon.

I'm not into the details of trading father point rewards, but I would assume it to make sense to make the points awarded being multiplied with the percentage of the price given to you, meaning better warehouses provides more points. Also since none of them provides 100%, it will always be better to sell in Europe from a fp point of view.

[ShadesOT]

Boah, you are asking me to keep the quirks code? :-D

My vote goes to 3 as well.

We can think ahead meanwhile. Rewarding father trade points for all auto-selling raises some rebalancing questions:

1. Do we need to rebalance at all? The change would be the same for all human players.
2. The AI is only affected in the way that it loses a head start. Because the AI, as of now cheats hardcoded, in the way, that it has the customs house settings unlocked for all it's cities from the start. It does not understand how to use them, but as this example shows, they also have other benefits. From 2nd or 3rd lowest difficulty level on, the AI also gets a free base loss sell percentage ( = starting with a whopping 50% for not having any storage building at all). So maybe it starts gaining father trade points already as early as when there are 350 units of yield in the base camp. But I don't know if it ever is in the minor "uncomfortable" situation of having overflow and loss.

Maybe erik has some AI related thoughts on this topic.

[devolution79]

I've always disliked the customs house \ sell-overflow since I think that it removes an essential part of the game which is to physically bring yields to Europe. However, I realize that we lack the automation that would be necessary to alleviate the increased burden of micromanagement if we were to choose a different system. The AI also likely benefits from not having to transport goods to Europe since it is generally inept when it comes to transport management and planning. Having said that, I clearly do not want to reward anyone for bypassing\short-circuiting this important game system and thus I prefer option 2)

[Nightinggale]

Boah, you are asking me to keep the quirks code? :-D
No. If you are comparing solution 2 and 3, the difference is granting points. Now if you do "if (A==3) { grant points}", then we can use A to switch between 2 and 3 and as such we don't have to decide on hardcoding either of those solutions right now.

We could even postpone it until CivEffects are merged and then add a "percentage multiplier" to CivEffects. This way if a warehouse grants 50%, it grants 50% and of the result it grants X% where X is the combined value of all CivEffects owned by the player. The result is cached as an int in CvPlayer, meaning the check for A becomes "if (player.getfpMultiplier() > 0)" and then this can be controlled from xml rather than DLL hardcoding. It can also be adjusted like using one civic grants a different multiplier than other civics etc.

[ShadesOT]

I have been working on doYields as you know -trying to untangle everything. And now I got something that is presentable to you and I also want your feed back. doYields as of right now works fine and as intended. Everything is straigthened out and code and comments are super verbose. Even if someone has never seen the code before, one should be able to understand it by reading it one time only.

Because the game mechanics of the customs house and overflow is also logically very intermingled, I needed to mint some new terminology to be able to describe the game mechanics in full.

So first some words on terminology, then the questions on the messages, then what comes next.

Terminology

I don't look back on what was, I just exlpain to you what is now.

For the coders

• Each city has a warehouse capacity.
• Overflow:
  If there are too many goods in the warehouse then everything above the capacity is overflow.
• Overflow protection settings:
  What we usually refer to as the "customs house screen" or "customs house popup" are the overflow protection settings, because there the player can set, which yields and amounts should be protected from getting pushed into the overflow, should overflow happen. (this also touches Protect some yields from overflow #181)
• Overflow of protected yield or protected overflow
  If the player choses to protect more amounts from overflow than there is warehouse capacity, parts of the protected yield amounts will be pushed into the overflow. - The player can not cheat by protecting all yields. Eg if the capacity is 1000 and there are 1200 hemp in the warehouse, which the player chose to protect by selecting "never sell" on hemp, 200 hemp will still be overflow.
• Removal:
  A part of the overflow is the removal. This is the part that will not be available to the city or in the city any longer.
• Removal of protected yield or protected removal
  Analogous to "overflow of protected yield".
• Sale:
  This is the part of the removal that could be sold to europe.
• Loss:
  This is the part of the removal that could not be sold and vanishes without any trace and profit for the palyer. It rotted away so to say.

The imprtant things here are, that the removal does not equal the overflow and that overflow and removal of protected yield can happen.

Special for the players

• Important goods:
  Instead of the technical term yield "protected from overflow" I chose goods, "deemed important" as in game terminology. Because ... it sounds better.
• There are no more messages mentioning the customs house in particular. This is because the different mechanics which all were combined in the logical or unlogical thing called "customs house" are now separated and could be activated by any building and maybe civ effect in the future.

Messages or what information is presented to the player - and how

I have been fiddeling around with what information about all the above to present to the players in game and how. I did some iterations on how mesages could look like and now I have 3 slightly different versions, about which I want to know, which you like best and all your other thoughts.

Preamble

Almost everything about the messages can be set with flags now. If they are shown at all, summaries, details what kind of summaries, what kind of details, sounds, colors, the messages version in general, ... . If we ever have a settings file, we can chose what values we let the players set themselves.

In the following screenshots everything is activated. I marked the summarizing messages with an S or in purple/red and the detail messages with a D or in light green. Also, when you look at the different versions and build your opinion about which one is best understandable, think about which information we should switch off. The examples are short - just messages about 2 or 3 yields per city. But if a full warehouse overflows and messages about 50 yields are displayed it will get very annoying - i think. As always, scalability is important.

Don't be confused, the names of the 3 cities are "CaseLosses", "CaseProtected" and "CaseMixSaleLoss". If If you want to play it yourself, here is the savegame of the testcase
test.message.zip
and the commit on the branch refactoringDoYields_issue#75 is 7b98402.

I leave you to it without any further explanation, so you have to figure it out yourself and so you can give me feedback like from a players point of view.

Version 4:

Version 5a:

Version 5b:

What's next

The rewrite is pretty much done. The code is straightened out now, easily readable. It is a bit slower at the moment, because the yields are iterated like 10 times instead of maybe 4, meaning literaly only the incrementation is done 500 times instead of 200 times. I will keep it in that direction for easier readability, to be honest.

Things to do:

1. Some code simplifications and cleanup.
2. Then maybe extracting parts of the code into their own functions, so they can be used elsewhere (like Add overflow icons to the city billboards #208).
3. After that a code review by someone (night, erik, some else ? )
4. And there are like 10ish issues about odd game mechanics which i noticed and some ideas that came to me, which all I am going to spin of in to their own issues.

[ShadesOT]

I forgot to mention: Summarizing messages have a sound - which can be switched off. Messages about details never have a sound.