From 46962f6bdb1cc9635e151d645e8e9275bc6f5389 Mon Sep 17 00:00:00 2001 From: Oliver Wipfli Date: Wed, 5 Mar 2025 16:59:40 +0100 Subject: [PATCH 1/2] roads unittests (#406) Improve unit tests for roads layer --- .../protomaps/basemap/layers/RoadsTest.java | 492 +++++++++++++++--- 1 file changed, 414 insertions(+), 78 deletions(-) diff --git a/tiles/src/test/java/com/protomaps/basemap/layers/RoadsTest.java b/tiles/src/test/java/com/protomaps/basemap/layers/RoadsTest.java index feb14a23..00e86212 100644 --- a/tiles/src/test/java/com/protomaps/basemap/layers/RoadsTest.java +++ b/tiles/src/test/java/com/protomaps/basemap/layers/RoadsTest.java @@ -10,135 +10,471 @@ import java.util.List; import java.util.Map; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; + class RoadsTest extends LayerTest { - @Test - void simple() { - assertFeatures(12, - List.of(Map.of("kind", "highway", "kind_detail", "motorway", "ref", "1", "network", "US:US", - "shield_text_length", 1)), - process(SimpleFeature.create( - newLineString(0, 0, 1, 1), - new HashMap<>(Map.of( - "layer", "1", - "highway", "motorway", - "ref", "US 1" - )), - "osm", - null, - 0 - ))); + private FeatureCollector processWith(String... arguments) { + Map tags = new HashMap<>(); + List argumentList = List.of(arguments); + if (argumentList.size() % 2 == 0) { + for (int i = 0; i < argumentList.size(); i += 2) { + tags.put(argumentList.get(i), argumentList.get(i + 1)); + } + } + return process(SimpleFeature.create( + newLineString(0, 0, 1, 1), + tags, + "osm", + null, + 0 + )); } - @Test - void relation1() { - // highway=motorway is part of a US Interstate relation and is located in the US -> minzoom should be 3 + private FeatureCollector processWithRelationAndCoords(String network, double startLon, double startLat, double endLon, + double endLat, String... arguments) { var relationResult = profile.preprocessOsmRelation(new OsmElement.Relation(1, Map.of( "type", "route", "route", "road", - "network", "US:I" + "network", network ), List.of( new OsmElement.Relation.Member(OsmElement.Type.WAY, 2, "role") ))); - FeatureCollector features = process(SimpleFeature.createFakeOsmFeature( - newLineString(-104.97235, 39.73867, -105.260503, 40.010771), // Denver - Boulder - new HashMap<>(Map.of("highway", "motorway")), + Map tags = new HashMap<>(); + List argumentList = List.of(arguments); + if (argumentList.size() % 2 == 0) { + for (int i = 0; i < argumentList.size(); i += 2) { + tags.put(argumentList.get(i), argumentList.get(i + 1)); + } + } + + return process(SimpleFeature.createFakeOsmFeature( + newLineString(startLon, startLat, endLon, endLat), + tags, "osm", null, 2, relationResult.stream().map(info -> new OsmReader.RelationMember<>("role", info)).toList() )); + } + @Test + void simple() { + assertFeatures(12, + List.of(Map.of("kind", "highway", + "kind_detail", "motorway", + "ref", "1", + "network", "US:US", + "shield_text_length", 1 + )), + processWith("layer", "1", + "highway", "motorway", + "ref", "US 1" + ) + ); + } + + @Test + void relation1() { + // highway=motorway is part of a US Interstate relation and is located in the US -> minzoom should be 3 + // Denver - Boulder assertFeatures(0, List.of(Map.of( "_minzoom", 3 )), - features + processWithRelationAndCoords("US:I", -104.97235, 39.73867, -105.260503, 40.010771, "highway", "motorway") ); } @Test void relation2() { // highway=motorway is part of US State network and is located in the US -> minzoom should be 6 - var relationResult = profile.preprocessOsmRelation(new OsmElement.Relation(1, Map.of( - "type", "route", - "route", "road", - "network", "US:US" - ), List.of( - new OsmElement.Relation.Member(OsmElement.Type.WAY, 2, "role") - ))); - - FeatureCollector features = process(SimpleFeature.createFakeOsmFeature( - newLineString(-104.97235, 39.73867, -105.260503, 40.010771), // Denver - Boulder - new HashMap<>(Map.of("highway", "motorway")), - "osm", - null, - 2, - relationResult.stream().map(info -> new OsmReader.RelationMember<>("role", info)).toList() - )); - + // Denver - Boulder assertFeatures(0, List.of(Map.of( "_minzoom", 6 )), - features + processWithRelationAndCoords("US:US", -104.97235, 39.73867, -105.260503, 40.010771, "highway", "motorway") ); } @Test void relation3() { // highway=motorway is not part of US Interstate/State network and is located in the US -> minzoom should be 7 - var relationResult = profile.preprocessOsmRelation(new OsmElement.Relation(1, Map.of( - "type", "route", - "route", "road", - "network", "some:network" - ), List.of( - new OsmElement.Relation.Member(OsmElement.Type.WAY, 2, "role") - ))); - - FeatureCollector features = process(SimpleFeature.createFakeOsmFeature( - newLineString(-104.97235, 39.73867, -105.260503, 40.010771), // Denver - Boulder - new HashMap<>(Map.of("highway", "motorway")), - "osm", - null, - 2, - relationResult.stream().map(info -> new OsmReader.RelationMember<>("role", info)).toList() - )); - + // Denver - Boulder assertFeatures(0, List.of(Map.of( "_minzoom", 7 )), - features + processWithRelationAndCoords("some:network", -104.97235, 39.73867, -105.260503, 40.010771, "highway", "motorway") ); } @Test void relation4() { // highway=motorway is part of US State network and is located ouside of the US -> minzoom should be 3 - var relationResult = profile.preprocessOsmRelation(new OsmElement.Relation(1, Map.of( - "type", "route", - "route", "road", - "network", "US:US" - ), List.of( - new OsmElement.Relation.Member(OsmElement.Type.WAY, 2, "role") - ))); - - FeatureCollector features = process(SimpleFeature.createFakeOsmFeature( - newLineString(2.424, 48.832, 8.52332, 47.36919), // Paris - Zurich - new HashMap<>(Map.of("highway", "motorway")), - "osm", - null, - 2, - relationResult.stream().map(info -> new OsmReader.RelationMember<>("role", info)).toList() - )); - + // Paris - Zurich assertFeatures(0, List.of(Map.of( "_minzoom", 3 )), - features + processWithRelationAndCoords("US:US", 2.424, 48.832, 8.52332, 47.36919, "highway", "motorway") + ); + } + + @ParameterizedTest + @CsvSource({ + "proposed", + "abandoned", + "razed", + "demolished", + "removed", + "construction", + "elevator" + }) + void testHighwayExcluded(String highway) { + assertFeatures(12, + List.of(), + processWith("highway", highway) + ); + } + + @Test + void testHighwayOther() { + assertFeatures(12, + List.of(Map.of("kind", "other", + "kind_detail", "a", + "_minzoom", 14 + )), + processWith("highway", "a") + ); + + assertFeatures(12, + List.of(Map.of("kind", "other", + "kind_detail", "b", + "_minzoom", 14 + )), + processWith("highway", "a", + "service", "b") + ); + } + + @ParameterizedTest + @CsvSource({ + "motorway, highway, motorway, 3, 7", + "motorway_link, highway, motorway_link, 3, 7", + "trunk, major_road, trunk, 6, 7", + "trunk_link, major_road, trunk_link, 6, 7", + "primary, major_road, primary, 7, 7", + "primary_link, major_road, primary_link, 7, 7", + "secondary, major_road, secondary, 9, 9", + "secondary_link, major_road, secondary_link, 9, 9", + "tertiary, major_road, tertiary, 9, 9", + "tertiary_link, major_road, tertiary_link, 9, 9", + "residential, minor_road, residential, 12, 12", + "service, minor_road, service, 13, 13", + "residential, minor_road, residential, 12, 12", + "unclassified, minor_road, unclassified, 12, 12", + "road, minor_road, road, 12, 12", + "raceway, minor_road, raceway, 12, 12", + "pedestrian, path, pedestrian, 12, 12", + "track, path, track, 12, 12", + "path, path, path, 13, 13", + "cycleway, path, cycleway, 13, 13", + "bridleway, path, bridleway, 13, 13", + "footway, path, footway, 13, 13", + "steps, path, steps, 13, 13", + "corridor, path, corridor, 14, 14", + }) + void testHighways(String highway, String kind, String kindDetail, int genericMinZoom, int usMinZoom) { + + // generic + assertFeatures(12, + List.of(Map.of("kind", kind, + "kind_detail", kindDetail, + "_minzoom", genericMinZoom + )), + processWith("highway", highway) + ); + + // US + assertFeatures(12, + List.of(Map.of("kind", kind, + "kind_detail", kindDetail, + "_minzoom", usMinZoom + )), + processWithRelationAndCoords("", + -104.97235, 39.73867, -105.260503, 40.010771, + "highway", highway + ) + ); + + // US with relation US:US + assertFeatures(12, + List.of(Map.of("kind", kind, + "kind_detail", kindDetail, + "_minzoom", 6 + )), + processWithRelationAndCoords("US:US", + -104.97235, 39.73867, -105.260503, 40.010771, + "highway", highway + ) + ); + + // US with relation US:I + assertFeatures(12, + List.of(Map.of("kind", kind, + "kind_detail", kindDetail, + "_minzoom", 3 + )), + processWithRelationAndCoords("US:I", + -104.97235, 39.73867, -105.260503, 40.010771, + "highway", highway + ) + ); + } + + @ParameterizedTest + @CsvSource({ + "pedestrian", + "track", + "path", + "cycleway", + "bridleway", + "footway", + "steps", + "corridor" + }) + void testFootways(String highway) { + assertFeatures(12, + List.of(Map.of("kind_detail", "sidewalk", + "_minzoom", 14 + )), + processWithRelationAndCoords("", + 0, 0, 1, 1, + "highway", highway, + "footway", "sidewalk" + ) + ); + + assertFeatures(12, + List.of(Map.of("kind_detail", "crossing", + "_minzoom", 14 + )), + processWithRelationAndCoords("", + 0, 0, 1, 1, + "highway", highway, + "footway", "crossing" + ) + ); + } + + @Test + void testService() { + assertFeatures(12, + List.of(Map.of("service", "a", + "_minzoom", 14 + )), + processWithRelationAndCoords("", + 0, 0, 1, 1, + "highway", "service", + "service", "a" + ) + ); + } + + @Test + void testRailway() { + assertFeatures(12, + List.of(Map.of("kind", "rail", + "kind_detail", "a", + "_minzoom", 11 + )), + processWithRelationAndCoords("", + 0, 0, 1, 1, + "railway", "a" + ) + ); + + assertFeatures(12, + List.of(Map.of("kind", "rail", + "kind_detail", "service", + "_minzoom", 13 + )), + processWithRelationAndCoords("", + 0, 0, 1, 1, + "railway", "service" + ) + ); + + assertFeatures(12, + List.of(Map.of("kind", "rail", + "kind_detail", "service", + "_minzoom", 14 + )), + processWithRelationAndCoords("", + 0, 0, 1, 1, + "railway", "service", + "service", "a" + ) + ); + } + + @ParameterizedTest + @CsvSource({ + "funicular", + "light_rail", + "miniature", + "monorail", + "narrow_gauge", + "preserved", + "subway", + "tram" + }) + void testRailwaysSpecial(String railway) { + assertFeatures(12, + List.of(Map.of("kind", "rail", + "kind_detail", railway, + "_minzoom", 14 + )), + processWithRelationAndCoords("", + 0, 0, 1, 1, + "railway", railway + ) + ); + } + + @Test + void testRailwayDisused() { + assertFeatures(12, + List.of(Map.of("kind", "rail", + "kind_detail", "disused", + "_minzoom", 15 + )), + processWithRelationAndCoords("", + 0, 0, 1, 1, + "railway", "disused" + ) + ); + } + + @Test + void testAerialwayCableCar() { + assertFeatures(12, + List.of(Map.of("kind", "aerialway", + "kind_detail", "cable_car", + "_minzoom", 11 + )), + processWithRelationAndCoords("", + 0, 0, 1, 1, + "aerialway", "cable_car" + ) + ); + } + + @Test + void testManMadePier() { + assertFeatures(12, + List.of(Map.of("kind", "path", + "kind_detail", "pier", + "_minzoom", 13 + )), + processWithRelationAndCoords("", + 0, 0, 1, 1, + "man_made", "pier" + ) + ); + } + + @Test + void testRouteFerry() { + assertFeatures(12, + List.of(Map.of("kind", "ferry", + "_minzoom", 11 + )), + processWithRelationAndCoords("", + 0, 0, 1, 1, + "route", "ferry" + ) + ); + } + + @Test + void testAerowayTaxiway() { + assertFeatures(12, + List.of(Map.of("kind", "aeroway", + "kind_detail", "taxiway", + "_minzoom", 10 + )), + processWithRelationAndCoords("", + 0, 0, 1, 1, + "aeroway", "taxiway" + ) + ); + } + + @Test + void testAerowayRunway() { + assertFeatures(12, + List.of(Map.of("kind", "aeroway", + "kind_detail", "runway", + "_minzoom", 9 + )), + processWithRelationAndCoords("", + 0, 0, 1, 1, + "aeroway", "runway" + ) + ); + } + + @Test + void testAvoidBuildings() { + assertFeatures(12, + List.of(), + processWithRelationAndCoords("", + 0, 0, 1, 1, + "aeroway", "runway", + "building", "a" + ) + ); + } + + @ParameterizedTest + @CsvSource({ + "abandoned", + "razed", + "demolished", + "removed", + "construction", + "platform", + "proposed" + }) + void testRailwayExcluded(String railway) { + assertFeatures(12, + List.of(), + processWithRelationAndCoords("", + 0, 0, 1, 1, + "railway", railway + ) + ); + } + + @ParameterizedTest + @CsvSource({ + "yard", + "siding", + "crossover" + }) + void testRailwayService(String service) { + assertFeatures(13, + List.of(Map.of("_minzoom", 13)), + processWithRelationAndCoords("", + 0, 0, 1, 1, + "railway", "a", + "service", service + ) ); } From 2eb7c51c6ddcd188edc8e44f6a725d6fdce31e41 Mon Sep 17 00:00:00 2001 From: Brandon Liu Date: Wed, 5 Mar 2025 08:14:40 -0800 Subject: [PATCH 2/2] Fix dam and grass in landuse * Add kind=dam to landuse, remove from water [#404] * fix natural=grassland in landuse [#405] * Suppress sonar cloud duplicated string literals warning on index * tiles 4.6.0 --------- Co-authored-by: Oliver Wipfli --- CHANGELOG.md | 5 +++++ styles/src/base_layers.ts | 2 +- .../main/java/com/protomaps/basemap/Basemap.java | 2 +- .../com/protomaps/basemap/layers/Landuse.java | 6 +++++- .../java/com/protomaps/basemap/layers/Water.java | 5 +++-- .../com/protomaps/basemap/layers/LanduseTest.java | 9 +++++++-- .../com/protomaps/basemap/layers/WaterTest.java | 15 +++++++++++++++ 7 files changed, 37 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 427267aa..f12f1880 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,8 @@ +Tiles 4.6.0 +------ +- Symbolize kind=dam in landuse layer instead of waterway layer [#404] +- Fix kind=grassland [#405] + Tiles 4.5.0 ------ - Refactor of landuse to use Matchers [#399] diff --git a/styles/src/base_layers.ts b/styles/src/base_layers.ts index d7ee3f50..72127faf 100644 --- a/styles/src/base_layers.ts +++ b/styles/src/base_layers.ts @@ -299,7 +299,7 @@ export function nolabels_layers( type: "fill", source: source, "source-layer": "landuse", - filter: ["==", "kind", "pedestrian"], + filter: ["in", "kind", "pedestrian", "dam"], paint: { "fill-color": t.pedestrian, }, diff --git a/tiles/src/main/java/com/protomaps/basemap/Basemap.java b/tiles/src/main/java/com/protomaps/basemap/Basemap.java index 758fd046..d9cb18df 100644 --- a/tiles/src/main/java/com/protomaps/basemap/Basemap.java +++ b/tiles/src/main/java/com/protomaps/basemap/Basemap.java @@ -115,7 +115,7 @@ public String description() { @Override public String version() { - return "4.5.0"; + return "4.6.0"; } @Override diff --git a/tiles/src/main/java/com/protomaps/basemap/layers/Landuse.java b/tiles/src/main/java/com/protomaps/basemap/layers/Landuse.java index 95e9f4b9..d5be1c0f 100644 --- a/tiles/src/main/java/com/protomaps/basemap/layers/Landuse.java +++ b/tiles/src/main/java/com/protomaps/basemap/layers/Landuse.java @@ -128,7 +128,7 @@ public class Landuse implements ForwardingProfile.LayerPostProcessor { beach wood glacier - grass + grassland scrub sand wetland @@ -145,6 +145,10 @@ public class Landuse implements ForwardingProfile.LayerPostProcessor { with("area", "yes"), use("kind", "pedestrian") ), + rule( + with("waterway", "dam"), + use("kind", "dam") + ), rule( with("railway", "platform"), use("kind", "platform") diff --git a/tiles/src/main/java/com/protomaps/basemap/layers/Water.java b/tiles/src/main/java/com/protomaps/basemap/layers/Water.java index 9da55794..0f34174b 100644 --- a/tiles/src/main/java/com/protomaps/basemap/layers/Water.java +++ b/tiles/src/main/java/com/protomaps/basemap/layers/Water.java @@ -13,6 +13,7 @@ import com.protomaps.basemap.names.OsmNames; import java.util.List; +@SuppressWarnings("java:S1192") // Duplicated string literals public class Water implements ForwardingProfile.LayerPostProcessor { private static final double WORLD_AREA_FOR_70K_SQUARE_METERS = @@ -109,7 +110,7 @@ public void processNe(SourceFeature sf, FeatureCollector features) { public void processOsm(SourceFeature sf, FeatureCollector features) { // polygons if (sf.canBePolygon() && (sf.hasTag("water") || - sf.hasTag("waterway") || + (sf.hasTag("waterway") && !sf.hasTag("waterway", "dam")) || sf.hasTag("natural", "water") || sf.hasTag("landuse", "reservoir") || sf.hasTag("leisure", "swimming_pool"))) { @@ -191,7 +192,7 @@ public void processOsm(SourceFeature sf, FeatureCollector features) { // lines if (sf.canBeLine() && !sf.canBePolygon() && sf.hasTag("waterway") && - (!sf.hasTag("waterway", "riverbank", "reservoir"))) { + (!sf.hasTag("waterway", "riverbank", "reservoir", "dam"))) { int minZoom = 12; String kind = "other"; if (sf.hasTag("waterway")) { diff --git a/tiles/src/test/java/com/protomaps/basemap/layers/LanduseTest.java b/tiles/src/test/java/com/protomaps/basemap/layers/LanduseTest.java index f0df25ce..f0e0aca3 100644 --- a/tiles/src/test/java/com/protomaps/basemap/layers/LanduseTest.java +++ b/tiles/src/test/java/com/protomaps/basemap/layers/LanduseTest.java @@ -214,8 +214,13 @@ void testFromTagNatural() { ); assertFeatures(15, - List.of(Map.of("kind", "grass")), - processWith("natural", "grass") + List.of(Map.of("kind", "grassland")), + processWith("natural", "grassland") + ); + + assertFeatures(15, + List.of(Map.of("kind", "dam")), + processWith("waterway", "dam") ); assertFeatures(15, diff --git a/tiles/src/test/java/com/protomaps/basemap/layers/WaterTest.java b/tiles/src/test/java/com/protomaps/basemap/layers/WaterTest.java index 70ed3972..fe0dfef6 100644 --- a/tiles/src/test/java/com/protomaps/basemap/layers/WaterTest.java +++ b/tiles/src/test/java/com/protomaps/basemap/layers/WaterTest.java @@ -63,6 +63,21 @@ void kindRiver() { ))); } + @Test + void kindDam() { + assertFeatures(15, + List.of(), + process(SimpleFeature.create( + newLineString(0, 0, 1, 1), + new HashMap<>(Map.of( + "waterway", "dam" + )), + "osm", + null, + 0 + ))); + } + @Test void oceanLabel() { assertFeatures(12,