From 747246e79218fca148f9975e414e69b8d7988792 Mon Sep 17 00:00:00 2001 From: Thomas Gran Date: Fri, 8 Mar 2024 17:46:32 +0100 Subject: [PATCH] Fix two bugs in the WorldEnvelope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The envelope calculation was wrong when envelope included0º (Greenwich) - The medianCenter was wrong when the envelope included 180º --- .../worldenvelope/model/WorldEnvelope.java | 23 ++- .../model/WorldEnvelopeBuilder.java | 44 ++++-- .../model/WorldEnvelopeTest.java | 142 ++++++++++-------- 3 files changed, 126 insertions(+), 83 deletions(-) diff --git a/src/main/java/org/opentripplanner/service/worldenvelope/model/WorldEnvelope.java b/src/main/java/org/opentripplanner/service/worldenvelope/model/WorldEnvelope.java index 8d211fd33db..27a1fb4d30f 100644 --- a/src/main/java/org/opentripplanner/service/worldenvelope/model/WorldEnvelope.java +++ b/src/main/java/org/opentripplanner/service/worldenvelope/model/WorldEnvelope.java @@ -6,8 +6,7 @@ import org.opentripplanner.framework.tostring.ToStringBuilder; /** - * This class calculates borders of envelopes that can be also on 180th meridian The same way as it - * was previously calculated in GraphMetadata constructor + * This class calculates borders of envelopes that can be also on 180th meridian. */ public class WorldEnvelope implements Serializable { @@ -53,14 +52,6 @@ public WgsCoordinate upperRight() { return upperRight; } - /** - * This is the center of the Envelope including both street vertexes and transit stops - * if they exist. - */ - public WgsCoordinate meanCenter() { - return meanCenter; - } - /** * If transit data exist, then this is the median center of the transit stops. The median * is computed independently for the longitude and latitude. @@ -68,13 +59,21 @@ public WgsCoordinate meanCenter() { * If not transit data exist this return `empty`. */ public WgsCoordinate center() { - return transitMedianCenter().orElse(meanCenter); + return medianCenter().orElse(meanCenter); + } + + /** + * This is the center of the Envelope including both street vertexes and transit stops + * if they exist. + */ + public WgsCoordinate meanCenter() { + return meanCenter; } /** * Return the transit median center [if it exist] or the mean center. */ - public Optional transitMedianCenter() { + public Optional medianCenter() { return Optional.ofNullable(transitMedianCenter); } diff --git a/src/main/java/org/opentripplanner/service/worldenvelope/model/WorldEnvelopeBuilder.java b/src/main/java/org/opentripplanner/service/worldenvelope/model/WorldEnvelopeBuilder.java index abddba9c5fd..7c5bf13d5e5 100644 --- a/src/main/java/org/opentripplanner/service/worldenvelope/model/WorldEnvelopeBuilder.java +++ b/src/main/java/org/opentripplanner/service/worldenvelope/model/WorldEnvelopeBuilder.java @@ -58,8 +58,23 @@ public WorldEnvelopeBuilder expandToIncludeTransitEntities( var medianCalculator = new MedianCalcForDoubles(collection.size()); - collection.forEach(v -> medianCalculator.add(lonProvider.apply(v))); - double lon = medianCalculator.median(); + double lon = 0.0; + if (includeLongitude180()) { + collection.forEach(v -> { + double c = lonProvider.apply(v); + if (c < 0) { + c += 360.0; + } + medianCalculator.add(c); + }); + lon = medianCalculator.median(); + if (lon > 180.0) { + lon -= 180; + } + } else { + collection.forEach(v -> medianCalculator.add(lonProvider.apply(v))); + lon = medianCalculator.median(); + } medianCalculator.reset(); collection.forEach(v -> medianCalculator.add(latProvider.apply(v))); @@ -79,19 +94,26 @@ public WorldEnvelope build() { if (minLonEast == MIN_NOT_SET) { return new WorldEnvelope(minLat, minLonWest, maxLat, maxLonWest, transitMedianCenter); } - // Envelope intersects with either 0º or 180º - double dist0 = minLonEast - minLonWest; - double dist180 = 360d - maxLonEast + minLonWest; - - // A small gap between the east and west longitude at 0 degrees implies that the Envelope - // should include the 0 degrees longitude(meridian), and be split at 180 degrees. - if (dist0 < dist180) { - return new WorldEnvelope(minLat, maxLonWest, maxLat, maxLonEast, transitMedianCenter); - } else { + if (includeLongitude180()) { return new WorldEnvelope(minLat, minLonEast, maxLat, minLonWest, transitMedianCenter); + } else { + return new WorldEnvelope(minLat, minLonWest, maxLat, maxLonEast, transitMedianCenter); } } + /** + * A small gap between the east and west longitude at 180º degrees implies that the Envelope + * should include the 180º longitude, and be split at 0 degrees. + */ + boolean includeLongitude180() { + if (minLonWest == MIN_NOT_SET || minLonEast == MIN_NOT_SET) { + return false; + } + double dist0 = minLonEast - minLonWest; + double dist180 = 360d - maxLonEast + minLonWest; + return dist180 < dist0; + } + private WorldEnvelopeBuilder expandToInclude(double latitude, double longitude) { minLat = Math.min(minLat, latitude); maxLat = Math.max(maxLat, latitude); diff --git a/src/test/java/org/opentripplanner/service/worldenvelope/model/WorldEnvelopeTest.java b/src/test/java/org/opentripplanner/service/worldenvelope/model/WorldEnvelopeTest.java index bff99800fdb..136caf31a5b 100644 --- a/src/test/java/org/opentripplanner/service/worldenvelope/model/WorldEnvelopeTest.java +++ b/src/test/java/org/opentripplanner/service/worldenvelope/model/WorldEnvelopeTest.java @@ -5,6 +5,9 @@ import java.util.List; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import org.opentripplanner.framework.geometry.WgsCoordinate; class WorldEnvelopeTest { @@ -18,83 +21,102 @@ class WorldEnvelopeTest { private static final int W60 = -60; private static final int W170 = -170; - private static final WorldEnvelope EAST = WorldEnvelope - .of() - .expandToIncludeStreetEntities(S10, E50) - .expandToIncludeStreetEntities(S20, E160) - .build(); - private static final WorldEnvelope WEST = WorldEnvelope - .of() - .expandToIncludeStreetEntities(N30, W60) - .expandToIncludeStreetEntities(N40, W170) - .build(); - private static final WorldEnvelope GREENWICH = WorldEnvelope - .of() - .expandToIncludeStreetEntities(N30, W60) - .expandToIncludeStreetEntities(S10, E50) - .build(); - private static final WorldEnvelope MERIDIAN_180 = WorldEnvelope - .of() - .expandToIncludeStreetEntities(N40, W170) - .expandToIncludeStreetEntities(S20, E160) - .build(); - - @Test - void testEast() { - var expectedCenter = new WgsCoordinate(-15d, 105d); - - assertEquals(S20, EAST.lowerLeft().latitude()); - assertEquals(E50, EAST.lowerLeft().longitude()); - assertEquals(S10, EAST.upperRight().latitude()); - assertEquals(E160, EAST.upperRight().longitude()); - assertEquals(expectedCenter, EAST.meanCenter()); - assertEquals(expectedCenter, EAST.center()); - assertTrue(EAST.transitMedianCenter().isEmpty()); + /** + * To make sure we cover all cases we add a case for each combination of: + * - latitude + * - south hemisphere + * - north hemisphere + * - both sides of the equator + * - longitude + * - east side of 0º (Greenwich) + * - west side of 0º + * - both sides of 0º + * - both sides of 180º + * Skip cases for North- and South-pole - not relevant - obscure cases) + */ + static List testCases() { + return List.of( + // name, lower-lat, left-lon, upper-lat, right-lon, center-lat, center-lon + Arguments.of("South-East", S20, E50, S10, E160, -15d, 105d), + Arguments.of("Equator-East", S10, E50, N30, E160, 10d, 105d), + Arguments.of("North-East", N30, E50, N40, E160, 35d, 105d), + Arguments.of("South-West", S20, W170, S10, W60, -15d, -115d), + Arguments.of("Equator-West", S10, W170, N30, W60, 10d, -115d), + Arguments.of("North-West", N30, W170, N40, W60, 35d, -115d), + Arguments.of("North-Greenwich", N30, W60, N40, E50, 35d, -5d), + Arguments.of("Equator-Greenwich", S10, W60, N30, E50, 10d, -5d), + Arguments.of("South-Greenwich", S20, W60, S10, E50, -15d, -5d), + Arguments.of("North-180º", N30, E160, N40, W170, 35d, 175d), + Arguments.of("Equator-180º", S10, E160, N30, W170, 10d, 175d), + Arguments.of("South-180º", S20, E160, S10, W170, -15d, 175d) + ); } - @Test - void transitMedianCenter() { - var expectedCenter = new WgsCoordinate(S10, E50); + @ParameterizedTest + @MethodSource("testCases") + void testWorldEnvelope( + String name, + double lowerLat, + double leftLon, + double upperLat, + double rightLon, + double centerLat, + double centerLon + ) { + // Add a point close to the center + var median = new WgsCoordinate(centerLat + 1.0, centerLon + 1.0); - var subject = WorldEnvelope + // WorldEnvelope should normalize to lower-left and upper-right + // Add lower-right & upper-left the world-envelope + var subjectWithoutMedian = WorldEnvelope + .of() + .expandToIncludeStreetEntities(lowerLat, rightLon) + .expandToIncludeStreetEntities(upperLat, leftLon) + .build(); + // Add the ~middle point between each corner of the envelope + median point + // We offset the one center value to the "other" side of the median by adding 2.0 + var subjectWithMedian = WorldEnvelope .of() .expandToIncludeTransitEntities( List.of( - new WgsCoordinate(S10, E50), - new WgsCoordinate(S20, E160), - new WgsCoordinate(N40, W60) + new WgsCoordinate(upperLat, centerLon), + new WgsCoordinate(lowerLat, centerLon + 2d), + new WgsCoordinate(centerLat, rightLon), + new WgsCoordinate(centerLat + 2d, leftLon), + median ), WgsCoordinate::latitude, WgsCoordinate::longitude ) .build(); - assertTrue(subject.transitMedianCenter().isPresent(), subject.transitMedianCenter().toString()); - assertEquals(expectedCenter, subject.transitMedianCenter().get()); - assertEquals(expectedCenter, subject.center()); - assertEquals( - "WorldEnvelope{lowerLeft: (-20.0, -60.0), upperRight: (40.0, 160.0), meanCenter: (10.0, 50.0), transitMedianCenter: (-10.0, 50.0)}", - subject.toString() + for (WorldEnvelope subject : List.of(subjectWithoutMedian, subjectWithMedian)) { + assertEquals(lowerLat, subject.lowerLeft().latitude(), name + " lower-latitude"); + assertEquals(leftLon, subject.lowerLeft().longitude(), name + " left-longitude"); + assertEquals(upperLat, subject.upperRight().latitude(), name + " upper-latitude"); + assertEquals(rightLon, subject.upperRight().longitude(), name + " right-longitude"); + assertEquals(centerLat, subject.meanCenter().latitude(), name + " center-latitude"); + assertEquals(centerLon, subject.meanCenter().longitude(), name + " center-longitude"); + } + + assertTrue( + subjectWithoutMedian.medianCenter().isEmpty(), + "First envelope does not have a median" ); + assertTrue(subjectWithMedian.medianCenter().isPresent(), "Second envelope does have a median"); + assertEquals(median, subjectWithMedian.medianCenter().get(), name + " median"); } @Test - void testToString() { - assertEquals( - "WorldEnvelope{lowerLeft: (-20.0, 50.0), upperRight: (-10.0, 160.0), meanCenter: (-15.0, 105.0)}", - EAST.toString() - ); - assertEquals( - "WorldEnvelope{lowerLeft: (30.0, -170.0), upperRight: (40.0, -60.0), meanCenter: (35.0, -115.0)}", - WEST.toString() - ); - assertEquals( - "WorldEnvelope{lowerLeft: (-10.0, -60.0), upperRight: (30.0, 50.0), meanCenter: (10.0, -5.0)}", - GREENWICH.toString() - ); + void testWorldEnvelopeToString() { assertEquals( - "WorldEnvelope{lowerLeft: (-20.0, 160.0), upperRight: (40.0, -170.0), meanCenter: (10.0, 175.0)}", - MERIDIAN_180.toString() + "WorldEnvelope{lowerLeft: (-10.0, -60.0), upperRight: (40.0, 50.0), meanCenter: (15.0, -5.0)}", + WorldEnvelope + .of() + .expandToIncludeStreetEntities(S10, E50) + .expandToIncludeStreetEntities(N40, W60) + .build() + .toString() ); } }