Merge pull request #2453 from joto/expire-polygon

Do area expire based on the actual polygon instead of the bbox

Author: lonvia

scripts/osm2pgsql-test-style

@@ -93,7 +93,6 @@ class ReplicationServerMock:
         numdiffs = int((max_size + 1023)/1024)
         return min(self.state_infos[-1].sequence, start_id + numdiffs - 1)
 
-
 # Replication module is optional
 _repfl_spec = importlib.util.spec_from_loader(
     'osm2pgsql_replication',
@@ -510,6 +509,30 @@ def execute_osm2pgsql(context, output):
     context.osm2pgsql_returncode = proc.returncode
     context.osm2pgsql_params = {'-d': context.user_args.test_db}
 
+@when("running osm2pgsql-expire with parameters")
+def execute_osm2pgsql_expire(context):
+
+    cmdline = [context.user_args.osm2pgsql_binary + '-expire']
+
+    test_dir = (context.user_args.test_data_dir or Path('.')).resolve()
+    def _template(param):
+        return param.replace('{TEST_DATA_DIR}', str(test_dir))
+
+    if context.table:
+        assert not any('<' in h for h in context.table.headings), \
+            "Substition in the first line of a table are not supported."
+        cmdline.extend(_template(h) for h in context.table.headings if h)
+        for row in context.table:
+            cmdline.extend(_template(c) for c in row if c)
+
+    proc = Popen(cmdline, cwd=str(context.workdir),
+                 stdout=PIPE, stderr=PIPE)
+
+    outdata = proc.communicate()
+    context.osm2pgsql_cmdline = ' '.join(cmdline)
+    context.osm2pgsql_outdata = [d.decode('utf-8').replace('\\n', '\n') for d in outdata]
+    context.osm2pgsql_returncode = proc.returncode
+
 @then("execution is successful")
 def osm2pgsql_check_success(context):
     assert context.osm2pgsql_returncode == 0, \
@@ -541,6 +564,21 @@ def check_program_output(context, kind):
         assert line in s,\
                f"Output '{line}' not found in {kind} output:\n{s}\n"
 
+@then(r"the (?P<kind>\w+) output matches contents of (?P<result_file>.*)")
+def check_program_output_against_file(context, kind, result_file):
+    if kind == 'error':
+        s = context.osm2pgsql_outdata[1]
+    elif kind == 'standard':
+        s = context.osm2pgsql_outdata[0]
+    else:
+        assert not "Expect one of error, standard"
+
+    test_dir = (context.user_args.test_data_dir or Path('.')).resolve()
+    with open(test_dir / result_file, 'r') as fd:
+        expected = fd.read()
+        assert s == expected, \
+               f"Output does not match contents of {result_file}. Actual output:\n{s}"
+
 ################### Steps: Running Replication #####################
 
 @given("the replication service at (?P<base_url>.*)")

src/expire-tiles.cpp

@@ -136,28 +136,90 @@ expire_mode decide_expire_mode(TGEOM const &geom,
 
 } // anonymous namespace
 
-void expire_tiles_t::from_geometry(geom::polygon_t const &geom,
-                                   expire_config_t const &expire_config)
+void expire_tiles_t::build_tile_list(std::vector<uint32_t> *tile_x_list,
+                                     geom::ring_t const &ring, double tile_y)
 {
-    geom::box_t box;
-    auto const mode = decide_expire_mode(geom, expire_config, &box);
-
-    if (mode == expire_mode::boundary_only) {
-        from_polygon_boundary(geom, expire_config);
-        return;
+    assert(!ring.empty());
+    for (std::size_t i = 1; i < ring.size(); ++i) {
+        auto const t1 = coords_to_tile(ring[i]);
+        auto const t2 = coords_to_tile(ring[i - 1]);
+
+        if ((t1.y() < tile_y && t2.y() >= tile_y) ||
+            (t2.y() < tile_y && t1.y() >= tile_y)) {
+            auto const pos =
+                (tile_y - t1.y()) / (t2.y() - t1.y()) * (t2.x() - t1.x());
+            tile_x_list->push_back(static_cast<uint32_t>(std::clamp(
+                t1.x() + pos, 0.0, static_cast<double>(m_map_width - 1))));
+        }
     }
+}
 
+void expire_tiles_t::from_polygon_area(geom::polygon_t const &geom,
+                                       geom::box_t box)
+{
     if (!box.valid()) {
         box = geom::envelope(geom);
     }
-    from_bbox(box, expire_config);
+
+    // This uses a variation on a simple polygon fill algorithm, for instance
+    // described on https://alienryderflex.com/polygon_fill/ . For each row
+    // of tiles we find the intersections with the area boundary and "fill" in
+    // the tiles between them. Note that we don't need to take particular care
+    // about the boundary, because we simply use the algorithm we use for
+    // expiry along a line to do that, which will also take care of the buffer.
+
+    // Coordinates are numbered from bottom to top, tiles are numbered from top
+    // to bottom, so "min" and "max" are switched here.
+    auto const max_tile_y = static_cast<std::uint32_t>(
+        m_map_width * (0.5 - box.min().y() / tile_t::EARTH_CIRCUMFERENCE));
+    auto const min_tile_y = static_cast<std::uint32_t>(
+        m_map_width * (0.5 - box.max().y() / tile_t::EARTH_CIRCUMFERENCE));
+
+    std::vector<uint32_t> tile_x_list;
+
+    //  Loop through the tile rows from top to bottom
+    for (std::uint32_t tile_y = min_tile_y; tile_y < max_tile_y; ++tile_y) {
+
+        // Build a list of tiles crossed by the area boundary
+        tile_x_list.clear();
+        build_tile_list(&tile_x_list, geom.outer(),
+                        static_cast<double>(tile_y));
+        for (auto const &ring : geom.inners()) {
+            build_tile_list(&tile_x_list, ring, static_cast<double>(tile_y));
+        }
+
+        // Sort list of tiles from left to right
+        std::sort(tile_x_list.begin(), tile_x_list.end());
+
+        // Add the tiles between entering and leaving the area to expire list
+        assert(tile_x_list.size() % 2 == 0);
+        for (std::size_t i = 0; i < tile_x_list.size(); i += 2) {
+            if (tile_x_list[i] >= static_cast<uint32_t>(m_map_width - 1)) {
+                break;
+            }
+            if (tile_x_list[i + 1] > 0) {
+                for (std::uint32_t tile_x = tile_x_list[i];
+                     tile_x < tile_x_list[i + 1]; ++tile_x) {
+                    expire_tile(tile_x, tile_y);
+                }
+            }
+        }
+    }
 }
 
-void expire_tiles_t::from_polygon_boundary(geom::multipolygon_t const &geom,
-                                           expire_config_t const &expire_config)
+void expire_tiles_t::from_geometry(geom::polygon_t const &geom,
+                                   expire_config_t const &expire_config)
 {
-    for (auto const &sgeom : geom) {
-        from_polygon_boundary(sgeom, expire_config);
+    geom::box_t box;
+    auto const mode = decide_expire_mode(geom, expire_config, &box);
+
+    from_polygon_boundary(geom, expire_config);
+
+    // Only need to expire area if in full_area mode. If there is only a
+    // single tile expired the whole polygon is inside that tile and we
+    // don't need to do the polygon expire.
+    if (mode == expire_mode::full_area && m_dirty_tiles.size() > 1) {
+        from_polygon_area(geom, box);
     }
 }
 
@@ -167,15 +229,18 @@ void expire_tiles_t::from_geometry(geom::multipolygon_t const &geom,
     geom::box_t box;
     auto const mode = decide_expire_mode(geom, expire_config, &box);
 
-    if (mode == expire_mode::boundary_only) {
-        from_polygon_boundary(geom, expire_config);
-        return;
+    for (auto const &sgeom : geom) {
+        from_polygon_boundary(sgeom, expire_config);
     }
 
-    if (!box.valid()) {
-        box = geom::envelope(geom);
+    // Only need to expire area if in full_area mode. If there is only a
+    // single tile expired the whole polygon is inside that tile and we
+    // don't need to do the polygon expire.
+    if (mode == expire_mode::full_area && m_dirty_tiles.size() > 1) {
+        for (auto const &sgeom : geom) {
+            from_polygon_area(sgeom, geom::box_t{});
+        }
     }
-    from_bbox(box, expire_config);
 }
 
 // False positive: Apparently clang-tidy can not see through the visit()
@@ -258,35 +323,6 @@ void expire_tiles_t::from_line_segment(geom::point_t const &a,
     }
 }
 
-/*
- * Expire tiles within a bounding box
- */
-int expire_tiles_t::from_bbox(geom::box_t const &box,
-                              expire_config_t const &expire_config)
-{
-    /* 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 - 1);
-    int const min_tile_y =
-        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 - 1);
-    int const max_tile_y =
-        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);
-        for (int iterator_y = min_tile_y; iterator_y <= max_tile_y;
-             ++iterator_y) {
-            expire_tile(norm_x, static_cast<uint32_t>(iterator_y));
-        }
-    }
-    return 0;
-}
-
 quadkey_list_t expire_tiles_t::get_tiles()
 {
     quadkey_list_t tiles;

src/expire-tiles.hpp

@@ -40,8 +40,7 @@ class expire_tiles_t
     void from_polygon_boundary(geom::polygon_t const &geom,
                                expire_config_t const &expire_config);
 
-    void from_polygon_boundary(geom::multipolygon_t const &geom,
-                               expire_config_t const &expire_config);
+    void from_polygon_area(geom::polygon_t const &geom, geom::box_t box);
 
     void from_geometry(geom::nullgeom_t const & /*geom*/,
                        expire_config_t const & /*expire_config*/)
@@ -74,8 +73,6 @@ class expire_tiles_t
     void from_geometry_if_3857(geom::geometry_t const &geom,
                                expire_config_t const &expire_config);
 
-    int from_bbox(geom::box_t const &box, expire_config_t const &expire_config);
-
     /**
      * Get tiles as a vector of quadkeys and remove them from the expire_tiles_t
      * object.
@@ -107,6 +104,9 @@ class expire_tiles_t
 
     uint32_t normalise_tile_x_coord(int x) const;
 
+    void build_tile_list(std::vector<uint32_t> *tile_x_list,
+                         geom::ring_t const &ring, double tile_y);
+
     void from_line_segment(geom::point_t const &a, geom::point_t const &b,
                            expire_config_t const &expire_config);
 

tests/bdd/expire/expire.feature

@@ -0,0 +1,44 @@
+Feature: Test osm2pgsql-expire
+
+    Scenario: Test with invalid options
+        When running osm2pgsql-expire with parameters
+            | -z18 | -m | abc | {TEST_DATA_DIR}/expire/test-data.osm |
+        Then execution fails
+        Then the error output contains
+            """
+            Value for --mode must be 'boundary_only', 'full_area', or 'hybrid'
+            """
+
+    Scenario: Test with invalid options
+        When running osm2pgsql-expire with parameters
+            | -z18 | -m | full_area | -f | foo | {TEST_DATA_DIR}/expire/test-data.osm |
+        Then execution fails
+        Then the error output contains
+            """
+            Value for --format must be 'tiles' or 'geojson'
+            """
+
+    Scenario: Test expire for various objects on zoom 18 (geojson format)
+        When running osm2pgsql-expire with parameters
+            | -z18 | -m | full_area | -f | geojson | {TEST_DATA_DIR}/expire/test-data.osm |
+        Then execution is successful
+        Then the standard output matches contents of expire/test-z18-b0.geojson
+
+    Scenario: Test expire for various objects on zoom 18 (tiles format)
+        When running osm2pgsql-expire with parameters
+            | -z18 | -m | full_area | -f | tiles | {TEST_DATA_DIR}/expire/test-data.osm |
+        Then execution is successful
+        Then the standard output matches contents of expire/test-z18-b0.tiles
+
+    Scenario: Test expire for various objects on zoom 18 (geojson format)
+        When running osm2pgsql-expire with parameters
+            | -z18 | -m | full_area | -f | geojson | -b | 0.5 | {TEST_DATA_DIR}/expire/test-data.osm |
+        Then execution is successful
+        Then the standard output matches contents of expire/test-z18-b05.geojson
+
+    Scenario: Test expire for various objects on zoom 18 (tiles format)
+        When running osm2pgsql-expire with parameters
+            | -z18 | -m | full_area | -f | tiles | -b | 0.5 | {TEST_DATA_DIR}/expire/test-data.osm |
+        Then execution is successful
+        Then the standard output matches contents of expire/test-z18-b05.tiles
+