-
Notifications
You must be signed in to change notification settings - Fork 823
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
Also render junction names on areas #1271
Also render junction names on areas #1271
Conversation
This resolves gravitystorm#1253.
@pnorman This explicitly converts LINESTRINGs into POLYGONs. Do you think this code makes sense? It seems to work, even if there are LINESTRINGs in the database that are not POLYGONs (they get ignored). |
UNION | ||
SELECT ST_BuildArea(way), highway, junction, ref, name | ||
FROM planet_osm_line | ||
WHERE highway = 'motorway_junction' OR highway = 'traffic_signals' OR junction = 'yes' |
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 using this you need a you need a !bbox! && way
condition in here
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.
Maybe it’s better not to add highway=motorway_junction and highway=traffic_signals here. Both are only defined for nodes, not for areas. (Or is there a technical reason to keep this?)
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.
No technical reason, we could drop it. Are you sure highway=traffic_signals is not defined on junctions, with the recent changes to the tagging scheme?
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.
Yes. Finally it was only about junction=yes, leaving complex traffic signal systems for later.
(Maybe I’ll try to re-launch discussion about the traffic signal systems, but I would not wait for this. Currently, I would drop on areas both, highway=motorway_junction and highway=traffic_signals)
You'll want to check @gravitystorm the |
@pnorman as I understand it, we're hacking around our .style being incorrect, and all the SQL cleverness is to avoid changing it and therefore avoid people having to reload their databases. So we need to weigh up whether this is important enough to carry the baggage of complexity, or whether we can wait for the next change in the .style file. We don't have a cadence for that yet (i.e. we've never done it). I'd prefer to wait and do it properly, but without a cadence that's asking to wait forever. So I say we take the complexity, and remove the complexity as soon as we can. All this presupposes that we're only talking about complexity of the code, and that there's no particular performance issue. |
@gravitystorm @pnorman As far as I know, this would not only require a change to the style file, but also a change to the osm2pgsql code that allows it to make the polygon/line decision based on the value instead of the key only. Can such a change be expected in the near future? |
If that's something we need, we should make sure there's a ticket for it upstream. |
As an osm2pgsql maintainer, I'd ask if Lua tag tranforms are an option for your needs. |
I'm not sure what Lua tag transform can do exactly - could we create a lua file that makes sure that closed ways with highway=junction are treated like areas and closed ways with other highway values are treated as lines? Also, I remember last time Lua tag transforms came up, somebody warned that this might still be buggy - are they stable now? |
See https://github.com/openstreetmap/osm2pgsql/blob/master/docs/lua.md https://github.com/openstreetmap/osm2pgsql/blob/master/style.lua is an almost replacement for the C tag processing. I consider the pgsql backend and Lua transforms production ready - and know those like @systemed who use it in production. Obviously you want to test any transform you write. I've been working with the multi backend and Lua transforms, which are not production ready, but the example multi backend scripts do show some other cases of pre-processing that can be useful. For example, yes/no/unset values can be normalized so you can use boolean columns. My main objections to adding logic to allow some values for keys to be areas and other values for the same keys to be linear is the complexity it adds to the .style files, which already almost no one fully understands. |
After having looked at the lua transforms, I think that's the way to go, especially since treating junction=yes on polygons as areas is something that should be style independent. We could add something like this to the end of
I will close this PR. |
This resolves #1253.