From d378521372cde0125faa8da3cb82ca126e7fb34e Mon Sep 17 00:00:00 2001 From: Peter Hanecak <115141505+phanecak-maptiler@users.noreply.github.com> Date: Mon, 5 Dec 2022 12:48:14 +0100 Subject: [PATCH] fix for issue #18: adm0_l and adm0_r at z4 (#53) * testCountryLeftRightName() extended to cover adm0_{l,r} not showing up at Z4 * fix for #18 + alignment with OMT: adm0_{l,r} only for non-disputed and only from z5 * clean-up: if-else simplified since same for z4 * other tests updated to match adm0_{l,r} change --- .../org/openmaptiles/layers/Boundary.java | 12 +++- .../org/openmaptiles/layers/BoundaryTest.java | 67 +++++++++++++------ 2 files changed, 57 insertions(+), 22 deletions(-) diff --git a/src/main/java/org/openmaptiles/layers/Boundary.java b/src/main/java/org/openmaptiles/layers/Boundary.java index f566687..a506e51 100644 --- a/src/main/java/org/openmaptiles/layers/Boundary.java +++ b/src/main/java/org/openmaptiles/layers/Boundary.java @@ -312,16 +312,22 @@ public class Boundary implements BorderingRegions borderingRegions = getBorderingRegions(countryBoundaries, key.regions, lineString); var features = featureCollectors.get(SimpleFeature.fromWorldGeometry(lineString)); - features.line(LAYER_NAME).setBufferPixels(BUFFER_SIZE) + var newFeature = features.line(LAYER_NAME).setBufferPixels(BUFFER_SIZE) .setAttr(Fields.ADMIN_LEVEL, key.adminLevel) .setAttr(Fields.DISPUTED, key.disputed ? 1 : 0) .setAttr(Fields.MARITIME, key.maritime ? 1 : 0) .setAttr(Fields.CLAIMED_BY, key.claimedBy) .setAttr(Fields.DISPUTED_NAME, key.disputed ? editName(key.name) : null) - .setAttr(Fields.ADM0_L, borderingRegions.left == null ? null : regionNames.get(borderingRegions.left)) - .setAttr(Fields.ADM0_R, borderingRegions.right == null ? null : regionNames.get(borderingRegions.right)) .setMinPixelSizeAtAllZooms(0) .setMinZoom(key.minzoom); + if (key.adminLevel == 2 && !key.disputed) { + // only non-disputed admin 2 boundaries get to have adm0_{l,r}, at zoom 5 and more + newFeature + .setAttrWithMinzoom(Fields.ADM0_L, + borderingRegions.left == null ? null : regionNames.get(borderingRegions.left), 5) + .setAttrWithMinzoom(Fields.ADM0_R, + borderingRegions.right == null ? null : regionNames.get(borderingRegions.right), 5); + } for (var feature : features) { emit.accept(feature); } diff --git a/src/test/java/org/openmaptiles/layers/BoundaryTest.java b/src/test/java/org/openmaptiles/layers/BoundaryTest.java index 310d3b6..4274b69 100644 --- a/src/test/java/org/openmaptiles/layers/BoundaryTest.java +++ b/src/test/java/org/openmaptiles/layers/BoundaryTest.java @@ -398,8 +398,7 @@ class BoundaryTest extends AbstractLayerTest { )); } - @Test - void testCountryLeftRightName() { + private List setupCountryLeftRightNameTest(Map tags) { var country1 = new OsmElement.Relation(1); country1.setTag("type", "boundary"); country1.setTag("admin_level", "2"); @@ -414,7 +413,7 @@ class BoundaryTest extends AbstractLayerTest { // shared edge assertFeatures(14, List.of(), process(SimpleFeature.createFakeOsmFeature( newLineString(0, 0, 0, 10), - Map.of(), + tags, OpenMapTilesProfile.OSM_SOURCE, null, 3, @@ -428,7 +427,7 @@ class BoundaryTest extends AbstractLayerTest { // other 2 edges of country 1 assertFeatures(14, List.of(), process(SimpleFeature.createFakeOsmFeature( newLineString(0, 0, 5, 10), - Map.of(), + tags, OpenMapTilesProfile.OSM_SOURCE, null, 4, @@ -438,7 +437,7 @@ class BoundaryTest extends AbstractLayerTest { )); assertFeatures(14, List.of(), process(SimpleFeature.createFakeOsmFeature( newLineString(0, 10, 5, 10), - Map.of(), + tags, OpenMapTilesProfile.OSM_SOURCE, null, 4, @@ -450,7 +449,7 @@ class BoundaryTest extends AbstractLayerTest { // other 2 edges of country 2 assertFeatures(14, List.of(), process(SimpleFeature.createFakeOsmFeature( newLineString(0, 0, -5, 10), - Map.of(), + tags, OpenMapTilesProfile.OSM_SOURCE, null, 4, @@ -460,7 +459,7 @@ class BoundaryTest extends AbstractLayerTest { )); assertFeatures(14, List.of(), process(SimpleFeature.createFakeOsmFeature( newLineString(0, 10, -5, 10), - Map.of(), + tags, OpenMapTilesProfile.OSM_SOURCE, null, 4, @@ -471,34 +470,64 @@ class BoundaryTest extends AbstractLayerTest { List features = new ArrayList<>(); profile.finish(OpenMapTilesProfile.OSM_SOURCE, new FeatureCollector.Factory(params, stats), features::add); + + return features; + } + + @Test + void testCountryLeftRightName() { + List features = setupCountryLeftRightNameTest(Map.of()); assertEquals(3, features.size()); - // ensure shared edge has country labels on right sides + // ensure shared edge has country labels on right sides, from z5 var sharedEdge = features.stream() - .filter(c -> c.getAttrsAtZoom(0).containsKey("adm0_l") && c.getAttrsAtZoom(0).containsKey("adm0_r")).findFirst() + .filter(c -> c.getAttrsAtZoom(5).containsKey("adm0_l") && c.getAttrsAtZoom(5).containsKey("adm0_r")).findFirst() .get(); if (sharedEdge.getGeometry().getCoordinate().y == 0.5) { // going up - assertEquals("C1", sharedEdge.getAttrsAtZoom(0).get("adm0_r")); - assertEquals("C2", sharedEdge.getAttrsAtZoom(0).get("adm0_l")); + assertEquals("C1", sharedEdge.getAttrsAtZoom(5).get("adm0_r")); + assertEquals("C2", sharedEdge.getAttrsAtZoom(5).get("adm0_l")); } else { // going down - assertEquals("C2", sharedEdge.getAttrsAtZoom(0).get("adm0_r")); - assertEquals("C1", sharedEdge.getAttrsAtZoom(0).get("adm0_l")); + assertEquals("C2", sharedEdge.getAttrsAtZoom(5).get("adm0_r")); + assertEquals("C1", sharedEdge.getAttrsAtZoom(5).get("adm0_l")); } var c1 = features.stream() .filter(c -> c.getGeometry().getEnvelopeInternal().getMaxX() > 0.5).findFirst() .get(); if (c1.getGeometry().getCoordinate().y == 0.5) { // going up - assertEquals("C1", c1.getAttrsAtZoom(0).get("adm0_l")); + assertEquals("C1", c1.getAttrsAtZoom(5).get("adm0_l")); } else { // going down - assertEquals("C1", c1.getAttrsAtZoom(0).get("adm0_r")); + assertEquals("C1", c1.getAttrsAtZoom(5).get("adm0_r")); } var c2 = features.stream() .filter(c -> c.getGeometry().getEnvelopeInternal().getMinX() < 0.5).findFirst() .get(); if (c2.getGeometry().getCoordinate().y == 0.5) { // going up - assertEquals("C2", c2.getAttrsAtZoom(0).get("adm0_r")); + assertEquals("C2", c2.getAttrsAtZoom(5).get("adm0_r")); } else { // going down - assertEquals("C2", c2.getAttrsAtZoom(0).get("adm0_l")); + assertEquals("C2", c2.getAttrsAtZoom(5).get("adm0_l")); + } + + // but not at z4, see https://github.com/openmaptiles/planetiler-openmaptiles/issues/18 + assertNull(sharedEdge.getAttrsAtZoom(4).get("adm0_r")); + assertNull(sharedEdge.getAttrsAtZoom(4).get("adm0_l")); + } + + @Test + void testCountryLeftRightNameDisputed() { + Map tags = Map.of("disputed", 1); + List features = setupCountryLeftRightNameTest(tags); + assertEquals(3, features.size()); + + // ensure shared edge does not have country labels at z5 ... + for (var feature : features) { + assertNull(feature.getAttrsAtZoom(5).get("adm0_r")); + assertNull(feature.getAttrsAtZoom(5).get("adm0_l")); + } + + // ... and not even on z4 + for (var feature : features) { + assertNull(feature.getAttrsAtZoom(4).get("adm0_r")); + assertNull(feature.getAttrsAtZoom(4).get("adm0_l")); } } @@ -563,7 +592,7 @@ class BoundaryTest extends AbstractLayerTest { List features = new ArrayList<>(); profile.finish(OpenMapTilesProfile.OSM_SOURCE, new FeatureCollector.Factory(params, stats), features::add); - assertFeatures(0, List.of(Map.of( + assertFeatures(5, List.of(Map.of( "adm0_l", "C1", "adm0_r", "" ), Map.of( @@ -628,7 +657,7 @@ class BoundaryTest extends AbstractLayerTest { List features = new ArrayList<>(); profile.finish(OpenMapTilesProfile.OSM_SOURCE, new FeatureCollector.Factory(params, stats), features::add); - assertFeatures(0, List.of(Map.of( + assertFeatures(5, List.of(Map.of( "adm0_l", "", "adm0_r", "" ), Map.of(