Skip to content
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

Refactor water-features #991

Merged
merged 1 commit into from
Mar 18, 2015

Conversation

matthijsmelissen
Copy link
Collaborator

Dam, weir, lock:

  • Add rendering for dam areas (supercedes and closes Rendering of Dam Area's #227, based on code by
    @chrisfleming), dam and weir nodes, and lock_gate line
  • Render weir in right layer (resolves Render weir on top of waterways #403)
  • Change rendering of weir so it renders with small dashes
  • Use marker instead of image for lock_gate node
  • Add labels for dam nodes, dam areas, weir, and lock_gate
  • Improve labels for dam lines and lock lines
  • Remove labels for undocumented and less common waterway=lock

Groyne, breakwater:

Other:

  • In general rewrite and clean up code

@matthijsmelissen
Copy link
Collaborator Author

Before:
screenshot from 2015-02-11 13 34 29

After:
screenshot from 2015-02-11 13 33 09

@mboeringa
Copy link

I like these changes. What is the unlabeled grey line on the left though? Is it a lock_gate?

I will probably copy the weir symbol in my renderer, as it represent this feature better with the overflow of water.

Be aware by the way that the "lock" tagging scheme is ill defined. There are essentially three competing tagging schemes in use:

  • waterway=lock (not documented, but >1000 uses)
  • natural=water + water=lock (documented, but it could well be tagged as just water=lock!, double tagging schemes often fail, so better to check against water=lock only..., just 261 uses currently according to taginfo)
  • lock=yes (7170 uses according to taginfo)

And there is the distinction of being as node, way and closed way of course...

You may consider rendering lock name label parallel to lock if it fits the length of the object.

@pnorman
Copy link
Collaborator

pnorman commented Oct 1, 2014

cc @systemed, as I'd like his thoughts

@systemed
Copy link
Contributor

systemed commented Oct 1, 2014

This looks good, especially the weirs and dams.

Lock tagging has pretty much evolved to be lock=yes on a way, and waterway=lock_gate on the nodes at either end. I wouldn't particularly worry about supporting other schemes.

I recently saw a rendering (the MapOut app for iOS) where the lock itself (i.e. waterway=*, lock=yes) was rendered as a deeper blue than the waterway itself. That works really well and I'd suggest it for inclusion here.

Please don't render name labels on waterway=*, lock=yes. The most common way of tagging lock names is to use name=Grand Union Canal, lock_name=Slapton Lock, because the lock is both the Grand Union Canal and Slapton Lock. (Similarly, name=High Street, bridge_name=Turnover Bridge.) Rendering name labels on locks will encourage people to tag for the renderer and put the lock name in the name= tag instead. Rendering lock_name would be great but will require a db schema change (see #828).

Finally, I'd suggest not rendering name labels on lock gates. Gates rarely have a name themselves distinct from the lock. They are often very close together - 70ft (21m) in the case of a typical UK lock - so a label is more likely to obscure than to clarify.

@mboeringa
Copy link

@systemed, do lock gates actually need rendering themselves, or is it possible to render from the endpoints of the ways tagged with lock=yes?

I ask this, because it potentially allows for a more sophisticated rendering, where you could use a ">" type symbol on the endpoints, with the point directed in the opposite direction of the way and water flow. This seems a quite common way of depicting lock gates on topographical maps.

@matthijsmelissen
Copy link
Collaborator Author

do lock gates actually need rendering themselves, or is it possible to render from the endpoints of the ways tagged with lock=yes?

Not sure if it matters, but there are locks with more than two gates.

Are there locks with less than two gates, or would such a thing not be called a lock?

@systemed
Copy link
Contributor

systemed commented Oct 1, 2014

A pound lock by definition has at least two gates, and this accounts for 95%+ of locks.

There are two frequent variations:

  • A 'staircase lock', where the top gates of one lock are the bottom gates of another.
  • A lock with an additional, intermediate set of gates, used to save water when a shorter boat is locking through.

But neither of these should prevent rendering from the endpoints. (The intermediate gates simply wouldn't be shown, and that isn't a problem.)

There are also a few much rarer edge cases, such as the circular lock in France which has three exits.

(There were also "flash locks" which are simply a gate in a weir, but these are an antiquated technology and pretty much extinct: I don't know of any still surviving. There are a few very rare single weir-gates, such as that on the River Dee at Chester, but these are so infrequent I wouldn't base any rendering decisions on them.)

On waterway maps, the > symbol is typically used for lock gates at very large scales (i.e. one at each gate). At smaller scales, it is used to symbolise the lock itself (i.e. just one in the centre of the lock). Mapnik isn't currently able to render the latter: I did supply a patch at mapnik/mapnik#1682 but it never went anywhere, I think because @gravitystorm suggested a bunch of extra functionality which is beyond my C++ skills.

@matthijsmelissen
Copy link
Collaborator Author

The intermediate gates simply wouldn't be shown, and that isn't a problem.

I think it is. I think only rendering the first and last lock here will not do.

@mboeringa
Copy link

@systemed:

On waterway maps, the > symbol is typically used for lock gates at very large scales (i.e. one at each gate). At smaller scales, it is used to symbolise the lock itself (i.e. just one in the centre of the lock).

Besides the sophisticated solution you are suggesting with an adjustment to scale, you have to wonder if you really want to render a lock symbol at small scale, e.g. 1:100000 - 1M. Maybe just putting the lock name label is enough, as it already will eat away at valuable space at these scales... and maybe locks and lock names simply shouldn't be rendered at these scales, just on high zoom (Z16-19)?

@math1985:

I think it is. I think only rendering the first and last lock here will not do.

Aren't these all separate locks? They certainly should be mapped / tagged like that in my opinion, considering the distance between them, but I do see the point of "intermediate" lock gates in step wise constructed locks...

@systemed
Copy link
Contributor

systemed commented Oct 1, 2014

@math1985: Those (Farmers Bridge) are 13 separate locks. They are not intermediate gates of one lock. Tuel Lane on the Rochdale Canal is an example of a lock with intermediate gates, though as it happens they're not mapped on OSM.

@mboeringa: Indeed - whether you want to render lock symbols at small scales depends on your audience. Ordnance Survey Landranger (1:50k) and Philips Navigator (1:100k) are examples of paper maps that do show lock symbols. Your choice as to what's appropriate for your audience - I don't use the openstreetmap-carto style as a reference map any more so can't help you there, I'm afraid.

When you refer to "step wise constructed locks" I think you mean staircases. These are mapped as two separate ways sharing a lock-gate node, and therefore your solution would still work.

@mboeringa
Copy link

Ordnance Survey Landranger (1:50k) and Philips Navigator (1:100k) are examples of paper maps that do show lock symbols.

I made a typo (corrected now), 1:10000 should have been 1:100000 (1:100k). I think, at least for 1:50k, rendering a lock symbol could still be valid, but you do run in the double lock gate symbol issue as you point out.

I don't use the openstreetmap-carto style as a reference map any more so can't help you there, I'm afraid.

What do you use?, the two map series you pointed out?, and why did you stop using OSM as reference map, if I may ask (I am well aware of tagging issues and incompleteness, but what are your personal reasons besides that)?

@systemed
Copy link
Contributor

systemed commented Oct 1, 2014

For OSM-based mapping I use my own cycle.travel style mostly, OpenCycleMap and MapQuest Open sometimes. I find openstreetmap-carto near unreadable largely because of the prominence of landuse, but also some offputtingly ugly rendering (e.g. maritime boundaries, and the shields need revisiting - which I can say with some impunity as I drew them in the first place ;) ). And I'm afraid #542 is still a blocker for me.

