Merge pull request #2143 from joto/fix-expire-off-by-one

Fix off-by-one error in expire code generating out of bounds tiles
osm2pgsql-dev · Mar 8, 2024 · 13af586 · 13af586
2 parents d58a6c4 + 76ba82c
commit 13af586
Show file tree

Hide file tree

Showing 3 changed files with 105 additions and 4 deletions.
diff --git a/src/expire-tiles.cpp b/src/expire-tiles.cpp
@@ -243,15 +243,15 @@ int expire_tiles::from_bbox(geom::box_t const &box,
     /* Convert the box's Mercator coordinates into tile coordinates */
     auto const tmp_min = coords_to_tile({box.min_x(), box.max_y()});
     int const min_tile_x =
-        std::clamp(int(tmp_min.x() - expire_config.buffer), 0, m_map_width);
+        std::clamp(int(tmp_min.x() - expire_config.buffer), 0, m_map_width - 1);
     int const min_tile_y =
-        std::clamp(int(tmp_min.y() - expire_config.buffer), 0, m_map_width);
+        std::clamp(int(tmp_min.y() - expire_config.buffer), 0, m_map_width - 1);
 
     auto const tmp_max = coords_to_tile({box.max_x(), box.min_y()});
     int const max_tile_x =
-        std::clamp(int(tmp_max.x() + expire_config.buffer), 0, m_map_width);
+        std::clamp(int(tmp_max.x() + expire_config.buffer), 0, m_map_width - 1);
     int const max_tile_y =
-        std::clamp(int(tmp_max.y() + expire_config.buffer), 0, m_map_width);
+        std::clamp(int(tmp_max.y() + expire_config.buffer), 0, m_map_width - 1);
 
     for (int iterator_x = min_tile_x; iterator_x <= max_tile_x; ++iterator_x) {
         uint32_t const norm_x = normalise_tile_x_coord(iterator_x);

diff --git a/tests/test-expire-tiles.cpp b/tests/test-expire-tiles.cpp
@@ -144,6 +144,70 @@ TEST_CASE("simple expire z18", "[NoDB]")
     CHECK(*(itr++) == tile_t(18, 131072, 131072));
 }
 
+TEST_CASE("simple expire z10 bounds 0, 0", "[NoDB]")
+{
+    uint32_t const minzoom = 10;
+    uint32_t const maxzoom = 10;
+    expire_tiles et{minzoom, defproj};
+
+    et.from_geometry(geom::point_t{-20037508.34, 20037508.34},
+                     expire_config_t{});
+
+    auto const tiles = get_tiles_ordered(&et, minzoom, maxzoom);
+    CHECK(tiles.size() == 1);
+
+    auto itr = tiles.begin();
+    CHECK(*(itr++) == tile_t(10, 0, 0));
+}
+
+TEST_CASE("simple expire z10 bounds 0, 1023", "[NoDB]")
+{
+    uint32_t const minzoom = 10;
+    uint32_t const maxzoom = 10;
+    expire_tiles et{minzoom, defproj};
+
+    et.from_geometry(geom::point_t{-20037508.34, -20037508.34},
+                     expire_config_t{});
+
+    auto const tiles = get_tiles_ordered(&et, minzoom, maxzoom);
+    CHECK(tiles.size() == 1);
+
+    auto itr = tiles.begin();
+    CHECK(*(itr++) == tile_t(10, 0, 1023));
+}
+
+TEST_CASE("simple expire z10 bounds 1023, 0", "[NoDB]")
+{
+    uint32_t const minzoom = 10;
+    uint32_t const maxzoom = 10;
+    expire_tiles et{minzoom, defproj};
+
+    et.from_geometry(geom::point_t{20037508.34, 20037508.34},
+                     expire_config_t{});
+
+    auto const tiles = get_tiles_ordered(&et, minzoom, maxzoom);
+    CHECK(tiles.size() == 1);
+
+    auto itr = tiles.begin();
+    CHECK(*(itr++) == tile_t(10, 1023, 0));
+}
+
+TEST_CASE("simple expire z10 bounds 1023, 1023", "[NoDB]")
+{
+    uint32_t const minzoom = 10;
+    uint32_t const maxzoom = 10;
+    expire_tiles et{minzoom, defproj};
+
+    et.from_geometry(geom::point_t{20037508.34, -20037508.34},
+                     expire_config_t{});
+
+    auto const tiles = get_tiles_ordered(&et, minzoom, maxzoom);
+    CHECK(tiles.size() == 1);
+
+    auto itr = tiles.begin();
+    CHECK(*(itr++) == tile_t(10, 1023, 1023));
+}
+
 TEST_CASE("expire a simple line", "[NoDB]")
 {
     uint32_t const zoom = 18;

diff --git a/tests/test-reprojection.cpp b/tests/test-reprojection.cpp
@@ -47,6 +47,43 @@ TEST_CASE("projection 3857", "[NoDB]")
     REQUIRE(ct.y() == Approx(6982997.92));
 }
 
+TEST_CASE("projection 3857 bounds", "[NoDB]")
+{
+    osmium::Location const loc1{0.0, 0.0};
+    osmium::Location const loc2{-180.0, -85.0511288};
+    osmium::Location const loc3{180.0, 85.0511288};
+    int const srs = 3857;
+    auto const reprojection = reprojection::create_projection(srs);
+
+    {
+        auto const c = reprojection->reproject(geom::point_t{loc1});
+        REQUIRE(c.x() == Approx(0.0));
+        REQUIRE(c.y() == Approx(0.0));
+
+        auto const ct = reprojection->target_to_tile(c);
+        REQUIRE(ct.x() == Approx(0.0));
+        REQUIRE(ct.y() == Approx(0.0));
+    }
+    {
+        auto const c = reprojection->reproject(geom::point_t{loc2});
+        REQUIRE(c.x() == Approx(-20037508.34));
+        REQUIRE(c.y() == Approx(-20037508.34));
+
+        auto const ct = reprojection->target_to_tile(c);
+        REQUIRE(ct.x() == Approx(-20037508.34));
+        REQUIRE(ct.y() == Approx(-20037508.34));
+    }
+    {
+        auto const c = reprojection->reproject(geom::point_t{loc3});
+        REQUIRE(c.x() == Approx(20037508.34));
+        REQUIRE(c.y() == Approx(20037508.34));
+
+        auto const ct = reprojection->target_to_tile(c);
+        REQUIRE(ct.x() == Approx(20037508.34));
+        REQUIRE(ct.y() == Approx(20037508.34));
+    }
+}
+
 #ifdef HAVE_GENERIC_PROJ
 TEST_CASE("projection 5651", "[NoDB]")
 {