Use flex backend #4431
Use flex backend #4431
Conversation
The flex backend allows the style transformation to configure much more, including features that were not possible with the pgsql backend.
Because one way may have many parent relatiolns, a table is used so at query time the data in the line table can be joined with the route table. Also, this avoids stage 2 osm2pgsql processing.
The Flex backend requires a more recent osm2pgsql than Travis has, so skip them for now.
Tag v5.3.1
The code was splitting based on columns for the wrong table.
This is an osm2pgsql phase 2 table, so we need to keep around the IDs of all ways that are members of admin relations, and then in phase 2, add those ways to the admin table.
|
An example of the route table is The admin table is harder to show, but an example is The key is that there are no overlapping linestrings. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This ifxes a bug where admin level 10 would be treated as more important than admin level 4.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This table contains linear transportation features, which will allow z_order and layer to be removed from the line table.
This is a table for transport features (i.e. those with z_order) which will allow the removal of z_order from the general polygon table
This doesn't yet take advantage of the deduplication done in the Lua transforms, so there's room for improvement, but this makes use of the new table.
|
I have not actually tested this yet but based on a look over the code i have some concerns about the introduction of new tables. For the route relation membership this is not really an issue, this seems to be a natural and generic way to store relation membership of ways for use where needed. My concern is about the other tables - it seems they are going to implement a style specific and subjective interpretation of the data at the import level. This IMO has two big issues:
Now that does not mean i am against introducing new tables - but i think this should be carefully thought about beforehand in particular under the questions:
Long story short:: I would like to see a discussion of these points regarding tables (in particular geometry tables) to be newly introduced. |
No, it works as expected. I'm in the process of converting the admin MSS and I'm able to get rid of all the complex comp-op, white lines, and attachments.
It's as generic as our existing z_order implementation, and more generic than the roads table. |
This allows significant MSS simplification, no longer relying on comp-op and allowing more flexibility in styling.
I doubt that this will work at triple junctions (like here) but i look forward to seeing what you come up with.
The roads table is not the frame of reference here - that has always been a weird construct and is only used for the low zoom levels and for admin boundaries (where it contains geometries generated from multipolygon relations but in the form of linestrings - leading to rendering artefacts at the ends of the rings). |
pnorman
requested review from
imagico and
mboeringa
and removed request for
mboeringa
|
Reviews of this would be appreciated. I believe all work is done, and don't see the need any additional changes that are needed. There's other things I'd like to work on (e.g. route relations) that are blocked needing this. |
|
I have this on my list but i need to find the time to do a thorough review. |
|
First things first - I was able to follow https://github.com/gravitystorm/openstreetmap-carto/blob/flex/master/INSTALL.md and get tiles rendered identically to before (I'm even seeing the same mapnik artifacts in both - a horizontally sliced highway shield at https://www.openstreetmap.org/#map=15/54.0080/-0.9981 ). Also, (not that it's relevant to this style per se) the database contents that I use elsewhere for QA etc. all seems to be still there. The minor table changes are just that; I doubt they will confuse anyone. The new lua file has lots of detail about what it's doing, which is great. The $64,000 test of that is modifying the style in a way that requires a lua change (for e.g. "is a way an object such as a guidepost part of a route relation") - I'll have a more in-depth look at that later. One question for now though - how do I find out the "carto -a" value that I am supposed to use? I'm using https://packages.ubuntu.com/jammy/libmapnik-dev from Ubuntu 22.04 LTS, which I'd expect to be 3.1 or 3.1.0. In the end I ran it without "-a" to no obvious ill-effects. |
Nothing has changed here - Mapnik release engineering is a mess across multiple repos which may not agree. We specify |
pnorman
requested review from
jeisenbe and
mboeringa
and removed request for
mboeringa
There was a problem hiding this comment.
(leaving a review as requested on IRC; see also previous comment #4431 (comment) )
I was able to follow https://github.com/gravitystorm/openstreetmap-carto/blob/flex/master/INSTALL.md and get tiles rendered identically to before (I'm even seeing the same mapnik artifacts in both - a horizontally sliced highway shield at https://www.openstreetmap.org/#map=15/54.0080/-0.9981 ).
Also, (not that it's relevant to this style per se) the database contents that I use elsewhere for QA etc. all seems to be still there. The minor table changes are just that; I doubt they will confuse anyone.
The new lua file has lots of detail about what it's doing, which is great. I've not tried lua modifications yet, but it seems a solid basis to start from.
The only slight caveat was that I got confused by the "carto -a" request in INSTALL, but "just using the latest" worked, as usual.
There was a problem hiding this comment.
I am sorry for the delay in looking at this again, properly reviewing this is fairly elaborate work that is difficult to be done in between and i have had difficulty finding longer periods of time to give this sufficient attention.
For now i have concentrated on the administrative boundary changes and i want to state in advance that i have not tested this on a global level with actual OSM data so far, just in abstract tests.
- There is a bug noted in the inline comment that prevents admin boundary line labels from turning up before z16.
Other issues i observed - some of them were already pointed out in earlier:
-
Boundary lines with this change would be exclusively rendered from
type=boundaryrelations - which matches mapper consensus so it is good. However, the country/state labels (sourced fromplanet_osm_polygon) are still rendered also fromtype=multipolygonrelations - which would be inconsistent and confusing. -
Assembling boundary ways on a tile level with
ST_LineMerge()leads to frequent discontinuities at metatile boundaries in the dashing pattern. While this might not seem overly problematic with our choice of boundary styling this is a potential issues with other line stylings and therefore conflicts with our goal of adaptability and being an exemplar style sheet for OSM based maps. Example:
This branch:
Master:
The same phenomenon would also prelude any line simplification from being used in the style (which - to be fair - is also impossible with rendering boundary lines from the polygons as we do it right now).
- The
ST_LineMerge()also - like the current rendering fromplanet_osm_roads(which contains linestrings for the admin boundaries) causes line end artefacts in admin boundaries forming simple closed rings - top left here:
- Triple junctions with angles > 180 degrees result in similar artefacts (and in contrast to the previous point this is new with this change):
- Triple junctions where boundaries of different admin level meet have overlapping transparencies, creating additional noise and color mixing (example at 2x resolution to make it better visible):
- Something similar happens when two coincident boundaries do not use the same way but are mapped separately (which is not uncommon and AFAIK no consensus exists that this is not allowed - not to mention cases where in reality different administrative levels defined the boundary slightly differently):
This might be considered a positive thing (highlighting unusual cases rather than paving over them) - but i am not sure if this kind of mushiness, without it being clearly visible what the cause is for this, is of benefit.
The last four issues can be addressed by
- building polygons from all closed rings in the
ST_LineMerge()results and rendering those instead of the multilinestring. - using round line caps on the wide lines.
- using layer level opacity and attachments.
Code doing this can be found in
https://github.com/imagico/osm-carto-alternative-colors/tree/flex-admin
That would leave the discontinuities at metatile boundaries - which inevitably result from assembling the boundaries on a metatile level and not on a relation level - as the main issue of this change w.r.t. boundary rendering. On the other hand it allows removing the need for the white line + comp-op: darken technique and the subtle artefacts this causes - which is a substantial benefit.
On the whole i consider this change w.r.t. admin boundaries - if the issues that are easy to address (1,2,4-7) are fixed - a net benefit, even if it does not bring us closer to fixing the core issues of rendering the boundaries at low zoom levels - see discussion in #2172.
Side note: If i am not mistaken, however, it should be possible to use the same approach used here with ST_LineMerge() on the tile level on the relation level (instead of merging all boundary ways intersecting the tile merging all member ways of all boundary relations that have members intersecting the tile). That is probably going to be substantially more expensive (so i am not proposing it here at this time) but it would avoid the metatile boundary inconsistency issues.
So much for the admin boundary part of this change. I intend to look at the rest as well again in more depth when i find the time.
| [zoom >= 13][admin_level = '7'], | ||
| [zoom >= 14][admin_level = '8'], | ||
| [zoom >= 15][admin_level = '9'], | ||
| [admin_level = 1][way_pixels >= 360000], |
There was a problem hiding this comment.
This does not work - the admin-text layer is sourced from planet_osm_polygon where admin_level is a text column.
Moves creation of dictionaries storing admin_level for stage 2 processing of ways from the 'select_relation_members' function to the 'process_relation' function.
Potential fix for the issue raised in #4431 (review)
|
I do not anticipate having the time and energy to work on merging this. I'll leave it open for awhile in case someone wants to take the process over. If so, please indicate here. I've been using this code for awhile for various purposes, so I'm generally happy with it. |
Based on extensive usage of a personal version of this style that closely follows this one, I agree the introduction of the flex Lua style file at least, is likely ready for production usage, and will allow potentially new (styling) options like the admin boundary work in the future, thereby overcoming some of the limitations of the currently used 'pgsql' mode of osm2pgsql, that simply doesn't offer the flexibility of processing of the new 'flex' mode of osm2pgsql. This is especially so in the light of the finishing off of the current good work on osm2pgsql by Jochen and Sarah, and the last fixes they introduced to handle 'flex' mode of osm2pgsql, where most of osm2pgsql's new functions, except maybe the here unused experimental generalization options, appear in their final stages of development. I think this pull request has been lying dead in the waters for to long. I am not a maintainer, so can't help out with getting it merged, but if someone else of the true maintainers group feels the same, I think it wise to start merging this and start further processes to switch to 'flex' mode of osm2pgsql for OSM's Default style. |
|
To document the current state of this PR - there is consensus among the maintainers for moving to use the flex backend. There are still some issues with the admin boundary rendering (explained in #4431 (review)) but possible ways to solve most of these are already identified and shown there. There is unresolved disagreement about if or not to introduce new additional tables for roads (lack of broader input on that matter was an issue here - see #4431 (comment)) - but that part of the change is not tied to the move to the flex backend of course - it could be omitted. That part of the change has also not yet received a thorough review - and there are likely also a few smaller issues to be addressed there - for example it seems that the issue pointed out in my first review (#4431 (review)), that the table planet_osm_transport_polygon is generated but not used, is still present. |
It's been a few months and no one has indicated they want to take this on. |
Fixes #4112
This PR implements the flex backend and is ready for a first review of the changes. So far only new tables are added, but I'd like to get feedback on the lua changes.
Two new tables are added.
planet_osm_routecontains route relation information but no geometries. This allows shields from route relations.planet_osm_admincontains linestrings for ways that are part of admin relations. This enables deduplicating the linestrings on import, rather than relying on tricky overlaps and transparency.I have not yet made any SQL or MML changes