Skip to content
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: Reworked recessions setting to allow durations up to 10 years #8403

Closed
wants to merge 1 commit into from

Conversation

SamuXarick
Copy link
Contributor

@SamuXarick SamuXarick commented Dec 20, 2020

The description is that it does what it says in the title.

I thought it would be an interesting idea to increase the duration of recessions, to see, in my case, how AIs would cope with it.

@TrueBrain
Copy link
Member

Once again ... no description. I am not sure how much more clear we can get, if this is asked every single time :) Make a checklist for yourself!

So, again: please supply a motivation why you think this change is needed.

@TrueBrain TrueBrain added candidate: needs considering This Pull Request needs more opinions size: small This Pull Request is small, and should be relative easy to process work: missing intention This Pull Request is missing "why" it exists labels Dec 21, 2020
@SamuXarick
Copy link
Contributor Author

SamuXarick commented Dec 22, 2020

When I try to think about the why this exists, I can't think of a good explanation of the reasoning behind it that you'd like to hear. It is more about how AIs proceed during longer periods of a recession, than the gameplay it offers to players. Player companies are usually rich with money at the time the first recession occurs, it can pratically go unnoticed, no matter how long it lasts. But for AIs, I've noticed that some of them, could actually struggle to survive should the duration be longer. It was mainly this that motivated me to create the PR.

Initial testing, proved that, 1 year of recession wasn't damaging enough for an AI company, the routes were long enough to cause any lasting delays to the profits. At the end of the recession, they would recover. But with longer recession periods, vehicles had more time to go back and forth multiple times, and suffer the full impact of a recession by staying most of the time waiting for cargo to be loaded than travelling with it.

@SamuXarick
Copy link
Contributor Author

By the way, an older config file causes this:
screenshot#1
The workaround is to re-set recessions setting and exit openttd to update the config.

I'd like to ask for help in solving this issue.

@TrueBrain TrueBrain removed the work: missing intention This Pull Request is missing "why" it exists label Dec 22, 2020
@andythenorth
Copy link
Contributor

andythenorth commented Dec 22, 2020

Hi Samu :) I am inclined to close this one.

I refer you to the project goals https://github.com/OpenTTD/OpenTTD/blob/master/CONTRIBUTING.md#project-goals

Specifically, the section about why we don't want to add more settings, and the problems of combinatorial cases.

This PR adds a setting, and recessions are already combinatorial due to the economy (EDIT - economy type) setting.

With limited dev time, I would rather see how things like 'economy' could be exposed as an API. I don't think it's a good use of time for devs to be reviewing 'settings' PRs for both value and quality :)

@SamuXarick
Copy link
Contributor Author

It's not adding a new setting. It picks up the existing recession setting which is named "difficulty.economy" and turns it from a 'on/off' toggle into a range from 0 to 10 years, 0 meaning it's disabled.

The 'on' is still available, but in a different form. Recessions always lasted up to 1 year with 'on'. The change I'm doing is to allow recessions lasting more than 1 year.

I read that this should be achieved via GS API instead, but there's not much of a difference between this and increasing the max permitted loan setting, or adding a 'frozen' economy type. Those weren't extended by using the NewGRF/GS APIs, and have been merged already.

@TrueBrain
Copy link
Member

It's not adding a new setting. It picks up the existing recession setting which is named "difficulty.economy" and turns it from a 'on/off' toggle into a range from 0 to 10 years, 0 meaning it's disabled.

The 'on' is still available, but in a different form. Recessions always lasted up to 1 year with 'on'. The change I'm doing is to allow recessions lasting more than 1 year.

I read that this should be achieved via GS API instead, but there's not much of a difference between this and increasing the max permitted loan setting, or adding a 'frozen' economy type. Those weren't extended by using the NewGRF/GS APIs, and have been merged already.

You are making connections that do not exist. Just because it looks like a duck, smells like a duck, doesn't mean it is a duck.

The "max loan" change extends the maximum amount of loan a game can have. This can never be done via a NewGRF or a GS, and I am not sure why you would think that. Just because it also changes a setting, doesn't make it the same as this change.
The "frozen" economy, extends on a setting that existed to also allow disabling it completely. This is something different than allowing a enable/disable functionality to have values too.

So dragging those two commits in here, doesn't get you anywhere. Even if they were near identical, it doesn't change that there has to be a good reason for this PR to be merged. So please stop with the "but he is doing it too" (and I am not only referring to this PR, also in the broader context). It is not helping your case, in fact, quiet the opposite.

That said: in your own description, you created this to challenge your AIs. Which is a perfectly valid reason to make this change. But does that mean it deserves to be in vanilla? In your own words, a player would not want to use this. So ... it is only a setting for AI developers that want to know how their AI is doing? In that case, I fully agree with @andythenorth , that this PR is fine in your personal build, but doesn't belong in vanilla OpenTTD: it complicates a simple setting for only a small group of AI developers. That is not convincing to me.

So I am sorry, but I am going to decline this PR. As always, we can be convinced otherwise, but it needs stronger argumentation than: I use this to personally challenge my AIs. Again, that is a fine goal, but it is not a change that belongs in vanilla.

@TrueBrain TrueBrain closed this Dec 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: needs considering This Pull Request needs more opinions size: small This Pull Request is small, and should be relative easy to process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants