-
-
Notifications
You must be signed in to change notification settings - Fork 118
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
Tile hashing fix #436
Tile hashing fix #436
Conversation
…different hash even though encoded result is same
…ayer (some duplicates still remain due to faulty hash calculation)
I'm not quite sure why the actions didn't run so I merged in main and pushed - they seem to be going now. |
planetiler-core/src/main/java/com/onthegomap/planetiler/VectorTile.java
Outdated
Show resolved
Hide resolved
planetiler-core/src/main/java/com/onthegomap/planetiler/mbtiles/MbtilesWriter.java
Outdated
Show resolved
Hide resolved
planetiler-core/src/main/java/com/onthegomap/planetiler/VectorTile.java
Outdated
Show resolved
Hide resolved
planetiler-core/src/main/java/com/onthegomap/planetiler/mbtiles/MbtilesWriter.java
Show resolved
Hide resolved
planetiler-core/src/test/java/com/onthegomap/planetiler/collection/FeatureGroupTest.java
Outdated
Show resolved
Hide resolved
@@ -110,6 +110,8 @@ void integrationTest(@TempDir Path tmpDir) throws Exception { | |||
"name", "EuroVelo 8 - Mediterranean Route - part Monaco", | |||
"ref", "EV8" | |||
), GeoUtils.WORLD_LAT_LON_BOUNDS, 25, LineString.class); | |||
|
|||
TestUtils.assertTileDuplicates(mbtiles, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there somewhere in the core module we can put this? I just want to make sure core functionality is verified there in case we remove/change examples in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far no other UT is triggering the root-cause.
Hence I've tried to make a new UT in planetiler-core
but then realized I would need to replicate quite a lot of code from planetiler-examples
.
E.g. both options are "bad".
Conclusion so far was to go with "smaller bad", e.g. the current option, since it adds much smaller amount of code.
https://github.com/onthegomap/planetiler/actions/runs/3895562578 ℹ️ Base Logs 640ce50
ℹ️ This Branch Logs 31c1cba
|
As per "Example project" failure: It looks like chicken-egg problem: unless we merge this, it will fail, since it is running with |
…right one for this
Kudos, SonarCloud Quality Gate passed! |
Looks good! Thanks for the fix. |
While generating maps for Slovakia and some other countries I've noticed that certain tiles are still stored in the .mbtiles more than once, e.g. with same encoded content, despite what was done in PR #219 .
For smaller maps (like for Slovakia), following can be used directly in SQLite to detect duplicates:
result:
(Query is not very nice and does not scale well to bigger maps, but works.)
First observation was, that heuristic determining when to compute hashes was skipping those tiles. Then it was observed that hash calculation done in
FeatureFroup
(see https://github.com/onthegomap/planetiler/blob/3fd094d/planetiler-core/src/main/java/com/onthegomap/planetiler/collection/FeatureGroup.java#L377 ) is producing different hashes for tiles which when encoded and compressed, are byte-to-byte same.Hence this PR does following:
BikeRouteOverlayTest
unit test was extended to count duplicates in generated .mbtilesBikeRouteOverlayTest
was identified as triggering the root-causes and thus extended to determine duplicate tiles.FeatureGroup
and features.Updated Planetiler seems working better (=fewer duplicates, same performance), at least based on the unit test and a test with
slovakia.mbtiles
:(No count bigger than 1, hence no duplicates.)
Side note reg.
BikeRouteOverlayTest
: While debugging, following was observed: