[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

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 <mbgl/util/constants.hpp>
 #include <mbgl/util/interpolate.hpp>
 #include <mbgl/util/projection.hpp>
+#include <mbgl/util/tile_coordinate.hpp>
 #include <mbgl/math/log2.hpp>
 #include <mbgl/math/clamp.hpp>
 
@@ -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<double> p = util::interpolate(p0, p1, t) / scale * static_cast<double>(1 << atZoom);
+    return { { p.x, p.y }, static_cast<double>(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<double>;
 
 // 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<UnwrappedTileID> tileCover(const Point<double>& tl,
                                        const Point<double>& br,
                                        const Point<double>& bl,
                                        const Point<double>& 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<UnwrappedTileID> tileCover(const LatLngBounds& bounds_, int32_t z) {
+std::vector<UnwrappedTileID> 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<UnwrappedTileID> tileCover(const LatLngBounds& bounds_, int32_t z) {
         z);
 }
 
-std::vector<UnwrappedTileID> tileCover(const TransformState& state, int32_t z) {
+std::vector<UnwrappedTileID> tileCover(const TransformState& state, uint8_t z) {
     assert(state.valid());
 
     const double w = state.getSize().width;
@@ -173,7 +173,7 @@ std::vector<UnwrappedTileID> tileCover(const TransformState& state, int32_t z) {
         z);
 }
 
-std::vector<UnwrappedTileID> tileCover(const Geometry<double>& geometry, int32_t z) {
+std::vector<UnwrappedTileID> tileCover(const Geometry<double>& geometry, uint8_t z) {
     std::vector<UnwrappedTileID> result;
     TileCover tc(geometry, z, true);
     while (tc.hasNext()) {
@@ -213,7 +213,7 @@ uint64_t tileCount(const Geometry<double>& 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<TileCover::Impl>(z, p, false);
 }
 
-TileCover::TileCover(const Geometry<double>& geom, int32_t z, bool project/* = true*/)
+TileCover::TileCover(const Geometry<double>& geom, uint8_t z, bool project/* = true*/)
  : impl( std::make_unique<TileCover::Impl>(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<double>&, int32_t z, bool project = true);
+    TileCover(const Geometry<double>&, uint8_t z, bool project = true);
     ~TileCover();
 
     optional<UnwrappedTileID> next();
@@ -33,9 +33,9 @@ private:
 
 int32_t coveringZoomLevel(double z, style::SourceType type, uint16_t tileSize);
 
-std::vector<UnwrappedTileID> tileCover(const TransformState&, int32_t z);
-std::vector<UnwrappedTileID> tileCover(const LatLngBounds&, int32_t z);
-std::vector<UnwrappedTileID> tileCover(const Geometry<double>&, int32_t z);
+std::vector<UnwrappedTileID> tileCover(const TransformState&, uint8_t z);
+std::vector<UnwrappedTileID> tileCover(const LatLngBounds&, uint8_t z);
+std::vector<UnwrappedTileID> tileCover(const Geometry<double>&, 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<UnwrappedTileID>{
+        { 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 });