Change: Cargo type is label #11719

PeterN · 2024-01-08T02:33:21Z

Motivation / Problem

CargoType is landscape specific, so values change depending on which climate is selected, and it is used for default cargos of engines, industries and houses.

These default cargo types are not mapped at all, so if a NewGRF disables a cargo slot, or moves it from one slot to another, incorrect or invalid cargo can be produced.

There are 4 ways to identify cargo types:

CargoType - climate specific cargo types, up to 12 per climate. Some climates have holes.
CargoLabel - FourCC labels, defined for default cargo types, and used for setting up cargo for each climate, but otherwise only used by NewGRFs.
bitnum - obsolete, nobody cares, except 0xFF is used to disable a cargo slot.
CargoID - game-dependent cargo types, up to 64 cargo types, varies depending on newgrf configuration.

This is too many.

Description

This PR completely removes CargoType. Default engines, industries and houses are defined in terms of their cargo label which, for convenience, are defined using the same names as the CargoType enum, e.g. instead of CT_PASSENGERS = 0, CT_PASSENGERS = 'PASS'.

This means that passengers no longer has to be defined as slot 0, nor mail as slot 2. Core cargo types can now be moved or undefined without causing invalid cargo to appear.

This is built on top of #11378, which already removes hardcoded town cargo types.

Limitations

Possibly breaks some ancient cargo NewGRFs that don't define labels?

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')
- newgrf_debug_data.h may need updating.
- PR must be added to API tracker

src/newgrf.cpp

2TallTyler

Do NewGRF cargos inherit from base game cargos based on the chosen ID? (Similar to houses, industrytiles, etc.) And if so, does this break anything related to this?

src/cargo_type.h

src/industry_gui.cpp

PeterN · 2024-01-08T14:47:25Z

Do NewGRF cargos inherit from base game cargos based on the chosen ID? (Similar to houses, industrytiles, etc.) And if so, does this break anything related to this?

Cargoes directly edit the base game cargoes (after being copied from the original const definitions to the runtime definitions) by slot number, rather than making copies, so I don't think there will be any breakages there.

PeterN · 2024-01-09T08:09:14Z

Parts of this draft are now in #11720 in a revised form, and this draft should be considered as depending on that PR too—this breaks all the changes into more manageable logical pieces.

rubidium42 · 2024-02-03T08:10:49Z

src/table/cargo_const.h

+	{ CT_PASSENGERS, CT_COAL,   CT_MAIL, CT_OIL,  CT_LIVESTOCK, CT_GOODS, CT_GRAIN,  CT_WOOD, CT_IRON_ORE,     CT_STEEL,   CT_VALUABLES, 33, },
+	{ CT_PASSENGERS, CT_COAL,   CT_MAIL, CT_OIL,  CT_LIVESTOCK, CT_GOODS, CT_WHEAT,  CT_WOOD, 34,              CT_PAPER,   CT_GOLD,      CT_FOOD, },
+	{ CT_PASSENGERS, CT_RUBBER, CT_MAIL, 4,       CT_FRUIT,     CT_GOODS, CT_MAIZE,  11,      CT_COPPER_ORE,   CT_WATER,   CT_DIAMONDS,  CT_FOOD, },
+	{ CT_PASSENGERS, CT_SUGAR,  CT_MAIL, CT_TOYS, CT_BATTERIES, CT_CANDY, CT_TOFFEE, CT_COLA, CT_COTTON_CANDY, CT_BUBBLES, CT_PLASTIC,   CT_FIZZY_DRINKS, },


I'm not quite sure about this. I do not think the 33 and 34 ever meant something, so maybe use CT_INVALID instead?
The numeric tropic data seems to be respectively CT_OIL and CT_WOOD based upon https://newgrf-specs.tt-wiki.net/wiki/CargoDefaultProps. Or is there a good reason not to have them mapped?

For tropic, they are labels that are reused, but the stats are different from the first definition. 33 & 34 map to be unused slots with different data, that NewGRF could rely on being set. (Unlikely but possible)

This should be set up the same way as before, if not that's a bug.

Looking further into this I've got the feeling this is a hot mess, but can't think of any reasonable solution to make it any better. Making it a variant at least helps a bit in noticing there might be vastly different behaviour.

rubidium42 · 2024-02-03T14:33:37Z

src/table/cargo_const.h

+	{ CT_PASSENGERS, CT_COAL,   CT_MAIL, CT_OIL,  CT_LIVESTOCK, CT_GOODS, CT_GRAIN,  CT_WOOD, CT_IRON_ORE,     CT_STEEL,   CT_VALUABLES, 33, },
+	{ CT_PASSENGERS, CT_COAL,   CT_MAIL, CT_OIL,  CT_LIVESTOCK, CT_GOODS, CT_WHEAT,  CT_WOOD, 34,              CT_PAPER,   CT_GOLD,      CT_FOOD, },
+	{ CT_PASSENGERS, CT_RUBBER, CT_MAIL, 4,       CT_FRUIT,     CT_GOODS, CT_MAIZE,  11,      CT_COPPER_ORE,   CT_WATER,   CT_DIAMONDS,  CT_FOOD, },
+	{ CT_PASSENGERS, CT_SUGAR,  CT_MAIL, CT_TOYS, CT_BATTERIES, CT_CANDY, CT_TOFFEE, CT_COLA, CT_COTTON_CANDY, CT_BUBBLES, CT_PLASTIC,   CT_FIZZY_DRINKS, },


