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

Add: [NewGRF] Patch flag to test if inflation is on or off. #8427

Merged
merged 2 commits into from Dec 27, 2020

Conversation

michicc
Copy link
Member

@michicc michicc commented Dec 25, 2020

Motivation / Problem

NewGRFs don't know if their costs are affected by inflation or not.

Description

The inflation setting is added to global var 0x85 (TTDPatch flags).
This is especially useful combined with e.g. #7589 to allow NewGRFs proper cost balance.

Limitations

The TTDPatch flag variable is not updated if settings are changed during a running game, but the corresponding Action 7/9 is only run during NewGRF load anyway.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

@michicc michicc added the needs review: NewGRF Review requested from a NewGRF expert label Dec 25, 2020
@TrueBrain TrueBrain added candidate: yes This Pull Request is a candidate for being merged size: trivial This Pull Request is trivial labels Dec 25, 2020
@frosch123
Copy link
Member

At some point someone got annoyed by NewGRF desyncs on join and added SGF_NO_NETWORK to all settings, which can be read by NewGRF.
Not sure what is more annoying:

  • Not being able to enable/disable inflation in multiplayer.
  • People using NewGRF that would desync joining players in that case.

@andythenorth
Copy link
Contributor

I like the intent. I'm curious how newgrf would use it in a way that benefits the player. I'm not sure what I would do with this in a vehicle grf. Ideas?

@michicc
Copy link
Member Author

michicc commented Dec 25, 2020

I like the intent. I'm curious how newgrf would use it in a way that benefits the player. I'm not sure what I would do with this in a vehicle grf. Ideas?

You could (especially in conjunction with 7589) have two sets of costs balanced with and without inflation.

@frosch123 No idea how often servers really switch that except if the proper value was not set right at the start.

@michicc
Copy link
Member Author

michicc commented Dec 25, 2020

Well, I just added a commit to make inflation a no-network setting.

@JGRennison
Copy link
Contributor

Presumably, for a NewGRF to be able to compensate for inflation, the NewGRF would also need to know the value of the inflation setting (the percentage rate), not only whether inflation is enabled.

Arguably, having some NewGRFs try to reverse the effect of inflation sort of defeats the point of having inflation as a difficulty mechanism.

@2TallTyler
Copy link
Member

2TallTyler commented Dec 26, 2020

Putting the onus on NewGRF developers to counteract a broken feature seems like the wrong approach, IMHO.

Perhaps I'm just lazy and unimaginative, but I have no interest in maintaining two cost structures and would be much more likely to simply throw a fatal error saying my NewGRF doesn't work with inflation enabled.

I'm happy to see some movement on fixing inflation, but I'm not sure about this approach.

@Eddi-z
Copy link
Contributor

Eddi-z commented Dec 26, 2020

i'm inclined to agree with @2TallTyler, it's probably better if NewGRF don't know about inflation

@andythenorth
Copy link
Contributor

andythenorth commented Dec 26, 2020

[I]...would be much more likely to simply throw a fatal error saying my NewGRF doesn't work with inflation enabled.

That's actually not a bad suggestion, in all seriousness. It does solve the problem of unwanted bug reports, and it gives players a way to resolve the issue directly (by turning inflation off) that they wouldn't otherwise discover. Patch flags are helpful for this type of check.

@Eddi-z
Copy link
Contributor

Eddi-z commented Dec 26, 2020

and it gives players a way to resolve the issue directly (by turning inflation off)

i would probably just uninstall the grf instead. or hack it so it doesn't turn off. also, it likely just generates unnecessary forum support noise.

@michicc
Copy link
Member Author

michicc commented Dec 26, 2020

Nobody would be forced to do anything with it. It just gives NewGRF authors more options, just like they can vary stuff on e.g. wagon speed limits, vehicles never expire, longer bridges and more right now.

@TrueBrain
Copy link
Member

To me it reads we all say the same. Let me explain what I am reading:

