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

Use flex backend #4431

Closed
wants to merge 67 commits into from
Closed

Use flex backend #4431

wants to merge 67 commits into from

Conversation

pnorman
Copy link
Collaborator

@pnorman pnorman commented Jun 26, 2021

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_route contains route relation information but no geometries. This allows shields from route relations. planet_osm_admin contains 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

pnorman and others added 12 commits April 26, 2020 17:11
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.
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.
@pnorman pnorman added the code label Jun 26, 2021
@pnorman pnorman requested a review from imagico June 26, 2021 16:51
@pnorman
Copy link
Collaborator Author

pnorman commented Jun 26, 2021

An example of the route table is

   osm_id │ member_id │ member_position │                                          tags                                           │  route  │   ref  │ network
──────────┼───────────┼─────────────────┼─────────────────────────────────────────────────────────────────────────────────────────┼─────────┼────────┼─────────
    19528 │ 470894739 │               1 │ "name"=>"2 Oak Bay/James Bay", "operator"=>"BC Transit"                                 │ bus     │ 2      │ ¤
    19528 │  25734178 │               2 │ "name"=>"2 Oak Bay/James Bay", "operator"=>"BC Transit"                                 │ bus     │ 2      │ ¤
    19528 │ 738693230 │               3 │ "name"=>"2 Oak Bay/James Bay", "operator"=>"BC Transit"                                 │ bus     │ 2      │ ¤
    19528 │ 738693231 │               4 │ "name"=>"2 Oak Bay/James Bay", "operator"=>"BC Transit"                                 │ bus     │ 2      │ ¤
 11247580 │ 448047306 │               1 │ "name"=>"Highway 15", "network:type"=>"rcn", "cycle_network"=>"CA:BC"                   │ bicycle │ Hwy 15 │ rcn
 11247580 │ 469438062 │               2 │ "name"=>"Highway 15", "network:type"=>"rcn", "cycle_network"=>"CA:BC"                   │ bicycle │ Hwy 15 │ rcn
 11247580 │ 469438056 │               3 │ "name"=>"Highway 15", "network:type"=>"rcn", "cycle_network"=>"CA:BC"                   │ bicycle │ Hwy 15 │ rcn
 11247580 │ 469438059 │               4 │ "name"=>"Highway 15", "network:type"=>"rcn", "cycle_network"=>"CA:BC"                   │ bicycle │ Hwy 15 │ rcn
 11247580 │ 448048483 │               5 │ "name"=>"Highway 15", "network:type"=>"rcn", "cycle_network"=>"CA:BC"                   │ bicycle │ Hwy 15 │ rcn
  5385983 │  51249294 │               1 │ "name"=>"Haines Highway", "description"=>"https://en.wikipedia.org/wiki/Haines_Highway" │ road    │ 3      │ CA:YT
  5385983 │  51249292 │               2 │ "name"=>"Haines Highway", "description"=>"https://en.wikipedia.org/wiki/Haines_Highway" │ road    │ 3      │ CA:YT
  5385983 │  51249293 │               3 │ "name"=>"Haines Highway", "description"=>"https://en.wikipedia.org/wiki/Haines_Highway" │ road    │ 3      │ CA:YT
  5385983 │ 207975649 │               4 │ "name"=>"Haines Highway", "description"=>"https://en.wikipedia.org/wiki/Haines_Highway" │ road    │ 3      │ CA:YT
  5385983 │ 207975648 │               5 │ "name"=>"Haines Highway", "description"=>"https://en.wikipedia.org/wiki/Haines_Highway" │ road    │ 3      │ CA:YT

The admin table is harder to show, but an example is
image

The key is that there are no overlapping linestrings.

@mboeringa

This comment was marked as outdated.

@mboeringa

This comment was marked as outdated.

@pnorman

This comment was marked as outdated.

This ifxes a bug where admin level 10 would be treated as more
important than admin level 4.
@mboeringa

This comment was marked as outdated.

@pnorman

This comment was marked as outdated.

@mboeringa

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.
@imagico
Copy link
Collaborator

