Cluster buildings #725
Open
Cluster buildings #725
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
frodrigo
force-pushed
the
building_cluster
branch
from
36f106f
to
468f8bc
Compare
frodrigo
force-pushed
the
building_cluster
branch
from
468f8bc
to
75abdfe
Compare
|
needs rebase |
frodrigo
force-pushed
the
building_cluster
branch
from
75abdfe
to
48faacd
Compare
|
It can break existing applications if we lose osm_id. I would prefer to not do that at least on Z14. Only performance is the reason for that? Why do you need it? |
|
This PR is only on z14. |
|
Is this a pure performance optimization - described at Is the pain on upper zoom levels (12, 13?) which is being solved here? We wanted to have something similar for upper zoom levels (13 and perhaps 12) - with "blocks of buildings" in town centers merged into a polygon - this means modifying the geometry - not only doing a single MULTI-POLYGON from multiple polygons within a 100x100 meters areas. Losing the osm_id on deepest zoom levels on buildings is not really a great advantage of this approach. |
|
Let's discuss on the next community meeting. |
|
Yes. English version here : https://medium.com/@frederic.rodrigo/optimization-of-openstreetmap-background-vector-tile-production-for-an-on-request-service-makina-5f6dba98a232 On Z13 only large building are already on tiles. This approach does not match this existing zoom level. My test does not have issue on z12 or z13 like on z14 for dense urban area. Merge geometries cost more than just collect. My approach was to lost less attribute as possible on buildings. The grouping of building is done with all other attributes. So it is able to keep the height for 3D render eg. And un-grouping the cluster keep the same result as before, except for osm id. Merging polygons for z12 or z13 will have major impact on perf. Maybe Z12 and z13 should have a different strategy than z14. Keeping all detail on z14 (with my approach or not). |
|
Nevertheless, my other PRs are more elementaries and should be instructed to simply the buildings layer. |
frodrigo
force-pushed
the
building_cluster
branch
from
288fee4
to
733734d
Compare
frodrigo
changed the title
|
If this is still a |
This was referenced Apr 23, 2020
frodrigo
force-pushed
the
building_cluster
branch
from
733734d
to
f950d60
Compare
|
@daliborjanak @klokan About the osm_id I make a prof of concept and I am able to do it. We can switch it to osm_id, but it is an other issue. |
|
Cool! Well done - if you can now preserve the feature_ids @frodrigo ! Let's check the implication on the updating process and import step time and tile generating performance & size of the produced tiles on this (pretty cool) PR. |
|
@klokan but do you not prefer first we switch the feature_id of building layer to osm_id alone in a PR? |
|
Results evaluating commit 7baba38 (merged with base 9bb1779 as 450a016). See run details. PostgreSQL DB size in MB: 2665 ⇒ 2792 (4.8% change)
expand for details... |
frodrigo
force-pushed
the
building_cluster
branch
from
44f7e83
to
f950d60
Compare
frodrigo
force-pushed
the
building_cluster
branch
from
4009fe3
to
9017741
Compare
|
The proof of concept for keeping osm_id is on this extra branch : https://github.com/frodrigo/openmaptiles/commits/building_cluster_with_osm_id Waiting for update on switching to osm_id for feature_id or not before moving on. (#827) |
frodrigo
force-pushed
the
building_cluster
branch
from
88eaeea
to
40ba67c
Compare
frodrigo
force-pushed
the
building_cluster
branch
from
40ba67c
to
f071f69
Compare
nyurik
pushed a commit
that referenced
this pull request
May 20, 2020
Replacing materialized view by a tables with update from trigger on change only. Start with the most simple cases. Just replicate the change on: * `osm_water_polygon` to `osm_water_lakeline`, * `osm_water_polygon` to `osm_water_point`. Use a view to factorize the `osm_water_lakeline` and `osm_water_point_view` definition and reuse it in the trigger. The update of `osm_important_waterway_linestring` is more complex, as it is a merge of `osm_waterway_linestring`. It not done in the same way. At the end of the transaction we remove impacted and recompute them. The goal is to update more quickly the content of derivated table by just updating the changing content. It replaces the update of materialized view because their need a full recompute (with lock issue). Note, an advanced version of differential update over materialized view as already implemented in the building cluster PR #725. It addresses #814 and a part of #809.
frodrigo
force-pushed
the
building_cluster
branch
from
f071f69
to
1b16ab5
Compare
TomPohys
pushed a commit
that referenced
this pull request
May 26, 2020
Change the feature_id of objects in the tiles from arbitrary number to osm_id. From discussion here #725 (comment) Note the multiploygon building are load with negative relation osm_id. But the use in feature_id do an overflow (eg -7980888 -> 18446744073701570000). We can keep as is or not. No stats done for yet. Trying to let the CI do it.
|
@frodrigo I merged this branch with master, resolving a few conflicts, let me know if changes are ok |
frodrigo
force-pushed
the
building_cluster
branch
2 times, most recently
from
08cddc3
to
f1349ba
Compare
frodrigo
force-pushed
the
building_cluster
branch
from
f1349ba
to
e4cdca1
Compare
|
I updated with the support of the osm_id of the building. |
frodrigo
force-pushed
the
building_cluster
branch
from
e4cdca1
to
7baba38
Compare
|
Results evaluating commit d412521 (merged with base 81ddab9 as dff66bd). See run details. PostgreSQL DB size in MB: 2933 ⇒ 3054 (4.1% change)
expand for details... |
frodrigo
force-pushed
the
building_cluster
branch
from
7baba38
to
d412521
Compare
|
I rebased on the last master and removed the Draft/WIP status. I used this code now for years ! But since I wrote it, the generalized z13 building levels was added and its not compatible with. If you willing to merge this PR I suggest: Note this PR add a table intermediate, and so it grows the database. |
|
Thank @frodrigo for updating this PR. I need to look closer into this PR. |
frodrigo
force-pushed
the
building_cluster
branch
from
d412521
to
755b7a0
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The buildings layer on z14 is so far the heaviest one. It is particularly the case on dense urban area where the median time to generate tiles is about 5 000 ms.
Using materialized view of building reduce this median time to 3 200 ms. But still impracticable for real time tiles production.
Clustering the buildings using a grid highly reduce the number of objects to be fetched from database in dense area. The clustering is made using the other attributes to keep them.
Nevertheless clustering buildings requite a precomputation step and a storage to a table with index on geometry.
On Aquitaine, France
On Europe
The clustering take more time at preprocessing step, but less at tiles generation.
But the big change is on median time to produce z14 tiles in dense urban area, is now only at 225 ms.
At counter part, we lost the
osm_idon clustering.The table of buildings cluster take time to build and can be improved using differential update. Updating only cluster of changed buildings.
This is a work in progress.