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

Expire (multi)polygons based on their shape, not the complete bounding
box. We use the existing implementation for the boundary expire, to get
the boundary right including the configured buffer. For the rest of the
area inside the polygon we use a "polygon drawing algorithm" to figure
out all tiles which are inside the polygon.

This also contains some changes to the BDD test code to allow running
the osm2pgsql-expire program and check its output against a file.

Author: joto

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
+