But no two people's tastes are the same, of course!

@mboeringa
Copy link

And I'm afraid #542 is still a blocker for me.

That one is about railway=abandoned no longer being rendered? Is it that you use that data of abandoned lines for hiking and biking trips? (this is off topic I realize, but it may help to some extent in future style development decisions)

@systemed
Copy link
Contributor

systemed commented Oct 1, 2014

They're something I'm interested in per se; in rural areas they're significant landscape features; and they're important to me as someone who volunteers for a cycle route charity which frequently reuses such lines, and who writes about such reuse. But the decision has been made in #542 and that's fine - it just makes me much less likely to use osm-carto.

@mboeringa
Copy link

I find openstreetmap-carto near unreadable largely because of the prominence of landuse, but also some offputtingly ugly rendering (e.g. maritime boundaries, and the shields need revisiting - which I can say with some impunity as I drew them in the first place ;)

I think some of this is currently being adressed by @math1985 and @mkoniecz, including the style change for this pull request, and future plans for adjustments to the style.

It remains a hugely difficult task to represent a rich - multi-scale - base map like the one on OSM, and maintain readability. I think it will take time and many small steps to get in the desired direction of a more readable map.

Your restricted cycle style is a nice example of a targeted rendering though, with a well chosen subset of features suiting the main purpose of the map.

