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: Show rail/road/tram NewGRF name in Land Area Information window #8794

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

perezdidac
Copy link
Contributor

@perezdidac perezdidac commented Mar 2, 2021

Motivation / Problem

Currently there is no way of retrieving NewGRF name for a rail, road or tram track.

Closes #8779

Description

This code change introduces extra fields in the Land Area Information window to display the NewGRF names for rail, road and tram tracks, as well as for stations. It works for railroad crossings and overlapping road and tram tracks.

image

Limitations

Not that I am aware of.

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 affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@LordAro
Copy link
Member

LordAro commented Mar 3, 2021

Looks fine on the whole. Few warnings for you to take a look at - grffile is not null

It also concerns me that you've apparently taken a photo of your screen ;)

@perezdidac
Copy link
Contributor Author

Looks fine on the whole. Few warnings for you to take a look at - grffile is not null

It also concerns me that you've apparently taken a photo of your screen ;)

Took the photo because screenshots are not working in the most recent 1.11 beta ;-)
Thanks for the comment, will work on those warnings.

Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

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

Can I be mildly unreasonable and ask that the (initial) TrackDesc changes be in a separate commit? Then adding the GRF name is clean and easy to read & review

src/tile_cmd.h Outdated Show resolved Hide resolved
src/station_cmd.cpp Outdated Show resolved Hide resolved
src/station_cmd.cpp Outdated Show resolved Hide resolved
@LordAro
Copy link
Member

LordAro commented Mar 14, 2021

Not really what I meant. I'd rather an actual constructor:
TrackDesc(StringID type, uint16 speed, GRFFile *grffile);

(and you can also have a "default" constructor that "zero-initialises" the members too)

src/tile_cmd.h Outdated Show resolved Hide resolved
src/newgrf_config.cpp Outdated Show resolved Hide resolved
@perezdidac
Copy link
Contributor Author

FYI this is ready for review at this point :)

@@ -2808,8 +2808,7 @@ static bool ClickTile_Track(TileIndex tile)
static void GetTileDesc_Track(TileIndex tile, TileDesc *td)
{
const RailtypeInfo *rti = GetRailTypeInfo(GetRailType(tile));
td->rail_speed = rti->max_speed;
td->railtype = rti->strings.name;
td->rail_desc = TrackDesc(rti->strings.name, rti->max_speed, rti->grffile[0]);
Copy link
Member

Choose a reason for hiding this comment

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

The console command dump_info railtypes (that probably no one knows about) uses rti->grffile[RTSG_GROUND] (i.e. index 2) for GRF info output.

(Also applies to all other instances where track GRF is queried).

@@ -2098,14 +2098,12 @@ static void GetTileDesc_Road(TileIndex tile, TileDesc *td)
RoadType tram_rt = GetRoadTypeTram(tile);
if (road_rt != INVALID_ROADTYPE) {
const RoadTypeInfo *rti = GetRoadTypeInfo(road_rt);
td->roadtype = rti->strings.name;
td->road_speed = rti->max_speed / 2;
td->road_desc = TrackDesc(rti->strings.name, rti->max_speed / 2, rti->grffile[0]);
Copy link
Member

Choose a reason for hiding this comment

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

Same as above for road, ROTSG_GROUND (2).

(Also applies to all other instances where road/tram GRF is queried).

@2TallTyler 2TallTyler added waiting on author work: minor details This Pull Request has some minor details left to do labels Nov 8, 2022
@TrueBrain
Copy link
Member

Friendly ping to see if you are still interested in what appears the last few comments to get this merged? Let us know! Tnx :)

@TrueBrain TrueBrain added the candidate: yes This Pull Request is a candidate for being merged label Sep 13, 2023
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 work: minor details This Pull Request has some minor details left to do
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add: Display referencing NewGRF set when query is used on rail, road and tram tiles
5 participants