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 railroad tracks #476

Merged
merged 53 commits into from
Aug 1, 2022
Merged

Add railroad tracks #476

merged 53 commits into from
Aug 1, 2022

Conversation

claysmalley
Copy link
Member

@claysmalley claysmalley commented Jul 7, 2022

Closes #101.

At the moment, this is a draft and only implements railway=rail. Once finished, this PR will implement all other values of railway surfaced by OpenMapTiles:

  • rail
  • subway
  • light_rail
  • tram
  • monorail
  • funicular
  • narrow_gauge
  • preserved

Screenshot from 2022-07-06 22-38-40 Screenshot from 2022-07-06 22-26-00 Screenshot from 2022-07-06 22-24-59 Screenshot from 2022-07-06 22-24-16

@claysmalley claysmalley added the enhancement New feature or request label Jul 7, 2022
@claysmalley claysmalley added this to the 1.0.0 milestone Jul 7, 2022
@claysmalley
Copy link
Member Author

Changed to gray:

Screenshot from 2022-07-07 08-51-20 Screenshot from 2022-07-07 08-52-05

@ZeLonewolf
Copy link
Member

ZeLonewolf commented Jul 7, 2022

I think the monochrome makes more sense than yellow with the conflict with toll roads. When we get to station POIs, I'm thinking they should be purple like airports, so that the conceptual transit layer sits at a common color. Also, not sure that we want to show rail at lower zooms. Google starts showing rail in hairline visibility at the equivalent to our z10 for example.

@michaelblyons
Copy link
Contributor

I'm for having the rails themselves purple as well, but perhaps desaturated relative to the POIs. The grey ones kind of look like minor roads at low zoom.

src/americana.js Outdated Show resolved Hide resolved
@wmisener
Copy link
Collaborator

wmisener commented Jul 7, 2022

I agree that the gray looks too similar to minor roads, and that having a unified transport color is desirable, and so far that's purple. Any purple hue for rail lines might have to be checked against the admin boundaries halo to make sure it doesn't clash.

@claysmalley
Copy link
Member Author

claysmalley commented Jul 7, 2022

I'm hesitant to assign purple to railroads because the majority of railroad lines in North America don't carry passenger trains, and there's not much precedent in US cartographic tradition for purple railroads. I'd rather just keep them grayscale, and reserve purple for stations.

Screenshot from 2022-07-07 17-17-14

@claysmalley claysmalley mentioned this pull request Jul 8, 2022
@claysmalley
Copy link
Member Author

If busways are rendered in purple in #477, it may set a precedent for public transit railways (subway, tram, funicular, etc.) to be purple as well.

@claysmalley
Copy link
Member Author

Added narrow gauge railways, with slightly thinner lines and dashes a little closer together than railway=rail.

Screenshot from 2022-07-15 07-49-32
Screenshot from 2022-07-15 07-50-21

@1ec5
Copy link
Member

1ec5 commented Jul 15, 2022

Remember to update scripts/taginfo_template.json with any tags for which this PR would introduce noteworthy treatment.

@claysmalley claysmalley force-pushed the clay-rail branch 2 times, most recently from 0eaed0e to 47cc928 Compare July 16, 2022 20:10
@claysmalley
Copy link
Member Author

claysmalley commented Jul 16, 2022

Now with bridge casing:
Screenshot from 2022-07-16 16-19-38

src/layer/rail.js Outdated Show resolved Hide resolved
@claysmalley claysmalley marked this pull request as ready for review July 29, 2022 21:59
@claysmalley
Copy link
Member Author

In all, this adds 214 layers, increasing the layer count from 620 to 834. I could probably reduce the count by using data-driven styling for the fill, but Maplibre does not yet support data-driven styling for dash arrays, so unfortunately the tie pattern is going to give this a high layer count for the time being.

@claysmalley
Copy link
Member Author

I managed to combine railway fill into just a few layers, bringing the added layer count to 102. The layer count in main has since been reduced to 615, and this PR will make it 717.

@claysmalley
Copy link
Member Author

claysmalley commented Jul 30, 2022

615 → 701, +86. I think that's the best I can optimize this without support for Maplibre expressions in dash arrays.

@ZeLonewolf
Copy link
Member

Are there any lessons learned from this PR that can be applied to the rest of the road layer?

@claysmalley
Copy link
Member Author

claysmalley commented Jul 30, 2022

Yeah, and I plan on working on that next 😅

In particular, styling things with expressions wherever possible makes it easier to convert everything else to expressions when new features are supported. So I think converting as many parameters of the road styling as possible into one big expression would be helpful.

@ZeLonewolf
Copy link
Member

Awesome!

Do we have an open issue for the dashed array expressions in Americana that can be linked to the maplibre issue as a blocker?

@claysmalley
Copy link
Member Author

Tracked in #526.

src/layer/rail.js Outdated Show resolved Hide resolved
@ZeLonewolf
Copy link
Member

ZeLonewolf commented Jul 30, 2022

This looks pretty good to me, visually. There is maybe a some exploration that can happen to find a way to draw a better visual distinction from the road network, but I think the style choice feels right in terms of the line style with the cross-ties representing rail.

Rail going over/under objects seems to make sense also. In this case where a rail goes from above ground, to under a building, to underground, the map makes sense:

image

If someone has an example of rail going OVER a building, we should examine that case to see if the ordering still looks right

Overall a 👍 from me with one minor comment in the code, but like to have more eyes on this.

@1ec5
Copy link
Member

1ec5 commented Jul 31, 2022

If someone has an example of rail going OVER a building, we should examine that case to see if the ordering still looks right

The AirTrain over the International Terminal at SFO:

AirTrain

src/layer/rail.js Outdated Show resolved Hide resolved
scripts/taginfo_template.json Outdated Show resolved Hide resolved
@claysmalley
Copy link
Member Author

railway=preserved is now rendered identically to railway=rail, pending OpenMapTiles support of railway:preserved=yes.

Copy link
Member

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

I’m on board with these changes! Some of the screenshots of Americana on the wiki and elsewhere are going to look really outdated now that the actual style fills in the gaps with this intricate line work.

scripts/taginfo_template.json Outdated Show resolved Hide resolved
@claysmalley claysmalley merged commit 3d65b9e into main Aug 1, 2022
@claysmalley claysmalley deleted the clay-rail branch August 1, 2022 12:18
This was referenced Aug 1, 2022
@1ec5 1ec5 mentioned this pull request Feb 6, 2023
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Railroad tracks
7 participants