From 9eff849c743c6e861404a2c39afa53c4eecdca3d Mon Sep 17 00:00:00 2001 From: Aleksandar Stojiljkovic Date: Fri, 23 Aug 2019 12:04:32 +0300 Subject: [core] Fix tileCover / TileCoordinate fromScreenCoordinate(#15442) Remove unecessary tile coordinate unproject -> project step. Unproject+project isn't always a noop and this was the issue leading to wrong cover calculation in high pitch values. Fixes: https://github.com/mapbox/mapbox-gl-native/issues/15442 --- src/mbgl/map/transform_state.cpp | 13 ++++++++++--- src/mbgl/map/transform_state.hpp | 3 +++ src/mbgl/util/tile_coordinate.hpp | 5 +++-- src/mbgl/util/tile_cover.cpp | 12 ++++++------ src/mbgl/util/tile_cover.hpp | 10 +++++----- test/util/tile_cover.test.cpp | 14 ++++++++++++++ 6 files changed, 41 insertions(+), 16 deletions(-) diff --git a/src/mbgl/map/transform_state.cpp b/src/mbgl/map/transform_state.cpp index 77309a2a55..951c5a6202 100644 --- a/src/mbgl/map/transform_state.cpp +++ b/src/mbgl/map/transform_state.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include @@ -289,9 +290,9 @@ ScreenCoordinate TransformState::latLngToScreenCoordinate(const LatLng& latLng) return { p[0] / p[3], size.height - p[1] / p[3] }; } -LatLng TransformState::screenCoordinateToLatLng(const ScreenCoordinate& point, LatLng::WrapMode wrapMode) const { +TileCoordinate TransformState::screenCoordinateToTileCoordinate(const ScreenCoordinate& point, uint8_t atZoom) const { if (size.isEmpty()) { - return {}; + return { {}, 0 }; } float targetZ = 0; @@ -325,7 +326,13 @@ LatLng TransformState::screenCoordinateToLatLng(const ScreenCoordinate& point, L double z1 = coord1[2] / w1; double t = z0 == z1 ? 0 : (targetZ - z0) / (z1 - z0); - return Projection::unproject(util::interpolate(p0, p1, t), scale / util::tileSize, wrapMode); + Point p = util::interpolate(p0, p1, t) / scale * static_cast(1 << atZoom); + return { { p.x, p.y }, static_cast(atZoom) }; +} + +LatLng TransformState::screenCoordinateToLatLng(const ScreenCoordinate& point, LatLng::WrapMode wrapMode) const { + auto coord = screenCoordinateToTileCoordinate(point, 0); + return Projection::unproject(coord.p, 1 / util::tileSize, wrapMode); } mat4 TransformState::coordinatePointMatrix() const { diff --git a/src/mbgl/map/transform_state.hpp b/src/mbgl/map/transform_state.hpp index cca42db20f..e81ebed27e 100644 --- a/src/mbgl/map/transform_state.hpp +++ b/src/mbgl/map/transform_state.hpp @@ -17,6 +17,7 @@ namespace mbgl { class UnwrappedTileID; +class TileCoordinate; class TransformState { friend class Transform; @@ -81,6 +82,8 @@ public: // Conversion ScreenCoordinate latLngToScreenCoordinate(const LatLng&) const; LatLng screenCoordinateToLatLng(const ScreenCoordinate&, LatLng::WrapMode = LatLng::Unwrapped) const; + // Implements mapbox-gl-js pointCoordinate() : MercatorCoordinate. + TileCoordinate screenCoordinateToTileCoordinate(const ScreenCoordinate&, uint8_t atZoom) const; double zoomScale(double zoom) const; double scaleZoom(double scale) const; diff --git a/src/mbgl/util/tile_coordinate.hpp b/src/mbgl/util/tile_coordinate.hpp index b6bdc5f590..6d78804947 100644 --- a/src/mbgl/util/tile_coordinate.hpp +++ b/src/mbgl/util/tile_coordinate.hpp @@ -13,6 +13,7 @@ using TileCoordinatePoint = Point; // Has floating point x/y coordinates. // Used for computing the tiles that need to be visible in the viewport. +// In mapbox-gl-js, this is named MercatorCoordinate. class TileCoordinate { public: TileCoordinatePoint p; @@ -23,8 +24,8 @@ public: return { Projection::project(latLng, scale) / util::tileSize, zoom }; } - static TileCoordinate fromScreenCoordinate(const TransformState& state, double zoom, const ScreenCoordinate& screenCoordinate) { - return fromLatLng(zoom, state.screenCoordinateToLatLng(screenCoordinate)); + static TileCoordinate fromScreenCoordinate(const TransformState& state, uint8_t zoom, const ScreenCoordinate& screenCoordinate) { + return state.screenCoordinateToTileCoordinate(screenCoordinate, zoom); } TileCoordinate zoomTo(double zoom) const { diff --git a/src/mbgl/util/tile_cover.cpp b/src/mbgl/util/tile_cover.cpp index 5189b79f26..9e2451e1af 100644 --- a/src/mbgl/util/tile_cover.cpp +++ b/src/mbgl/util/tile_cover.cpp @@ -84,7 +84,7 @@ std::vector tileCover(const Point& tl, const Point& br, const Point& bl, const Point& c, - int32_t z) { + uint8_t z) { const int32_t tiles = 1 << z; struct ID { @@ -139,7 +139,7 @@ int32_t coveringZoomLevel(double zoom, style::SourceType type, uint16_t size) { } } -std::vector tileCover(const LatLngBounds& bounds_, int32_t z) { +std::vector tileCover(const LatLngBounds& bounds_, uint8_t z) { if (bounds_.isEmpty() || bounds_.south() > util::LATITUDE_MAX || bounds_.north() < -util::LATITUDE_MAX) { @@ -159,7 +159,7 @@ std::vector tileCover(const LatLngBounds& bounds_, int32_t z) { z); } -std::vector tileCover(const TransformState& state, int32_t z) { +std::vector tileCover(const TransformState& state, uint8_t z) { assert(state.valid()); const double w = state.getSize().width; @@ -173,7 +173,7 @@ std::vector tileCover(const TransformState& state, int32_t z) { z); } -std::vector tileCover(const Geometry& geometry, int32_t z) { +std::vector tileCover(const Geometry& geometry, uint8_t z) { std::vector result; TileCover tc(geometry, z, true); while (tc.hasNext()) { @@ -213,7 +213,7 @@ uint64_t tileCount(const Geometry& geometry, uint8_t z) { return tileCount; } -TileCover::TileCover(const LatLngBounds&bounds_, int32_t z) { +TileCover::TileCover(const LatLngBounds&bounds_, uint8_t z) { LatLngBounds bounds = LatLngBounds::hull( { std::max(bounds_.south(), -util::LATITUDE_MAX), bounds_.west() }, { std::min(bounds_.north(), util::LATITUDE_MAX), bounds_.east() }); @@ -233,7 +233,7 @@ TileCover::TileCover(const LatLngBounds&bounds_, int32_t z) { impl = std::make_unique(z, p, false); } -TileCover::TileCover(const Geometry& geom, int32_t z, bool project/* = true*/) +TileCover::TileCover(const Geometry& geom, uint8_t z, bool project/* = true*/) : impl( std::make_unique(z, geom, project)) { } diff --git a/src/mbgl/util/tile_cover.hpp b/src/mbgl/util/tile_cover.hpp index c953d764d2..5ac3537bf6 100644 --- a/src/mbgl/util/tile_cover.hpp +++ b/src/mbgl/util/tile_cover.hpp @@ -18,9 +18,9 @@ namespace util { // Helper class to stream tile-cover results per row class TileCover { public: - TileCover(const LatLngBounds&, int32_t z); + TileCover(const LatLngBounds&, uint8_t z); // When project == true, projects the geometry points to tile coordinates - TileCover(const Geometry&, int32_t z, bool project = true); + TileCover(const Geometry&, uint8_t z, bool project = true); ~TileCover(); optional next(); @@ -33,9 +33,9 @@ private: int32_t coveringZoomLevel(double z, style::SourceType type, uint16_t tileSize); -std::vector tileCover(const TransformState&, int32_t z); -std::vector tileCover(const LatLngBounds&, int32_t z); -std::vector tileCover(const Geometry&, int32_t z); +std::vector tileCover(const TransformState&, uint8_t z); +std::vector tileCover(const LatLngBounds&, uint8_t z); +std::vector tileCover(const Geometry&, uint8_t z); // Compute only the count of tiles needed for tileCover uint64_t tileCount(const LatLngBounds&, uint8_t z); diff --git a/test/util/tile_cover.test.cpp b/test/util/tile_cover.test.cpp index e35e6e2e99..af9f0c4884 100644 --- a/test/util/tile_cover.test.cpp +++ b/test/util/tile_cover.test.cpp @@ -43,6 +43,20 @@ TEST(TileCover, Pitch) { util::tileCover(transform.getState(), 2)); } +TEST(TileCover, PitchIssue15442) { + Transform transform; + transform.resize({ 412, 691 }); + + transform.jumpTo(CameraOptions().withCenter(LatLng { 59.116898740996106, 91.565660781803615, }) + .withZoom(2.0551126748417214).withBearing(0.74963938256567264 * util::RAD2DEG) + .withPitch(1.0471975511965976 * util::RAD2DEG)); + + EXPECT_EQ((std::vector{ + { 2, 3, 1 }, { 2, 2, 1 }, { 2, 3, 0 }, { 2, 2, 0 }, { 1, { 2, 0, 0 } }, { 1, { 2, 1, 0 } } + }), + util::tileCover(transform.getState(), 2)); +} + TEST(TileCover, PitchOverAllowedByContentInsets) { Transform transform; transform.resize({ 512, 512 }); -- cgit v1.2.1