gravitystorm / openstreetmap-carto

A general-purpose OpenStreetMap mapnik style, in CartoCSS
Other
1.53k stars 819 forks source link

Combine road lines and areas to one layer #2046

Open pnorman opened 8 years ago

pnorman commented 8 years ago

From https://github.com/gravitystorm/openstreetmap-carto/issues/1975#issuecomment-160467512

Doing this will reduce the number of layers and allow future MSS simplifications

pnorman commented 8 years ago

@math1985

Right now the layers we have are

Entries in bold are versions of the large 6kb road query.

I've successfully brought highway-area-fill into roads-fill. On initial inspection, roads-casing is the same as roads-fill, so there will be two layers returning the same results.

Would it then be possible to combine them into one layer?

matthijsmelissen commented 8 years ago

Great work!

Yes, roads-casing and roads-fill can be merged without problem. Merging highway-area-fill and roads-fill was the hard part. For tunnels and bridges, roads-casing and roads-fill are in fact already merged.

It probably also makes sense then to bring highway-area-casing into roads-casing in the same way.

To merge tunnels with the main road query, we have two options: 1) Include ferry-routes and turning-circle-casing into the main road query. This will lead to an even huger query, but with less SQL in total as we don't need a separate tunnels query. As a side effect, it would also improve the rendering of turning circles on layers/bridges. 2) Render ferry-routes and turning-circle-casing under tunnels. It is an option, but it would probably not look great.

To merge bridges with the main road query, we need to get rid of the layers turning-circle-fill, aerialways, roads-low-zoom, and waterway-bridges.

I think aerialwayscan be moved above bridges without problem. This seems a good idea even independent from the other changes, as most aerialways go indeed over bridges.

The layer roads-low-zoom can also be moved above bridges without problem, as it is only used in layers up to z9, while turning-circle-fill and waterway-bridges are only used in higher zoomlevels

For the remaining layers turning-circle-fill and waterway-bridges, we have two options again. 1) Include turning-circle-fill and waterway-bridges into the main road query. That would lead again to a huge query, but with less total SQL. It might also improve rendering a bit. 2) Render turning circles and waterway-bridges above bridges. The first would probably not look great, the second will improve the rendering in some places, and make it worse in others.

pnorman commented 8 years ago

It probably also makes sense then to bring highway-area-casing into roads-casing in the same way.

Right now if you have road 1 with layer 1 and road 2 with layer 2, you'd get

If you used attachments instead of having two layers, you'd get

I guess this works, since it's already being done for tunnels and bridges?

2) Render ferry-routes and turning-circle-casing under tunnels. It is an option, but it would probably not look great.

Ferries might be okay. As it is, they're not designed to work well over tunnels. image

I'll try moving ferries before tunnels without doing any refactoring, since that should let me know how it works. New York City is the only test area I have for this - overlapping ferries and tunnels are not the most common.

I think aerialways can be moved above bridges without problem. This seems a good idea even independent from the other changes, as most aerialways go indeed over bridges.

I'll split this into its own PR, as it stands by itself.


I don't want to include either ferries or waterway-bridges in the road/rail query. Turning circles, maybe - that will require some thought.

Mapbox Streets Vector Tiles end up using a tunnel, road, and bridge layer. That would have the advantage it'd make it easier to place stuff between tunnels and roads. But getting down to one query for all roads except low zoom has a huge advantage for reducing duplication, and should open up some MSS simplifications.

mboeringa commented 8 years ago

If you used attachments instead of having two layers, you'd get

road 1 casing
road 1 fill
road 2 casing
road 2 fill

I guess this works, since it's already being done for tunnels and bridges?

