-
Notifications
You must be signed in to change notification settings - Fork 822
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
Do not render railway crossings if only tramways are involved (#4559). #4579
base: master
Are you sure you want to change the base?
Do not render railway crossings if only tramways are involved (#4559). #4579
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.
Thanks for the pull request. This is in principle the correct approach to implement rendering of railway crossings differentiated by type or railway.
My assessment of the idea from #4559 (comment) is unchanged:
Considering how widely these are mapped and how significant they are often for navigation i would be skeptical about removing rendering. But adjusting the starting zoom level or the symbol specifically for tram crossings might be a good idea.
I would encourage you to try modifying your PR along these lines, i.e. instead of dropping rendering of railway crossings when they are located on a tram track to render those in a distinct styling with a later starting zoom level and a less massive symbol. Starting at z16/z17 and using the smaller symbol currently used for z14/z15 would be a starting point but you can also try different symbol variants.
crossing.access AS access, | ||
crossing.railway AS type, | ||
crossing.railway AS railway, | ||
track.railway AS int_lc_type |
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.
These appear to be unused so should be removed.
JOIN planet_osm_line track | ||
ON ST_DWithin(crossing.way, track.way, 0.1) -- Assumes Mercator | ||
WHERE crossing.railway IN ('crossing', 'level_crossing') | ||
AND track.railway IS NOT NULL AND track.railway <> 'tram' |
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.
That means all railway crossings that are wrongly mapped not on any kind of railway (tram or otherwise) are hidden and not visible to the map user. That is incompatible with our goal of providing constructive mapper feedback i think. We - as a general rule - do not hide features from rendering based on spatial analytics telling us that they are mapped incorrectly. If a mapper maps a railway crossing in the middle of nowhere we show that because the mapper either has a good reason for that or it is an error and should be corrected and showing it helps mappers in seeing such errors.
ON ST_DWithin(crossing.way, track.way, 0.1) -- Assumes Mercator | ||
WHERE crossing.railway IN ('crossing', 'level_crossing') | ||
AND track.railway IS NOT NULL AND track.railway <> 'tram' | ||
ORDER BY crossing.way |
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.
Although this is not a newly introduced problem i would like to mention that we generally want to use explicit ordering in queries where the order of features returned matters (which is the case here because the symbols are blocking each other). It would therefore be advisable to order also by (a) priority based on railway type and (b) osm_id to always have a defined drawing order.
<<: *extents | ||
Datasource: | ||
<<: *osm2pgsql | ||
table: &railway-crossing_sql |- |
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.
The &railway-crossing_sql
is unused and therefore should be removed.
@@ -1471,7 +1471,8 @@ | |||
} | |||
} | |||
|
|||
#amenity-low-priority { | |||
#amenity-low-priority, | |||
#railway-crossings { |
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.
Since there is no overlap in style rules between the amenity-low-priority
and railway-crossings
layers the style rules for those should be separate.
Fixes #4559
Changes proposed in this pull request:
Do not render railway crossings if only tramways are involved.
Test rendering with links to the example places:
https://www.openstreetmap.org/#map=16/47.5403/7.5733
Before
After