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

Render railway:preserved=yes as preserved railway #4965

Merged
merged 21 commits into from
Jun 13, 2024

Conversation

hiddewie
Copy link
Contributor

@hiddewie hiddewie commented May 5, 2024

Fixes #2615

Previous PR related to that issue: #4361

Changes proposed in this pull request:

  • Stop rendering railway = preserved
  • Handle railway:preserved = yes for all other existing railway features.
    • Use the preserved style from the starting zoom of the railways.
    • The level of grayness of the preserved style differs somewhat per type of rail
    • Tram and subway only show the preserved style from zoom 15 onwards.
    • Tunnels never show the preserved style.

Background

Tagging: https://wiki.openstreetmap.org/wiki/Tag:railway%3Dpreserved#Alternatives_to_railway.3Dpreserved
Cancelled proposal can be found in https://wiki.openstreetmap.org/wiki/Proposal:Deprecate_railway%3Dpreserved

Taginfo comparison of tagging: https://taghistory.raifer.tech/#***/railway/preserved&***/railway%3Apreserved/yes

image

From the discussion in #2615 (comment), not all railways with railway:preserved = yes can be rendered as preserved railways, because abandoned, disused and razed railways are tagged with railway:preserved = yes as well. Such problematic tagging is out of scope of this pull request.

Preserved rail

http://0.0.0.0:6789/openstreetmap-carto/#10/52.2084/6.7731
image

http://0.0.0.0:6789/openstreetmap-carto/#11/52.1859/6.7752
image

http://0.0.0.0:6789/openstreetmap-carto/#12/52.1860/6.7752
image

http://0.0.0.0:6789/openstreetmap-carto/#13/52.1861/6.7753
image

Preserved tram

http://0.0.0.0:6789/openstreetmap-carto/#19/52.07662/6.22474
image

Preserved narrow gauge

http://0.0.0.0:6789/openstreetmap-carto/#12/50.2403/9.2913
image

http://0.0.0.0:6789/openstreetmap-carto/#15/50.2411/9.2929
image

http://0.0.0.0:6789/openstreetmap-carto/#17/50.24024/9.29116
image

railway=preserved no longer rendered

(notice the railway crossing icon)

http://0.0.0.0:6789/openstreetmap-carto/#16/52.2435/6.8035
image

@hiddewie hiddewie marked this pull request as ready for review May 5, 2024 12:56
Copy link
Collaborator

@imagico imagico left a comment

Choose a reason for hiding this comment

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

It seems that railway:preserved=yes has quite clearly superseded railway=preserved as consensus tagging. As the taghistory curves show this seems to be the result of a genuine shift in mapping practice without mass re-tagging endeavors. Hence i am in support of following this shift in OSM-Carto.

But i think this should mean:

  • not rendering railway=preserved any more - we have the long standing practice of not rendering new synonyms. So if we move to the new tagging, we should move with a clear cut.
  • the whole idea of railway:preserved=yes is that it can be combined with any railway=* value so the information on the class of railway is not lost. Hence we should support all railway=* values we also support otherwise in combination with railway:preserved=yes. Preferably with the starting zoom level matching that of the normal rendering.

@hiddewie
Copy link
Contributor Author

hiddewie commented May 6, 2024

Thank you, good points. I will get to work 👍

@hiddewie
Copy link
Contributor Author

hiddewie commented May 6, 2024

Analysis so far of useful railway values to support combined with railway:preserved = yes:

Other combinations do not make much sense to me to render differently in any way, for example nodes like railway=stop, railway=station, railway=switch, railway=halt or railway=platform.

@imagico
Copy link
Collaborator

imagico commented May 6, 2024

I think i probably need to explain my suggestions a bit better. When i wrote all railway=* values we also support otherwise i meant those values we support in the road layers for railway tracks of various types. Those are:

  • railway=rail
  • railway=light_rail
  • railway=funicular
  • railway=narrow_gauge
  • railway=miniature
  • railway=tram
  • railway=subway
  • railway=monorail
  • railway=disused

These should IMO ideally all be rendered with the 'preserved' line signature when combined with railway:preserved=yes despite some of these combinations not being in the database at the moment. Limiting our support to those combinations in use at the moment would not be in line with our goals. And there is nothing inherently making railway:preserved=yes incompatible with the other rail track types.

