Merge 5851111 into eba244b

planetiler-core/src/main/java/com/onthegomap/planetiler/FeatureMerge.java

@@ -9,6 +9,8 @@
 import com.onthegomap.planetiler.geo.GeometryException;
 import com.onthegomap.planetiler.geo.GeometryType;
 import com.onthegomap.planetiler.geo.MutableCoordinateSequence;
+import com.onthegomap.planetiler.stats.DefaultStats;
+import com.onthegomap.planetiler.stats.Stats;
 import java.util.ArrayList;
 import java.util.BitSet;
 import java.util.Collection;
@@ -233,12 +235,13 @@ public static List<VectorTile.Feature> mergeOverlappingPolygons(List<VectorTile.
    * @param minDist     the minimum threshold in tile pixels between polygons to combine into a group
    * @param buffer      the amount (in tile pixels) to expand then contract polygons by in order to combine
    *                    almost-touching polygons
+   * @param stats       for counting data errors
    * @return a new list containing all unaltered features in their original order, then each of the merged groups
    *         ordered by the index of the first element in that group from the input list.
    * @throws GeometryException if an error occurs encoding the combined geometry
    */
   public static List<VectorTile.Feature> mergeNearbyPolygons(List<VectorTile.Feature> features, double minArea,
-    double minHoleArea, double minDist, double buffer) throws GeometryException {
+    double minHoleArea, double minDist, double buffer, Stats stats) throws GeometryException {
     List<VectorTile.Feature> result = new ArrayList<>(features.size());
     Collection<List<VectorTile.Feature>> groupedByAttrs = groupByAttrs(features, result, GeometryType.POLYGON);
     for (List<VectorTile.Feature> groupedFeatures : groupedByAttrs) {
@@ -272,7 +275,7 @@ public static List<VectorTile.Feature> mergeNearbyPolygons(List<VectorTile.Featu
           if (!(merged instanceof Polygonal) || merged.getEnvelopeInternal().getArea() < minArea) {
             continue;
           }
-          merged = GeoUtils.snapAndFixPolygon(merged).reverse();
+          merged = GeoUtils.snapAndFixPolygon(merged, stats, "merge").reverse();
         } else {
           merged = polygonGroup.get(0);
           if (!(merged instanceof Polygonal) || merged.getEnvelopeInternal().getArea() < minArea) {
@@ -289,6 +292,11 @@ public static List<VectorTile.Feature> mergeNearbyPolygons(List<VectorTile.Featu
     return result;
   }
 
+  public static List<VectorTile.Feature> mergeNearbyPolygons(List<VectorTile.Feature> features, double minArea,
+    double minHoleArea, double minDist, double buffer) throws GeometryException {
+    return mergeNearbyPolygons(features, minArea, minHoleArea, minDist, buffer, DefaultStats.get());
+  }
+
 
   /**
    * Returns all the clusters from {@code geometries} where elements in the group are less than {@code minDist} from

planetiler-core/src/main/java/com/onthegomap/planetiler/geo/GeoUtils.java

@@ -1,6 +1,7 @@
 package com.onthegomap.planetiler.geo;
 
 import com.onthegomap.planetiler.collection.LongLongMap;
+import com.onthegomap.planetiler.stats.Stats;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.stream.Stream;
@@ -22,6 +23,7 @@
 import org.locationtech.jts.geom.TopologyException;
 import org.locationtech.jts.geom.impl.PackedCoordinateSequence;
 import org.locationtech.jts.geom.impl.PackedCoordinateSequenceFactory;
+import org.locationtech.jts.geom.util.GeometryFixer;
 import org.locationtech.jts.geom.util.GeometryTransformer;
 import org.locationtech.jts.io.WKBReader;
 import org.locationtech.jts.precision.GeometryPrecisionReducer;
@@ -294,8 +296,8 @@ public static Geometry combinePoints(List<Point> points) {
    * Returns a copy of {@code geom} with coordinates rounded to {@link #TILE_PRECISION} and fixes any polygon
    * self-intersections or overlaps that may have caused.
    */
-  public static Geometry snapAndFixPolygon(Geometry geom) throws GeometryException {
-    return snapAndFixPolygon(geom, TILE_PRECISION);
+  public static Geometry snapAndFixPolygon(Geometry geom, Stats stats, String stage) throws GeometryException {
+    return snapAndFixPolygon(geom, TILE_PRECISION, stats, stage);
   }
 
   /**
@@ -304,21 +306,29 @@ public static Geometry snapAndFixPolygon(Geometry geom) throws GeometryException
    *
    * @throws GeometryException if an unrecoverable robustness exception prevents us from fixing the geometry
    */
-  public static Geometry snapAndFixPolygon(Geometry geom, PrecisionModel tilePrecision) throws GeometryException {
+  public static Geometry snapAndFixPolygon(Geometry geom, PrecisionModel tilePrecision, Stats stats, String stage)
+    throws GeometryException {
     try {
+      if (!geom.isValid()) {
+        geom = fixPolygon(geom);
+        stats.dataError(stage + "_snap_fix_input");
+      }
       return GeometryPrecisionReducer.reduce(geom, tilePrecision);
-    } catch (IllegalArgumentException e) {
+    } catch (TopologyException | IllegalArgumentException e) {
       // precision reduction fails if geometry is invalid, so attempt
       // to fix it then try again
-      geom = fixPolygon(geom);
+      geom = GeometryFixer.fix(geom);
+      stats.dataError(stage + "_snap_fix_input2");
       try {
         return GeometryPrecisionReducer.reduce(geom, tilePrecision);
-      } catch (IllegalArgumentException e2) {
+      } catch (TopologyException | IllegalArgumentException e2) {
         // give it one last try but with more aggressive fixing, just in case (see issue #511)
         geom = fixPolygon(geom, tilePrecision.gridSize() / 2);
+        stats.dataError(stage + "_snap_fix_input3");
         try {
           return GeometryPrecisionReducer.reduce(geom, tilePrecision);
-        } catch (IllegalArgumentException e3) {
+        } catch (TopologyException | IllegalArgumentException e3) {
+          stats.dataError(stage + "_snap_fix_input3_failed");
           throw new GeometryException("snap_third_time_failed", "Error reducing precision");
         }
       }

planetiler-core/src/main/java/com/onthegomap/planetiler/reader/osm/OsmReader.java

@@ -793,10 +793,11 @@ protected Geometry computeWorldGeometry() throws GeometryException {
         if (member.type() == OsmElement.Type.WAY) {
           if (poly != null && !poly.isEmpty()) {
             rings.add(poly);
-          } else if (relation.hasTag("type", "multipolygon")) {
+          } else {
             // boundary and land_area relations might not be complete for extracts, but multipolygons should be
-            LOGGER.warn(
-              "Missing " + role + " OsmWay[" + member.ref() + "] for " + relation.getTag("type") + " " + this);
+            stats.dataError("osm_" + relation.getTag("type") + "_missing_way");
+            LOGGER.trace(
+              "Missing {} OsmWay[{}] for {} {}", role, member.ref(), relation.getTag("type"), this);
           }
         }
       }

planetiler-core/src/main/java/com/onthegomap/planetiler/render/FeatureRenderer.java

@@ -237,7 +237,7 @@ private void writeTileFeatures(int zoom, long id, FeatureCollector.Feature featu
            * See https://docs.mapbox.com/vector-tiles/specification/#simplification for issues that can arise from naive
            * coordinate rounding.
            */
-          geom = GeoUtils.snapAndFixPolygon(geom);
+          geom = GeoUtils.snapAndFixPolygon(geom, stats, "render");
           // JTS utilities "fix" the geometry to be clockwise outer/CCW inner but vector tiles flip Y coordinate,
           // so we need outer CCW/inner clockwise
           geom = geom.reverse();

planetiler-core/src/main/java/com/onthegomap/planetiler/stats/DefaultStats.java

@@ -0,0 +1,18 @@
+package com.onthegomap.planetiler.stats;
+
+/**
+ * Holder for default {@link Stats} implementation to use for this process.
+ */
+public class DefaultStats {
+  private DefaultStats() {}
+
+  private static Stats defaultValue = null;
+
+  public static Stats get() {
+    return defaultValue;
+  }
+
+  public static void set(Stats stats) {
+    defaultValue = stats;
+  }
+}

planetiler-core/src/main/java/com/onthegomap/planetiler/stats/PrometheusStats.java

@@ -23,6 +23,7 @@
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentSkipListMap;
 import java.util.concurrent.Executors;
 import java.util.concurrent.ScheduledExecutorService;
@@ -49,6 +50,7 @@ class PrometheusStats implements Stats {
   private ScheduledExecutorService executor;
   private final String job;
   private final Map<String, Path> filesToMonitor = new ConcurrentSkipListMap<>();
+  private final Map<String, Long> dataErrorCounters = new ConcurrentHashMap<>();
   private final Map<String, MemoryEstimator.HasEstimate> heapObjectsToMonitor = new ConcurrentSkipListMap<>();
 
   /** Constructs a new instance but does not start polling (for tests). */
@@ -128,6 +130,7 @@ public void processedElement(String elemType, String layer) {
 
   @Override
   public void dataError(String errorCode) {
+    Stats.super.dataError(errorCode);
     dataErrors.labels(errorCode).inc();
   }
 
@@ -203,6 +206,11 @@ public List<MetricFamilySamples> collect() {
     }.register(registry);
   }
 
+  @Override
+  public Map<String, Long> dataErrors() {
+    return dataErrorCounters;
+  }
+
   @Override
   public void close() {
     executor.shutdown();

planetiler-core/src/main/java/com/onthegomap/planetiler/stats/Stats.java

@@ -9,6 +9,7 @@
 import java.nio.file.Path;
 import java.time.Duration;
 import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentSkipListMap;
 import java.util.function.LongSupplier;
 import java.util.function.Supplier;
@@ -26,15 +27,19 @@ public interface Stats extends AutoCloseable {
 
   /** Returns a new stat collector that stores basic stats in-memory to report through {@link #printSummary()}. */
   static Stats inMemory() {
-    return new InMemory();
+    var stats = new InMemory();
+    DefaultStats.set(stats);
+    return stats;
   }
 
   /**
    * Returns a new stat collector pushes stats at a regular interval to a
    * <a href="https://github.com/prometheus/pushgateway">prometheus push gateway</a> at {@code destination}.
    */
   static Stats prometheusPushGateway(String destination, String job, Duration interval) {
-    return PrometheusStats.createAndStartPushing(destination, job, interval);
+    var stats = PrometheusStats.createAndStartPushing(destination, job, interval);
+    DefaultStats.set(stats);
+    return stats;
   }
 
   /**
@@ -47,6 +52,11 @@ default void printSummary() {
     if (logger.isInfoEnabled()) {
       logger.info("");
       logger.info("-".repeat(40));
+      logger.info("data errors:");
+      dataErrors().entrySet().stream()
+        .sorted(Map.Entry.<String, Long>comparingByValue().reversed())
+        .forEachOrdered(entry -> logger.info("\t{}\t{}", entry.getKey(), format.integer(entry.getValue())));
+      logger.info("-".repeat(40));
       timers().printSummary();
       logger.info("-".repeat(40));
       for (var entry : monitoredFiles().entrySet()) {
@@ -156,11 +166,16 @@ default Counter.MultiThreadCounter nanoCounter(String name) {
    */
   void counter(String name, String label, Supplier<Map<String, LongSupplier>> values);
 
+  /** Returns all the data error counters. */
+  Map<String, Long> dataErrors();
+
   /**
    * Records that an invalid input feature was discarded where {@code errorCode} can be used to identify the kind of
    * failure.
    */
-  void dataError(String errorCode);
+  default void dataError(String errorCode) {
+    dataErrors().merge(errorCode, 1L, Long::sum);
+  }
 
   /**
    * A stat collector that stores top-level metrics in-memory to report through {@link #printSummary()}.
@@ -174,6 +189,7 @@ private InMemory() {}
 
     private final Timers timers = new Timers();
     private final Map<String, Path> monitoredFiles = new ConcurrentSkipListMap<>();
+    private final Map<String, Long> dataErrors = new ConcurrentHashMap<>();
 
     @Override
     public void wroteTile(int zoom, int bytes) {}
@@ -208,10 +224,12 @@ public Counter.MultiThreadCounter nanoCounter(String name) {
     public void counter(String name, String label, Supplier<Map<String, LongSupplier>> values) {}
 
     @Override
-    public void processedElement(String elemType, String layer) {}
+    public Map<String, Long> dataErrors() {
+      return dataErrors;
+    }
 
     @Override
-    public void dataError(String errorCode) {}
+    public void processedElement(String elemType, String layer) {}
 
     @Override
     public void gauge(String name, Supplier<Number> value) {}

planetiler-core/src/test/java/com/onthegomap/planetiler/PlanetilerTests.java

@@ -1710,6 +1710,54 @@ void testIssue509LenaDelta() throws Exception {
     ), counts);
   }
 
+  @Test
+  void testIssue546Terschelling() throws Exception {
+    Geometry geometry = new WKBReader()
+      .read(
+        new InputStreamInStream(Files.newInputStream(TestUtils.pathToResource("issue_546_terschelling.wkb"))));
+    geometry = GeoUtils.worldToLatLonCoords(geometry);
+
+    assertNotNull(geometry);
+
+    // automatically checks for self-intersections
+    var results = runWithReaderFeatures(
+      Map.of("threads", "1"),
+      List.of(
+        newReaderFeature(geometry, Map.of())
+      ),
+      (in, features) -> features.polygon("ocean")
+        .setBufferPixels(4.0)
+        .setMinZoom(0)
+    );
+
+
+    // this lat/lon is in the middle of an island and should not be covered by
+    for (int z = 4; z <= 14; z++) {
+      double lat = 53.391958;
+      double lon = 5.2438441;
+
+      var coord = TileCoord.aroundLngLat(lon, lat, z);
+      var problematicTile = results.tiles.get(coord);
+      if (z == 14) {
+        assertNull(problematicTile);
+        continue;
+      }
+      double scale = Math.pow(2, coord.z());
+
+      double tileX = (GeoUtils.getWorldX(lon) * scale - coord.x()) * 256;
+      double tileY = (GeoUtils.getWorldY(lat) * scale - coord.y()) * 256;
+
+      var point = newPoint(tileX, tileY);
+
+      assertEquals(1, problematicTile.size());
+      var geomCompare = problematicTile.get(0).geometry();
+      geomCompare.validate();
+      var geom = geomCompare.geom();
+
+      assertFalse(geom.covers(point), "z" + z);
+    }
+  }
+
   @ParameterizedTest
   @ValueSource(strings = {
     "",