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

Do not render railway crossings if only tramways are involved (#4559). #4579

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

johsin18
Copy link
Contributor

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
grafik

After
grafik

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.

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.

Comment on lines +2290 to +2293
crossing.access AS access,
crossing.railway AS type,
crossing.railway AS railway,
track.railway AS int_lc_type
Copy link
Collaborator

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'
Copy link
Collaborator

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
Copy link
Collaborator

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 |-
Copy link
Collaborator

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 {
Copy link
Collaborator

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.

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.

Do not render railway=level_crossing for tramways
2 participants