From 2f4d162debd7e4accfc0b20360058304dce40801 Mon Sep 17 00:00:00 2001 From: Bruno de Oliveira Abinader Date: Tue, 11 Apr 2017 17:07:25 +0300 Subject: [core] Tighten geo.hpp ctors --- include/mbgl/util/geo.hpp | 111 +++++++++++++++++++------------ include/mbgl/util/projection.hpp | 4 +- platform/android/src/native_map_view.cpp | 2 +- platform/darwin/src/MGLMapCamera.mm | 8 +-- platform/ios/src/MGLMapView.mm | 4 +- platform/qt/src/qmapboxgl.cpp | 10 +-- src/mbgl/map/map.cpp | 12 ++-- src/mbgl/map/transform.cpp | 4 +- src/mbgl/util/geo.cpp | 4 +- test/map/transform.test.cpp | 6 +- test/util/geo.test.cpp | 69 +++++++++++++++++++ test/util/projection.test.cpp | 10 +-- 12 files changed, 170 insertions(+), 74 deletions(-) diff --git a/include/mbgl/util/geo.hpp b/include/mbgl/util/geo.hpp index edd40e73ca..5f5f8bb33b 100644 --- a/include/mbgl/util/geo.hpp +++ b/include/mbgl/util/geo.hpp @@ -70,27 +70,42 @@ public: LatLng(const CanonicalTileID& id); LatLng(const UnwrappedTileID& id); - friend constexpr bool operator==(const LatLng& a, const LatLng& b) { + friend bool operator==(const LatLng& a, const LatLng& b) { return a.lat == b.lat && a.lon == b.lon; } - friend constexpr bool operator!=(const LatLng& a, const LatLng& b) { + friend bool operator!=(const LatLng& a, const LatLng& b) { return !(a == b); } }; class ProjectedMeters { +private: + double _northing; // Distance measured northwards. + double _easting; // Distance measured eastwards. + public: - double northing; - double easting; + ProjectedMeters(double n_ = 0, double e_ = 0) + : _northing(n_), _easting(e_) { + if (std::isnan(_northing)) { + throw std::domain_error("northing must not be NaN"); + } + if (std::isnan(_easting)) { + throw std::domain_error("easting must not be NaN"); + } + } - ProjectedMeters(double n = 0, double e = 0) - : northing(n), easting(e) {} -}; + double northing() const { return _northing; } + double easting() const { return _easting; } -constexpr bool operator==(const ProjectedMeters& a, const ProjectedMeters& b) { - return a.northing == b.northing && a.easting == b.easting; -} + friend bool operator==(const ProjectedMeters& a, const ProjectedMeters& b) { + return a._northing == b._northing && a._easting == b._easting; + } + + friend bool operator!=(const ProjectedMeters& a, const ProjectedMeters& b) { + return !(a == b); + } +}; class LatLngBounds { public: @@ -188,17 +203,14 @@ private: LatLngBounds(LatLng sw_, LatLng ne_) : sw(std::move(sw_)), ne(std::move(ne_)) {} - friend constexpr bool operator==(const LatLngBounds&, const LatLngBounds&); - friend constexpr bool operator!=(const LatLngBounds&, const LatLngBounds&); -}; - -constexpr bool operator==(const LatLngBounds& a, const LatLngBounds& b) { - return a.sw == b.sw && a.ne == b.ne; -} + friend bool operator==(const LatLngBounds& a, const LatLngBounds& b) { + return a.sw == b.sw && a.ne == b.ne; + } -constexpr bool operator!=(const LatLngBounds& a, const LatLngBounds& b) { - return !(a == b); -} + friend bool operator!=(const LatLngBounds& a, const LatLngBounds& b) { + return !(a == b); + } +}; // Determines the orientation of the map. enum class NorthOrientation : uint8_t { @@ -210,43 +222,60 @@ enum class NorthOrientation : uint8_t { /// The distance on each side between a rectangle and a rectangle within. class EdgeInsets { -public: - double top = 0; // Number of pixels inset from the top edge. - double left = 0; // Number of pixels inset from the left edge. - double bottom = 0; // Number of pixels inset from the bottom edge. - double right = 0; // Number of pixels inset from the right edge. +private: + double _top; // Number of pixels inset from the top edge. + double _left; // Number of pixels inset from the left edge. + double _bottom; // Number of pixels inset from the bottom edge. + double _right; // Number of pixels inset from the right edge. - EdgeInsets() {} +public: + EdgeInsets(double t_ = 0, double l_ = 0, double b_ = 0, double r_ = 0) + : _top(t_), _left(l_), _bottom(b_), _right(r_) { + if (std::isnan(_top)) { + throw std::domain_error("top must not be NaN"); + } + if (std::isnan(_left)) { + throw std::domain_error("left must not be NaN"); + } + if (std::isnan(_bottom)) { + throw std::domain_error("bottom must not be NaN"); + } + if (std::isnan(_right)) { + throw std::domain_error("right must not be NaN"); + } + } - EdgeInsets(const double t, const double l, const double b, const double r) - : top(t), left(l), bottom(b), right(r) {} + double top() const { return _top; } + double left() const { return _left; } + double bottom() const { return _bottom; } + double right() const { return _right; } bool isFlush() const { - return top == 0 && left == 0 && bottom == 0 && right == 0; + return _top == 0 && _left == 0 && _bottom == 0 && _right == 0; } void operator+=(const EdgeInsets& o) { - top += o.top; - left += o.left; - bottom += o.bottom; - right += o.right; + _top += o._top; + _left += o._left; + _bottom += o._bottom; + _right += o._right; } EdgeInsets operator+(const EdgeInsets& o) const { return { - top + o.top, left + o.left, bottom + o.bottom, right + o.right, + _top + o._top, _left + o._left, _bottom + o._bottom, _right + o._right, }; } ScreenCoordinate getCenter(uint16_t width, uint16_t height) const; -}; -constexpr bool operator==(const EdgeInsets& a, const EdgeInsets& b) { - return a.top == b.top && a.left == b.left && a.bottom == b.bottom && a.right == b.right; -} + friend bool operator==(const EdgeInsets& a, const EdgeInsets& b) { + return a._top == b._top && a._left == b._left && a._bottom == b._bottom && a._right == b._right; + } -constexpr bool operator!=(const EdgeInsets& a, const EdgeInsets& b) { - return !(a == b); -} + friend bool operator!=(const EdgeInsets& a, const EdgeInsets& b) { + return !(a == b); + } +}; } // namespace mbgl diff --git a/include/mbgl/util/projection.hpp b/include/mbgl/util/projection.hpp index e50e29e22b..ed34261b18 100644 --- a/include/mbgl/util/projection.hpp +++ b/include/mbgl/util/projection.hpp @@ -38,8 +38,8 @@ public: } static LatLng latLngForProjectedMeters(const ProjectedMeters& projectedMeters) { - double latitude = (2 * std::atan(std::exp(projectedMeters.northing / util::EARTH_RADIUS_M)) - (M_PI / 2.0)) * util::RAD2DEG; - double longitude = projectedMeters.easting * util::RAD2DEG / util::EARTH_RADIUS_M; + double latitude = (2 * std::atan(std::exp(projectedMeters.northing() / util::EARTH_RADIUS_M)) - (M_PI / 2.0)) * util::RAD2DEG; + double longitude = projectedMeters.easting() * util::RAD2DEG / util::EARTH_RADIUS_M; latitude = util::clamp(latitude, -util::LATITUDE_MAX, util::LATITUDE_MAX); longitude = util::clamp(longitude, -util::LONGITUDE_MAX, util::LONGITUDE_MAX); diff --git a/platform/android/src/native_map_view.cpp b/platform/android/src/native_map_view.cpp index 78e021c321..e5edc1c30f 100755 --- a/platform/android/src/native_map_view.cpp +++ b/platform/android/src/native_map_view.cpp @@ -643,7 +643,7 @@ jni::jdouble NativeMapView::getMetersPerPixelAtLatitude(JNIEnv&, jni::jdouble la jni::Object NativeMapView::projectedMetersForLatLng(JNIEnv& env, jni::jdouble latitude, jni::jdouble longitude) { mbgl::ProjectedMeters projectedMeters = map->projectedMetersForLatLng(mbgl::LatLng(latitude, longitude)); - return ProjectedMeters::New(env, projectedMeters.northing, projectedMeters.easting); + return ProjectedMeters::New(env, projectedMeters.northing(), projectedMeters.easting()); } jni::Object NativeMapView::pixelForLatLng(JNIEnv& env, jdouble latitude, jdouble longitude) { diff --git a/platform/darwin/src/MGLMapCamera.mm b/platform/darwin/src/MGLMapCamera.mm index 49c7e70900..1611dbf4a3 100644 --- a/platform/darwin/src/MGLMapCamera.mm +++ b/platform/darwin/src/MGLMapCamera.mm @@ -32,11 +32,11 @@ BOOL MGLEqualFloatWithAccuracy(CGFloat left, CGFloat right, CGFloat accuracy) mbgl::ProjectedMeters centerMeters = mbgl::Projection::projectedMetersForLatLng(centerLatLng); mbgl::ProjectedMeters eyeMeters = mbgl::Projection::projectedMetersForLatLng(eyeLatLng); - heading = std::atan((centerMeters.northing - eyeMeters.northing) / - (centerMeters.easting - eyeMeters.easting)); + heading = std::atan((centerMeters.northing() - eyeMeters.northing()) / + (centerMeters.easting() - eyeMeters.easting())); - double groundDistance = std::hypot(centerMeters.northing - eyeMeters.northing, - centerMeters.easting - eyeMeters.easting); + double groundDistance = std::hypot(centerMeters.northing() - eyeMeters.northing(), + centerMeters.easting() - eyeMeters.easting()); pitch = std::atan(eyeAltitude / groundDistance); } diff --git a/platform/ios/src/MGLMapView.mm b/platform/ios/src/MGLMapView.mm index 5ae588d619..6badb1d857 100644 --- a/platform/ios/src/MGLMapView.mm +++ b/platform/ios/src/MGLMapView.mm @@ -4559,8 +4559,8 @@ public: mbgl::LatLng targetLatLng = MGLLatLngFromLocationCoordinate2D(self.targetCoordinate); mbgl::ProjectedMeters userMeters = mbgl::Projection::projectedMetersForLatLng(userLatLng); mbgl::ProjectedMeters targetMeters = mbgl::Projection::projectedMetersForLatLng(targetLatLng); - double angle = atan2(targetMeters.easting - userMeters.easting, - targetMeters.northing - userMeters.northing); + double angle = atan2(targetMeters.easting() - userMeters.easting(), + targetMeters.northing() - userMeters.northing()); direction = mbgl::util::wrap(MGLDegreesFromRadians(angle), 0., 360.); } else diff --git a/platform/qt/src/qmapboxgl.cpp b/platform/qt/src/qmapboxgl.cpp index 2f5b3d84db..143af2ed16 100644 --- a/platform/qt/src/qmapboxgl.cpp +++ b/platform/qt/src/qmapboxgl.cpp @@ -1130,7 +1130,7 @@ double QMapboxGL::metersPerPixelAtLatitude(double latitude, double zoom) const QMapbox::ProjectedMeters QMapboxGL::projectedMetersForCoordinate(const QMapbox::Coordinate &coordinate_) const { auto projectedMeters = d_ptr->mapObj->projectedMetersForLatLng(mbgl::LatLng { coordinate_.first, coordinate_.second }); - return QMapbox::ProjectedMeters(projectedMeters.northing, projectedMeters.easting); + return QMapbox::ProjectedMeters(projectedMeters.northing(), projectedMeters.easting()); } /*! @@ -1229,10 +1229,10 @@ void QMapboxGL::setMargins(const QMargins &margins_) QMargins QMapboxGL::margins() const { return QMargins( - d_ptr->margins.left, - d_ptr->margins.top, - d_ptr->margins.right, - d_ptr->margins.bottom + d_ptr->margins.left(), + d_ptr->margins.top(), + d_ptr->margins.right(), + d_ptr->margins.bottom() ); } diff --git a/src/mbgl/map/map.cpp b/src/mbgl/map/map.cpp index 19b1222f0e..fde466054c 100644 --- a/src/mbgl/map/map.cpp +++ b/src/mbgl/map/map.cpp @@ -600,8 +600,8 @@ CameraOptions Map::cameraForLatLngs(const std::vector& latLngs, const Ed if (width > 0 || height > 0) { double scaleX = double(getSize().width) / width; double scaleY = double(getSize().height) / height; - scaleX -= (padding.left + padding.right) / width; - scaleY -= (padding.top + padding.bottom) / height; + scaleX -= (padding.left() + padding.right()) / width; + scaleY -= (padding.top() + padding.bottom()) / height; minScale = util::min(scaleX, scaleY); } double zoom = getZoom() + util::log2(minScale); @@ -610,12 +610,12 @@ CameraOptions Map::cameraForLatLngs(const std::vector& latLngs, const Ed // Calculate the center point of a virtual bounds that is extended in all directions by padding. ScreenCoordinate centerPixel = nePixel + swPixel; ScreenCoordinate paddedNEPixel = { - padding.right / minScale, - padding.top / minScale, + padding.right() / minScale, + padding.top() / minScale, }; ScreenCoordinate paddedSWPixel = { - padding.left / minScale, - padding.bottom / minScale, + padding.left() / minScale, + padding.bottom() / minScale, }; centerPixel = centerPixel + paddedNEPixel - paddedSWPixel; centerPixel /= 2.0; diff --git a/src/mbgl/map/transform.cpp b/src/mbgl/map/transform.cpp index 67729f7fe2..f9a687480c 100644 --- a/src/mbgl/map/transform.cpp +++ b/src/mbgl/map/transform.cpp @@ -200,8 +200,8 @@ void Transform::flyTo(const CameraOptions &camera, const AnimationOptions &anima /// w₀: Initial visible span, measured in pixels at the initial scale. /// Known henceforth as a screenful. - double w0 = std::max(state.size.width - padding.left - padding.right, - state.size.height - padding.top - padding.bottom); + double w0 = std::max(state.size.width - padding.left() - padding.right(), + state.size.height - padding.top() - padding.bottom()); /// w₁: Final visible span, measured in pixels with respect to the initial /// scale. double w1 = w0 / state.zoomScale(zoom - startZoom); diff --git a/src/mbgl/util/geo.cpp b/src/mbgl/util/geo.cpp index 1244a975a4..f38aba20c4 100644 --- a/src/mbgl/util/geo.cpp +++ b/src/mbgl/util/geo.cpp @@ -34,8 +34,8 @@ LatLngBounds::LatLngBounds(const CanonicalTileID& id) ScreenCoordinate EdgeInsets::getCenter(uint16_t width, uint16_t height) const { return { - (width - left - right) / 2.0f + left, - (height - top - bottom) / 2.0f + top, + (width - left() - right()) / 2.0 + left(), + (height - top() - bottom()) / 2.0 + top(), }; } diff --git a/test/map/transform.test.cpp b/test/map/transform.test.cpp index 8a86598a1d..accf7d293c 100644 --- a/test/map/transform.test.cpp +++ b/test/map/transform.test.cpp @@ -471,14 +471,12 @@ TEST(Transform, DefaultTransform) { // Cannot assign invalid sizes. std::vector invalidSizes = { {}, { min, max }, { max, min } }; for (const Size& size : invalidSizes) { - ASSERT_TRUE(size.isEmpty()); - bool pass = false; try { transform.resize(size); + ASSERT_TRUE(false) << "Should throw"; } catch (...) { - pass = true; + ASSERT_TRUE(size.isEmpty()); } - ASSERT_TRUE(pass) << "Expected to throw"; } Size validSize { max, max }; diff --git a/test/util/geo.test.cpp b/test/util/geo.test.cpp index 3f625f21fa..3dfa8e1cca 100644 --- a/test/util/geo.test.cpp +++ b/test/util/geo.test.cpp @@ -6,6 +6,75 @@ using namespace mbgl; +TEST(LatLng, InvalidLatLng) { + try { + LatLng { NAN }; + ASSERT_TRUE(false) << "should throw"; + } catch (const std::domain_error& error) { + ASSERT_EQ(std::string(error.what()), "latitude must not be NaN"); + } + try { + LatLng { 0, NAN }; + ASSERT_TRUE(false) << "should throw"; + } catch (const std::domain_error& error) { + ASSERT_EQ(std::string(error.what()), "longitude must not be NaN"); + } + try { + LatLng { 91.0 }; + ASSERT_TRUE(false) << "should throw"; + } catch (const std::domain_error& error) { + ASSERT_EQ(std::string(error.what()), "latitude must be between -90 and 90"); + } + try { + LatLng { 0, std::numeric_limits::infinity() }; + ASSERT_TRUE(false) << "should throw"; + } catch (const std::domain_error& error) { + ASSERT_EQ(std::string(error.what()), "longitude must not be infinite"); + } +} + +TEST(ProjectedMeters, InvalidProjectedMeters) { + try { + ProjectedMeters { NAN }; + ASSERT_TRUE(false) << "should throw"; + } catch (const std::domain_error& error) { + ASSERT_EQ(std::string(error.what()), "northing must not be NaN"); + } + try { + ProjectedMeters { 0, NAN }; + ASSERT_TRUE(false) << "should throw"; + } catch (const std::domain_error& error) { + ASSERT_EQ(std::string(error.what()), "easting must not be NaN"); + } +} + +TEST(EdgeInsets, InvalidEdgeInsets) { + try { + EdgeInsets { NAN }; + ASSERT_TRUE(false) << "should throw"; + } catch (const std::domain_error& error) { + ASSERT_EQ(std::string(error.what()), "top must not be NaN"); + } + try { + EdgeInsets { 0, NAN }; + ASSERT_TRUE(false) << "should throw"; + } catch (const std::domain_error& error) { + ASSERT_EQ(std::string(error.what()), "left must not be NaN"); + } + try { + EdgeInsets { 0, 0, NAN }; + ASSERT_TRUE(false) << "should throw"; + } catch (const std::domain_error& error) { + ASSERT_EQ(std::string(error.what()), "bottom must not be NaN"); + } + try { + EdgeInsets { 0, 0, 0, NAN }; + ASSERT_TRUE(false) << "should throw"; + } catch (const std::domain_error& error) { + ASSERT_EQ(std::string(error.what()), "right must not be NaN"); + } +} + TEST(LatLngBounds, World) { auto result = LatLngBounds::world(); ASSERT_DOUBLE_EQ(-90, result.south()); diff --git a/test/util/projection.test.cpp b/test/util/projection.test.cpp index b0fa0bd060..9f27fe3d14 100644 --- a/test/util/projection.test.cpp +++ b/test/util/projection.test.cpp @@ -38,18 +38,18 @@ TEST(Projection, MetersPerPixelAtLatitude) { TEST(Projection, ProjectedMeters) { auto latLng = LatLng {}; auto projectedMeters = Projection::projectedMetersForLatLng(latLng); - EXPECT_EQ(projectedMeters.northing, projectedMeters.easting); + EXPECT_EQ(projectedMeters.northing(), projectedMeters.easting()); EXPECT_EQ(latLng, Projection::latLngForProjectedMeters(projectedMeters)); const auto southWest = LatLng { -util::LATITUDE_MAX, -util::LONGITUDE_MAX }; projectedMeters = Projection::projectedMetersForLatLng(southWest); - EXPECT_DOUBLE_EQ(projectedMeters.northing, -20037508.342789274); - EXPECT_DOUBLE_EQ(projectedMeters.easting, -20037508.342789244); + EXPECT_DOUBLE_EQ(projectedMeters.northing(), -20037508.342789274); + EXPECT_DOUBLE_EQ(projectedMeters.easting(), -20037508.342789244); const auto northEast = LatLng { util::LATITUDE_MAX, util::LONGITUDE_MAX }; projectedMeters = Projection::projectedMetersForLatLng(northEast); - EXPECT_DOUBLE_EQ(projectedMeters.northing, 20037508.342789274); - EXPECT_DOUBLE_EQ(projectedMeters.easting, 20037508.342789244); + EXPECT_DOUBLE_EQ(projectedMeters.northing(), 20037508.342789274); + EXPECT_DOUBLE_EQ(projectedMeters.easting(), 20037508.342789244); projectedMeters = ProjectedMeters { std::numeric_limits::lowest(), std::numeric_limits::lowest() }; latLng = Projection::latLngForProjectedMeters(projectedMeters); -- cgit v1.2.1