Looking further into this I've got the feeling this is a hot mess, but can't think of any reasonable solution to make it any better. Making it a variant at least helps a bit in noticing there might be vastly different behaviour.

Cargo types of default engines, industries and houses are now specified in terms of label.

PeterN · 2024-02-03T15:10:17Z

Re-ordering of CalculateRefitMasks() and FinaliseEngineArray() broke automatic cargo types for default vehicles, so this has been put back to how it was before, and the mapping from default CargoLabel to CargoID is done in CalculateRefitMasks() instead.

github-advanced-security bot found potential problems Jan 8, 2024

View reviewed changes

src/newgrf.cpp Fixed Show fixed Hide fixed

2TallTyler reviewed Jan 8, 2024

View reviewed changes

src/cargo_type.h Outdated Show resolved Hide resolved

src/cargo_type.h Outdated Show resolved Hide resolved

src/industry_gui.cpp Outdated Show resolved Hide resolved

PeterN mentioned this pull request Jan 8, 2024

Codechange: Split CT_INVALID/INVALID_CARGO and relocate special cargo placeholders. #11720

Merged

PeterN force-pushed the cargo-type-is-label branch from a5d6e2c to e03573b Compare January 28, 2024 10:01

PeterN mentioned this pull request Jan 28, 2024

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

Merged

PeterN force-pushed the cargo-type-is-label branch from e03573b to bfa2753 Compare January 28, 2024 10:26

PeterN marked this pull request as ready for review January 28, 2024 10:26

PeterN force-pushed the cargo-type-is-label branch 2 times, most recently from 879b556 to eeff2ee Compare January 28, 2024 12:58

PeterN mentioned this pull request Jan 28, 2024

Fix: Moving cargo definition slots prevents normal town/industry behaviour. #10726

Closed

TrueBrain added this to the 14.0 milestone Feb 1, 2024

PeterN force-pushed the cargo-type-is-label branch from eeff2ee to 25e4eec Compare February 2, 2024 21:19

rubidium42 reviewed Feb 3, 2024

View reviewed changes

rubidium42 previously approved these changes Feb 3, 2024

View reviewed changes

Change: Decouple and remove landscape-dependent cargo types.

1d27616

Cargo types of default engines, industries and houses are now specified in terms of label.

PeterN dismissed rubidium42’s stale review via 1d27616 February 3, 2024 15:08

PeterN force-pushed the cargo-type-is-label branch from 25e4eec to 1d27616 Compare February 3, 2024 15:08

rubidium42 approved these changes Feb 4, 2024

View reviewed changes

PeterN merged commit 2fd9096 into OpenTTD:master Feb 4, 2024
20 checks passed

PeterN deleted the cargo-type-is-label branch February 4, 2024 10:26

PeterN mentioned this pull request Feb 5, 2024

Fix 2fd90960: Missing default vehicles and industry acceptance/production. #12000

Merged

JGRennison mentioned this pull request Mar 15, 2024

[Bug]: #11719 changes the availablity of some vehicles for some GRFs #12302

Closed

PeterN mentioned this pull request Mar 17, 2024

Fix #12302: Allow empty train engines to use an invalid cargo type. #12325

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change: Cargo type is label #11719

Change: Cargo type is label #11719

PeterN commented Jan 8, 2024

2TallTyler left a comment •

edited

PeterN commented Jan 8, 2024

PeterN commented Jan 9, 2024

rubidium42 Feb 3, 2024 •

edited

PeterN Feb 3, 2024 •

edited

rubidium42 Feb 3, 2024

rubidium42 Feb 3, 2024

PeterN commented Feb 3, 2024

Change: Cargo type is label #11719

Change: Cargo type is label #11719

Conversation

PeterN commented Jan 8, 2024

Motivation / Problem

Description

Limitations

Checklist for review

2TallTyler left a comment • edited

Choose a reason for hiding this comment

PeterN commented Jan 8, 2024

PeterN commented Jan 9, 2024

rubidium42 Feb 3, 2024 • edited

Choose a reason for hiding this comment

PeterN Feb 3, 2024 • edited

Choose a reason for hiding this comment

rubidium42 Feb 3, 2024

Choose a reason for hiding this comment

rubidium42 Feb 3, 2024

Choose a reason for hiding this comment

PeterN commented Feb 3, 2024

2TallTyler left a comment •

edited

rubidium42 Feb 3, 2024 •

edited

PeterN Feb 3, 2024 •

edited