Open ShadesOT opened 6 years ago
There is no description whatsoever of what this ticket is about... Refactor what, and to which purpose?
Notice that these bugs are public to newcomers and the community: we should explain whenever possible what we intend to do...
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.
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, ...
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
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:
How should we move along?
In my opinion, Option 3. Reward father trade points for all auto-selling. Always.
I quite like 2.
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.
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:
Maybe erik has some AI related thoughts on this topic.
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)
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.
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.
I don't look back on what was, I just exlpain to you what is now.
The imprtant things here are, that the removal does not equal the overflow and that overflow and removal of protected yield can happen.
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.
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.
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:
I forgot to mention: Summarizing messages have a sound - which can be switched off. Messages about details never have a sound.
Please do not carry out this refactoring without us agreeing on how to solve #87