-
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
Moving ford to a new tagging scheme #2743
Conversation
Related to #484. I think we would need to add an amenity_line layer to accomplish this. |
Looks like |
Did you add a rendering rule for #amenity-line? |
I haven't touched openstreetmap-carto/amenity-points.mss Lines 141 to 146 in 89ef164
but I don't know how to add this rule. |
OK, now it works - the icon is visible. The question is if this whole block have to be duplicated to work with points and lines or there's a way to combine it? |
You can add In CartoCSS you can select either based on the class (with .) or based on the id (with #). The difference is that id's specify a unique layer, but multiple layers can have the same class. If you need different rules for points and lines you can do something like this:
|
I want exactly the same rules for both. |
Then adding the points class should be enough. However, you might find you still need different rules though, for example the 'interior' property doesn't really makes sense for lines. But maybe it will just get ignored. |
Adding points class made me adding a lot of columns to amenity-line layer, so it makes more sense to just define additional rules for lines. Thanks for help! |
Any comments or reviews? From my point of view it's ready and it blocks resolving #484 (I guess both need separate PRs to decide). |
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.
Some small fixes are needed, aside from that the code looks okay. Haven't loaded it up yet.
Because the amenity-line layer only has one type of feature, we could simplify its SQL significantly. On the other hand, having it mirror the other queries in form makes it easier to expand later, so the current way works too.
I support having the amenity-line stuff outside of the big amenities class selector as there's not substantial overlap.
project.mml
Outdated
COALESCE( | ||
'highway_' || CASE WHEN tags->'ford' IN ('yes', 'stepping_stones') THEN 'ford' ELSE NULL END | ||
) AS feature | ||
FROM planet_osm_line |
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.
This needs a WHERE
clause.
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.
Done.
project.mml
Outdated
@@ -1525,7 +1541,8 @@ Layer: | |||
OR "natural" IN ('peak', 'volcano', 'saddle', 'spring', 'cave_entrance') | |||
OR historic IN ('memorial', 'monument', 'archaeological_site', 'wayside_cross') | |||
OR tags->'memorial' IN ('plaque') | |||
OR highway IN ('bus_stop', 'elevator', 'traffic_signals', 'ford') | |||
OR highway IN ('bus_stop', 'elevator', 'traffic_signals') | |||
OR tags->'ford' IN ('yes', 'stepping_stones') |
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.
OR tags @> 'ford=>yes' OR tags @> 'ford=>stepping_stones'
project.mml
Outdated
@@ -1474,7 +1489,8 @@ Layer: | |||
'historic_' || CASE WHEN historic IN ('memorial', 'monument', 'archaeological_site') | |||
THEN concat_ws('_', historic, CASE WHEN tags->'memorial' IN ('plaque') THEN tags->'memorial' ELSE NULL END) | |||
ELSE NULL END, | |||
'highway_'|| CASE WHEN highway IN ('bus_stop', 'elevator', 'traffic_signals', 'ford') THEN highway ELSE NULL END, | |||
'highway_'|| CASE WHEN highway IN ('bus_stop', 'elevator', 'traffic_signals') THEN highway | |||
WHEN tags->'ford' IN ('yes', 'stepping_stones') THEN 'ford' ELSE NULL END, |
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.
WHEN tags @> 'ford=>yes' OR tags @> 'ford=>stepping_stones' THEN ...
This will be adding another layer which has to retrieve all the objects for an area from disk, even if it doesn't send them. This is best avoided, but makes sense here. Any partial indexes would have to involve the |
project.mml
Outdated
(SELECT | ||
way, | ||
COALESCE( | ||
'highway_' || CASE WHEN tags->'ford' IN ('yes', 'stepping_stones') THEN 'ford' ELSE NULL END |
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.
What about notation in this line - maybe it should be:
'highway_' || CASE WHEN tags @> 'ford=>yes' OR tags @> 'ford=>stepping_stones' THEN 'ford' ELSE NULL END
It's all too database related, so I'd like to have some kind of a rule of thumb when to use this notation and when it should be tags->'xxx'.
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.
Within the output expressions it's purely a style issue. Our guidelines currently call for @>
. The style disadvantage to @>
is that you have to repeat tags @>
for each option instead of something like && for arrays
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 understand that this is not an error, because it was working also with the first notation and I tested that it still works. I can follow this style just to have clean and consistent code, I just don't know when should we use it and when not.
So what about this line?
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.
@>
to be consistent with our style guide.
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.
Done.
Strange - it's a vector image after all... |
That's got nothing to do with being blurry, it's probably not pixel aligned. |
Resolves #267.
Simply removing support for deprecated tagging scheme and adding support for the new one (much more popular already).
I need some help, since it works for points, but not for ways. Another thing is deciding on rendering names - 3% have them, but I suspect most of them are the names of the highways.