@matthijsmelissen
Copy link
Collaborator Author

I think the building refactoring will also increase readability tremendously.

@mboeringa
Copy link

Looking at the huge lock complex near Ijmuiden, the Netherlands, I think it isn't as clear cut as I presented.

As @math1985 pointed out, some locks have extra doors, even though they do not necessarily function as separate steps to overcome large water level differences. For example, the Middensluis in Ijmuiden not only seems to have to "internal" sets of lock gate doors, but also double lock gates at the ends:

https://www.google.nl/maps/@52.4656526,4.5992349,188m/data=!3m1!1e3

Also, the lock doors can in reality both open and close against and with the genaral flow of the water, so pointing arrows will only represent the general main water flow direction through the lock, but not necessarily the way the lock gate doors are positioned (and I guess this may also be the way topographical maps use the arrow symbols, for main waterflow direction and not door configuration).

By the way, the particular lock was micro-mapped by Hendrikklaas, even drawing the lock doors individually, and tagging them an undocumented building=lock_door...:

http://www.openstreetmap.org/way/238510352

@mboeringa
Copy link

Just for reference, here is an example rendering of locks based on the endpoints of the way objects with lock=yes and using the "topographic" style ">" symbol.

Rendered by my OSM Renderer for ArcGIS:

lock_rendering_osm_renderer_for_arcgis

@dieterdreist
Copy link

this would probably better be suited for tagging ml
Wouldn't it be nicer to have a dedicated polygon object for the lock, which could get a proper "name" (rather than a "lock_name"), and would convey the width as well (you could also use a width tag on a linear way, but from a practical point of view drawing an area is easier with orthofotos than estimating a width)=

@mboeringa
Copy link

this would probably better be suited for tagging ml

Probably

Wouldn't it be nicer to have a dedicated polygon object for the lock, which could get a proper "name" (rather than a "lock_name"), and would convey the width as well (you could also use a width tag on a linear way, but from a practical point of view drawing an area is easier with orthofotos than estimating a width)=

This is the inherent "dual nature" of almost any object / tagging scheme that exists in the OSM database, and makes the life of those trying to render the data so difficult at times.

The problem is that we essentially have an OSM database storing objects that represent data in a vast range of scales, varying from the huge objects drawn at 1:1M and above, to the absolute tiny only sensibly rendered at scales of 1:100 or so... This is a major challenge.

Even if objects are defined in the Wiki as being line / non-closed way only, someone at some point in time will start doing micro-mapping, and draw the object as a polygon / closed way.

This all depends on the scale at which people are mapping. You can't draw a lock as a polygon / closed way at scale 1:10M, you can draw a polygon / closed way representing a lock at scale 1:10k or bigger.

Another thing to remember is that we have both closed ways representing water surfaces, and non-closed ways representing line networks. The latter can be used for routing, the former for things like area calculations. Both may be entirely valid representations of reality.

But as you can understand, I am just waiting with joy on the first person to map his home town with all "highway=street_lamp"s being drawn as closed ways... and then complain "it doesn't render"... ;)

In fact, I wouldn't even be surprised if overpass turbo returned instances in a search against the entire globe...

@ghost
Copy link

ghost commented Jan 13, 2015

How about waterfall icons?

#336

@matkoniecz
Copy link
Contributor

@math1985 Well, as experts are unavailable I will review it. Math, can you rebase it?

@pnorman
Copy link
Collaborator

pnorman commented Mar 13, 2015