NewGRFs authors currently get bug reports about their NewGRF being broken with inflation enabled (which is still the default for new players). This PR allows NewGRFs to do something about that: either they negate inflation in one way or the other, or they can tell the user: "yo dude, listen up, if you want to use me, PLEASE disable inflation, or you will have a bad experience. I promise you, I did tune my NewGRF awesomely, and you will enjoy this immensely". (personally, I would prefer if it was just a message, and not a fatal error, but that is just me)

Sure, some people are going to disagree that a NewGRF says that, but I wonder what generates more reports: behaviour that feels like the NewGRF is broken or a message telling you it will be broken if you continue on.

In the end, being informed always wins from not being informed, in my opinion, so yeah, I think we should do this.

That said, there is one caveat in my opinion: if we take this route, NewGRFs will refuse to work with inflation enabled. So with this we can no longer rework inflation as a drop-in replacement, as NewGRFs will still refuse to work despite inflation being "fixed". But, I would consider it highly unlikely any solution to inflation will be a drop-in replacement anyway, so I think we can ignore this without hurting ourselves in the future.

@2TallTyler
Perhaps I'm just lazy and unimaginative, but I have no interest in maintaining two cost structures and would be much more likely to simply throw a fatal error saying my NewGRF doesn't work with inflation enabled.

I think that is completely your choice! (and that is a good thing!) Now you don't have it .. so adding this PR, makes it possible for you to pick a side. And the eco-system will work out who is on the right side :D

@Eddi-z
i would probably just uninstall the grf instead. or hack it so it doesn't turn off

This is exactly why I think we should do this. Let people uninstall the GRF if they really disagree with this behaviour. I mean, that means this will work itself out one way or the other. This was in fact for me the convincing argument we should do this :)

@2TallTyler
I'm happy to see some movement on fixing inflation, but I'm not sure about this approach.

I think both are true. We are already working towards disabling it by default (I hope :P), and we already have a discussion: now what? This PR does not mean we have to stop the movement on fixing inflation. It does however allow NewGRF authors to deal with current inflation, especially as it is still enabled by default :D

@michicc
Copy link
Member Author

michicc commented Dec 26, 2020

One could argue for also exposing the inflation percentage, as @JGRennison did, but my intention for this, at least, is not about NewGRFs somehow fixing inflation. It's just about giving authors more power.

Technically speaking, exposing the inflation percentage isn't complicated (except of course you'd have to fix it as a non-network setting as well). It would just need a decision if it would work better as an Action2 global variable or an ActionD Patch variable.

@Eddi-z
Copy link
Contributor

Eddi-z commented Dec 26, 2020

Imho, before we actually do this, we should consider that "inflation" actually does two different things:

  1. it cosmetically increases numbers over time
  2. it applies difficulty by increasing costs faster than income

we should probably separate those two features first

@andythenorth
Copy link
Contributor

andythenorth commented Dec 26, 2020

Re: future of inflation

Please see #8429

Re: being able to warn players about broken gameplay

If someone wants to drive this PR through, I will happily provide a patched Iron Horse test grf.

I might be able to read the flag without an nml patch, but not sure.

Let me know if you do want that.

@2TallTyler
Copy link
Member

Thanks for expanding on your arguments. You have changed my mind. 🙂

I suppose a non-fatal error would probably be sufficient, as users would likely go looking for a problem in NewGRF settings.

@LordAro LordAro merged commit 1478fa9 into OpenTTD:master Dec 27, 2020
@michicc michicc deleted the pr/grf_flag_inflation branch January 7, 2021 22:48
frosch123 added a commit to frosch123/nml that referenced this pull request Jan 30, 2021
FLHerne pushed a commit to OpenTTD/nml that referenced this pull request Jan 30, 2021
@andythenorth
Copy link
Contributor

Now implemented in Iron Horse, FWIW. Will be in IH 2.45.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: yes This Pull Request is a candidate for being merged needs review: NewGRF Review requested from a NewGRF expert size: trivial This Pull Request is trivial
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants