New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature: Setting to scale cargo production of towns and industries #10606
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
a89d464
to
b00771f
Compare
I like such changes so much... It's not controversial at all, they don't destroy anything, they don't impose anything, but they only give new possibilities. :) I don't know how it will work in detail in combination with other sets, but in general I realy like this feature. |
I don't actually have an opinion about this PR. Code-wise it looks good and clean. So I am curious what users will think of it. And best way to find out .. is to just merge it :) Curious if other developers have a (strong) opinion one way or the other :) |
b00771f
to
ee6af34
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main question I have for users and other developers is whether we want one setting to scale all cargo, or one each for town cargo (including HQ production) and industry cargo. JGRPP has them separated, and it would be easy for me to split them. On the other hand...more settings. 🙂
Disclaimer: I am not a dev or even a player who is likely to ever use this feature. With that said, I can guarantee you that if you don't allow people to scale each option separately you will inevitably be confronted with complaints about not allowing enough control for different playstyles. And while I generally agree about the stance on not adding new settings, this feature seems major enough to overlook that and give the (more vocal) playerbase the option to finetune their settings how they see fit. This is, after all, aimed more at said vocal playerbase than the general audience, so I say just go for it. 99% of players will never have a clue what these settings even mean anyways (just like the rest of the settings), so what's the harm in adding one more for the players who do understand it for such a (potential) major difference in gameplay? If it's not too complicated on your end (as you say it isn't), I suggest just making each option separate now - if nothing else just to avoid the headache of having to justify later why you didn't. But that's just my 2 cents. 🙂 |
I guess the question back is: what playstyles are possible / not possible with splitting them. Although I understand where @Andrew350 is coming from, and not even disagreeing, I don't fancy adding more settings because we are afraid people will complain :) I mean, clearly it is in JGRPP, so they can just use JGRPP instead. I would rather have we tailor the vanilla game to the 99%, and that we pick something that makes sense to them; not the vocal minority. But again, I don't understand any of this (never bothered with economy in OpenTTD), so the question back is: what playstyle does it unlock :) And, to stay within the definition of vanilla's goal: can it be handled by NewGRFs instead? |
ee6af34
to
93d3a87
Compare
Updated, fixed, rebased, etc., and I split town and industry scaling into two separate settings, since I agree that's a better choice for players — including me, probably.
See the PR description for answers. 😄 |
93d3a87
to
bf1affb
Compare
src/newgrf.cpp
Outdated
|
||
case 0x27: // industry cargo scale setting | ||
*value = _settings_game.economy.industry_cargo_scale_percentage; | ||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should this be exposed to NewGRFs? We are 'going behind their back' to change adjust production, but then let them know anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, hide it from NewGRF.
Also: settings should go into GetPatchVariable(). GetGlobalVariable() is for dynamic values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an aside, putting it into GetPatchVariable would mean that you would not be able to modify the value of the settings during multiplayer games at all, because of the use of Action D.
There is otherwise no reason for the added settings to have the SF_NO_NETWORK flag.
Changing the value during a single-player game would result in it not being visible to GRFs until the next save/load or GRF reload.
This would not be ideal, given that it's quite typical that you'll want to adjust the scaling as the game develops (usually, reduce it further as the network size increases).
If these variables were exposed to GRFs, GetGlobalVariable would be more useful than GetPatchVariable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to not expose it to GRF. The original intention was to allow industry sets like FIRS to provide accurate helptext, but I agree that the potential for misuse might outweigh this.
bf1affb
to
c904a46
Compare
Does this distinguish between primary and secondary industries? If output of primary industries is reduced, then logically input of secondary industries is reduced, so already their output will lower, and then it is reduced further? (And then tertiary...) |
Yes. The modified scale-the-amount code only affects primary industry production, while secondary industries are not affected. NewGRF industries like ECS or Industries of the Caribbean that use stockpiling and the every-256-ticks callback produce less often, so it takes them longer to work through the stockpile. As the designer of one of these (Caribbean), that is how I would intend it to work, since the production rate is balanced against primary industry production. Also, as @JGRennison mentioned on Discord, a similar implementation (I started with cherry-picking that commit) has been in JGRPP for several years. There, it's used to counteract the effects of daylength, typically turned up to compensate for everything else running more slowly. 😃 |
Okay... because the section where secondary production occurs is elsewhere and not touched by this change. Makes sense.
Lovely, but experience of a change in another build that many of us have not used doesn't mean much when it comes to code reviews. |
Oh, agreed. I don't say that as a "gotcha," but I figured it was worth noting in case anyone wanted to see how it works over there. 🙂 |
@nielsmh As we discussed on Discord, I'll be changing the control of calendar progress speed in #11428 to "minutes per game year" units. What are your thoughts on the units for this pair of settings? Make them match calendar speed somehow, or keep them as percentages? (Bear in mind that industry production cannot be slower than about 13% of vanilla, or some months will have no production with a callback-based industry set, leading to all sorts of usability problems. This could be fixed with averaging or something, but that's probably out of scope for now...) |
I think actually entering minutes for direct entry and up-down buttons makes the most sense. However, the setting itself should ideally be displayed with a unit conversion too:
The last one is perhaps a bit extreme, but ideally I think printing a percentage with two significant digits, even if those need to be decimal, is preferable. As for production scaling, can it perhaps be done in the same way? "Minutes per production year"? |
This comment was marked as outdated.
This comment was marked as outdated.
c904a46
to
2f36816
Compare
2f36816
to
b56eb49
Compare
b56eb49
to
aac9a7c
Compare
aac9a7c
to
b3bf19f
Compare
Eh, already answered. Better eyesight needed :) |
In the implementation in my branch, there are deliberately not any such variables. The scaling is intended to be as transparent as practical from the point of view of the GRF. This is to make it more likely that the scaling works correctly, and as a secondary factor, to discourage GRF attempts to undo it or apply the scaling a second time, or to refuse to work if scaling is detected. Past month production/transported values are used by GRFs such as BSPI for stockpiling mechanics. The actual values are what is presented back to the GRF. |
b3bf19f
to
001fcbe
Compare
001fcbe
to
0979b28
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plague
Motivation / Problem
In #11428 I present my solution to the old daylength patch which is so popular in JGR's Patchpack.
In my conversations with players, daylength actually has two common usecases:
Slowing down economy time has several pieces:
All of these are possible with NewGRFs. However, the last two are much easier with the Base Costs GRF and #11346.
Reducing town and industry production would require the player to fork the industry and house sets of their choice and make changes to the source code before recompiling. This is problematic with closed-source GRFs and abandoned GRFs that would need decompiling into uncommented NFO.
I think it's worth offering players this control in OpenTTD itself.
JGRPP has settings that do the same thing although it's applied on top of the daylength speed and the setting value is exponential so I have no idea how to interpret it). But the survey results indicate plenty of players tweaking production to suit their play styles:
Description
Adds two settings to scale the cargo production of:
For houses, HQ, and basic industries, this simply scales the cargo by the selected percentage (using random to make up for rounding-to-zero losses).
Industries which use the "every 256 ticks" callback often do their own math with input cargos, and we don't want to break them by changing their output without their knowledge. We handle this by scaling the callback interval instead.
NewGRFs are unaware of the scale. They do not need to know about it, and giving them variables just gives authors footguns. Game Scripts can access settings directly so they don't need API additions.
The lower limit of cargo scaling for both industries and towns is 15%. This is a nicer number than the actual minimum (13%), which is determined by the 256-tick industry production callback. Any slower and the industry would not produce every month, leading to confusing feedback in the UI ("no production last month?"). Towns technically have no lower limit, but 15% is a nice limit where towns still produce any cargo. The actual minimum, 1%, typically has zero production even in large cities, which will confuse cargodist... So we make it 15% to match industries. 🙂
Limitations
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.