Am I right to understand that this would mean that two road sections interconnected but with different layer=x tag, would no longer visually merge their fills, like what you see happening when normal roads connect to tunnels (e.g. here: http://www.openstreetmap.org/#map=18/40.70733/-74.01535)? This would lead to many undesirable artefacts of apparently unconnected roads near bridges and viaducts. I think this would be undesirable in that case. It might actually be solvable by no longer rendering the casing "rounded" at the end point of lines, but instead to cut them straight, and have the rounded fill extend it allowing visual connection.

matthijsmelissen commented 8 years ago

I guess this works, since it's already being done for tunnels and bridges?

No, actually this wouldn't work, good point.

For tunnels, this is solved by keeping the fill visible, as in @mboeringa's link. This is good for tunnels, but wouldn't work for regular roads.

For bridges, this is solved by having casing straight instead of rounded. This leads to gaps in casing for rounded roads, for example here. It's the same issue as #470. I don't think introducing such gaps would look great.

pnorman commented 8 years ago

I've been working on this in a slightly different context, and ran into an issue I can't seem to get around with line-caps. We have line-cap: butt on bridges to let the fill cover the bridge casing at the end of bridges. The problem I run into when I have all roads in one layer is a way with layer=0 joined to a way with layer=1, neither with bridge. There is typically then a way with layer=1 bridge=yes, but this one does not take part in the bug.

image

I cannot see a way around it. Using line-cap: butt would cause problems at every dead-end street.

The current code does not have this problem because the order is road layer 0 casing, road layer 1 casing, road layer 0 fill, road layer 1 fill.

Using one layer would change it to road layer 0 casing, road layer 0 fill, road layer 1 casing, road layer 1 fill. This is the same order that is used in the current code for tunnels and bridges. This means that there is a bug with bridges that have a dead end, but these are very rare.

image

matthijsmelissen commented 8 years ago

True. However, our ordering means that we don't really use the layer tag for non-bridge/tunnel roads. If two non-bridge/tunnel roads with a different layer cross, we don't show which way is on top by the layers. The only way the layer tag has an effect if when two roads with different colour cross: then the intersection gets the colour of the road on top. However, that's a fairly subtle effect compared to using casing to show which road is on top, so I think we can say in practice we ignore the layer tag for non-bridge/tunnel roads. That's in line with the wiki by the way, as it specifies that a layer-tag needs a tunnel or bridge tag,

As we basically ignore the layer tag now for non-bridge/tunnel roads, I think a new solution could also ignore the layer tag for these roads. We could for example rewrite the layer tag to 0 in the query if there is no tunnel or bridge tag. That would avoid this problem, wouldn't it?

gravitystorm commented 8 years ago

That's in line with the wiki by the way, as it specifies that a layer-tag needs a tunnel or bridge tag,

Then the wiki is wrong (again, frustratingly). Layer tags are useful for roads that are vertically separated yet parallel, e.g. North Street and the M8 through Glasgow:

20452 jpeg image 620 x 414 pixels

(North street is on the left of the photo, running parallel to the sunken motorway but neither of them are bridges nor tunnels. Since at many zoom levels the roads are drawn wider than reality, the drawn roads overlap, and which ends up on top can be controlled by the layer tags. Although separate highway tags in this situation, it's easy to see they might have the same highway tag elsewhere in the world).

I would like to see the layer information used for the stacking of roads, and use bridge/tunnel tags only for decoration. That makes it most robust, and avoids the potential for people adding fake bridge/tunnel tags to situations where the layer tags are being ignored.

matthijsmelissen commented 8 years ago

North Street and the M8 through Glasgow:

These streets don't seem to be crossing or overlapping, so I'm not sure if a layer tag makes sense here.

gravitystorm commented 8 years ago

I added more details under the photo, but you can see in the map extract that they are overlapping.

dieterdreist commented 8 years ago

sent from a phone

Am 31.03.2016 um 10:13 schrieb math1985 notifications@github.com:

That's in line with the wiki by the way, as it specifies that a layer-tag needs a tunnel or bridge tag,

the wiki doesn't say this, it says: "With some exceptions, layer=* on ways should be used only in combination with one of tunnel=, bridge=, highway=steps, highway=elevator, covered=* or indoor=yes."

pnorman commented 8 years ago

So

Taking this a bit farther,

matthijsmelissen commented 8 years ago

Sounds good, I think this should work.

matthijsmelissen commented 8 years ago

Upon closed thought, I think it actually does not work. Consider the situation where two roads meet under an angle of 90 degrees., one road on layer 1 and the other road on layer 0. The top (normal, butt-end) casing of the road on layer 1 will now be extended too far into the intersection.

pnorman commented 8 years ago

Upon closed thought, I think it actually does not work. Consider the situation where two roads meet under an angle of 90 degrees., one road on layer 1 and the other road on layer 0. The top (normal, butt-end) casing of the road on layer 1 will now be extended too far into the intersection.

I initially thought you were wrong, so I drew the layers below, except put the bottom casing as a different color.

image

And we currently see this problem with bridges

image

Doing an initial bottom casing would fill in the missing casing on the left, but the casing at the right protruding into the fill is the problem we're talking about.

image

Because we don't have topology, I think this inherently unsolvable while maintaining casings separating roads on different layers.

matthijsmelissen commented 8 years ago

And yet another problem with this approach: dead-end, or sharply bent, streets on top of tunnels. Real-life example Your proposed approach would drop the round casing from this dead-end way, making it look like the service road leads to the tunnel.

Also inherently unsolvable without topology.

mboeringa commented 8 years ago

Because we don't have topology, I think this inherently unsolvable while maintaining casings separating roads on different layers.

+ 1, at some point, people should stop pixel peeking and accept this is not a hand-drawn map.

In my ArcGIS Renderer, I am facing similar minor issues. But the overall map look, even laser-printed, is OK, so I don't bother.

pnorman commented 8 years ago

A minor technical detail: Even though end points involve less rendering, I've come to prefer rendering the entire road to just the end point because that way everything involves line-* properties and some MSS can be combined.

The query I had for road ends was

(WITH
  bbox_roads AS (SELECT way, z_order, class FROM roads WHERE way && !bbox!),
  end_nodes AS (SELECT unnest(ARRAY[ST_PointN(way, 1), ST_PointN(way,ST_NPoints(way))]) AS way, z_order, class FROM bbox_roads)
SELECT DISTINCT ON (way)
    way,
    class
  FROM end_nodes
  ORDER BY way, z_order DESC -- take the topmost most important road
) AS road_ends
StyXman commented 8 years ago

I noticed the n-plication of very similar 9and sometimes identical) queries on the layers. All those queries are run independently, (over)loading the database (and its connections). What would actually be needed to solve this is two different things: a) support in the styling/layering code to reference a single query and b) support in mapnik to actually execute it as such.

