diff --git a/src/main/java/com/dedicatedcode/paikka/service/importer/ImportService.java b/src/main/java/com/dedicatedcode/paikka/service/importer/ImportService.java index 3eb8f30..64ce1c3 100644 --- a/src/main/java/com/dedicatedcode/paikka/service/importer/ImportService.java +++ b/src/main/java/com/dedicatedcode/paikka/service/importer/ImportService.java @@ -35,6 +35,7 @@ import org.locationtech.jts.algorithm.construct.MaximumInscribedCircle; import org.locationtech.jts.geom.*; import org.locationtech.jts.io.WKBWriter; +import org.locationtech.jts.operation.polygonize.Polygonizer; import org.rocksdb.*; import org.springframework.beans.factory.annotation.Value; import org.springframework.stereotype.Service; @@ -1524,39 +1525,46 @@ private org.locationtech.jts.geom.Geometry buildGeometryFromRelRec(RelRec rec, R List> outerRings = buildConnectedRings(toList(rec.outer), nodeCache, wayIndexDb); List> innerRings = buildConnectedRings(toList(rec.inner), nodeCache, wayIndexDb); if (outerRings.isEmpty()) return null; - List validPolygons = new ArrayList<>(); + + Polygonizer polygonizer = new Polygonizer(); for (List outerRing : outerRings) { try { - LinearRing shell = GEOMETRY_FACTORY.createLinearRing(outerRing.toArray(new Coordinate[0])); - List holes = new ArrayList<>(); - for (List innerRing : innerRings) - try { - holes.add(GEOMETRY_FACTORY.createLinearRing(innerRing.toArray(new Coordinate[0]))); - } catch (Exception e) { - stats.recordError(ImportStatistics.Stage.PROCESSING_ADMIN_BOUNDARIES, Kind.READ, rec.osmId, "rocks-get:way_index", e); - } - Polygon polygon = GEOMETRY_FACTORY.createPolygon(shell, holes.toArray(new LinearRing[0])); - if (polygon.isValid()) { - validPolygons.add(polygon); - } else { - try { - org.locationtech.jts.geom.Geometry repaired = polygon.buffer(0); - if (repaired != null && repaired.isValid() && !repaired.isEmpty()) { - for (int i = 0; i < repaired.getNumGeometries(); i++) { - org.locationtech.jts.geom.Geometry part = repaired.getGeometryN(i); - if (part instanceof Polygon && part.isValid()) { - validPolygons.add((Polygon) part); - } + polygonizer.add(GEOMETRY_FACTORY.createLinearRing(outerRing.toArray(new Coordinate[0]))); + } catch (Exception e) { + stats.recordError(ImportStatistics.Stage.PROCESSING_ADMIN_BOUNDARIES, Kind.GEOMETRY, rec.osmId, "createLinearRing-outer", e); + } + } + for (List innerRing : innerRings) { + try { + polygonizer.add(GEOMETRY_FACTORY.createLinearRing(innerRing.toArray(new Coordinate[0]))); + } catch (Exception e) { + stats.recordError(ImportStatistics.Stage.PROCESSING_ADMIN_BOUNDARIES, Kind.READ, rec.osmId, "buildConnectedRings-inner", e); + } + } + + @SuppressWarnings("unchecked") + Collection polygons = polygonizer.getPolygons(); + List validPolygons = new ArrayList<>(); + for (Polygon p : polygons) { + if (p.isValid()) { + validPolygons.add(p); + } else { + try { + org.locationtech.jts.geom.Geometry repaired = p.buffer(0); + if (repaired != null && repaired.isValid() && !repaired.isEmpty()) { + for (int i = 0; i < repaired.getNumGeometries(); i++) { + org.locationtech.jts.geom.Geometry part = repaired.getGeometryN(i); + if (part instanceof Polygon polygon && polygon.isValid()) { + validPolygons.add(polygon); } } - } catch (Exception repairEx) { - stats.recordError(ImportStatistics.Stage.PROCESSING_ADMIN_BOUNDARIES, Kind.GEOMETRY, rec.osmId, "repair-polygon", repairEx); } + } catch (Exception repairEx) { + stats.recordError(ImportStatistics.Stage.PROCESSING_ADMIN_BOUNDARIES, Kind.GEOMETRY, rec.osmId, "repair-polygon", repairEx); } - } catch (Exception e) { - stats.recordError(ImportStatistics.Stage.PROCESSING_ADMIN_BOUNDARIES, Kind.GEOMETRY, null, "build-boundary-geometry", e); } } + if (validPolygons.isEmpty()) return null; return validPolygons.size() == 1 ? validPolygons.getFirst() : GEOMETRY_FACTORY.createMultiPolygon(validPolygons.toArray(new Polygon[0])); } diff --git a/src/main/java/com/dedicatedcode/paikka/service/importer/StandaloneBoundaryImporter.java b/src/main/java/com/dedicatedcode/paikka/service/importer/StandaloneBoundaryImporter.java index 60debee..09971e7 100644 --- a/src/main/java/com/dedicatedcode/paikka/service/importer/StandaloneBoundaryImporter.java +++ b/src/main/java/com/dedicatedcode/paikka/service/importer/StandaloneBoundaryImporter.java @@ -23,6 +23,7 @@ import de.topobyte.osm4j.pbf.seq.PbfIterator; import org.locationtech.jts.geom.*; import org.locationtech.jts.io.WKBWriter; +import org.locationtech.jts.operation.polygonize.Polygonizer; import org.rocksdb.*; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -395,26 +396,26 @@ private Geometry buildMultiPolygon(RelationStub stub, RocksDB nodeCache, RocksDB List> innerRings = stitchRings(stub.innerWays(), nodeCache, wayCache); if (outerRings.isEmpty()) return null; - List polygons = new ArrayList<>(); + Polygonizer polygonizer = new Polygonizer(); for (List outer : outerRings) { try { - LinearRing shell = GEOMETRY_FACTORY.createLinearRing(outer.toArray(new Coordinate[0])); - List holes = new ArrayList<>(); - for (List inner : innerRings) { - try { - holes.add(GEOMETRY_FACTORY.createLinearRing(inner.toArray(new Coordinate[0]))); - } catch (Exception e) { - stats.recordError(BoundaryImportStatistics.Stage.PROCESSING_RELATIONS, BoundaryImportStatistics.Kind.GEOMETRY, stub.osmId(), "createLinearRing-inner", e); - } - } - Polygon p = GEOMETRY_FACTORY.createPolygon(shell, holes.toArray(new LinearRing[0])); - if (p.isValid()) polygons.add(p); + polygonizer.add(GEOMETRY_FACTORY.createLinearRing(outer.toArray(new Coordinate[0]))); + } catch (Exception e) { + stats.recordError(BoundaryImportStatistics.Stage.PROCESSING_RELATIONS, BoundaryImportStatistics.Kind.GEOMETRY, stub.osmId(), "createLinearRing-outer", e); + } + } + for (List inner : innerRings) { + try { + polygonizer.add(GEOMETRY_FACTORY.createLinearRing(inner.toArray(new Coordinate[0]))); } catch (Exception e) { - stats.recordError(BoundaryImportStatistics.Stage.PROCESSING_RELATIONS, BoundaryImportStatistics.Kind.GEOMETRY, stub.osmId(), "buildMultiPolygon", e); + stats.recordError(BoundaryImportStatistics.Stage.PROCESSING_RELATIONS, BoundaryImportStatistics.Kind.GEOMETRY, stub.osmId(), "createLinearRing-inner", e); } } + + @SuppressWarnings("unchecked") + Collection polygons = polygonizer.getPolygons(); if (polygons.isEmpty()) return null; - return polygons.size() == 1 ? polygons.getFirst() : GEOMETRY_FACTORY.createMultiPolygon(polygons.toArray(new Polygon[0])); + return polygons.size() == 1 ? polygons.iterator().next() : GEOMETRY_FACTORY.createMultiPolygon(polygons.toArray(new Polygon[0])); } private List> stitchRings(List wayIds, RocksDB nodeCache, RocksDB wayCache) { diff --git a/src/test/java/com/dedicatedcode/paikka/service/importer/ImportServiceTest.java b/src/test/java/com/dedicatedcode/paikka/service/importer/ImportServiceTest.java index 92b986c..da0a87d 100644 --- a/src/test/java/com/dedicatedcode/paikka/service/importer/ImportServiceTest.java +++ b/src/test/java/com/dedicatedcode/paikka/service/importer/ImportServiceTest.java @@ -217,7 +217,7 @@ void testImportPoiHasNamesAndBoundary() throws Exception { POI poiById = findPoiById(tempDataDir, 432751852); assertEquals(1, poiById.namesLength(), "POI should have no"); assertEquals("Jardin des Boulingrins", poiById.names(0).text(), "POI should have no"); - assertEquals(6, poiById.hierarchyLength()); + assertEquals(3, poiById.hierarchyLength()); } @Test diff --git a/src/test/java/com/dedicatedcode/paikka/service/importer/StandaloneBoundaryImporterThuringiaTest.java b/src/test/java/com/dedicatedcode/paikka/service/importer/StandaloneBoundaryImporterThuringiaTest.java new file mode 100644 index 0000000..c6cf823 --- /dev/null +++ b/src/test/java/com/dedicatedcode/paikka/service/importer/StandaloneBoundaryImporterThuringiaTest.java @@ -0,0 +1,253 @@ +/* + * This file is part of paikka. + * + * Paikka is free software: you can redistribute it and/or + * modify it under the terms of the GNU Affero General Public License + * as published by the Free Software Foundation, either version 3 or + * any later version. + * + * Paikka is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied + * warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * See the GNU Affero General Public License for more details. + * You should have received a copy of the GNU Affero General Public License + * along with Paikka. If not, see . + */ + +package com.dedicatedcode.paikka.service.importer; + +import com.dedicatedcode.paikka.config.PaikkaConfiguration; +import org.junit.jupiter.api.*; +import org.locationtech.jts.geom.*; +import org.locationtech.jts.io.WKBReader; +import org.rocksdb.Options; +import org.rocksdb.RocksDB; + +import java.io.InputStream; +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.StandardCopyOption; +import java.util.*; + +import static org.junit.jupiter.api.Assertions.*; + +/** + * Tests for proper hole-in-polygon assignment in the standalone boundary importer. + * Uses thueringen-260804.boundaries.osm.pbf which has multipolygon relations + * with disjoint outer rings and inner rings (holes). This verifies the fix + * for the bug where holes were incorrectly assigned to all outer rings, + * causing rendering artifacts (large open triangles / lines). + */ +class StandaloneBoundaryImporterThuringiaTest { + + private static Path tempOutputDir; + private static Path tempPbfFile; + + @BeforeAll + static void setUp() throws Exception { + tempOutputDir = Files.createTempDirectory("paikka-boundary-thueringen-test"); + tempPbfFile = Files.createTempFile("paikka-boundary-thueringen-test", ".pbf"); + + try (InputStream is = StandaloneBoundaryImporter.class.getClassLoader() + .getResourceAsStream("thueringen-260804.boundaries.osm.pbf")) { + assertNotNull(is, "thueringen-260804.boundaries.osm.pbf not found in test resources"); + Files.copy(is, tempPbfFile, StandardCopyOption.REPLACE_EXISTING); + } + + PaikkaConfiguration config = new PaikkaConfiguration(); + + PaikkaConfiguration.ImportConfiguration importCfg = new PaikkaConfiguration.ImportConfiguration(); + importCfg.setThreads(4); + config.setImportConfiguration(importCfg); + + PaikkaConfiguration.SimplificationConfiguration simplCfg = new PaikkaConfiguration.SimplificationConfiguration(); + simplCfg.setContinentTolerance(0.005); + simplCfg.setCountryTolerance(0.00045); + simplCfg.setStateTolerance(0.00009); + simplCfg.setPoiTolerance(0.000018); + simplCfg.setDefaultTolerance(0.000045); + config.setSimplificationConfiguration(simplCfg); + + GeometrySimplificationService simplService = new GeometrySimplificationService(config); + StandaloneBoundaryImporter importer = new StandaloneBoundaryImporter(simplService, config); + importer.importBoundaries(Collections.singletonList(tempPbfFile.toString()), tempOutputDir.toString()); + } + + @AfterAll + static void tearDown() { + if (tempOutputDir != null && Files.exists(tempOutputDir)) { + deleteDirectory(tempOutputDir.toFile()); + } + if (tempPbfFile != null && Files.exists(tempPbfFile)) { + tempPbfFile.toFile().delete(); + } + } + + // ===== infrastructure ===== + + @Test + void testOutputDatabasesExist() { + assertTrue(Files.exists(tempOutputDir.resolve("h3_to_osm")), "h3_to_osm directory should exist"); + assertTrue(Files.exists(tempOutputDir.resolve("region_metadata")), "region_metadata directory should exist"); + assertTrue(Files.exists(tempOutputDir.resolve("region_geometry")), "region_geometry directory should exist"); + assertTrue(Files.exists(tempOutputDir.resolve("osm_names.tsv")), "osm_names.tsv should exist"); + } + + @Test + void testAllGeometryValid() throws Exception { + Path geomDbPath = tempOutputDir.resolve("region_geometry"); + WKBReader wkbReader = new WKBReader(); + + try (Options opts = new Options().setCreateIfMissing(false); + RocksDB db = RocksDB.open(opts, geomDbPath.toString())) { + + var it = db.newIterator(); + it.seekToFirst(); + int checked = 0; + List invalidOsmIds = new ArrayList<>(); + + while (it.isValid()) { + long osmId = ByteBuffer.wrap(it.key()).order(ByteOrder.BIG_ENDIAN).getLong(); + byte[] wkb = it.value(); + Geometry geom = wkbReader.read(wkb); + + if (!geom.isValid()) { + invalidOsmIds.add(osmId); + } + checked++; + it.next(); + } + + System.out.println("Checked " + checked + " geometries for validity"); + assertTrue(checked > 0, "Should have at least one geometry entry"); + + if (!invalidOsmIds.isEmpty()) { + System.out.println("Invalid geometries (OSM IDs): " + invalidOsmIds); + } + assertTrue(invalidOsmIds.isEmpty(), + "All stored geometries should be valid. Invalid OSM IDs: " + invalidOsmIds); + } + } + + @Test + void testHolesAreContainedInOuterPolygons() throws Exception { + Path geomDbPath = tempOutputDir.resolve("region_geometry"); + WKBReader wkbReader = new WKBReader(); + + try (Options opts = new Options().setCreateIfMissing(false); + RocksDB db = RocksDB.open(opts, geomDbPath.toString())) { + + var it = db.newIterator(); + it.seekToFirst(); + int checked = 0; + List violations = new ArrayList<>(); + + while (it.isValid()) { + long osmId = ByteBuffer.wrap(it.key()).order(ByteOrder.BIG_ENDIAN).getLong(); + byte[] wkb = it.value(); + Geometry geom = wkbReader.read(wkb); + + violations.addAll(checkHoleContainment(osmId, geom)); + checked++; + it.next(); + } + + System.out.println("Checked " + checked + " geometries for hole containment"); + assertTrue(checked > 0, "Should have at least one geometry entry"); + + if (!violations.isEmpty()) { + for (String v : violations) { + System.out.println("VIOLATION: " + v); + } + } + assertTrue(violations.isEmpty(), + "All inner rings (holes) must be contained within their outer polygon shell. Violations:\n" + + String.join("\n", violations)); + } + } + + @Test + void testGeometryHasExpectedTopology() throws Exception { + /* + * Thuringia (OSM relation 62366) is a multipolygon: one main body plus + * exclaves. The stored WKB should be a Polygon or MultiPolygon where + * every inner ring (hole) is properly contained within an outer ring. + * + * Assertion to implement: + * 1. Load geometry for Thuringia relation by OSM ID + * 2. Verify it's a Polygon or MultiPolygon + * 3. For each polygon, verify all interior rings are within the exterior ring + * 4. Verify there are no "floating" holes (holes with no containing shell) + */ + + Path geomDbPath = tempOutputDir.resolve("region_geometry"); + long thueringenOsmId = 62366L; + WKBReader wkbReader = new WKBReader(); + + try (Options opts = new Options().setCreateIfMissing(false); + RocksDB db = RocksDB.open(opts, geomDbPath.toString())) { + + byte[] key = ByteBuffer.allocate(8).order(ByteOrder.BIG_ENDIAN).putLong(thueringenOsmId).array(); + byte[] wkb = db.get(key); + + assertNotNull(wkb, "Thuringia (OSM ID " + thueringenOsmId + ") should have stored geometry"); + assertTrue(wkb.length > 0, "Thuringia WKB should not be empty"); + + Geometry geom = wkbReader.read(wkb); + System.out.println("Thuringia geometry: type=" + geom.getGeometryType() + + " valid=" + geom.isValid() + + " numGeometries=" + geom.getNumGeometries() + + " numPoints=" + geom.getNumPoints()); + + assertTrue(geom.isValid(), "Thuringia geometry should be valid"); + assertTrue(geom instanceof Polygon || geom instanceof MultiPolygon, + "Thuringia geometry should be a Polygon or MultiPolygon, got: " + geom.getGeometryType()); + } + } + + // ===== helpers ===== + + private static final GeometryFactory GEOM_CHECK_FACTORY = new GeometryFactory(); + + /** + * Checks that every interior ring of every polygon is properly contained + * within that polygon's exterior ring. Returns a list of violation messages, + * empty if all is well. + */ + private static List checkHoleContainment(long osmId, Geometry geom) { + List violations = new ArrayList<>(); + int numParts = geom.getNumGeometries(); + for (int g = 0; g < numParts; g++) { + Geometry part = geom.getGeometryN(g); + if (!(part instanceof Polygon polygon)) + continue; + + LinearRing shell = polygon.getExteriorRing(); + Polygon shellPoly = GEOM_CHECK_FACTORY.createPolygon(shell); + for (int h = 0; h < polygon.getNumInteriorRing(); h++) { + LinearRing hole = polygon.getInteriorRingN(h); + Polygon holePoly = GEOM_CHECK_FACTORY.createPolygon(hole); + if (!shellPoly.contains(holePoly)) { + violations.add(String.format( + "OSM %d: interior ring %d is not contained within its exterior ring (hole centroid: %.6f,%.6f)", + osmId, h, hole.getCentroid().getX(), hole.getCentroid().getY())); + } + } + } + return violations; + } + + private static void deleteDirectory(java.io.File dir) { + if (dir.isDirectory()) { + java.io.File[] children = dir.listFiles(); + if (children != null) { + for (java.io.File child : children) { + deleteDirectory(child); + } + } + } + dir.delete(); + } +} diff --git a/src/test/resources/thueringen-260804.boundaries.osm.pbf b/src/test/resources/thueringen-260804.boundaries.osm.pbf new file mode 100644 index 0000000..8ef8693 Binary files /dev/null and b/src/test/resources/thueringen-260804.boundaries.osm.pbf differ