-
Notifications
You must be signed in to change notification settings - Fork 819
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
Add rendering for waterway=waterfall #3164
Conversation
What about rendering height? I guess this would be the same code as in #3158 (comment). |
Please remember to add the links to the testing places, so other people can test it too. |
I think height is an important factor to consider initial zoom level - see for example this one (just 6.1 m): https://www.openstreetmap.org/node/4247958331#map=19/45.57022/-122.12423 This is probably the testing place: https://www.openstreetmap.org/node/1017565710#map=16/45.5758/-122.1210 |
It may not be too hard to render height, assuming I can lift the code from natural=peak or towers. I'll give it a go. |
Please note that icon is rendered below the cliff features which decreases its readability. This is well visible in picture 3 and the combination of cliff with waterfall is not so rare. The icon of the lower waterfall is even nearly unreadable. Additional question: is it normal to separate waterfall line 2155 from line 2164 ? |
No, that's probably not normal, I'll fix it. As mentioned above I am aware that cliffs and highways are overlapping the icon and I suspect that it's some weirdness with kosmtik. Partly why this PR is WIP. Suggestions anyone? |
@meased Oops I missed you have already mentioned this. I think that water features are localised in a lower layer than cliffs. |
Just to be clear: I think also about setting initial zoom related to the height, just like we do with masts and towers, for example: openstreetmap-carto/amenity-points.mss Lines 371 to 375 in 94fe461
|
The layering has been sorted out, I had added waterfalls to the water-barriers-points class, which is lower than highways (assumably to allow roads on top of dams). I moved it to the amenity-point class and everything looks good now. I also got the height filtering working so we will be able to show taller waterfalls at lower zoom. I believe sorting by height is also working (lifted from natural=peak) so when two waterfalls overlap the taller will be preferred, though my test area only has one case of this. I still would like to get the height rendered in the name like peaks. I'll post some updated sample images later... |
Height rendering is working: The original area to show current icon rendering (none of these have height here, I recently added height to a handful of them for test rendering but have to reload my database to get it): |
That's a good question. It seems like oblique fonts are usually used on areas, whereas single points like peaks & springs tend to have non-oblique fonts. (I'm sure there are exceptions.) At any rate, I prefer the non-oblique font in this case. |
Currently using this filter: [feature = 'waterway_waterfall'] {
[zoom >= 13][height > 20],
[zoom >= 14][height > 10],
[zoom >= 15][name != null],
[zoom >= 16] { Which will show waterfalls
In action at https://www.openstreetmap.org/#map=15/45.5756/-122.1161: Unnamed waterfall at the highest zoom level it would be displayed in city context (the only one I could find): Named water fall in city context (this one is named so it would appear at zoom 15, but a city label eclipses it in this case and I couldn't find another case): Large river waterfall (this one would be in higher zoom levels if it had a height tag): |
I like the system you invented (height+name) and the testing you've made. I will try to make independent testing soon, but it looks really good. |
Would it be possible to have smaller waterfalls appear on the map sooner (i.e., at lower zoom levels), but without their label? That way they wouldn't clutter the map much, but there'd be an indication that something's there. |
project.mml
Outdated
@@ -1571,13 +1574,18 @@ Layer: | |||
WHEN tags->'ele' ~ '^-?\d{1,4}(\.\d+)?$' THEN (tags->'ele')::NUMERIC | |||
ELSE NULL | |||
END | |||
WHEN "waterway" IN ('waterfall') THEN | |||
CASE | |||
WHEN tags->'height' ~ '^\d{1,4}(\.\d+)?$' THEN (tags->'height')::NUMERIC |
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.
Check this solution please for height values with "m" unit: #3158 (comment)
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.
Right now a trailing "m" would not match the regular expression and would be dropped as if there were no height tag.
Not hard to parse around the "m", but looking at the wiki (https://wiki.openstreetmap.org/wiki/Key:height), height may be in any of the following formats:
- 123
- 123.4
- 123 m
- 123.4 m
- 12'
- 12'3"
Supporting feet and inches will take a little parsing and unit conversion, and would also affect the tower code. (The tower code could also use a fix for 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.
Wow... I would have thought feet or feet/inches was simply not allowed! I know that meters are required in the "ele" tag.
How many waterfalls are tagged with the "'" unit? If it's not too many, maybe we should just convert them all to meters and be done with it.
EDIT: In the U.S., there were 28. Plus a handful with "ft" or "feet." I've fixed them all.
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.
My suggestion would be to make this PR about waterfalls and add support for feet/inches in another PR (if we decide to do it at all).
Wether or not to support an "m" unit in the string doesn't matter much to me. This handles all of the above cases except feet/inches:
WHEN tags->'height' ~ '^\d{1,4}(\.\d+)?\s?m?$' THEN (SUBSTRING(tags->'height', '^(\d{1,4}(\.\d+)?)\s?m?$'))::NUMERIC
Thanks to @rrzefox we can now test rendering around the planet: http://bl.ocks.org/matthijsmelissen/raw/af7a602c222dbf1ff1a2c0d84ed755b7/#13.00/45.5565/-122.1355 |
project.mml
Outdated
@@ -2102,13 +2119,15 @@ Layer: | |||
access, | |||
CONCAT( | |||
name, | |||
CASE WHEN name IS NOT NULL AND elevation IS NOT NULL THEN E'\n' ELSE NULL END, | |||
CASE WHEN elevation IS NOT NULL THEN CONCAT(REPLACE(ROUND(elevation)::TEXT, '-', U&'\2212'), U&'\00A0', 'm') ELSE NULL END | |||
CASE WHEN elevation IS NOT NULL THEN CONCAT(E'\n', REPLACE(ROUND(elevation)::TEXT, '-', U&'\2212'), U&'\00A0', 'm') 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.
I think such change should be made in a separate PR, since it's about different objects. It also needs discussing, because it's not clear for me what it really does.
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 made a mistake here.
This is the code that appends the elevation text to the name column. It does need to be modified by this PR because now both elevation and height are rendered, whereas before it was only elevation.
I tried to simplify the expression by forcing a newline before the elevation/height text, but that doesn't work when the name is null. I have put it back to the original structure.
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 meant only this line of code, but now it's the same as before, so no problem here.
Could |
Seems like a good idea. Done. |
Current tagging of Niagara Falls is incomplete (no height), but detailed (specific names), so it's visible from z15: The highest waterfall (Angel Falls) is visible from z13: I have also tested few others and they all look sane, also the presented examples are OK, so thanks for all the work, especially for subsequent tuning! |
Thanks, everyone, for getting this done! |
Fixes #336
Adds icon and name rendering for waterway=waterfall.
Before
After
Various zoom levels:
(https://www.openstreetmap.org/#map=16/45.5762/-122.1140)
This uses the waterfall v3 icon from @gpsvisualizer (I did some slight pixel alignment) with both icon and text using the standard water text color.
I'm not sure why cliffs/highways are rendering on top of the waterfall icon. Can anyone offer some advice here? Is this just kosmtik?
I'm also not too happy with the low zoom. I think waterfalls are prominant features that primarily occur in rural areas and deserve to be rendered fairly low, but I'd like to try rendering only those with names at lower zooms.