It could also be interesting to be able to reuse a previous query for a certain (meta)tile for rendering the tiles in the next zoom level (thinking of a pyramid rendering scheme). I guess I'm simply calling for another renderer altogether.

For the moment, I can only think of creating more _parts and then referencing them in the .yaml, but that doesn't solve the performance part of the problem.

pnorman commented 8 years ago

I did a clean implementation of roads rendering from scratch

layer definitions MSS

The road styling is simpler, combining some classifications, but it does handle the way as highway and railway case. It ends up being a lot cleaner than other implementations, and has a few features I'd like to implement in osm-carto. It doesn't yet have road text or shields.

Given the huge reduction in code size and maintenance burden we could, I'm convinced that going forward is a good idea, even if some edge-cases with layering and casing look worse.

matthijsmelissen commented 8 years ago

Looks promising.

Of course the decrease in code size alone doesn't tell too much, as is for a large part achieved by removing functionality and by moving complexity to the .lua file.

I hacked the code a bit to get it to work with a standard rendering database. In my set-up, your code renders all roads on top of bridges. Does it do the same for you or is my set-up to blame?

Apart from that I don't immediately see major issues in Luxembourg.

pnorman commented 7 years ago

In my set-up, your code renders all roads on top of bridges. Does it do the same for you or is my set-up to blame?

Just to note that this is probably an issue with the hacking you did on the code, in particular, something related to enum types and sorting.


After doing a bit of editing of the SQL queries, I think how I'm going to tackle this is to reimplement the roads code, rather than trying to change it bit by bit.

pnorman commented 7 years ago

I started writing SQL, and I'm thinking of a statement along the lines of