imagico commented Jun 29, 2021

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:

  • it potentially clashes with out goal of adaptability - which IMO includes the mandate to try to design our database schema in a sufficiently generic way that it can be used by other styles and style variants as well in most cases.
  • we might this way roll back parts of the significant improvement the introduction of hstore has brought to the flexibility in style design by not requiring a database re-import every time a new key is getting used. Any changes to the database schema will require a database re-import and if the table layouts are designed and (over)optimized very specifically to the styling decisions of the moment regarding the features those tables are used for, that will put a fairly massive hurdle on revising those styling decisions in the future.

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:

  • is the additional table actually needed and useful for what it is introduced for? I am not sure if that is the case for the planet_osm_admin table. Without geometry processing (which is not something osm2pgsql currently offers on the flex backend i think) i don't think we can use transparency in administrative boundary rendering without the current comp-op technique.
  • is the design of the additional table as generic as it should be not to prejudice OSM data interpretation in a way that significantly limits (without necessity) our styling options in the future or those of other styles/style variations using the same database layout. Any such limitations of genericity should IMO be supported by very good reasons to avoid shortsightedness in design limiting us unnecessarily in the future.

Long story short:: I would like to see a discussion of these points regarding tables (in particular geometry tables) to be newly introduced.

@pnorman
Copy link
Collaborator Author

pnorman commented Jun 29, 2021

  • is the additional table actually needed and useful for what it is introduced for? I am not sure if that is the case for the planet_osm_admin table. Without geometry processing (which is not something osm2pgsql currently offers on the flex backend i think) i don't think we can use transparency in administrative boundary rendering without the current comp-op technique.

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.

  • is the design of the additional table as generic as it should be not to prejudice OSM data interpretation in a way that significantly limits (without necessity) our styling options in the future or those of other styles/style variations using the same database layout

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.
@imagico
Copy link
Collaborator

imagico commented Jun 29, 2021

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.

I doubt that this will work at triple junctions (like here) but i look forward to seeing what you come up with.

It's as generic as our existing z_order implementation, and more generic than the roads table.

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
Copy link
Collaborator Author

pnorman commented Mar 7, 2023

Just for completeness, I guess this is referring to

The documentation is the former - the latter are examples. Googling for "osm2pgsql flex example" turns up a few other examples. The flex documentation is complete enough I'm not having to look through the osm2pgsql source to see how something works, unlike some of the edge cases with the pgsql backend.

@mboeringa
Copy link

mboeringa commented Mar 7, 2023

I agree with @pnorman that Jochen and Sarah did a great job documenting osm2pgsql on the new support website, and not only flex output, even pgsql output documentation was rewritten, or at least included there, as well.

I also think Paul developed a nice, and well setup, LUA flex style for 'openstreetmap-carto'. Even with no prior LUA knowledge, I was able to make adjustments, although admittedly it took a bit of time to familiarize myself with his work and LUA syntax in general.

@imagico, as to a more "flexible" 'flex' output that doesn't require deep knowledge of LUA or tag transforms and would be suitable for cartographers without programming skills, I actually developed a variant of Paul's work on the LUA flex style file of openstreetmap-carto, that features a top section with basic configuration options that just require simple 'boolean' or short 'string' settings to adjust the configuration, and thus ultimately also determine output database schema. I documented the basic function of each setting in the comments.

It allows you to easily set things like:

  • Output projection (3837/4326 and in fact any other projection supported by osm2pgsql 1.8.0+ via PROJ)
  • Switch between hstore-only, or dedicated columns + hstore
  • Switch between jsonb or hstore
  • Set type of unique id column ('SERIAL' / 'BIGSERIAL' / 'IDENTITY_INT' / 'IDENTITY_BIGINT') on each table, and whether you actually want such extra column or not, it is optional.
  • Set database schema ('public' or some user schema, note that PG15 more or less requires dedicated schema).
  • Set PostgreSQL tablespaces for defined tables
  • Set prefixes for table names
  • Include or exclude some tables (e.g. 'planet_osm_roads' versus the new 'planet_osm_transport_line').

If you are truly serious about wanting to support:

"map designers with a predominantly cartographic/artistic background a suitable environment to make meaningful changes to sustainably fulfill our mission in the long term"

then you should definitely have a look. I haven't yet made it officially public through a dedicated Github repository, but posted copies as ZIP files in the issue tracker of osm2pgsql at several occasions when requested by the maintainers.

The most recent version, I have now attached here:

openstreetmap-carto.zip

Note that although in its default setup, this is very close to Paul's work, there are a couple of important (configurable) differences:

  • I added an extra 'waterway' relations table, storing OSM's waterway relations. This is not present in Paul's work here. Note that this table deliberately only selects waterway relation members with role "main_stream". It is therefor designed for low zoom, small scale display of waterways, especially in combination with generalization. Leaving out anything but "main_stream" members, removes a lot of clutter for low zoom display, but also reveals deficiencies in the current waterway relations, where member roles are not always properly defined.
  • In the default configuration, admin boundary lines that are also tagged natural=coastline, are excluded from the admin boundaries Line geometry table (this is a separate table from Polygon type boundaries stored in 'planet_osm_polygon'). I did this for cartographic purposes, as I don't like to see admin boundaries coincide with coastlines. This is again not present in Paul's original work. And also this setting is configurable, see the style file.

@SomeoneElseOSM
Copy link
Contributor

(this is more a comment about me than anything else, but) personally, examples > documentation any day of the week :)

I'm sure that the code at https://github.com/gravitystorm/openstreetmap-carto/tree/flex/master will be a great resource.

@pnorman
Copy link
Collaborator Author

pnorman commented Mar 20, 2023

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.

@imagico
Copy link
Collaborator

imagico commented Mar 20, 2023

I have this on my list but i need to find the time to do a thorough review.

@SomeoneElseOSM
Copy link
Contributor

SomeoneElseOSM commented Apr 2, 2023

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?

$ carto -v
1.2.0

$ carto -a 3.1.0 project.mml > mapnik.xml
Error: Version 3.1.0 is not supported

$ carto -a 3.1 project.mml > mapnik.xml
Error: Invalid API version. A valid version is e.g. 3.0.0 or 3.0.10

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.

@pnorman
Copy link
Collaborator Author

pnorman commented Apr 18, 2023

One question for now though - how do I find out the "carto -a" value that I am supposed to use?

Nothing has changed here - Mapnik release engineering is a mess across multiple repos which may not agree. We specify 3.0.22 in CI but I don't think it matters if you specify it at all.

@pnorman pnorman requested review from jeisenbe and mboeringa and removed request for mboeringa May 2, 2023 06:10
Copy link
Contributor

@SomeoneElseOSM SomeoneElseOSM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.

Copy link
Collaborator

@imagico imagico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. 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:

  1. Boundary lines with this change would be exclusively rendered from type=boundary relations - which matches mapper consensus so it is good. However, the country/state labels (sourced from planet_osm_polygon) are still rendered also from type=multipolygon relations - which would be inconsistent and confusing.

  2. 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:
this branch

Master:
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).

  1. The ST_LineMerge() also - like the current rendering from planet_osm_roads (which contains linestrings for the admin boundaries) causes line end artefacts in admin boundaries forming simple closed rings - top left here:

this branch

  1. Triple junctions with angles > 180 degrees result in similar artefacts (and in contrast to the previous point this is new with this change):

this branch

  1. 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):

this branch

  1. 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 branch

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],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not work - the admin-text layer is sourced from planet_osm_polygon where admin_level is a text column.

openstreetmap-carto.lua Outdated Show resolved Hide resolved
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)
@pnorman
Copy link
Collaborator Author

pnorman commented Aug 31, 2023

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.

@mboeringa
Copy link

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.

@imagico
Copy link
Collaborator

imagico commented Sep 2, 2023

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.

@imagico imagico mentioned this pull request Nov 10, 2023
5 tasks
@pnorman
Copy link
Collaborator Author

pnorman commented Jan 30, 2024

I'll leave it open for awhile in case someone wants to take the process over. If so, please indicate here

It's been a few months and no one has indicated they want to take this on.

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

Successfully merging this pull request may close these issues.

Move to osm2pgsql flex backend
8 participants