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

Limit dual insertion in Line and Polygon table to "boundary=administrative" #4549

Closed
wants to merge 1 commit into from

Conversation

mboeringa
Copy link

@mboeringa mboeringa commented May 14, 2022

Hi Paul, this is a proposal for a minor change to the relation function of the osm2pgsql flex version of the style.

Currently, if a relation is tagged as either "type=boundary" or "type=multipolygon" with a boundary tag, then the feature will end up as both a geometry in the Line as well as Polygon table.

While this is the desired behavior for administrative boundary relations to allow sophisticated Line symbology and de-duplication of administrative boundary lines of different "admin_level", other "boundary" relation types like "type=boundary and boundary=protected_area", should likely only end up in the Polygon table, not the Line table, as they usually do not represent contiguous topological connected structures, but are usually "stand-alone".

This change now limits dual insertion in Line and Polygon table to "boundary=administrative" tagged features only.

…rative"

Hi Paul, this is a proposal for a minor change to the relation function. 

Currently, if a relation is tagged as either "type=boundary" or "type=multipolygon" with a boundary tag, then the feature will end up as both a geometry in the Line as well as Polygon table. 

While this is the desired behavior for administrative boundary relations to allow sophisticated Line symbology and de-duplication of administrative boundary lines of different "admin_level", other "boundary" relation types like "type=boundary and boundary=protected_area", should likely only end up in the Polygon table, not the Line table, as they usually do not represent contiguous topological connected structures, but are usually "stand-alone".

This change now limits dual insertion in Line and Polygon table to "boundary=administrative" tagged features only.
Copy link
Collaborator

@pnorman pnorman left a comment

Choose a reason for hiding this comment

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

This needs to be accompanied by changes to the tests.

I'm not sure this is the right change to make, as admin geometries are in the planet_osm_admin table, and I don't see a need for any boundary or multipolygon relations to go into the line or roads table anymore.

@mboeringa
Copy link
Author

mboeringa commented Jun 6, 2022

I'm not sure this is the right change to make, as admin geometries are in the planet_osm_admin table, and I don't see a need for any boundary or multipolygon relations to go into the line or roads table anymore.

@pnorman ,

Yes, I guess you may be right on this one... I hadn't yet really given that enough thought. So simply deleting the code lines involving the add_line and add_roads functions in the process_relation function would be the logical solution to this?

@pnorman
Copy link
Collaborator

pnorman commented Jun 7, 2022

It can be simplified further because it's then the same as the code in the next elseif block. And many tests will no longer apply.

@mboeringa
Copy link
Author

mboeringa commented Jun 7, 2022

On the other hand: wasn't this about a kind of "soft" backwards compatibility with existing openstreetmap-carto derived styles, to allow a more gradual transition to the new schema including the new transport and admin tables?

Did the old 'pgsql' output of osm2pgsql insert line versions of admin boundaries in the "planet_osm_roads" and "planet_osm_line" table?

If so, it might be better to maintain this, unless you want to deliberately break the backwards compatibility and remove the "planet_osm_roads" table entirely from the style definition, which would force users to switch over to the new "planet_osm_transport_line" table.

@pnorman
Copy link
Collaborator

pnorman commented Jun 20, 2022

On the other hand: wasn't this about a kind of "soft" backwards compatibility with existing openstreetmap-carto derived styles, to allow a more gradual transition to the new schema including the new transport and admin tables?

Preserving backwards compatibility isn't a hard requirement.

Did the old 'pgsql' output of osm2pgsql insert line versions of admin boundaries in the "planet_osm_roads" and "planet_osm_line" table?

osm2pgsql doesn't define that, the transforms you give it do. For the current OpenStreetMap Carto ones, it has the same logic as flex does right now.

@mboeringa
Copy link
Author

Preserving backwards compatibility isn't a hard requirement.

If that is the case, then I'd suggest ripping out the entire 'planet_osm_roads' table definition from the flex style. With the new 'planet_osm_transport_line' table, it no longer really has a function.

Other option is to make the creation of this specific table configurable, as I did in my variant of your style where simply switching a boolean determines which tables are created. But that only complicates the code, so ripping it out, is probably the better option.

But this is getting slightly off topic regarding the specific issue of all types of boundary relations ending up in both the 'planet_osm_line' and 'planet_osm_polygon' tables, that this pull request attempted to address.

@pnorman
Copy link
Collaborator

pnorman commented Jun 20, 2022

But this is getting slightly off topic regarding the specific issue of all types of boundary relations ending up in both the 'planet_osm_line' and 'planet_osm_polygon' tables, that this pull request attempted to address.

This PR would break backwards compatibility.

@mboeringa
Copy link
Author

mboeringa commented Jun 20, 2022

This PR would break backwards compatibility.

Yes, I understand that now based on your reference to the old 'pgsql' LUA style, thanks for referencing it.

That said, there is the interesting thing that I cannot remember seeing some of the same issues with duplicated labels I experienced now (based on Line and Polygon geometries), when I still used the old 'pgsql' style and older versions of osm2pgsql...

But maybe, or likely, I overlooked it, despite having extensively reviewed maps created with the 'old' and 'new' osm2pgsql and styles.

@mboeringa mboeringa closed this Jun 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants