-
Notifications
You must be signed in to change notification settings - Fork 20
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 NL signals #89
base: master
Are you sure you want to change the base?
Conversation
Please remove the pole from the images, only render the signal screen. What is the SVG used for? |
@DerDakon Done. I am still adding more signals (from the wiki, if they are actually present in OSM data) and updating the screenshots. I will mark the PR ready for review when its all committed and tested. |
I don't know how strict the maintainers of this project are, but the commits reverting stuff from earlier commits would be a no go in Wine. I'd recommend squashing them into their appropriate first commit using |
@besentv Yes I can squash the PR if you like. Github also has an option to squash PR on merge, which makes a nice timeline in the |
Sorry it took so long, this somehow got lost in the inbox. You may keep this in multiple commits, but the ones that actually fix errors in previous commits in the series should indeed be squashed, so we don't e.g. have an image file added and then removed or wrongly layouted comments introduced in the first place. |
Comments have been addressed and all commits have been squashed into 1. The screenshots still match with the current state of the PR. |
Thanks for the work on this PR so far :) |
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.
I approve it because I want it merged. But please keep in mind that rendering maps for printing will not be possible in the Netherlands because you included some icons as PNG files. Please submit a separate pull request for the three PNG icons.
@@ -408,6 +419,7 @@ Layer: | |||
ORDER BY | |||
-- distant signals are less important, signals for slower speeds are more important | |||
(CASE | |||
WHEN railway_has_key(tags, 'railway:signal:departure') THEN 15000 |
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.
Assigning 15000 to departure signals will make them rendered with higher priority than main signals. Is this intended?
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.
If I read the query correctly, the high numbers are low priority (rendered last, without overlap). The values are sorted ascending, so 15000 comes last.
2 screenshots for validation:
http://127.0.0.1:6789/openrailwaymap/#16/52.2577/6.1607
and zoomed out one level
http://127.0.0.1:6789/openrailwaymap/#15/52.2578/6.1612
Signal with ref 116
is rendered instead of the departure light.
Thanks!
Do you mean to transform the PNGs to SVG? |
Yes, vector drawings showing the same image. All other icons in our styles are vector drawings. |
I made a pull request in #105, on top of this branch, that deletes the PNG files and replaces them with equivalent SVG files. |
I merged the changes of #105 into this PR already because this PR is still open. All the icons are SVGs now in this branch. |
Departure signals Departure signals revert root departure remove unneeded distant symbols without pole Max speed light signals NL Repeated main light Fix a missing departure signal tagging combination revert maxspeed_signals.mss whitespace changes another one Main repeated states check Remove NL:light as light forms because of invalid tagging order Add support for signal categories like NL:voor NL:hoofd en NL:snelheid Refactor to NL:hoofd for main signals with speed limit box Squashed commit of the following: commit 339512c Author: Hidde Wieringa <[email protected]> Date: Thu Oct 26 20:39:31 2023 +0200 Update resource URLs as well commit c6d63a9 Author: Hidde Wieringa <[email protected]> Date: Thu Oct 26 20:34:15 2023 +0200 Modify Dutch icons to SVG commit 4a2a444 Author: Hidde Wieringa <[email protected]> Date: Wed Dec 28 17:03:15 2022 +0100 Support NL main and distant signals Departure signals Departure signals revert root departure remove unneeded distant symbols without pole Max speed light signals NL Repeated main light Fix a missing departure signal tagging combination revert maxspeed_signals.mss whitespace changes another one Main repeated states check Remove NL:light as light forms because of invalid tagging order Clean up PNG files
The icons are rendered very small. Therefore, a softening filter around the lights will not influence the rendered result. In addition, the filters are not supported by Mapnik. This commit removes dashed strokes from the lights, too.
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.
Looks good to me.
I tidied up the SVG code and removed the filters because Mapnik does not support them.
@DerDakon You can squash my commit into the previous one when you merge it.
@DerDakon What is needed to get this pull request merged? I have additional changes with more signals which are now tagged in The Netherlands. I can add those changes into this PR but I would rather make a separate PR and have the basic signalling visualized and ready for more iterative improvements. Also almost all signals in the Netherlands are now correctly tagged with a |
I suppose they are lacking the time to get to this, so we gotta wait for a bit. |
This PR enables rendering of Dutch (NL) signals:
These signals cover almost all tagged signals in The Netherlands.
Documentation about tagging in Wiki https://wiki.openstreetmap.org/wiki/OpenRailwayMap/Tagging_in_Netherlands.
Images used from Wiki, which are a compatible license and free to use in OSM. The images are slightly high but I think this suits the style fine with the label just below the signal.
Note: Not all Dutch signals haverailway:signal:direction
set which excludes them from rendering. This is fine for now.Note: Main signals with integrated light speed limits are not tagged. They will not be rendered as a combined signal (e.g. https://www.openstreetmap.org/node/6301171816 is a combined main signal with speed limit according to ProRail data, but is not tagged with
railway:signal:speed_limit:form=light
)@JJJWegdam You might be interested because you imported most of the ProRrail signals into OSM and update the Wiki and tagging scheme.
After this PR the legend in the OpenRailwayMap should also be updated with the Dutch signal images.
Around Deventer, main signals (http://127.0.0.1:6789/openrailwaymap/#14/52.2584/6.1603):
Deventer, in station (http://127.0.0.1:6789/openrailwaymap/#17/52.25686/6.16568):
Between Enschede and Gronau including distant signals (http://127.0.0.1:6789/openrailwaymap/#14/52.2194/6.9337) (note that all signals on Enschede station and on the line west of it are missing because they do not have a direction):
Max speed light signals (http://127.0.0.1:6789/openrailwaymap/#17/51.98356/5.92877):
Main repeat signals (http://127.0.0.1:6789/openrailwaymap/#16/51.9857/5.8723):
ETCS signs are rendered fine and are not modified