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

Change: Decouple town cargo production from cargo type. #11378

Merged
merged 5 commits into from Feb 2, 2024

Conversation

PeterN
Copy link
Member

@PeterN PeterN commented Oct 17, 2023

Motivation / Problem

Towns are hardcoded to produce CT_PASSENGERS and CT_MAIL. If those cargo types are not available, weird things happen.

Description

This change adds a Town Production Effect property that tells a town to produce a cargo as if it were passengers or mail. No adjustment is made in case of multiple candidate cargos, you will just get higher production.

This is distinct from Town Acceptance Effect (nee Town Effect) which controls what happens to cargo received by a town. This separating is necessary to not break existing NewGRFs, and to allow flexibility.

This is also applied for generating passenger subsidies, although instead of creating more subsidies, that simply picks one at random.

This means the game no longer particularly cares about CT_PASSENGERS or CT_MAIL and goes with the town production effect.

Limitations

The NewGRF variables for town production still deal only with CT_PASSENGERS/CT_MAIL, apparently these are meant to be avoided anyway.

Checklist for review

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

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR touches english.txt or translations? Check the guidelines
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, game_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@PeterN
Copy link
Member Author

PeterN commented Oct 17, 2023

Improved Town Industries replaces the slot used by CT_MAIL with waste. Because this is not mail, the town produces it anyway.

13.4:
image
This PR:
image

OTIS sets multiple cargo types to TE_PASSENGERS/TE_MAIL

13.4:
image
This PR:
image

@Eddi-z
Copy link
Contributor

Eddi-z commented Oct 17, 2023

This will probably break some NewGRFs as towns will unexpectedly start producing more cargo types...

I'm not sure whether this automatically disqualifies the approach.

@2TallTyler
Copy link
Member

I agree. My creative abuse in Improved Town Industries is not necessarily behavior we need to continue to support. 😉

@Andrew350
Copy link

Is it possible to apply a preview tag to a draft PR? Would be nice to play with this a bit to see the effects first hand 🙂

@glx22
Copy link
Contributor

glx22 commented Oct 18, 2023

It's possible to add preview label, but preview can't access content server. You would only test changes without newgrf and I think there's nothing to see in this case.

@Andrew350
Copy link

Hmm, ok. I could have sworn there was a way to add NewGRFs to the web version but maybe I misremembered 🙂

@glx22 glx22 added the preview This PR is receiving preview builds label Dec 5, 2023
@glx22
Copy link
Contributor

glx22 commented Dec 5, 2023

Added preview label (seems they can actually access content)

@PeterN PeterN changed the title Change: Use cargo Town Effect for town production Change: Use Town Production Effect for town production Jan 7, 2024
@PeterN PeterN changed the title Change: Use Town Production Effect for town production Change: Use Town Production Effect for town cargo production Jan 7, 2024
@PeterN PeterN marked this pull request as ready for review January 7, 2024 18:35
@PeterN
Copy link
Member Author

PeterN commented Jan 7, 2024

This is now updated to use a separate property from Town Effect, which avoids the main issue of behaviour change with existing NewGRFs.

@2TallTyler
Copy link
Member

I posted this on Discord, but for the record: I like this proposal a lot. We already allow houses to have custom production using a callback, but in practice this is rarely used to its full potential. This is because industry GRFs are typically the ones which define cargos and cargo chains, and do not include houses. Most industry GRF authors do not have their own house set, and vice versa, so there is little coordination between sets.

This PR gives industry GRFs a sensible tool to make towns produce cargos, without any footguns that I see and certainly with less issues than trying to do both. I did both in Improved Town Industries 1 and required players to use both it and ITL Houses, and it was such a nightmare that I overwrite ITI 1 with ITI 2, which does the mail ID slot kludge. (ITI 1 was recently resurrected as ITI Classic, which bundles both industries and houses into one GRF file, to appease players who asked for it).

ITI 2 will need an update if this PR is merged, but I've already prepared a PR so as long as we update NML before this patch is released, it's no problem. 🙂

src/cargotype.h Outdated Show resolved Hide resolved
src/town.h Outdated Show resolved Hide resolved
src/town_cmd.cpp Show resolved Hide resolved
@PeterN
Copy link
Member Author

PeterN commented Jan 13, 2024

Updated to remove NewGRF properties added on the sly. That can come as a separate Feature.

@PeterN PeterN changed the title Change: Use Town Production Effect for town cargo production Change: Decouple town cargo production from cargo type. Jan 19, 2024
@PeterN PeterN added this to the 14.0 milestone Jan 28, 2024
@PeterN
Copy link
Member Author

PeterN commented Jan 28, 2024

Needed to update as #11719 (which depends on this) conflicts with master :)

Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no argument against this, and I know you will fix problems that may come from this. So let's go!

@2TallTyler
Copy link
Member

It's already in the milestone, but I'd just like to note that #11947 is also required in order to not break Improved Town Industries. (I'll need to add the flags, but that's easy.) Totally biased, because it's my GRF, but if we break it I'll get all the bug reports. 😛

@TrueBrain
Copy link
Member

It's already in the milestone, but I'd just like to note that #11947 is also required in order to not break Improved Town Industries. (I'll need to add the flags, but that's easy.) Totally biased, because it's my GRF, but if we break it I'll get all the bug reports. 😛

Well, I have some excellent news for you! You can review that PR yourself! Isn't it absolutely amazing? :D

@PeterN PeterN merged commit 2e6c6b7 into OpenTTD:master Feb 2, 2024
20 checks passed
@PeterN PeterN deleted the town-effect-production branch February 2, 2024 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: NewGRF This issue is related to NewGRFs preview This PR is receiving preview builds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants