Skip to content

Commit

Permalink
Fix two bugs in the WorldEnvelope
Browse files Browse the repository at this point in the history 
 - The envelope calculation was wrong when envelope included0º (Greenwich)
 - The medianCenter was wrong when the envelope included 180º
  • Loading branch information
@t2gran @leonardehrenfried
t2gran authored and leonardehrenfried committed Mar 11, 2024
1 parent 0532203 commit 747246e
Show file tree
Hide file tree
Showing 3 changed files with 126 additions and 83 deletions.
23 changes: 11 additions & 12 deletions src/main/java/org/opentripplanner/service/worldenvelope/model/WorldEnvelope.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down Expand Up @@ -53,28 +52,28 @@ 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.
* <p>
* 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<WgsCoordinate> transitMedianCenter() {
public Optional<WgsCoordinate> medianCenter() {
return Optional.ofNullable(transitMedianCenter);
}

Expand Down
44 changes: 33 additions & 11 deletions src/main/java/org/opentripplanner/service/worldenvelope/model/WorldEnvelopeBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,23 @@ public <T> 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)));
Expand All @@ -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);
Expand Down
142 changes: 82 additions & 60 deletions src/test/java/org/opentripplanner/service/worldenvelope/model/WorldEnvelopeTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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<Arguments> 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()
);
}
}

0 comments on commit 747246e

Please sign in to comment.