Style sheet roads.mss needs refactoring

[matthijsmelissen]

Motivation

The roads.mss style sheet (and the order of its elements) contains some oddities which cause problems of rendering and maintenance. Therefore, I think it would be a good idea to refactor this file.

Problems of rendering

• Not all aspects of the order of layers seem to have been thought out well:
  • Foot tunnels are drawn above city walls, but under barriers.
  • Buildings are drawn above the casing of roads, but under their fill.
  • Foot tunnels are drawn above the casing of turning circles, but under their fill.
• The layer tag of turning circles and areas is not taken into account.
• Access restrictions overlap with neighbouring roads (Dotted line of private service ways erroneously render inside larger streets #119).
• Casings for roads on higher layers are sometimes hidden by casings for roads on lower layers.
• Footways in tunnels are rendered on top of residential ways in tunnels, but footways out of tunnels are rendered below residential ways out of tunnels.

Problems of maintenance

• There are many duplicate definitions and similar definitions that are far apart in the style sheet, which makes the code difficult to read and maintain. For example, all roads have a separate style for bridges, tunnels, and normal roads, although these styles are usually quite similar.
• The rendering of tracks and the rendering of the other highways are specified at a different place in the code.
• It is difficult to extend the set of objects for which the layer tag works.
• The layers #roads and #minor_roads are confusingly named.

I propose the following action plan to solve these problems (and would be willing to implement it if you guys agree).

Action plan

Step I.

Split off preserved railways and monorails from #roads, and rename #roads, to #roads-low-zoom, #minor-roads-casing to #roads-casing, and #minor-roads-fill to #roads-fill.

Step II.

Re-order layers in a more logical way. In particular, render aerialways directly above roads in layer 0, and render ferry-routes, buildings, area/line barriers, cliffs, landuse-overlay, and castlewalls directly below layer 0.

Step III:

Merge #tracks-notunnel-nobridge into #roads-fill (previously #minor-roads-fill).

Step IV:

Render access restrictions at the same time as #minor-roads-fill by merging .access into both #minor-roads-fill and .bridges.

Step V:

For all eleven layer tags (-5 to 5), create the following four TileMill layers (in this order).

1. directions
2. roads-fill (including access)
3. roads-casing
4. bridge (not necessary for negative layers)

For each of these four layers, create a class that handles rendering them. The class for directions exists already. The class .roads-casing is created in step I (currently #minor-roads-fill), but it will be extended with code to fill highways on bridges and in tunnels based on the presence of bridge/tunnel tags, excluding the black casing of the bridge itself . This will be done by merging the code from .bridges, #tunnels, #tracks-tunnels, and #footbikecycle-tunnels. This brings similar tags for bridges, tunnels, and normal roads together in the code. For .roads-casing, the same as for .roads-fill will be done. The class .bridge will only be used for the black casing of bridges. The layers #tunnels, #tracks-tunnels, #footbikecycle-tunnels, and #bridges-layerX can now retire.

Step VI:

For all eleven layer tags (-5 to 5), add layers for turning-circles, and highway-areas (in this order).

1. directions
2. roads-fill (including access)
3. turning-circle fill
4. highway-area-fill
5. roads-casing
6. turning-circle casing
7. highway-area-casing
8. bridge (not necessary for negative layers)

For each of the four new layers, create a class that handles rendering them. The classes will be based on the existing definitions of #turning-circle-fill, #highway-area-fill, #turning-circle-casing, and #highway-area-casing.

Performance

I am not sure how this plan affects performance. I don't expect major problems, but I'm not an expert on this area. If performance degrades too much, there are a couple of shortcuts possible:

• Instead of the nine layers, we could use nine attributes, if we can write a query that fetches all of the above objects out the database at once. This reduces the number of layers (and SQL queries) necessary, but makes it harder to add new features for which the layer tag should be taken into account.
• If we don't care about the layer of turning_circles and areas, we could display them all at layer 0 (as we currently do), and skip Step VI.

Final Remark

Please let me know whether you think this plan is a good idea, and whether you see any aspects that need improvement.

[dieterdreist]

Long list. Just a remark to the perceived problem: "Buildings are drawn above the casing of roads, but under their fill." I guess this is done on purpose. My own experiments suggest that this drawing order is the best for maps where roads should get a higher focus than buildings, otherwise the roads will almost disappear visually in settings where the buildings are close to eachother.

[matthijsmelissen]

I'm not sure if I understand. Currently the casings of roads are hidden by buildings? Wouldn't that make roads appear rather less prominent?

[dieterdreist]

2013/9/14 math1985 [email protected]

I'm not sure if I understand. Currently the casings of roads are hidden by
buildings? Wouldn't that make roads appear rather less prominent?

thought you wanted to bring the buildings over them...

[matthijsmelissen]

No, my main problem is that buildings are rendered between the casing and fill of roads. I don't care that much about the order, but I do believe that nothing should be rendered between an object's fill and its casing.

Rendering roads (both fill and casing) over buildings is indeed probably the best solution.

[matthijsmelissen]

An additional problem that will be solved by this plan: if two tunnels on different layers cross, the fill of the lower road will (incorrectly) overlap the fill of the higher layer. See for example here. The tunnel of road E411 (blue) crosses here the tunnel of R0 (green), which runs even deeper. The casing of the tunnel of road E411 gets interrupted at the crossing (as the fill of R0 is drawn over it), while one would expect that the casing would continue.

[gravitystorm]

If I understand you correctly, then in stages 5 and 6 you're proposing having first 44 and then 88 layers for roads in project.mml? That's not a good approach. Duplicating each layer 11 times with just a and layer = '-3' change in the SQL will make maintenance a nightmare.

I'd rather explore reducing the amount of duplication, rather than increasing it. Mapnik has a "group_by" attribute for layers now, intended to address this problem, which should be explored further. I use it on other projects to achieve roads ordering, but if that doesn't work for us we can add more features to mapnik.

I'd encourage you to break this down into smaller parts. You note the buildings are inserted above casing and below fill. Solve that separately. As you rightly point out, the layer names are confusing (due to hysterical raisins) and need changing. Tackle that separately.

There's a couple of points that I'm not sure I understand and some I don't agree with. For example, rendering buildings below layer 0 means that you won't be able to see road tunnels in urban areas. Aerialways (e.g. layer=2) might pass over the top of a road bridge (layer=1) over a river (layer=0), so they probably shouldn't be drawn immediately after layer=0. And preserved railways and monorails can go over and under roads on bridges (as per all different railways), so I don't see why they should be split off into their own layers.

I'd encourage refactoring of the file, but try to break down the changes into small, easy-to-examine changes wherever possible!

[matthijsmelissen]

Thanks for your comments. I totally agree that having 44/88 layers is not a beautiful solution, but I didn't see a way to avoid it. It is the same reason we currently have 15 (IIRC) layers for bridges. I agree that other solutions are preferable, and I will have a look at the "group_by" attribute.

I will try to split up things in different pull requests as much as possible. Of course parallelization is not always possible, as some changes depend on earlier changes.

rendering buildings below layer 0 means that you won't be able to see road tunnels in urban areas

No, because buildings are rendered transparent. This is the current situation.

Aerialways (e.g. layer=2) might pass over the top of a road bridge (layer=1) over a river (layer=0), so they probably shouldn't be drawn immediately after layer=0.

The layer of aerialways is currently ignored, so this is a problem with the current situation, not with my proposal.

Preserved railways and monorails can go over and under roads on bridges (as per all different railways), so I don't see why they should be split off into their own layers.

They are currently in the layer of low-zoom roads. They are better in a separate layer than in the current layer, but I could also add them to the roads-high-zoom layer.

[matthijsmelissen]

I think group-by will indeed be an outcome for the duplication of layers.

@gravitystorm, you mentioned in #3 that group-by "is supported (but not documented) in CartoCSS by using the "properties" of a layer definition". i'm not sure what you mean by that, could you expand on that?

[matthijsmelissen]

I have submitted pull requests for step 1 to 4. Step 5 and 6 need some rethinking, for two reasons:

• We don't want duplication of queries for every layer (we'll try to solve this with group_by)
• The original plan would have as a consequence that when a road in one layer continues as a road in a higher layer, the casing of the higher layer would be visible at the place where the layers change.

[gravitystorm]

I've merged everything up to and including the tracks refactoring. Great stuff.

I'll review the next PRs soon. However I know from previous work that there's a performance implication when cycling through many unmatched features. For example, if you cycle through all the roads to draw them, and then a second time to draw oneway arrows, that's more feature/filter testing than two separate layers, where the oneway layer only contains a small number of features.

So my plan is to review the roads-related PRs, accept them onto a branch, and then when we have the Carto the way we would like it (from a developer point of view) we can then performance test it to make sure there's not too much of a hit. If the performance is reasonable, then I'll merge the branch into master.

[matthijsmelissen]

Hi Andy,

Using a separate branch is fine with me.

Do you have some kind of methodology to measure performance?

How do we avoid too many conflicts when merging back? Do you want me to
commit all roads-related changed
in the roads branch, even if they are not directly related to the other
changes?

-- Matthijs

On 21 September 2013 09:47, Andy Allan [email protected] wrote:

I've merged everything up to and including the tracks refactoring. Great
stuff.

I'll review the next PRs soon. However I know from previous work that
there's a performance implication when cycling through many unmatched
features. For example, if you cycle through all the roads to draw them, and
then a second time to draw oneway arrows, that's more feature/filter
testing than two separate layers, where the oneway layer only contains a
small number of features.

So my plan is to review the roads-related PRs, accept them onto a branch,
and then when we have the Carto the way we would like it (from a developer
point of view) we can then performance test it to make sure there's not too
much of a hit. If the performance is reasonable, then I'll merge the branch
into master.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/162#issuecomment-24857880
.

[gravitystorm]

@pnorman has done performance measurements before, I'll discuss approaches with him and see what we can do.

As for the merging, do whatever suits you best and that you find easiest. That's the most important thing. Wherever possible, it helps me if you base your pull requests on the current master branch, and I'll take care of any merge conflicts - but where you need to base more work on an early unmerged pull request, then keep doing what you've been doing so far.

[matthijsmelissen]

Here is an updated plan of steps V and VI.

Step Va (done):

Merge all bridge-layers into one layer. This will be done by first merging the layers for roads and directions for each layer (1 to 5), and then merging the layers themselves using the group-by function of Mapnik.

Step Vb (done):

Merge the layers #tracks-tunnels and #footbikecycle-tunnels into #tunnels.

Step Vc (done):

Unify the definitions for roads, tunnels and bridges by giving the three layers that render them the class roads, and let the class road handle rendering of tunnels and bridges based on the road class.

Step Vd (partially done):

Reconsider rendering order based on layers of normal roads and tunnels (details to be worked out).

Step VI:

Make sure that the layer tag of turning circles and highway areas is taken into account.

[matthijsmelissen]

@pnorman Would you be able to test the performance of https://github.com/math1985/openstreetmap-carto/tree/roadsv5c ? I haven't done anything in particular to speed up the performance, so I wouldn't be surprised if performance goes down.

[matthijsmelissen]

Performance deteriorates significantly in my roadsv5c branch compared to the current master. I can't test reliably (I have only Luxembourg loaded in psql, and I test from a laptop, on which also other processes run), but it seems that the runtime has gone up by about 30%. Even worse, the size of osm.xml has increased by about 1000%.

The cause for that seems to be .directions::fill and .access::fill layers. The problem seems to be that rendering of oneway symbols, access symbols, and highways is done in the same attribute, and therefore there is a rule in the XML for every combination of oneway, access, and highway. Therefore, the total number of rules is the total number of oneway types times the total number of access types times the total number of road types.

However, this happens in the current version as well (but on a smaller scale), so we should try to find out why it suddenly is so much of a problem. I will do some more tests and see what I can do. If not, I might ask for help later.

[pnorman]

Even worse, the size of osm.xml has increased by about 1000%.

This will generally indicate worse performance

The SQL changes are themselves probably a big performance gain as my understanding is this brings more of the roads stuff into one query.

[matthijsmelissen]

| This will generally indicate worse performance
So much I figured :). I playing with the code now, and I think I might be able to prevent the explosion. I'm trying to understand how the xml is exactly generated now.

[matthijsmelissen]

Steps I, II, III, IV, and V(a)-(c) are done. Step V(d) is done for tunnels - for non-tunnel/bridge roads we don't need to take the layer order into account. Step VI we will skip.Taking into account layer tags of highway areas is not necessary because we render highway areas always below all roads. Taking into account layer tags of turning circles would be nice, but turning circles in tunnels or on bridges are fairly rare, and it complicates the code, so we'd better not implement this.

I will therefore close this issue as partially resolved, partially won't fix.

There might still more things that can be improved in the roads layer, but I will use separate issues for that.