Implementing that in MSS with the same starting zoom levels as the feature type otherwise is going to be a bit fiddly of course - you'd need to think about what is the most efficient strategy for that. If you are unsure we can discuss the different options.

@hiddewie
Copy link
Contributor Author

hiddewie commented May 6, 2024

Until commit 5b2c452 it now supports the preserved railways I could find good examples for. Examples have been updated.

I will generalize the style a bit to take the other (linear) railway values also into account as you mentioned, sounds good.

Implementing that in MSS with the same starting zoom levels as the feature type otherwise is going to be a bit fiddly of course - you'd need to think about what is the most efficient strategy for that. If you are unsure we can discuss the different options.

The preserved railway rendering style is only visible from zoom 12 onwards. Anything lower than that shows as a plain gray line, which seems fine to me. For the low zoom levels the preserved property is not even included in the query.

Indeed there will be some fiddling and testing to make it right.

@pnorman
Copy link
Collaborator

pnorman commented May 7, 2024

For the low zoom levels the preserved property is not even included in the query

I don't think we show preserved railways at low zooms, and this should remain the same.

@imagico
Copy link
Collaborator

imagico commented May 7, 2024

I don't think we show preserved railways at low zooms, and this should remain the same.

Yes, again i was not quite precise enough. Currently railway=preserved starts at z12 - and that should be the lower limit for railway:preserved=yes as well. My point is that this should not be the universal starting zoom level for all railway:preserved=yes because railway=tram + service=yard starts at z15 for example and adding railway:preserved=yes should not suddenly make it appear already at z12.

@dch0ph
Copy link
Contributor

dch0ph commented May 7, 2024

Note also #4288 , which would be sensibly addressed at the same time.

@hiddewie hiddewie changed the title Render railway = rail with railway:preserved=yes as preserved railway Render railway:preserved=yes as preserved railway May 8, 2024
@hiddewie
Copy link
Contributor Author

hiddewie commented May 8, 2024

I made a file with some test cases for different railway types and their preserved variant. (attached, remove .txt extension because Github does not like .osm files)

sample.osm.txt

Some test renderings (nothing changes below z11 or above z16): http://0.0.0.0:6789/openstreetmap-carto/#15/52.2167/6.8123

z11
image

z12
image

z13
image

z14
image

z15
image

z16
image
image

@hiddewie
Copy link
Contributor Author

hiddewie commented May 8, 2024

Seeing the test renders, my only doubt is if preserved trams and subways should be rendered from z12 with the preserved railway style. From z12 up and and including z14 it seems to work better to render them as the plain gray line, preserved or not. Or we could even choose to render preserved trams and subways only from z15 onwards.

@hiddewie hiddewie requested a review from imagico May 8, 2024 20:34
Copy link
Collaborator

@imagico imagico left a comment

Choose a reason for hiding this comment

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

You still have treatment of railway=preserved in the SQL code, removing that should simplify the code quite a bit.

And yes, avoiding the preserved version to look heavier than the plain version at the lower zoom levels is likely a good idea.

@hiddewie
Copy link
Contributor Author

hiddewie commented May 9, 2024

You still have treatment of railway=preserved in the SQL code, removing that should simplify the code quite a bit.

This was the change I made from the previous review:

But i think this should mean: not rendering railway=preserved any more - we have the long standing practice of not rendering new synonyms. So if we move to the new tagging, we should move with a clear cut.

So the comment means that railway = preserved will be handled as railway = rail and as such the feature from the SQL query should be railway_rail and not railway_preserved. I also mentioned this in the pull request description.

Or did you mean to exclude railway = preserved from all SQL queries, and not render it at all?

@imagico
Copy link
Collaborator

imagico commented May 9, 2024

I see what you mean - IMO it would be better to completely ignore railway=preserved and not treat it as a synonym for railway=rail. But i am open to arguments in support of your implementation.

@imagico
Copy link
Collaborator

imagico commented May 9, 2024

I also did some visual testing (top block is normal, bottom is preserved):

z12:
z12
z13:
z13
z14:
z14
z15:
z15
z16
z17
z18

Observations:

  • tunnels are not treated well at most zoom levels - IMO they can stay like the non-preserved version.
  • light_rail, tram, funicular and narrow_gauge are heavier at the high zoom levels than rail - that is not good.