My reluctance to review it is not many of the features modified are present locally - there's only one lock in western canada.

@matthijsmelissen
Copy link
Collaborator Author

Rebased.

@matkoniecz
Copy link
Contributor

@math1985 Github claims that

We can’t automatically merge this pull request.

@matthijsmelissen
Copy link
Collaborator Author

That's caused by a new merge conflict.

Rebased again.

@matkoniecz matkoniecz self-assigned this Mar 15, 2015
@matkoniecz
Copy link
Contributor

I started reviewing. Sample size is still low, but in 7 examples of tag usage two had major tagging problems (entire lake tagged as dam, circular dam without area=no).

@mboeringa
Copy link

I started reviewing. Sample size is still low, but in 7 examples of tag usage two had major tagging problems (entire lake tagged as dam, circular dam without area=no).

I think it is good that these problems no show up, so they can be fixed, because this sounds like a bad tagging practice.

@matkoniecz
Copy link
Contributor

Yes, obviously it is a good thing that more tagging errors will be visible.

@matkoniecz
Copy link
Contributor

There is one funny case - road on top of linear dam is nearly completely blocking rendering. But it is probably WONTFIX (I am not even making a new ticket for this one). https://www.openstreetmap.org/way/266343562#map=16/46.1778/33.6243

The one solution that I see is to render linear dams wider, but I am not convinced that it is a good idea.

Also, as rendering for nameless linear dams is not changed, this case is not affected by this PR.

@matkoniecz
Copy link
Contributor

@math1985 According to wiki[1] waterway=lock_gate should be used only on nodes. This seems to be sort-of followed. But this PR add rendering of waterway=lock_gate on ways.

[1] http://wiki.openstreetmap.org/wiki/Tag:waterway=lock%20gate

@matthijsmelissen
Copy link
Collaborator Author

5% of the lock gates are ways. Intuitively I think it also makes sense to tag lock gates as way when the river is tagged as area (the wiki documentation likely dates from the time there were no river areas yet).

However, I also don't mind following the wiki documentation, and dropping the ways from rendering.

@matkoniecz
Copy link
Contributor

Either PR or wiki should be changed. Unfortunately I have no opinion about what would be preferable.

@dieterdreist
Copy link

2015-03-15 19:50 GMT+01:00 Mateusz Konieczny [email protected]:

Either PR or wiki should be changed. Unfortunately I have no opinion about
what would be preferable

I would change the wiki to allow for ways, but only after "approval" on the
tagging mailing list.

@matkoniecz
Copy link
Contributor

@dieterdreist

I posted message on @tagging.

Dam, weir, lock:
* Add rendering for dam areas (supercedes and closes gravitystorm#227, based on code by
  @chrisfleming), dam and weir nodes, and lock_gate line
* Render weir in right layer (resolves gravitystorm#403)
* Change rendering of weir so it renders with small dashes
* Use marker instead of image for lock_gate node
* Add labels for dam nodes, dam areas, and weir
* Improve labels for dam lines and lock lines
* Don't render label for lock=yes
* Remove labels for undocumented and less common waterway=lock

Groyne, breakwater:
* Render groyne/breakwater area the same color as groyne/breakwater way
  (resolves gravitystorm#893)
* Add label for groyne, breakwater, and pier (resolves gravitystorm#474)

Other:
* In general rewrite and clean up code
@matthijsmelissen
Copy link
Collaborator Author

Rebased and squashed.

@matkoniecz
Copy link
Contributor

@math1985 Have you considered adding text-halo-fill: rgba(255,255,255,0.6); to new labels? Or is it not working well in that case?

@matkoniecz
Copy link
Contributor

@dieterdreist

I would change the wiki to allow for ways, but only after "approval" on the tagging mailing list.

I think that there was a clear approval.

pnorman added a commit that referenced this pull request Mar 18, 2015
@pnorman pnorman merged commit cede3c5 into gravitystorm:master Mar 18, 2015
@pnorman
Copy link
Collaborator

pnorman commented Mar 18, 2015

Reviewed last night, okay with it.

@matkoniecz
Copy link
Contributor

And I finished my review - and I am also OK with it, though it may be a good idea to apply #1273 also to labels introduced in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants