Render `railway:preserved=yes` as preserved railway

hiddewie commented 1 month ago

Fixes #2615

Previous PR related to that issue: https://github.com/gravitystorm/openstreetmap-carto/pull/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

From the discussion in https://github.com/gravitystorm/openstreetmap-carto/issues/2615#issuecomment-599210182, 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.

`railway=preserved` no longer rendered

(notice the railway crossing icon)

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

hiddewie commented 1 month ago

Thank you, good points. I will get to work :+1:

hiddewie commented 1 month ago

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

rail (obviously)
narrow_gauge, e.g. https://www.openstreetmap.org/way/38149955
tram: small pieces of tram-rail, e.g. https://www.openstreetmap.org/way/1015931165

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

hiddewie commented 1 month ago

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

z12

z13

z14

z15

z16

hiddewie commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 https://github.com/gravitystorm/openstreetmap-carto/issues/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.

hiddewie commented 1 month ago

After tweaking the tunnels and darkness of some of the preserved colors, it now looks like

z13

z14

z15

z16

(sidenote, in https://github.com/gravitystorm/openstreetmap-carto/pull/4965#issuecomment-2103013148, 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?)

imagico commented 1 month ago

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 https://github.com/gravitystorm/openstreetmap-carto/pull/4965#issuecomment-2103013148, 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 commented 1 month ago

OK, good, clear :+1:

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

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

pnorman commented 1 month ago

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 commented 1 month ago

@pnorman requested in https://github.com/gravitystorm/openstreetmap-carto/pull/4965#issuecomment-2097171634 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 https://github.com/gravitystorm/openstreetmap-carto/pull/4965#issuecomment-2101342058.

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 commented 1 month ago

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.

imagico commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

z9

z10

z11

z12

hiddewie commented 2 weeks ago

Thanks!

gravitystorm / openstreetmap-carto