-
-
Notifications
You must be signed in to change notification settings - Fork 188
Make tile extent runtime configurable with default 4096 #1565
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
base: main
Are you sure you want to change the base?
Changes from all commits
c05a49a
e539117
d96da68
ab69c4a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,6 +49,8 @@ public class FeatureRenderer implements Consumer<FeatureCollector.Feature>, Clos | |
| /** Constructs a new feature render that will send rendered features to {@code consumer}. */ | ||
| public FeatureRenderer(PlanetilerConfig config, Consumer<RenderedFeature> consumer, Stats stats, | ||
| Closeable closeable) { | ||
| VectorTile.setExtent(config.tileExtent()); | ||
| GeoUtils.setTileExtent(config.tileExtent()); | ||
| this.config = config; | ||
| this.consumer = consumer; | ||
| this.stats = stats; | ||
|
|
@@ -141,7 +143,7 @@ private void renderPoint(int zoom, Map<String, Object> attrs, FeatureCollector.F | |
| RenderedFeature.Group groupInfo = null; | ||
| if (hasLabelGrid && coords.length == 1) { | ||
| double labelGridTileSize = feature.getPointLabelGridPixelSizeAtZoom(zoom) / 256d; | ||
| groupInfo = labelGridTileSize < 1d / 4096d ? null : new RenderedFeature.Group( | ||
| groupInfo = labelGridTileSize < 1d / config.tileExtent() ? null : new RenderedFeature.Group( | ||
| GeoUtils.labelGridId(tilesAtZoom, labelGridTileSize, coords[0]), | ||
| feature.getPointLabelGridLimitAtZoom(zoom) | ||
| ); | ||
|
|
@@ -264,9 +266,11 @@ private void writeTileFeatures(int zoom, long id, FeatureCollector.Feature featu | |
| // post-processing. Features need to be "unscaled" in FeatureGroup after line merging, | ||
| // and before emitting to the output archive. | ||
| scale = Math.max(config.maxzoom(), 14) - zoom; | ||
| // need 14 bits to represent tile coordinates (4096 * 2 for buffer * 2 for zigzag encoding) | ||
| // need enough bits to represent tile coordinates (extent * 2 for buffer * 2 for zigzag encoding) | ||
| // so cap the scale factor to avoid overflowing 32-bit integer space | ||
| scale = Math.min(31 - 14, scale); | ||
| long maxCoordinate = config.tileExtent() * 4L; | ||
| int bits = 64 - Long.numberOfLeadingZeros(maxCoordinate - 1); | ||
| scale = Math.clamp(scale, 0, Math.max(0, 31 - bits)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are a few more subtle places that the 4096 assumption has snuck in that might not explicitly reference 4096 - this is one of them, thanks for fixing! I'm trying to think if there might be any others... |
||
| } | ||
|
|
||
| if (!geom.isEmpty()) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2088,11 +2088,91 @@ | |
| feature(newMultiLineString( | ||
| newLineString(32, 64.3125, 37, 64.0625, 42, 64.3125), | ||
| newLineString(32, 64, 37, 64.0625, 42, 64) | ||
| ), Map.of()) | ||
| ), "layer", Map.of(), 0) | ||
| ) | ||
| )), sortListValues(results.tiles)); | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add a test for end to end point/line/polygon for a few different values of tile extent? At least 8192 and 16384, possibly a lower value like 512 or 1024 as well? I think we should probably test a worst case for each of those with coordinates at (-255, -255) (513, -255) (513, 513) (-255, 513) and a few points in the middle
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added |
||
|
|
||
| @Test | ||
| void testTileExtentWorstCasePointLinePolygon() throws Exception { | ||
| var baseline = runTileExtentWorstCasePointLinePolygon(4096); | ||
|
|
||
| // Include both high and low extents to stress clipping/rounding behavior. | ||
| for (int tileExtent : List.of(512, 1024, 8192, 16384)) { | ||
| var result = runTileExtentWorstCasePointLinePolygon(tileExtent); | ||
| assertEquals(sortListValues(baseline.tiles), sortListValues(result.tiles), "tile_extent=" + tileExtent); | ||
| } | ||
| } | ||
|
|
||
| private PlanetilerResults runTileExtentWorstCasePointLinePolygon(int tileExtent) throws Exception { | ||
| var points = newMultiPoint( | ||
| z14Point(-255, -255), | ||
| z14Point(513, -255), | ||
| z14Point(513, 513), | ||
| z14Point(-255, 513), | ||
| z14Point(0, 0), | ||
| z14Point(128, 128), | ||
| z14Point(256, 256) | ||
| ); | ||
|
|
||
| var lines = newMultiLineString( | ||
| newLineString(z14CoordinatePixelList( | ||
| -255, -255, | ||
| 513, -255, | ||
| 513, 513, | ||
| -255, 513, | ||
| -255, -255 | ||
| )), | ||
| newLineString(z14CoordinatePixelList( | ||
| 0, 0, | ||
| 128, 128, | ||
| 256, 256 | ||
| )) | ||
| ); | ||
|
|
||
| var polygon = newPolygon(z14CoordinatePixelList( | ||
| -255, -255, | ||
| 513, -255, | ||
| 513, 513, | ||
| -255, 513, | ||
| -255, -255 | ||
| )); | ||
|
|
||
| return runWithReaderFeatures( | ||
| Map.of( | ||
| "threads", "1", | ||
| "maxzoom", "14", | ||
| "tile_extent", Integer.toString(tileExtent) | ||
| ), | ||
| List.of( | ||
| newReaderFeature(points, Map.of()), | ||
| newReaderFeature(lines, Map.of()), | ||
| newReaderFeature(polygon, Map.of()) | ||
| ), | ||
| (in, features) -> { | ||
| if (in.isPoint()) { | ||
| features.point("points") | ||
| .setZoomRange(14, 14) | ||
| .setBufferPixels(257); | ||
| } | ||
| if (in.canBeLine()) { | ||
| features.line("lines") | ||
| .setZoomRange(14, 14) | ||
| .setBufferPixels(257) | ||
| .setMinPixelSize(0) | ||
| .setPixelTolerance(0); | ||
| } | ||
| if (in.canBePolygon()) { | ||
| features.polygon("polygons") | ||
| .setZoomRange(14, 14) | ||
| .setBufferPixels(257) | ||
| .setMinPixelSize(0) | ||
| .setPixelTolerance(0); | ||
| } | ||
| } | ||
| ); | ||
| } | ||
|
|
||
| @ParameterizedTest | ||
| @ValueSource(booleans = {false, true}) | ||
| void testMergePolygons(boolean unionOverlapping) throws Exception { | ||
|
|
@@ -2687,7 +2767,7 @@ | |
| return TileCompression.GZIP; | ||
| } else if (args.contains("tile-compression=")) { | ||
| throw new IllegalArgumentException("unhandled tile compression"); | ||
| } else { | ||
|
Check warning on line 2770 in planetiler-core/src/test/java/com/onthegomap/planetiler/PlanetilerTests.java
|
||
| return TileCompression.GZIP; | ||
| } | ||
| } | ||
|
|
@@ -2767,7 +2847,7 @@ | |
| } | ||
| }) | ||
| .addOsmSource("osm", tempOsm) | ||
| .addNaturalEarthSource("ne", TestUtils.pathToResource("natural_earth_vector.sqlite")) | ||
|
Check warning on line 2850 in planetiler-core/src/test/java/com/onthegomap/planetiler/PlanetilerTests.java
|
||
| .addShapefileSource("shapefile", TestUtils.pathToResource("shapefile.zip")) | ||
| .addGeoPackageSource("geopackage", TestUtils.pathToResource("geopackage.gpkg.zip"), null) | ||
| .addGeoJsonSource("geojson", TestUtils.pathToResource("featurecollection.geojson"), null) | ||
|
|
@@ -2976,7 +3056,7 @@ | |
| void testPlanetilerRunnerParquet(String args) throws Exception { | ||
| Path mbtiles = tempDir.resolve("output.mbtiles"); | ||
|
|
||
| Planetiler.create(Arguments.fromArgs((args + " --tmpdir=" + tempDir.resolve("data")).split("\\s+"))) | ||
|
Check warning on line 3059 in planetiler-core/src/test/java/com/onthegomap/planetiler/PlanetilerTests.java
|
||
| .setProfile(new Profile.NullProfile() { | ||
| @Override | ||
| public void processFeature(SourceFeature source, FeatureCollector features) { | ||
|
|
@@ -3056,7 +3136,7 @@ | |
| Planetiler.create(Arguments.of("tmpdir", tempDir, "force", Boolean.toString(force))) | ||
| .setProfile(profile) | ||
| .addOsmSource("osm", TestUtils.pathToResource("monaco-latest.osm.pbf")) | ||
| .addNaturalEarthSource("ne", TestUtils.pathToResource("natural_earth_vector.sqlite")) | ||
|
Check warning on line 3139 in planetiler-core/src/test/java/com/onthegomap/planetiler/PlanetilerTests.java
|
||
| .addShapefileSource("shapefile", TestUtils.pathToResource("shapefile.zip")) | ||
| .addGeoPackageSource("geopackage", TestUtils.pathToResource("geopackage.gpkg.zip"), null) | ||
| .setOutput(tempDir.resolve("output.mbtiles")) | ||
|
|
||
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.
Ideally we move all references to 4096 out of static variables and either pass them as args to functions that need them or extract them from geoutils to a class that you instantiate with a tile extent. That might make this PR very big though, let me know what you think - if it's too much I could do in a followup PR.
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.
This issue also asks for the extent to be configurable per-zoom #1286 so we might even want to make it a global setting 🤔
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.
how would you implement per zoom tile extent configuration? per zoom args?
--tileExtentZ12, --tileExtentZ13, etc... ?