-
-
Notifications
You must be signed in to change notification settings - Fork 185
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 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,7 @@ public record PlanetilerConfig( | |
| int minzoom, | ||
| int maxzoom, | ||
| int maxzoomForRendering, | ||
| int tileExtent, | ||
| boolean force, | ||
| boolean append, | ||
| boolean compressTempStorage, | ||
|
|
@@ -90,6 +91,9 @@ public record PlanetilerConfig( | |
| if (maxzoom > MAX_MAXZOOM) { | ||
| throw new IllegalArgumentException("Max zoom must be <= " + MAX_MAXZOOM + ", was " + maxzoom); | ||
| } | ||
| if (tileExtent <= 0) { | ||
| throw new IllegalArgumentException("tile_extent must be > 0, was " + tileExtent); | ||
| } | ||
|
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. I think that extent needs to be a power of 2?
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. yep, added power of 2 check |
||
| if (httpRetries < 0) { | ||
| throw new IllegalArgumentException("HTTP Retries must be >= 0, was " + httpRetries); | ||
| } | ||
|
|
@@ -139,6 +143,8 @@ public static PlanetilerConfig from(Arguments arguments) { | |
| int renderMaxzoom = | ||
| arguments.getInteger("render_maxzoom", "maximum rendering zoom level up to " + MAX_MAXZOOM, | ||
| Math.max(maxzoom, DEFAULT_MAXZOOM)); | ||
| int tileExtent = arguments.getInteger("tile_extent", | ||
| "vector tile extent (default 4096)", 4096); | ||
| Path tmpDir = arguments.file("tmpdir|tmp", "temp directory", Path.of("data", "tmp")); | ||
| List<String> extraNameTags = arguments.getList("extra_name_tags", "Extra name tags to copy from OSM to output", | ||
| List.of()); | ||
|
|
@@ -162,6 +168,7 @@ public static PlanetilerConfig from(Arguments arguments) { | |
| minzoom, | ||
| maxzoom, | ||
| renderMaxzoom, | ||
| tileExtent, | ||
| arguments.getBoolean("force", "overwriting output file and ignore disk/RAM warnings", false), | ||
| arguments.getBoolean("append", | ||
| "append to the output file - only supported by " + Stream.of(TileArchiveConfig.Format.values()) | ||
|
|
@@ -196,13 +203,13 @@ public static PlanetilerConfig from(Arguments arguments) { | |
| "Maximum bandwidth to consume when downloading files in units mb/s, mbps, kbps, etc.", "")), | ||
| arguments.getDouble("min_feature_size_at_max_zoom", | ||
| "Default value for the minimum size in tile pixels of features to emit at the maximum zoom level to allow for overzooming", | ||
| 256d / 4096), | ||
| 256d / tileExtent), | ||
| arguments.getDouble("min_feature_size", | ||
| "Default value for the minimum size in tile pixels of features to emit below the maximum zoom level", | ||
| 1), | ||
| arguments.getDouble("simplify_tolerance_at_max_zoom", | ||
| "Default value for the tile pixel tolerance to use when simplifying features at the maximum zoom level to allow for overzooming", | ||
| 256d / 4096), | ||
| 256d / tileExtent), | ||
| arguments.getDouble("simplify_tolerance", | ||
| "Default value for the tile pixel tolerance to use when simplifying features below the maximum zoom level", | ||
| 0.1d), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
| import com.onthegomap.planetiler.stats.Stats; | ||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
| import java.util.concurrent.atomic.AtomicReference; | ||
| import java.util.regex.Pattern; | ||
| import java.util.stream.Stream; | ||
| import org.geotools.api.referencing.FactoryException; | ||
|
|
@@ -48,10 +49,24 @@ | |
| */ | ||
| public class GeoUtils { | ||
|
|
||
| /** Rounding precision for 256x256px tiles encoded using 4096 values. */ | ||
| public static final PrecisionModel TILE_PRECISION = new PrecisionModel(4096d / 256d); | ||
| /** | ||
| * Rounding precision for 256x256px tiles encoded using {@code tile_extent} values (default 4096). | ||
| */ | ||
| private static final AtomicReference<PrecisionModel> tilePrecision = | ||
| new AtomicReference<>(new PrecisionModel(4096d / 256d)); | ||
|
Comment on lines
+52
to
+56
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. 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.
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. This issue also asks for the extent to be configurable per-zoom #1286 so we might even want to make it a global setting 🤔
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. how would you implement per zoom tile extent configuration? per zoom args? |
||
| public static final GeometryFactory JTS_FACTORY = new GeometryFactory(PackedCoordinateSequenceFactory.DOUBLE_FACTORY); | ||
|
|
||
| public static PrecisionModel tilePrecision() { | ||
| return tilePrecision.get(); | ||
| } | ||
|
|
||
| public static void setTileExtent(int tileExtent) { | ||
| if (tileExtent <= 0) { | ||
| throw new IllegalArgumentException("tile_extent must be > 0, got " + tileExtent); | ||
| } | ||
| tilePrecision.set(new PrecisionModel(tileExtent / 256d)); | ||
| } | ||
|
|
||
| public static final Geometry EMPTY_GEOMETRY = JTS_FACTORY.createGeometryCollection(); | ||
| public static final CoordinateSequence EMPTY_COORDINATE_SEQUENCE = new PackedCoordinateSequence.Double(0, 2, 0); | ||
| public static final Point EMPTY_POINT = JTS_FACTORY.createPoint(); | ||
|
|
@@ -309,11 +324,11 @@ public static Geometry combinePoints(List<Point> points) { | |
| } | ||
|
|
||
| /** | ||
| * Returns a copy of {@code geom} with coordinates rounded to {@link #TILE_PRECISION} and fixes any polygon | ||
| * Returns a copy of {@code geom} with coordinates rounded to {@link #tilePrecision()} and fixes any polygon | ||
| * self-intersections or overlaps that may have caused. | ||
| */ | ||
| public static Geometry snapAndFixPolygon(Geometry geom, Stats stats, String stage) throws GeometryException { | ||
| return snapAndFixPolygon(geom, TILE_PRECISION, stats, stage); | ||
| return snapAndFixPolygon(geom, tilePrecision(), stats, stage); | ||
| } | ||
|
|
||
| private static class OrientationFixer extends GeometryTransformer { | ||
|
|
||
| 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,7 +2088,7 @@ void testMergeLineStringsIgnoresRoundingIntersections() throws Exception { | |
| 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 |
||
|
|
||
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.
Zig-zag encoding maps coordinates in a range from -2^14 -> 2^14 to 0 -> 2^15. That assumption was based off 4096x4096 tiles with a 4096 buffer, so -4096 -> 8192 that gets mapped to 0 -> 16384 (2^14 plus 1 for safety). I think we could probably bump 15 up to 16, but then if tile extent / size exceeds a certain threshold then we might need to shift x and y right by a certain amount to keep it in the right range.