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

Add support for polygons in changefiles (as Ways) #11

Merged
merged 18 commits into from
Jun 15, 2021

Conversation

acannistra
Copy link
Contributor

@acannistra acannistra commented May 10, 2021

This PR enables support for features of Polygon type to be exported from a conflation PostGIS database as OSM Closed Ways. Only support for <create> nodes was added at this time, though modifications of Polygons might work but probably not and hasn't been tested.

Description of Modifications

  • add support forRelation and RelationMember namedtuples to changewriter module
  • add support for detecting Polygon geometry types and creating either Way or Relation objects to represent them depending on the complexity of the polygon (simple polygons: closed Ways; complex polygons w/ holes: Ways + Relation)
  • add support for explicitly closing a Way (e.g. appending the first Node in the nodelist of the Way to the end of the Nodelist)
  • add support for creating just simple Polygons (e.g. a single closed Way) or more complex polygons (e.g. polygons with holes, which are represented as Relations).
  • make the --existing/-e argument optional to disable the search for intersections
  • add a --max_nodes_per_way option and allows for none as a value.

Potential Future Work

Because we've stopped ignoring polygon objects, we've opened the possibility that a given new object might intersect with a polygon. Creating <modify> nodes to represent changes to existing OSM polygons that intersect with new objects is out of the scope of this PR. This is because it requires tooling to access the ID of a constituent Way that requires modifications, which requires modifications / requirements to the schema in the PostGIS database we're using.

We've decided to not implement this, and have made an issue for it (#12).

@acannistra acannistra changed the base branch from master to tony/deletion May 10, 2021 21:46
@acannistra acannistra requested a review from JesseCrocker May 18, 2021 17:59
.pre-commit-config.yaml Show resolved Hide resolved
changegen/__main__.py Show resolved Hide resolved
@@ -277,6 +288,10 @@ def _modify_existing_way(way_geom, way_id, nodes, tags, intersection_db):
"""
new_nodes = nodes.copy()
way_geom_pts = list(way_geom.coords)
if len(way_geom_pts) > WAY_POINT_THRESHOLD:

Choose a reason for hiding this comment

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

It seems like in cases where there are long ways this is going to end up creating a lot of noise, because ways by default are getting split at 2000 points, so this will create a warning for every way that gets split.

)
new_nodes.extend(nodes)
new_ways.extend(ways)
_global_node_id_all_ways.extend(chain.from_iterable([w.nds for w in ways]))
if isinstance(wgs84_geom, sg.Polygon):
# simple polygons can be treated like Ways.
if len(wgs84_geom.interiors) == 0:

Choose a reason for hiding this comment

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

A relation also needs to be created If the exterior is more than 2000 nodes, is this handled somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't think of that. Does that mean anytime I split a long way into multiple ways I should create a relation joining them? I can see the case of a polygon exterior wanting to be a part of a multipolygon(?), that makes sense -- but what about regular Ways that are longer than 2000?

Choose a reason for hiding this comment

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

Anytime a polygon gets split you need to create a multipolygon relation, which is confusing, because it's not a multipolygon, it's just a single polygon. But there is no polygon relation. Then you just apply the same tags to the relation that would have been applied to the way, and have no tags on the way.

It's more complicated for non-polygon ways. If you split a trail, the most correct thing to do is probably to create a route relation, but i think most of our current code knows how to deal with rejoining things if the rest of the attributes match, so the route relation is not strictly needed, and would probably be a waste of time to implement now.

Base automatically changed from tony/deletion to master June 11, 2021 17:10
@acannistra
Copy link
Contributor Author

acannistra commented Jun 11, 2021

Was a little trigger-happy merging the deletions branch before this was reviewed. Now it has the deletions stuff in it. I'm going to take care of the merge conflict .

@acannistra
Copy link
Contributor Author

Cleaned up.

@acannistra acannistra marked this pull request as ready for review June 11, 2021 17:53
@acannistra acannistra requested a review from JesseCrocker June 11, 2021 17:53
@acannistra acannistra linked an issue Jun 11, 2021 that may be closed by this pull request
3 tasks
Copy link

@JesseCrocker JesseCrocker left a comment

Choose a reason for hiding this comment

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

Looked over code, LGTM. Did not test.

@acannistra acannistra merged commit 9f73d49 into master Jun 15, 2021
@acannistra acannistra deleted the tony/add-polygons branch June 15, 2021 21:19
@acannistra acannistra added this to the v0.1.0 milestone Jun 15, 2021
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.

Add (Multi)Polygon support
2 participants