SELECT osm_id, st_geometrytype(way) "type", highway, railway, link, construction, tunnel, bridge, layernotnull FROM 
(
WITH lines AS (
  SELECT
      osm_id, -- for dev debugging
      way,
      CASE 
        WHEN substr(highway, length(highway)-4, 5) = '_link' THEN substr(highway, 0, length(highway)-4) -- Handle link roads
        WHEN highway = 'path' -- Logic to disambiguate highway=path. Because there's no path_link, it's never caught by the previous logic.
          THEN CASE
            WHEN horse = 'designated' THEN 'bridleway'
            WHEN bicycle = 'designated' THEN 'cycleway'
            ELSE 'footway'
          END
        ELSE highway END AS highway,
      railway,
      construction,
      substr(highway, length(highway)-4, 5) = '_link' AS link,
      COALESCE(tunnel IN ('yes', 'building_passage') OR covered = 'yes', FALSE) AS tunnel,
      COALESCE(bridge IN ('yes', 'boardwalk', 'cantilever', 'covered', 'low_water_crossing', 'movable', 'trestle', 'viaduct'), FALSE) AS bridge,
      COALESCE(layer,0) AS layernotnull,
      z_order,
      surface IN ('paved', 'asphalt', 'cobblestone', 'cobblestone:flattened', 'sett', 'concrete', 'concrete:lanes', 'concrete:plates', 'paving_stones', 'metal', 'wood')
        OR NOT surface IN ('unpaved', 'compacted', 'dirt', 'earth', 'fine_gravel', 'grass', 'grass_paver', 'gravel', 'ground', 'mud', 'pebblestone', 'salt', 'sand', 'woodchips', 'clay')
        AS paved,
      tracktype,
      CASE WHEN access IN ('destination') THEN 'destination'
        WHEN access IN ('no', 'private') THEN 'no'
        ELSE NULL END AS access,
      COALESCE(service IN ('parking_aisle', 'drive-through', 'driveway', 'spur', 'siding', 'yard'), FALSE) AS service
    FROM planet_osm_line
    WHERE way && :'bbox'
      AND (highway IS NOT NULL OR railway IS NOT NULL)
),
tc_points AS (
  -- We need the turning_circle/loop point and the highway type
  SELECT
    DISTINCT ON (points.osm_id)
      points.osm_id, -- for dev debugging
      points.way,
      lines.highway,
      lines.service,
      points.highway AS turning_type,
      COALESCE(layer,0) AS layernotnull,
      0 AS z_order -- TODO: handle this somehow?
    FROM planet_osm_point points
      JOIN lines ON ST_DWithin(points.way, lines.way, 0.1)
    WHERE points.way && :'bbox'
      AND points.highway IN ('turning_circle', 'turning_loop')
      AND lines.highway IN ('tertiary', 'unclassified', 'residential', 'living_street', 'service', 'track')
      ORDER BY points.osm_id, -- for DISTINCT ON
        CASE lines.highway -- Use the most important type for cases where the TC is at an intersection. TODO: Should lines.layer be in here?
          WHEN 'tertiary' THEN 6
          WHEN 'unclassified' THEN 5
          WHEN 'residential' THEN 4
          WHEN 'living_street' THEN 3
          WHEN 'service' THEN 2
          WHEN 'track' THEN 1
        END DESC,
        service
),
areas AS (
  SELECT
      osm_id, -- for dev debugging
      way,
      highway,
      railway,
      aeroway,
      COALESCE(layer,0) AS layernotnull,
      0 AS z_order, -- TODO: handle this somehow?
      way_area
    FROM planet_osm_polygon
    WHERE way && :'bbox'
      AND (highway IN ('residential', 'unclassified', 'pedestrian', 'service', 'footway', 'living_street', 'track', 'path', 'platform', 'services')
      OR railway IN ('platform')
      OR aeroway IN ('runway', 'taxiway', 'helipad'))
)
SELECT
    * 
  FROM (
  SELECT
      osm_id, -- for dev debugging
      way,
      highway,
      railway,
      link,
      service,
      construction,
      tunnel,
      bridge,
      access,
      paved,
      layernotnull,
      z_order
    FROM lines
  UNION ALL
  SELECT
      osm_id, -- for dev debugging
      way,
      highway,
      NULL AS railway,
      NULL AS link,
      NULL AS service,
      NULL AS construction,
      NULL AS tunnel,
      NULL AS bridge,
      NULL AS access,
      NULL AS paved,
      layernotnull,
      z_order
    FROM tc_points
  UNION ALL
  SELECT
      osm_id, -- for dev debugging
      way,
      highway,
      CASE WHEN railway = 'platform' THEN railway ELSE aeroway END AS railway, -- pretend the aeroway tags are railway ones
      NULL AS link,
      NULL AS service,
      NULL AS construction,
      NULL AS tunnel,
      NULL AS bridge,
      NULL AS access,
      NULL AS paved,
      layernotnull,
      z_order
    FROM areas
  ) AS t
  ORDER BY
    layernotnull,
    z_order,
    service,
    CASE access WHEN 'no' THEN 0 WHEN 'destination' THEN 1 ELSE 2 END,
    paved DESC
) AS _;

:'bbox' and outer select are so I can test before I put it into Mapnik.

I'm looking at avoiding the join for roads and railways. I think this is possible with attachments.

matthijsmelissen commented 7 years ago

As a first step, i'd recommend thinking about the cartography of highway areas. That's the thing currently blocking simplification of the road SQL, not the fact that we don't know how to write simpler SQL wueries.

pnorman commented 7 years ago

As a first step, i'd recommend thinking about the cartography of highway areas. That's the thing currently blocking simplification of the road SQL, not the fact that we don't know how to write simpler SQL wueries.

The roads fill/casing layers have nothing between them and the highway area fill/casing layers, so we shouldn't need to change any cartography there.

What I've been thinking about more is if there's any way to combine tunnels/normal/bridges, because there are intermediate layers there.

matthijsmelissen commented 7 years ago

The roads fill/casing layers have nothing between them and the highway area fill/casing layers, so we shouldn't need to change any cartography there.

Yes, but simplifying the rendering of areas (like the Humanitarian style does) might allow us to entirely remove the areas part from the road query.

What I've been thinking about more is if there's any way to combine tunnels/normal/bridges, because there are intermediate layers there.

We could add aerialways/waterways to the above query? :) But probably not the direction we want to go.

pnorman commented 7 years ago

Yes, but simplifying the rendering of areas (like the Humanitarian style does) might allow us to entirely remove the areas part from the road query.

How would we do that? The two options I see are to treat it like landuse or to not render it at all. I can see the HOT style does paths on top of fills which means they could be treating it like landuse.

image

matthijsmelissen commented 7 years ago

I can see the HOT style does paths on top of fills which means they could be treating it like landuse.

Yes, probably (or just on top of landuse). Mapbox Streets does the same.

pnorman commented 7 years ago

Mapbox Streets does the same.

I thought MBS included road areas in their roads layer.

If we had it on top of or part of the landcover layer, we could interleave its ordering with other landcover which might bring advantages to some of the stuff in pedestrian square problems. Of course it would mean we'd then have casing problems where roads come up to squares.

pnorman commented 7 years ago

We could add aerialways/waterways to the above query? :) But probably not the direction we want to go.

I'm fine moving aerialways after roads. I'm not sure about waterway bridges as they're not something we have. Got any examples of a road above a waterway bridge? I'm curious how other styles handle it.

matthijsmelissen commented 7 years ago

If we had it on top of or part of the landcover layer, we could interleave its ordering with other landcover which might bring advantages to some of the stuff in pedestrian square problems.

Yes, this is related to #529 (to which I'm still not sure what would be desired behaviour).

I'm not sure about waterway bridges as they're not something we have.

We have quite a few of them. However, in the Netherlands usually the road seems to be drawn as tunnel rather than the waterway as bridge: map / picture. This is probably done because we don't support rendering waterway areas as bridges. Actually in the this case I'm not even sure if I would consider this a highway tunnel or a waterway bridge.

Another one that is drawn as waterway bridge.

And yet another one, image.

matthijsmelissen commented 7 years ago

I suppose you meant roads below waterway bridges, or did you actually mean roads above waterway bridges? My bet is you won't find the latter, as the entire point of waterway bridges, at least in the Netherlands, is to allow tall ships to pass through unhindered.

matthijsmelissen commented 7 years ago

Ok, never say never: here a canal near Birmingham crossing the River Tame, with a motorway bridge passing above: https://www.openstreetmap.org/way/115858978

pnorman commented 7 years ago

I suppose you meant roads below waterway bridges, or did you actually mean roads above waterway bridges?

Nope, I meant roads above waterway bridges. I'd expect roads below to be the common case.

If we move the waterway bridge layer above the roads layers then we'd be able to merge fill and tunnels, and get the common case right. We'd get the road over waterway bridge case wrong

matthijsmelissen commented 7 years ago

Ok, then the Birmingham example will do.

pnorman commented 7 years ago

Looking at it and three bridges I suspect we're currently getting waterway bridges over road bridges wrong. Anyways, the next step is to put my head down and write some MSS

matthijsmelissen commented 7 years ago

Looking at it and three bridges I suspect we're currently getting waterway bridges over road bridges wrong.

Correct.

pnorman commented 7 years ago

After work I did a bunch more CartoCSS, and now have the skeleton of something that works: https://github.com/pnorman/openstreetmap-carto/tree/roads_rewrite/master

The general goal is to keep all the code for a road type reasonably close together

sommerluk commented 7 years ago
+                CASE lines.highway -- Use the most important type for cases where the TC is at an intersection. TODO: Should lines.layer be in here?
+                  WHEN 'tertiary' THEN 6
+                  WHEN 'unclassified' THEN 5
+                  WHEN 'residential' THEN 4
+                  WHEN 'living_street' THEN 3
+                  WHEN 'service' THEN 2
+                  WHEN 'track' THEN 1
+                END DESC,

Is it possible to use simply z_order from highway instead this?

pnorman commented 7 years ago

Is it possible to use simply z_order from highway instead this?

Yep, good catch.

Once I do rail styling I'll open a PR so we can do a more detailed review. My main concern at this point is the general approach, which consists of

matthijsmelissen commented 5 years ago

I think we should have all areas beneath all roads (in general, area features should render below line features). For example, a tunnel should render on top of an area (oitherwise the tunnel would be invisible). Therefore, having two layers would suffice: one layer for area features, and one layer for road features. There is no need for merging the line and area layers.

I agree with the mentioned issues, but the solution proposed in this ticket is not the way forward.

matthijsmelissen commented 5 years ago

Re-reading this, I see this ticket is not about merhing lines and areas together, but merging all lines and all areas. That's still useful so I'm re-opening this.