Render implied “oneway” on roundabouts #1367
Conversation
|
Looks fine to me. |
|
What about oneway=reversible? It is mentioned on wiki[1] and is used (rarely, 900 instances but it is). This pull requests would result in rendering oneway arrows for such ways. [1]http://wiki.openstreetmap.org/wiki/Tag%3Aoneway%3Dreversible#Time_conditional_restrictions More broadly, I am unsure about benefit of implicit oneway values - I think that benefit is small and it is yet another weird thing that makes processing data harder. |
Good catch! Currently there is no element with both (junction=roundabout AND oneway=reversible) in the database, but the PR should be adapted. I assume also the most combinations of junction=roundabout AND oneway=no are tagging errors, but that’s another topic. However, I think first there should be made a decision about this: |
|
I see both the advantages and disadvantages of rendering this, so I'm rather neutral. |
Why would you assume this? An explicit oneway is preferable. |
|
Oh, sorry, I did want to say “I assume also the most combinations of junction=roundabout AND oneway=no are tagging errors”. |
Many of them are tagging mistakes, but this tag combination is also quite common (and correct) on bicycle paths in the Netherlands. |
|
I’m also rather neutral (I don’t have a personal interest in this, I just saw #467 and thought that this is something that’s easy to implement.) Rendering implied oneway on roundabouts but not on motorways would mean rendering implied oneways for some tags, but don’t do it for other tags – which could maybe also be confusing. So maybe it’s better to omit this PR (and close also #467 as WONTFIX) … |
|
Given that oneway=yes is often omitted, it's clear that that's the default, so I think treating oneway=no in our code too makes sense. @sommerluk If you add the reversible case I will merge this. |
|
@math1985 Done. |
|
@sommerluk |
Match the color of the following labels to their respective icons: * natural=peak * natural=volcano * natural=saddle * natural=cave_entrance * historic=memorial * historic=archaeological_site * man_made=lighthouse
* Make sure conditions are checked in both WHERE and COALESCE Checking the condition in the WHERE class is necessary for performance. Checking the condition in the COALESCE is necessary to prevent unrendered tags blocking rendering of tags further down in the COALESCE. See also the discussion [here](gravitystorm#1038 (comment)). This is an improvement of gravitystorm#1038. This resolves gravitystorm#985, resolves gravitystorm#1029, resolves gravitystorm#1336. * Give amenity priority over shop. This resolves gravitystorm#963. * Give tourism priority over amenity, shop, leisure, landuse, man_made, natural, and place. This resolves gravitystorm#1269, resolves first issue of gravitystorm#1232.
Include values that are rendered as icon without label to prevent mismatch beween icons and labels.
Rather than checking a huge list that contains all common shop values, we can just check for not null in the where clause. Because the COALESCE checks the shop value any additional shop values will come through as feature is null, and not match any rendering rule. With the shop list encompassing all common values, this will seldom happen. This allows query simplification and should be no performance loss and a potential gain.
By moving the join with the ordering table out of the innermost subselect we can have all the ordering information in one place. This also adds WHERE conditions to the inner subselects to reduce the number of rows that need to be COALESCED.
|
Sorry. I failed in doing a rebase. Follow-up PR at #1393 |
For the future, something like this should work: and then: |
|
@math1985 Thanks a lot! I’ve tried this with an old branch and it worked. So I hope in the future I’ll do correct rebasing. (Git is sometimes really, really complicate …) |
Treat roads with “junction=roundabout” like oneways, as long as no “oneway=no” is present.
Resolves #467
Substitutes #1363