Otherwise this looks fairly good, still need to look over the coding.

@hiddewie
Copy link
Contributor Author

hiddewie commented May 9, 2024

Thanks for the comments. I agree, the tunnels and heaviness of the non-rail railways on high zoom levels should be improved.

I see what you mean - IMO it would be better to completely ignore railway=preserved and not treat it as a synonym for railway=rail. But i am open to arguments in support of your implementation.

I went back to the original issue #2615 and the comments so far.

At the moment it seems there is no wish to not render railway = preserved. I agree with that. In particular because there are many railways still tagged with only railway = preserved. I think we should give mappers time to retag the railways with the correct railway = * value with railway:preserved = yes.

Then the next question is what should happen with railway = preserved. Should it be rendered in the preserved style, or in a normal railway style. I would vote for rendering it as a normal railway, because it gives incentive to retag the railways with the proper tagging. Hence the current implementation to 'remap' railway = preserved to railway = rail.

The effect is that the existing railways with railway = preserved are still visible, but as a normal railway.

If there is a wish to stop rendering railway = preserved entirely, then that is also possible of course. I could live with that, but it might make for some gaps in the maps where there are suddenly no more railway lines where people would expect them to be.

@imagico
Copy link
Collaborator

imagico commented May 9, 2024

As i have explained before - as a general principle when mappers by consensus move to a new tagging abandoning an older one in a form that is clearly reflected in the data (like here) we like to make a clean cut and not render both old and new tagging at the same time. This avoids to support tendencies for proliferation of tagging with different tags or tag combinations being used to map the same real world concepts.

