-
Notifications
You must be signed in to change notification settings - Fork 819
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
Conversation
There was a problem hiding this 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 anyrailway=*
value so the information on the class of railway is not lost. Hence we should support allrailway=*
values we also support otherwise in combination withrailway:preserved=yes
. Preferably with the starting zoom level matching that of the normal rendering.
Thank you, good points. I will get to work 👍 |
Analysis so far of useful
Other combinations do not make much sense to me to render differently in any way, for example nodes like |
I think i probably need to explain my suggestions a bit better. When i wrote all
These should IMO ideally all be rendered with the 'preserved' line signature when combined with 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. |
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)
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. |
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 |
Note also #4288 , which would be sensibly addressed at the same time. |
railway = rail
with railway:preserved=yes
as preserved railwayrailway:preserved=yes
as preserved railway
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) Some test renderings (nothing changes below z11 or above z16): http://0.0.0.0:6789/openstreetmap-carto/#15/52.2167/6.8123 |
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. |
There was a problem hiding this 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.
This was the change I made from the previous review:
So the comment means that Or did you mean to exclude |
I see what you mean - IMO it would be better to completely ignore |
I also did some visual testing (top block is normal, bottom is preserved): Observations:
Otherwise this looks fairly good, still need to look over the coding. |
Thanks for the comments. I agree, the tunnels and heaviness of the non-rail railways on high zoom levels should be improved.
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 Then the next question is what should happen with The effect is that the existing railways with If there is a wish to stop rendering |
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 Continuing to support 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.
I scale the geometries in PostGIS. This is part of the automated testing framework of Styleinfo - which is not open source. |
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 |
There was a problem hiding this 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.
We removed the old tagging because it had stopped being used and removing it wouldn't impact the map.
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. |
This is already the case in the current state of the PR. Also see the test renderings in #4965 (comment). Every of the |
For me at z<12 things look exactly the same currently with or without For completeness: here the current rendering: z9 z10 z11 z12 z13 z14 z15 z16 z17 z18
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 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 ( |
There was a problem hiding this 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, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
I had already checked that and the XML size difference is marginal. |
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. |
Preserved railways are now hidden from all rendering on zoom levels < 12:
For the trams specifically I also excluded them from the low zoom levels query, because they are anyway rendered from z12 onwards. |
There was a problem hiding this 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.
Thanks! |
Fixes #2615
Previous PR related to that issue: #4361
Changes proposed in this pull request:
railway = preserved
railway:preserved = yes
for all other existing railway features.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 #2615 (comment), not all railways with
railway:preserved = yes
can be rendered as preserved railways, because abandoned, disused and razed railways are tagged withrailway: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
http://0.0.0.0:6789/openstreetmap-carto/#11/52.1859/6.7752
http://0.0.0.0:6789/openstreetmap-carto/#12/52.1860/6.7752
http://0.0.0.0:6789/openstreetmap-carto/#13/52.1861/6.7753
Preserved tram
http://0.0.0.0:6789/openstreetmap-carto/#19/52.07662/6.22474
Preserved narrow gauge
http://0.0.0.0:6789/openstreetmap-carto/#12/50.2403/9.2913
http://0.0.0.0:6789/openstreetmap-carto/#15/50.2411/9.2929
http://0.0.0.0:6789/openstreetmap-carto/#17/50.24024/9.29116
railway=preserved
no longer rendered(notice the railway crossing icon)
http://0.0.0.0:6789/openstreetmap-carto/#16/52.2435/6.8035