Move rendering order road types from osm2pgsql to our SQL queries

[matthijsmelissen]

The render order of road types (defining which road types are rendered on top of which other roads) is currently hard-coded in osm2pgsl (in style.lua).

In any case, this is problematic:

• We need to have an extra attachment with separate rules for link roads. If we had full control over the render order ourselves, we could get rid of this additional attachment, and of most separate rules for link roads (for all road types but motorway, link roads are currently rendered the same as non-link roads).
• The rendering order is underspecified. For example, highway=pedestrian is rendered sometimes below and sometime above highway=living_street. This leads to inconsistent behaviour.

It might also be possible to specify this in the osm2pgsql style file. But that's still not an ideal solution, since any changes in the rendering order are hard to implement, because they require a database change.

Therefore, I propose that we specify the rendering order within our Carto code. The most logical place to do that is the SQL query.

The largest problem with this is the performance penalty. On a small database (Luxembourg), I did not measure any performance degradation. This might be because we are currently sorting anyway (by z_order). However, it would be good to test this on a larger example as well.

[matthijsmelissen]

@pnorman Would you be able to do a performance test of https://github.com/math1985/openstreetmap-carto/tree/road-order compared to https://github.com/gravitystorm/openstreetmap-carto/tree/7f5235b9b337aca551972b9a7188c8597c4f6dc6 ?
(This is a quick proof of concept and still has some details to be worked out)

[matthijsmelissen]

@gravitystorm Do you agree with controlling render order of roads from our SQL queries, provided the performance does not degrade significantly?

[matthijsmelissen]

See also:
https://trac.openstreetmap.org/ticket/3649 Race track service roads render above the race track

[pnorman]

@pnorman Would you be able to do a performance test of https://github.com/math1985/openstreetmap-carto/tree/road-order compared to https://github.com/gravitystorm/openstreetmap-carto/tree/7f5235b9b337aca551972b9a7188c8597c4f6dc6 ?
(This is a quick proof of concept and still has some details to be worked out)

Yes - but potentially not until post-SOTM hack day.

For reference, the compare is matthijsmelissen/openstreetmap-carto@gravitystorm:7f5235b9b337aca551972b9a7188c8597c4f6dc6...road-order

[gravitystorm]

I'm not really in favour of this, but I'm open to persuasion. This takes what are already very complex and hard to work with SQL queries, and adds on even more complexity.

I would rather see the problem fixed upstream in osm2pgsql, even though it will take a lot longer to work things through. If people would prefer (or if we believe our choices are style-specific rather than generically useful), we can create our own version of style.lua and control it in this repository.

[matthijsmelissen]

I think separation of concerns is more important than the code complexity argument (although I share @gravitystorm's worries on that aspect). So the question is: is render order something osm2pgsql should be concerned with? My intuition - but others might disagree - is that osm2pgsql is concerned with representing geospatial data in easy-readable form, and that the resulting SQL database should be independent of rendering decisions. For example, I believe that layer tags belong in the database, because that's a piece of geospatial data. However, rendering order (and the way we represent that) is not geospatial data, and thus not osm2pgsql's concern.

In tilemill 2, we should at least be able to use line-breaks in the queries.

[matthijsmelissen]

I raised the issue here: osm2pgsql-dev/osm2pgsql#132

[pnorman]

I'm not really in favour of this, but I'm open to persuasion. This takes what are already very complex and hard to work with SQL queries, and adds on even more complexity.

I don't like the queries either.

I'll have to look at the values in a JSON editor or unescape it myself rather than look at the text file if I'm going to provide a more detailed commentary, .mml is a horrible format to read the queries in with a text editor.

[matthijsmelissen]

Please don't study the queries in detail yet, this is just a proof of concept to study the performance. I didn't spend any time in writing the queries in a readable way.

[matthijsmelissen]

There is not only code that might be relevant in style.lua, but also in tagtransform.c (from line 16). Is there anyone here around who understands the osm2pgsql code and can help me explain which of both parts of the code is relevant (if not both)? I find the osm2pgsql code rather hard to read.

[pnorman]

Please don't study the queries in detail yet, this is just a proof of concept to study the performance. I didn't spend any time in writing the queries in a readable way.

Well, I'm used to reading a lot less clear queries than anything you'll write. The issue is the queries are a large part of the performance, so I have to study them.

There is not only code that might be relevant in style.lua

We're not using lua tag transforms, so nothing in there is used.

[matthijsmelissen]

Are you saying we're not using style.lua, or not using tagtransform.c? Code for layering seems to be duplicated in both files, and I don't see it anywhere else.

[pnorman]

Are you saying we're not using style.lua, or not using tagtransform.c? Code for layering seems to be duplicated in both files, and I don't see it anywhere else.

We're not using lua tag transforms, so style.lua is never used. See README_lua.md

[matthijsmelissen]

Ok, so then this would be the relevant code:
https://github.com/openstreetmap/osm2pgsql/blob/9e7874c5a3c306ff56f6b2f8518640b4d7ab24a8/tagtransform.c#L20

Unfortunately, there is no clear separation between code and rules, and having to keep an own version of tagtransform.c in the openstreetmap-carto would be rather annoying.

Unfortunately, I have not received any feedback on the issue i opened on the osm2pgsql Github.

[matthijsmelissen]

@lonvia has commented in the osm2pgsql github repo. She is against changing the z-order in the main code as it would break break backwards compatibility on other installations. She suggests that we move away from tagtransform.c and supply our own style.lua instead. The lua support is still in beta, though. @pnorman mentions that using style.lua comes with a performance penalty of at least 10% on import.

This leaves us basically with two options:

• Create our own style.lua sheet and define the rendering order in there
• Define the order in the SQL statements

I would still be in favour of the second option, because it's much easier to maintain order changes (they wouldn't require a full new import of the database). I agree this comes with a penalty in the form of harder to read SQL queries.

Alternatively, we could use the second option for now, so we get some experience with deciding what the best order is, and then move the order from SQL to style.lua at a later point when we do a new SQL import.

There are quite a few issues that depend on this:

https://trac.openstreetmap.org/ticket/2024
https://trac.openstreetmap.org/ticket/2992
https://trac.openstreetmap.org/ticket/3649
#163
#168
#266

[pnorman]

The performance penalty will be substantially worse with SQL than Lua, because the lua is a one-time cost on import, while SQL is every rendering

[matthijsmelissen]

Indeed, sorry for making that not clearer in my post.

[matthijsmelissen]

Changing the rendering order with osm2pgsql is a complex operation, because it requires a database reload. We don't want to do frequent database reloads, so if we change the rendering order with osm2pgsql, we must do it right the first time. However, changing the rendering order of roads can also easily have unforeseen consequences, and even with intensive testing, there is a significant risk that these consequences only show up after the changes are rolled out on the main map. Therefore, I would be hesitant to use osm2pgsql for the road order at this phase.

Instead, I would propose the following:

• For the moment, we move handling of the road order to the SQL query.
• After we are happy with the road order, and by the time we do a new database reload, we can consider moving handling of the road order back to osm2pgsql (by means of the lua file).

My tests indicate that moving handling of the road order to the road does not significantly harm performance. Handling of the road order allows us to get rid of a lot of the links overhead in roads.mss, which will significantly improve performance.

Note that this issue is relatively high priority: it allows simplifying roads.mss significantly (because we can get rid of the link road duplication), it significantly improves performance (for the same reason), and 6 other bugs depend on it.

@gravitystorm What do you say?

[matthijsmelissen]

This also solves #644.

[matthijsmelissen]

Solved by #626.