We did that for example with the move from highway=ford to ford=yes (#2743) and with the move from amenity=embassy to office=diplomatic + diplomatic=embassy (#4168).

Continuing to support railway=preserved as either a synonym for railway=rail + railway:preserved=yes or plain railway=rail is not compatible with that strategy.

That this has the effect that remaining features with the old tagging vanish from the map is known and accepted. It communicates to mappers that the tagging used is no more considered a generally supported way of mapping by the community at large. The alternative would be to explicitly indicate there is an error with the tagging (#4723) - but this is something we have decided we do not - as a general principle - want to do.

(sidenote, in #4965 (comment), how do you manage to make screenshots of multiple zoom levels with the railway lines in the same place, as if they are not being zoomed at all?)

I scale the geometries in PostGIS. This is part of the automated testing framework of Styleinfo - which is not open source.

@hiddewie
Copy link
Contributor Author

OK, good, clear 👍

These examples make it clear to me that fully removing the features from the map is a good move.

Commit 0581639 fully removes the railway = preserved features from the map. I will update the PR description as well.

@hiddewie hiddewie requested a review from imagico May 10, 2024 08:04
Copy link
Collaborator

@imagico imagico left a comment

Choose a reason for hiding this comment

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

Design wise this looks good now - in general the railway rendering could use an overall review of the zoom level progressions and the relative weights of the different signatures - but that is clearly a separate matter.

@pnorman requested in #4965 (comment) that preserved railways remain limited to z12+ and i concurred - so we will need to add conditions for that.

project.mml Outdated Show resolved Hide resolved
project.mml Outdated Show resolved Hide resolved
style/roads.mss Outdated Show resolved Hide resolved
style/roads.mss Show resolved Hide resolved
@pnorman
Copy link
Collaborator

pnorman commented May 10, 2024

We did that for example with the move from highway=ford to ford=yes (#2743) and with the move from amenity=embassy to office=diplomatic + diplomatic=embassy (#4168).

We removed the old tagging because it had stopped being used and removing it wouldn't impact the map.

The new tagging has proven to have replaced the old tagging in practical mapping

Then the next question is what should happen with railway = preserved. Should it be rendered in the preserved style, or in a normal railway style. I would vote for rendering it as a normal railway, because it gives incentive to retag the railways with the proper tagging. Hence the current implementation to 'remap' railway = preserved to railway = rail.

Rendering preserved railways as normal railways should not be done. It would be fine to render like it is right now (highway=preserved rendered as preserved, highway:preserved not used), render as we expect it to be in the future (highway=preserved not rendered, highway:preserved used), or some mix of the last two.

@hiddewie
Copy link
Contributor Author

@pnorman requested in #4965 (comment) that preserved railways remain limited to z12+ and i concurred - so we will need to add conditions for that.

This is already the case in the current state of the PR. Also see the test renderings in #4965 (comment).

Every of the [preserved = 'yes'] conditions is either inside a zoom condition or has a zoom condition attached. All for at least zoom 12.

@imagico
Copy link
Collaborator

imagico commented May 10, 2024

This is already the case in the current state of the PR. Also see the test renderings in #4965 (comment).

Every of the [preserved = 'yes'] conditions is either inside a zoom condition or has a zoom condition attached. All for at least zoom 12.

For me at z<12 things look exactly the same currently with or without railway:preserved=yes. @pnorman and i agreed that we should continue to not show preserved railways at z<12. Reason for showing preserved railways only at a higher zoom level is that they don't serve routine transportation purposes but are operated only for historic/touristic reasons.

For completeness: here the current rendering:

z9 z10 z11 z12 z13 z14 z15 z16 z17 z18

We removed the old tagging because it had stopped being used and removing it wouldn't impact the map.

For the record: In both cases we removed rendering support for the old tagging before it was fully out of use:

https://taghistory.raifer.tech/?#***/highway/ford
https://taghistory.raifer.tech/?#***/amenity/embassy

In both cases use had dropped to around half the peak use by the time we merged the PR.

But i think we are in agreement that the current implementation of this PR (railway=preserved not being rendered any more, railway:preserved=yes being newly interpreted) is an acceptable approach given the current state of tag use.

Copy link
Collaborator

@pnorman pnorman left a comment

Choose a reason for hiding this comment

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

Because we're adding a column to a layer with lots of columns we need to check the XML size to make sure we haven't had a combinatorial explosion

(CASE
WHEN tags->'railway:preserved' = 'yes' THEN 'yes'
ELSE 'no'
END) AS preserved,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason to use yes/no instead of a boolean?

Copy link
Collaborator

Choose a reason for hiding this comment

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

So far we only have one boolean column in the whole style i think (bridge in the aeroways layer). It would create an additional difficulty for MSS editing if different yes/no columns (or other binary/ternary attributes like surface) are of different type.

style/roads.mss Show resolved Hide resolved
@imagico
Copy link
Collaborator

imagico commented May 10, 2024

Because we're adding a column to a layer with lots of columns we need to check the XML size to make sure we haven't had a combinatorial explosion.

I had already checked that and the XML size difference is marginal.

@hiddewie
Copy link
Contributor Author

For me at z<12 things look exactly the same currently with or without railway:preserved=yes. @pnorman and i agreed that we should continue to not show preserved railways at z<12. Reason for showing preserved railways only at a higher zoom level is that they don't serve routine transportation purposes but are operated only for historic/touristic reasons.

Ah I misunderstood. This is about not showing anything preserved at zooms < 12, not about the styling of the preserved railways when they are shown.

Good, I will ensure nothing preserved is shown at zooms < 12.

@hiddewie
Copy link
Contributor Author

Preserved railways are now hidden from all rendering on zoom levels < 12:

  • The roads low query excludes the preserved railways.
  • For medium layers the styling excludes the preserved railways.

For the trams specifically I also excluded them from the low zoom levels query, because they are anyway rendered from z12 onwards.

z8
image

z9
image

z10
image

z11
image

z12
image

@hiddewie hiddewie requested a review from pnorman May 11, 2024 09:26
project.mml Outdated Show resolved Hide resolved
Copy link
Collaborator

@imagico imagico left a comment

Choose a reason for hiding this comment

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

Ok, looks good now to me and seems to work fine. There is certainly room for improvement in adjusting the zoom level progression - both for the preserved and for some of the other railway line signatures. But that is independent of this change to adopt the new tagging scheme.

@imagico imagico mentioned this pull request Jun 13, 2024
@pnorman pnorman merged commit 84c844d into gravitystorm:master Jun 13, 2024
2 checks passed
@hiddewie
Copy link
Contributor Author

Thanks!

@hiddewie hiddewie deleted the preserved-railway branch June 15, 2024 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

railway:preserved=yes + railway=rail not rendering as preserved
4 participants