Skip to content

[NewGRF] More options for setting vehicle refit masks - #13090

Merged
michicc merged 2 commits into
OpenTTD:masterfrom
michicc:pr/cargo_class_refit
Nov 18, 2024
Merged

michicc merged 2 commits into
OpenTTD:masterfrom
michicc:pr/cargo_class_refit

Conversation

@michicc

@michicc michicc commented Nov 17, 2024

Copy link
Copy Markdown
Member

Motivation / Problem

The existing properties to define vehicle refittability based on cargo classes are limiting.
There are properties for OR, AND NOT, and XOR, but no AND or any other boolean logic.

Description

This PR adds a new property to filter the allowed refits based on an AND of cargo classes, i.e. only cargos that have all cargo classes from the prop are added as an allowed refit.
In detail, the existing prop 28/1D/18/18 select a candidate list of refittable cargos that have at least one of the cargo classes from the prop set. The new prop then filters this list by only allowing cargos that have all of the cargo classes from the new prop set.

To future-proof the refit calculations, there's an additional new callback introduced that is run once for each vehicle type and defined cargo after NewGRF load that allows overriding the prop-based refit mask. The callback is passed the cargo classes and the local cargo ID and will either allow, disallow, or not change the refittability of each cargo.

These two features are fully independent, so there would be no problem dropping one of them.

Limitations

The new callback is run only once after NewGRF load and not cached, so a potential desync hazard if e.g. the date is included in the CB.

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')

@michicc michicc added the needs review: NewGRF Review requested from a NewGRF expert label Nov 17, 2024
@michicc
michicc force-pushed the pr/cargo_class_refit branch 3 times, most recently from 7dc929c to 87ed9e6 Compare November 17, 2024 15:12
@michicc
michicc force-pushed the pr/cargo_class_refit branch 2 times, most recently from a07edfe to 5232af7 Compare November 17, 2024 16:08
@michicc

michicc commented Nov 17, 2024

Copy link
Copy Markdown
Member Author

Test NewGRF: test_13090.zip
Made by fudging with the NFO output from NML.

It has to be used in sub-tropic climate as the only NewGRF.

It will add three new rail vehicles:

  1. "No prop 32": refittable_cargo_classes: bitmask(CC_EXPRESS, CC_BULK), which is the baseline and results in Copper Ore, Food, Fruit, Goods, and Maize.
  2. "With prop 32": Adds new_prop_32: bitmask(CC_REFRIGERATED) compared to the first vehicle. Resulting cargo is Food and Fruit, the two refrigerated cargos.
  3. "CB Test": refittable_cargo_classes: bitmask(CC_EXPRESS) and includes extra_callback_info1 & bitmask(CC_BULK, CC_REFRIGERATED) via the new CB, resulting in Food, Fruits, Goods.

@michicc
michicc marked this pull request as ready for review November 17, 2024 20:19
@michicc

michicc commented Nov 17, 2024

Copy link
Copy Markdown
Member Author

If somebody only wants to review/approve either the property or the callback, I'll happily split this into two PRs if necessary.

Added to the PR description, too: "The new callback is run only once after NewGRF load and not cached, so a potential desync hazard if e.g. the date is included in the CB."

@PeterN

PeterN commented Nov 17, 2024

Copy link
Copy Markdown
Member

The CBID_VEHICLE_REFIT_MASK name could be improved. From the point of view of an author it deals with the cargo classes and cargo slot, and returns a 0, 1, or 2. The fact the return value is used to update a mask internally isn't really relevant to the callback.

And perhaps this needs to be activated by a callback_mask bit (which of course varies by vehicle type). Iron Horse by itself has 10,000 engines, so enumerating 640,000 callbacks just for that "may" take some time. It may be fine...

@michicc
michicc force-pushed the pr/cargo_class_refit branch from 5232af7 to eed518a Compare November 17, 2024 23:09
@michicc

michicc commented Nov 17, 2024

Copy link
Copy Markdown
Member Author

Now with a callback flag. Updated test NewGRF: test_13090_v2.zip

@MnHebi

MnHebi commented Nov 18, 2024

Copy link
Copy Markdown

Best commit since the day PBS graced us with its presence.

@michicc
michicc merged commit d2496b6 into OpenTTD:master Nov 18, 2024
@michicc
michicc deleted the pr/cargo_class_refit branch November 18, 2024 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs review: NewGRF Review requested from a NewGRF expert

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants