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.

Sign up for GitHub

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

Jump to bottom

Unify building parts with their building #969

Merged
merged 2 commits into from
Aug 18, 2016
Merged

Unify building parts with their building #969

merged 2 commits into from
Aug 18, 2016

Conversation

rmarianski
Copy link
Member

Connects to #653

@nvkelso, @zerebubuth could you review please?

@rmarianski
Copy link
Member Author

The relation in the issue, the sf ferry building doesn't end up getting inserted into the database (it's in rels, but not in planet_osm_polygon). But, the outline does. This just means that currently the root_id that gets associated with those buildings is the outline and not the relation id (a feature with the relation id doesn't get returned in the tile).

I also tried to find an example of a tile that had a root_relation_id set in the pois layer, but haven't thus far. I'll continue to hunt down for one. I wanted to find one to write a test for it to verify that the commit in this pr helps.

zerebubuth
zerebubuth reviewed Aug 17, 2016
View reviewed changes
docs/layers.md
@@ -151,7 +151,7 @@ Combination of OpenStreetMap administrative boundaries (zoom >= 8) and Natural E

Polygons from OpenStreetMap representing building footprint, building label_placement points, building_part features, and address points. Starts at zoom 13 by including huge buildings, progressively adding all buildings at zoom 16+. Address points are available at zoom 16+, but marked with `min_zoom: 17` to suggest that they are suitable for display at zoom level 17 and higher.

Individual `building:part` geometries from OSM following the [Simple 3D Buildings](http://wiki.openstreetmap.org/wiki/Simple_3D_Buildings) tags at higher zoom levels are now exported as `building_part` features with specified `kind_detail`.
Individual `building:part` geometries from OSM following the [Simple 3D Buildings](http://wiki.openstreetmap.org/wiki/Simple_3D_Buildings) tags at higher zoom levels are now exported as `building_part` features with specified `kind_detail`. Building parts will receive a `root_id` that will correspond to the id of the `building` feature that they intersect.
Copy link
Member

Choose a reason for hiding this comment

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

Pedantic, but: This makes it sound like building:parts will always receive a root_id, but its possible for building:parts to not match any buildings. Would it be clearer if it were written as follows?

Building parts may receive a root_id corresponding to the building feature, if any, with which they intersect.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, updated in 3fa38ad.

@zerebubuth
Copy link
Member

Tile 16/32747/21793 has two station points for Waterloo station with root_relation_id: 1242762. Pulling in that relation should, I think, be enough to reproduce that in the tests.

One pedantic thing: There are test cases for the non-overlapping and overlapping case, but none for the case where it picks the most overlapping of several candidates.

Other than that, it looks great! 👍

@rmarianski
Copy link
Member Author

Tile 16/32747/21793 has two station points for Waterloo station with root_relation_id: 1242762. Pulling in that relation should, I think, be enough to reproduce that in the tests.

Thanks. Updated in 8a6cb49.

@rmarianski
Copy link
Member Author

One pedantic thing: There are test cases for the non-overlapping and overlapping case, but none for the case where it picks the most overlapping of several candidates.

Added unit test for best overlap in aa7d144.

@zerebubuth
Copy link
Member

👍 looks awesome! Nice work!

@rmarianski rmarianski force-pushed the unify-building-part branch from 1124614 to bf06a5b Compare August 18, 2016 15:10
@rmarianski rmarianski merged commit 6ec9637 into master Aug 18, 2016
@rmarianski rmarianski deleted the unify-building-part branch August 18, 2016 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rmarianski @zerebubuth