Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[core] Tighten LatLng and other geo.hpp class invariants #5286

Merged
merged 8 commits into from
Apr 6, 2017
Merged

[core] Tighten LatLng and other geo.hpp class invariants #5286

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9a41995
[darwin] Fix invalid coordinates in MGLShapeSource tests
jfirebaugh Apr 3, 2017
b47e2af
[core] Tighten LatLng and other geo.hpp classes
jfirebaugh Mar 17, 2016
ac7d57b
[darwin] Use MGLLatLngFromLocationCoordinate2D where appropriate
jfirebaugh Apr 3, 2017
c456679
[darwin] Preflight argument validity before constructing mbgl::LatLng
jfirebaugh Apr 3, 2017
1586ffa
[android] Preflight argument validity before constructing LatLng
jfirebaugh Apr 3, 2017
b19e9c0
[all] Make LatLng coordinates read-only
jfirebaugh Apr 4, 2017
11e0c1c
[ios, macos] Streamlined coordinate validation
1ec5 Apr 6, 2017
ea20dd5
[ios, macos] Fail gracefully on invalid coordinates
1ec5 Apr 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 59 additions & 54 deletions include/mbgl/util/geo.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include <mapbox/geometry/box.hpp>

#include <cmath>
#include <stdexcept>

namespace mbgl {

Expand All @@ -20,50 +21,62 @@ using ScreenLineString = mapbox::geometry::line_string<double>;
using ScreenBox = mapbox::geometry::box<double>;

class LatLng {
public:
struct null {};

double latitude;
double longitude;
private:
double lat;
double lon;

public:
enum WrapMode : bool { Unwrapped, Wrapped };

LatLng(null) : latitude(std::numeric_limits<double>::quiet_NaN()), longitude(latitude) {}

LatLng(double lat = 0, double lon = 0, WrapMode mode = Unwrapped)
: latitude(lat), longitude(lon) { if (mode == Wrapped) wrap(); }

LatLng wrapped() const { return { latitude, longitude, Wrapped }; }
LatLng(double lat_ = 0, double lon_ = 0, WrapMode mode = Unwrapped)
: lat(lat_), lon(lon_) {
if (std::isnan(lat)) {
throw std::domain_error("latitude must not be NaN");
}
if (std::isnan(lon)) {
throw std::domain_error("longitude must not be NaN");
}
if (std::abs(lat) > 90.0) {
throw std::domain_error("latitude must be between -90 and 90");
}
if (!std::isfinite(lon)) {
throw std::domain_error("longitude must not be infinite");
}
if (mode == Wrapped) {
wrap();
}
}

double latitude() const { return lat; }
double longitude() const { return lon; }

LatLng wrapped() const { return { lat, lon, Wrapped }; }

void wrap() {
longitude = util::wrap(longitude, -util::LONGITUDE_MAX, util::LONGITUDE_MAX);
lon = util::wrap(lon, -util::LONGITUDE_MAX, util::LONGITUDE_MAX);
}

// If the distance from start to end longitudes is between half and full
// world, unwrap the start longitude to ensure the shortest path is taken.
void unwrapForShortestPath(const LatLng& end) {
const double delta = std::abs(end.longitude - longitude);
const double delta = std::abs(end.lon - lon);
if (delta < util::LONGITUDE_MAX || delta > util::DEGREES_MAX) return;
if (longitude > 0 && end.longitude < 0) longitude -= util::DEGREES_MAX;
else if (longitude < 0 && end.longitude > 0) longitude += util::DEGREES_MAX;
}

explicit operator bool() const {
return !(std::isnan(latitude) || std::isnan(longitude));
if (lon > 0 && end.lon < 0) lon -= util::DEGREES_MAX;
else if (lon < 0 && end.lon > 0) lon += util::DEGREES_MAX;
}

// Constructs a LatLng object with the top left position of the specified tile.
LatLng(const CanonicalTileID& id);
LatLng(const UnwrappedTileID& id);
};

constexpr bool operator==(const LatLng& a, const LatLng& b) {
return a.latitude == b.latitude && a.longitude == b.longitude;
}
friend constexpr bool operator==(const LatLng& a, const LatLng& b) {
return a.lat == b.lat && a.lon == b.lon;
}

constexpr bool operator!=(const LatLng& a, const LatLng& b) {
return !(a == b);
}
friend constexpr bool operator!=(const LatLng& a, const LatLng& b) {
return !(a == b);
}
};

class ProjectedMeters {
public:
Expand All @@ -72,10 +85,6 @@ class ProjectedMeters {

ProjectedMeters(double n = 0, double e = 0)
: northing(n), easting(e) {}

explicit operator bool() const {
return !(std::isnan(northing) || std::isnan(easting));
}
};

constexpr bool operator==(const ProjectedMeters& a, const ProjectedMeters& b) {
Expand Down Expand Up @@ -111,26 +120,26 @@ class LatLngBounds {
// Constructs a LatLngBounds object with the tile's exact boundaries.
LatLngBounds(const CanonicalTileID&);

double south() const { return sw.latitude; }
double west() const { return sw.longitude; }
double north() const { return ne.latitude; }
double east() const { return ne.longitude; }
double south() const { return sw.latitude(); }
double west() const { return sw.longitude(); }
double north() const { return ne.latitude(); }
double east() const { return ne.longitude(); }

LatLng southwest() const { return sw; }
LatLng northeast() const { return ne; }
LatLng southeast() const { return LatLng(south(), east()); }
LatLng northwest() const { return LatLng(north(), west()); }

LatLng center() const {
return LatLng((sw.latitude + ne.latitude) / 2,
(sw.longitude + ne.longitude) / 2);
return LatLng((sw.latitude() + ne.latitude()) / 2,
(sw.longitude() + ne.longitude()) / 2);
}

void extend(const LatLng& point) {
if (point.latitude < sw.latitude) sw.latitude = point.latitude;
if (point.latitude > ne.latitude) ne.latitude = point.latitude;
if (point.longitude < sw.longitude) sw.longitude = point.longitude;
if (point.longitude > ne.longitude) ne.longitude = point.longitude;
sw = LatLng(std::min(point.latitude(), sw.latitude()),
std::min(point.longitude(), sw.longitude()));
ne = LatLng(std::max(point.latitude(), ne.latitude()),
std::max(point.longitude(), ne.longitude()));
}

void extend(const LatLngBounds& bounds) {
Expand All @@ -139,22 +148,22 @@ class LatLngBounds {
}

bool isEmpty() const {
return sw.latitude > ne.latitude ||
sw.longitude > ne.longitude;
return sw.latitude() > ne.latitude() ||
sw.longitude() > ne.longitude();
}

bool contains(const LatLng& point) const {
return (point.latitude >= sw.latitude &&
point.latitude <= ne.latitude &&
point.longitude >= sw.longitude &&
point.longitude <= ne.longitude);
return (point.latitude() >= sw.latitude() &&
point.latitude() <= ne.latitude() &&
point.longitude() >= sw.longitude() &&
point.longitude() <= ne.longitude());
}

bool intersects(const LatLngBounds area) const {
return (area.ne.latitude > sw.latitude &&
area.sw.latitude < ne.latitude &&
area.ne.longitude > sw.longitude &&
area.sw.longitude < ne.longitude);
return (area.ne.latitude() > sw.latitude() &&
area.sw.latitude() < ne.latitude() &&
area.ne.longitude() > sw.longitude() &&
area.sw.longitude() < ne.longitude());
}

private:
Expand Down Expand Up @@ -197,10 +206,6 @@ class EdgeInsets {
EdgeInsets(const double t, const double l, const double b, const double r)
: top(t), left(l), bottom(b), right(r) {}

explicit operator bool() const {
return !(std::isnan(top) || std::isnan(left) || std::isnan(bottom) || std::isnan(right))
&& (top || left || bottom || right);
}

void operator+=(const EdgeInsets& o) {
top += o.top;
Expand Down
8 changes: 4 additions & 4 deletions include/mbgl/util/projection.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ class Projection {
}

static ProjectedMeters projectedMetersForLatLng(const LatLng& latLng) {
const double constrainedLatitude = util::clamp(latLng.latitude, -util::LATITUDE_MAX, util::LATITUDE_MAX);
const double constrainedLongitude = util::clamp(latLng.longitude, -util::LONGITUDE_MAX, util::LONGITUDE_MAX);
const double constrainedLatitude = util::clamp(latLng.latitude(), -util::LATITUDE_MAX, util::LATITUDE_MAX);
const double constrainedLongitude = util::clamp(latLng.longitude(), -util::LONGITUDE_MAX, util::LONGITUDE_MAX);

const double m = 1 - 1e-15;
const double f = util::clamp(std::sin(util::DEG2RAD * constrainedLatitude), -m, m);
Expand All @@ -49,8 +49,8 @@ class Projection {

static Point<double> project(const LatLng& latLng, double scale) {
return Point<double> {
util::LONGITUDE_MAX + latLng.longitude,
util::LONGITUDE_MAX - util::RAD2DEG * std::log(std::tan(M_PI / 4 + latLng.latitude * M_PI / util::DEGREES_MAX))
util::LONGITUDE_MAX + latLng.longitude(),
util::LONGITUDE_MAX - util::RAD2DEG * std::log(std::tan(M_PI / 4 + latLng.latitude() * M_PI / util::DEGREES_MAX))
} * worldSize(scale) / util::DEGREES_MAX;
}

Expand Down
28 changes: 20 additions & 8 deletions platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/geometry/LatLng.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ public LatLng() {
* @param longitude Longitude in degrees
*/
public LatLng(double latitude, double longitude) {
this.latitude = latitude;
this.longitude = longitude;
setLatitude(latitude);
setLongitude(longitude);
}

/**
Expand All @@ -62,9 +62,9 @@ public LatLng(double latitude, double longitude) {
* @param altitude Altitude in meters
*/
public LatLng(double latitude, double longitude, double altitude) {
this.latitude = latitude;
this.longitude = longitude;
this.altitude = altitude;
setLatitude(latitude);
setLongitude(longitude);
setAltitude(altitude);
}

/**
Expand All @@ -88,12 +88,18 @@ public LatLng(LatLng aLatLng) {
}

protected LatLng(Parcel in) {
latitude = in.readDouble();
longitude = in.readDouble();
altitude = in.readDouble();
setLatitude(in.readDouble());
setLongitude(in.readDouble());
setAltitude(in.readDouble());
}

public void setLatitude(double latitude) {
if (Double.isNaN(latitude)) {
throw new IllegalArgumentException("latitude must not be NaN");
}
if (Math.abs(latitude) > 90.0) {
throw new IllegalArgumentException("latitude must be between -90 and 90");
}
this.latitude = latitude;
}

Expand All @@ -103,6 +109,12 @@ public double getLatitude() {
}

public void setLongitude(double longitude) {
if (Double.isNaN(longitude)) {
throw new IllegalArgumentException("longitude must not be NaN");
}
if (Double.isInfinite(longitude)) {
throw new IllegalArgumentException("longitude must not be infinite");
}
this.longitude = longitude;
}

Expand Down
23 changes: 9 additions & 14 deletions ...pboxGLAndroidSDKTestApp/src/test/java/com/mapbox/mapboxsdk/geometry/LatLngBoundsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

import com.mapbox.mapboxsdk.exceptions.InvalidLatLngBoundsException;
import com.mapbox.mapboxsdk.utils.MockParcel;
import com.mapbox.services.android.telemetry.constants.GeoConstants;

import org.junit.Before;
import org.junit.Test;
Expand Down Expand Up @@ -138,21 +137,17 @@ public void containsBoundsInWorld() {
}

@Test
public void containsNotBiggerBoundsInWorld() {
LatLngBounds biggerWorldBounds = new LatLngBounds.Builder()
.include(new LatLng(GeoConstants.MAX_LATITUDE + 10, GeoConstants.MIN_LONGITUDE - 10))
.include(new LatLng(GeoConstants.MIN_LATITUDE - 10, GeoConstants.MAX_LONGITUDE + 10))
public void containsBounds() {
LatLngBounds inner = new LatLngBounds.Builder()
.include(new LatLng(-5, -5))
.include(new LatLng(5, 5))
.build();
assertFalse("Bounds should not be contained ", LatLngBounds.world().contains(biggerWorldBounds));
}

@Test
public void containsNotBoundsInWorld() {
LatLngBounds outSideWorldBounds = new LatLngBounds.Builder()
.include(new LatLng(GeoConstants.MAX_LATITUDE + 10, GeoConstants.MAX_LONGITUDE + 10))
.include(new LatLng(GeoConstants.MAX_LATITUDE + 20, GeoConstants.MAX_LONGITUDE + 20))
LatLngBounds outer = new LatLngBounds.Builder()
.include(new LatLng(-10, -10))
.include(new LatLng(10, 10))
.build();
assertFalse("Bounds should not be contained ", LatLngBounds.world().contains(outSideWorldBounds));
assertTrue(outer.contains(inner));
assertFalse(inner.contains(outer));
}

@Test
Expand Down
75 changes: 75 additions & 0 deletions ...oid/MapboxGLAndroidSDKTestApp/src/test/java/com/mapbox/mapboxsdk/geometry/LatLngTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
import com.mapbox.mapboxsdk.utils.MockParcel;

import org.junit.Test;
import org.junit.Rule;
import org.junit.rules.ExpectedException;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotEquals;
Expand Down Expand Up @@ -79,6 +81,79 @@ public void testLongitudeSetter() {
assertEquals("longitude should match", 3, latLng.getLongitude(), DELTA);
}

@Rule
public final ExpectedException exception = ExpectedException.none();

@Test
public void testConstructorChecksLatitudeNaN() {
exception.expect(IllegalArgumentException.class);
exception.expectMessage("latitude must not be NaN");
new LatLng(Double.NaN, 0);
}

@Test
public void testConstructorChecksLongitudeNaN() {
exception.expect(IllegalArgumentException.class);
exception.expectMessage("longitude must not be NaN");
new LatLng(0, Double.NaN);
}

@Test
public void testConstructorChecksLatitudeGreaterThan90() {
exception.expect(IllegalArgumentException.class);
exception.expectMessage("latitude must be between -90 and 90");
new LatLng(95, 0);
}

@Test
public void testConstructorChecksLatitudeLessThanThanNegative90() {
exception.expect(IllegalArgumentException.class);
exception.expectMessage("latitude must be between -90 and 90");
new LatLng(-95, 0);
}

@Test
public void testConstructorChecksLongitudeInfinity() {
exception.expect(IllegalArgumentException.class);
exception.expectMessage("longitude must not be infinite");
new LatLng(0, Double.POSITIVE_INFINITY);
}

@Test
public void testLatitudeSetterChecksNaN() {
exception.expect(IllegalArgumentException.class);
exception.expectMessage("latitude must not be NaN");
new LatLng().setLatitude(Double.NaN);
}

@Test
public void testLongitudeSetterChecksNaN() {
exception.expect(IllegalArgumentException.class);
exception.expectMessage("longitude must not be NaN");
new LatLng().setLongitude(Double.NaN);
}

@Test
public void testLatitudeSetterChecksGreaterThan90() {
exception.expect(IllegalArgumentException.class);
exception.expectMessage("latitude must be between -90 and 90");
new LatLng().setLatitude(95);
}

@Test
public void testLatitudeSetterChecksLessThanThanNegative90() {
exception.expect(IllegalArgumentException.class);
exception.expectMessage("latitude must be between -90 and 90");
new LatLng().setLatitude(-95);
}

@Test
public void testLongitudeSetterChecksInfinity() {
exception.expect(IllegalArgumentException.class);
exception.expectMessage("longitude must not be infinite");
new LatLng().setLongitude(Double.NEGATIVE_INFINITY);
}

@Test
public void testAltitudeSetter() {
LatLng latLng = new LatLng(1.2, 3.4);
Expand Down
Loading