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

Aggregation of buildings #6

Open
wants to merge 7 commits into
base: buildings
Choose a base branch
from

Conversation

klokan
Copy link
Member

@klokan klokan commented May 4, 2018

This is an initial idea for implementation of aggregation of small buildings - which could be used on zoom 13 (or upper zooms).

Now we simply throw away small buildings in OpenMapTiles - so dense centres of towns such as Zurich with many small buildings practically disappear from the map. This PR is trying to fix the issue, see new green polygons:

zurgen
(pink buildings are now in our map on z13 - red are removed)

We need feedback on the approach. Probably some very early test on performance would be good.
Values for aggregation should not be fixed constants - but probably derived from zres.

Comments on how to prepare this for merging would be helpful.
This is a possible very visual feature for OpenMapTiles v3.9.

@klokan klokan requested a review from jirik May 4, 2018 08:28
@klokan klokan changed the title Aggregation of building Aggregation of buildings May 4, 2018
@klokan
Copy link
Member Author

klokan commented May 4, 2018

@jeleniste do you have any info about the extra processing time required for calculating these aggregated shapes?

@jeleniste
Copy link

@klokan it depends on tolerance for grouping, actually is ten meters (picture with pink and green polygons has lower, five, or four meters). Using ten meters tolerance for grouping isnt nescesary. But grouping was about ten minutes for Switzerland, on my localhost (old laptop, postgre isnt well tuned). For example simplification of whole layer osm_buildings_polygons cost about two minutes.

@klokan
Copy link
Member Author

klokan commented May 10, 2018

@jirik Could you please provide initial feedback on the direction of development?

@jirik
Copy link
Collaborator

jirik commented May 14, 2018

From my point of view it behaves well in case of dense built-up areas.

Suggestions

  • Use materialized view instead of table, because materialized view can be simply refreshed after updates.
  • Name the view osm_bouilgong_block_gen1 to better distinguish it from osm_building_polygon.
  • Refresh the generalized view after update. See how it's implemented in case of important waterways or aggregated POIs. It would be great to use the same approach (new schema building_block, trigger_flag, trigger_refresh, building_block.updates, building_block.flag, etc.).
  • Use zres() to set tolerances. E.g. zres(14) is about 9.55.

Performance
Switzerland Z0-Z14 buildings only: 5m 40s before, 5m 57s after, so the performance seems great

Visual comparison

Z13 before:
image

Z13 after:
image

@jeleniste
Copy link

@jirik Ok, i didnt know, how i can set zres into my sql before. I will get it there using this template.
There is not simple way, how to create matview from this, because it must be in steps (aggregation, removing small holes, removing small polygons, simplification. But it is possible make function and function can be trigered.

@jirik
Copy link
Collaborator

jirik commented May 15, 2018

But it is possible make function and function can be trigered.

Exactly, you can see it in this sample. Thanks for your work!

@jeleniste
Copy link

@jirik i can create recordset returning functions iterating over aggregated data and create matview from it. But it means use for loop and it can cost some calculating time (because of iterating). And, in my opinion, it isnt good solution use database triggers for computationally difficult jobs. For jobs like this is better to trigger refresh from application, because if some insert will be in more parts, matview will be refreshed on every statement.

@jeleniste
Copy link

@jirik version with matview commited. Without triggers, in this moment, i should look on triggering mechanism on another layers.

@jeleniste
Copy link

@jirik update triggers added, but im not able to test, if it works well

@jeleniste
Copy link

@jirik could you test this, or look at it, if it is OK? Maybe calculation time be worse, because of change to matviews, in this case i can use function.

@jirik
Copy link
Collaborator

jirik commented May 29, 2018

@jeleniste Thanks. I will look at it in following days. At this moment I am struggling with more critical issues :-/

@jirik
Copy link
Collaborator

jirik commented May 30, 2018

@klokan @jeleniste Unfortunately, I made a mistake when I was measuring the performance last time. Sorry for that. I measured it more precisely this time and results seem not-so-good.

For Switzerland the critical import-sql step took about 6 extra minutes (compared to previous 0m). Switzerland has about 2.25 millions of buildings, the whole planet about 288 millions, that's about 128 times more. 128 * 6m = estimated increase 13h (in case of linear time complexity). But I am really not sure about what time complexity we can expect.

Performance for Switzerland Z0-Z14, buildings only.

command before after 5584178 after 0fdf564
import-osm 2m13s 2m4s 2m5s
import-sql 0m2s 6m46s 5m52s
generate-tiles 5m57s 6m25s 5m54s

@jeleniste The code looks good to me now. Probably these two lines should be removed before merge.

@jeleniste
Copy link

done

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.

3 participants