Move linestring splitting into geom.cpp and fix it

Moves the linestring splitting into its own function split_linestring
and add tests. They show that the original function created invalid
linestrings in some cases, so this also fixes the algorithm.

src/geom.cpp

@@ -53,5 +53,77 @@ linestring_t::linestring_t(osmium::NodeRefList const &nodes,
     }
 }
 
+void split_linestring(linestring_t const &line, double split_at,
+                      std::vector<linestring_t> *out)
+{
+    double dist = 0;
+    osmium::geom::Coordinates prev_pt{};
+    out->emplace_back();
+
+    for (auto const this_pt : line) {
+        if (prev_pt.valid()) {
+            double const delta = distance(prev_pt, this_pt);
+
+            // figure out if the addition of this point would take the total
+            // length of the line in `segment` over the `split_at` distance.
+
+            if (dist + delta > split_at) {
+                auto const splits =
+                    (size_t)std::floor((dist + delta) / split_at);
+                // use the splitting distance to split the current segment up
+                // into as many parts as necessary to keep each part below
+                // the `split_at` distance.
+                osmium::geom::Coordinates ipoint;
+                for (size_t j = 0; j < splits; ++j) {
+                    double const frac =
+                        ((double)(j + 1) * split_at - dist) / delta;
+                    ipoint = interpolate(this_pt, prev_pt, frac);
+                    if (frac != 0.0) {
+                        out->back().add_point(ipoint);
+                    }
+                    // start a new segment
+                    out->emplace_back();
+                    out->back().add_point(ipoint);
+                }
+                // reset the distance based on the final splitting point for
+                // the next iteration.
+                if (this_pt == ipoint) {
+                    dist = 0;
+                    prev_pt = this_pt;
+                    continue;
+                } else {
+                    dist = distance(this_pt, ipoint);
+                }
+            } else {
+                dist += delta;
+            }
+        }
+
+        out->back().add_point(this_pt);
+
+        prev_pt = this_pt;
+    }
+
+    if (out->back().size() <= 1) {
+        out->pop_back();
+    }
+}
+
+void make_line(osmium::NodeRefList const &nodes, reprojection const &proj,
+               double split_at, std::vector<linestring_t> *out)
+{
+    linestring_t line{nodes, proj};
+
+    if (line.empty()) {
+        return;
+    }
+
+    if (split_at > 0.0) {
+        split_linestring(line, split_at, out);
+    } else {
+        out->emplace_back(std::move(line));
+    }
+}
+
 } // namespace geom
 

src/geom.hpp

@@ -118,6 +118,21 @@ operator<<(std::basic_ostream<TChar, TTraits> &out, const linestring_t &line)
     return out << ')';
 }
 
+/**
+ * Possibly split linestring into several linestrings making sure each one
+ * is no longer than split_at.
+ *
+ * \param line The input line string.
+ * \param split_at The maximum length (using Euclidean distance) of each
+ *                 resulting linestring.
+ * \param out Add resulting linestrings to this vector.
+ */
+void split_linestring(linestring_t const &line, double split_at,
+                      std::vector<linestring_t> *out);
+
+void make_line(osmium::NodeRefList const &nodes, reprojection const &proj,
+               double split_at, std::vector<linestring_t> *out);
+
 } // namespace geom
 
 #endif // OSM2PGSQL_GEOM_HPP

src/osmium-builder.cpp

@@ -71,75 +71,17 @@ osmium_builder_t::wkbs_t
 osmium_builder_t::get_wkb_line(osmium::WayNodeList const &nodes,
                                double split_at)
 {
-    wkbs_t ret;
-
-    bool const do_split = split_at > 0.0;
+    std::vector<linestring_t> linestrings;
+    geom::make_line(nodes, *m_proj, split_at, &linestrings);
 
-    double dist = 0;
-    osmium::geom::Coordinates prev_pt;
-    m_writer.linestring_start();
-    size_t curlen = 0;
+    wkbs_t ret;
 
-    for (auto const &node : nodes) {
-        if (!node.location().valid()) {
-            continue;
+    for (auto const &line : linestrings) {
+        m_writer.linestring_start();
+        for (auto const &coord : line) {
+            m_writer.add_location(coord);
         }
-
-        auto const this_pt = m_proj->reproject(node.location());
-        if (prev_pt.valid()) {
-            if (prev_pt == this_pt) {
-                continue;
-            }
-
-            if (do_split) {
-                double const delta = distance(prev_pt, this_pt);
-
-                // figure out if the addition of this point would take the total
-                // length of the line in `segment` over the `split_at` distance.
-
-                if (dist + delta > split_at) {
-                    auto const splits =
-                        (size_t)std::floor((dist + delta) / split_at);
-                    // use the splitting distance to split the current segment up
-                    // into as many parts as necessary to keep each part below
-                    // the `split_at` distance.
-                    osmium::geom::Coordinates ipoint;
-                    for (size_t j = 0; j < splits; ++j) {
-                        double const frac =
-                            ((double)(j + 1) * split_at - dist) / delta;
-                        ipoint = interpolate(this_pt, prev_pt, frac);
-                        m_writer.add_location(ipoint);
-                        ret.push_back(m_writer.linestring_finish(curlen + 1));
-                        // start a new segment
-                        m_writer.linestring_start();
-                        m_writer.add_location(ipoint);
-                        curlen = 1;
-                    }
-                    // reset the distance based on the final splitting point for
-                    // the next iteration.
-                    if (this_pt == ipoint) {
-                        dist = 0;
-                        m_writer.linestring_finish(0);
-                        m_writer.linestring_start();
-                        curlen = 0;
-                    } else {
-                        dist = distance(this_pt, ipoint);
-                    }
-                } else {
-                    dist += delta;
-                }
-            }
-        }
-
-        m_writer.add_location(this_pt);
-        ++curlen;
-
-        prev_pt = this_pt;
-    }
-
-    auto const wkb = m_writer.linestring_finish(curlen);
-    if (curlen > 1) {
-        ret.push_back(wkb);
+        ret.push_back(m_writer.linestring_finish(line.size()));
     }
 
     return ret;

tests/test-geom.cpp

@@ -62,3 +62,146 @@ TEST_CASE("geom::linestring_t", "[NoDB]")
     REQUIRE(it == ls1.cend());
 }
 
+TEST_CASE("geom::split_linestring w/o split", "[NoDB]")
+{
+    geom::linestring_t const line{Coordinates{0, 0}, Coordinates{1, 2},
+                                  Coordinates{2, 2}};
+
+    std::vector<geom::linestring_t> result;
+
+    geom::split_linestring(line, 10.0, &result);
+
+    REQUIRE(result.size() == 1);
+
+    REQUIRE(result[0].size() == 3);
+    REQUIRE(result[0][0] == Coordinates{0, 0});
+    REQUIRE(result[0][1] == Coordinates{1, 2});
+    REQUIRE(result[0][2] == Coordinates{2, 2});
+}
+
+TEST_CASE("geom::split_linestring with split 0.5", "[NoDB]")
+{
+    geom::linestring_t const line{Coordinates{0, 0}, Coordinates{1, 0}};
+
+    std::vector<geom::linestring_t> result;
+
+    geom::split_linestring(line, 0.5, &result);
+
+    REQUIRE(result.size() == 2);
+
+    REQUIRE(result[0].size() == 2);
+    REQUIRE(result[0][0] == Coordinates{0, 0});
+    REQUIRE(result[0][1] == Coordinates{0.5, 0});
+
+    REQUIRE(result[1].size() == 2);
+    REQUIRE(result[1][0] == Coordinates{0.5, 0});
+    REQUIRE(result[1][1] == Coordinates{1, 0});
+}
+
+TEST_CASE("geom::split_linestring with split 0.4", "[NoDB]")
+{
+    geom::linestring_t const line{Coordinates{0, 0}, Coordinates{1, 0}};
+
+    std::vector<geom::linestring_t> result;
+
+    geom::split_linestring(line, 0.4, &result);
+
+    REQUIRE(result.size() == 3);
+
+    REQUIRE(result[0].size() == 2);
+    REQUIRE(result[0][0] == Coordinates{0, 0});
+    REQUIRE(result[0][1] == Coordinates{0.4, 0});
+
+    REQUIRE(result[1].size() == 2);
+    REQUIRE(result[1][0] == Coordinates{0.4, 0});
+    REQUIRE(result[1][1] == Coordinates{0.8, 0});
+
+    REQUIRE(result[2].size() == 2);
+    REQUIRE(result[2][0] == Coordinates{0.8, 0});
+    REQUIRE(result[2][1] == Coordinates{1, 0});
+}
+
+TEST_CASE("geom::split_linestring with split 1.0 at start", "[NoDB]")
+{
+    geom::linestring_t const line{Coordinates{0, 0}, Coordinates{2, 0},
+                                  Coordinates{3, 0}, Coordinates{4, 0}};
+
+    std::vector<geom::linestring_t> result;
+
+    geom::split_linestring(line, 1.0, &result);
+
+    REQUIRE(result.size() == 4);
+
+    REQUIRE(result[0].size() == 2);
+    REQUIRE(result[0][0] == Coordinates{0, 0});
+    REQUIRE(result[0][1] == Coordinates{1, 0});
+
+    REQUIRE(result[1].size() == 2);
+    REQUIRE(result[1][0] == Coordinates{1, 0});
+    REQUIRE(result[1][1] == Coordinates{2, 0});
+
+    REQUIRE(result[2].size() == 2);
+    REQUIRE(result[2][0] == Coordinates{2, 0});
+    REQUIRE(result[2][1] == Coordinates{3, 0});
+
+    REQUIRE(result[3].size() == 2);
+    REQUIRE(result[3][0] == Coordinates{3, 0});
+    REQUIRE(result[3][1] == Coordinates{4, 0});
+}
+
+TEST_CASE("geom::split_linestring with split 1.0 in middle", "[NoDB]")
+{
+    geom::linestring_t const line{Coordinates{0, 0}, Coordinates{1, 0},
+                                  Coordinates{3, 0}, Coordinates{4, 0}};
+
+    std::vector<geom::linestring_t> result;
+
+    geom::split_linestring(line, 1.0, &result);
+
+    REQUIRE(result.size() == 4);
+
+    REQUIRE(result[0].size() == 2);
+    REQUIRE(result[0][0] == Coordinates{0, 0});
+    REQUIRE(result[0][1] == Coordinates{1, 0});
+
+    REQUIRE(result[1].size() == 2);
+    REQUIRE(result[1][0] == Coordinates{1, 0});
+    REQUIRE(result[1][1] == Coordinates{2, 0});
+
+    REQUIRE(result[2].size() == 2);
+    REQUIRE(result[2][0] == Coordinates{2, 0});
+    REQUIRE(result[2][1] == Coordinates{3, 0});
+
+    REQUIRE(result[3].size() == 2);
+    REQUIRE(result[3][0] == Coordinates{3, 0});
+    REQUIRE(result[3][1] == Coordinates{4, 0});
+}
+
+TEST_CASE("geom::split_linestring with split 1.0 at end", "[NoDB]")
+{
+    geom::linestring_t const line{Coordinates{0, 0}, Coordinates{1, 0},
+                                  Coordinates{2, 0}, Coordinates{4, 0}};
+
+    std::vector<geom::linestring_t> result;
+
+    geom::split_linestring(line, 1.0, &result);
+
+    REQUIRE(result.size() == 4);
+
+    REQUIRE(result[0].size() == 2);
+    REQUIRE(result[0][0] == Coordinates{0, 0});
+    REQUIRE(result[0][1] == Coordinates{1, 0});
+
+    REQUIRE(result[1].size() == 2);
+    REQUIRE(result[1][0] == Coordinates{1, 0});
+    REQUIRE(result[1][1] == Coordinates{2, 0});
+
+    REQUIRE(result[2].size() == 2);
+    REQUIRE(result[2][0] == Coordinates{2, 0});
+    REQUIRE(result[2][1] == Coordinates{3, 0});
+
+    REQUIRE(result[3].size() == 2);
+    REQUIRE(result[3][0] == Coordinates{3, 0});
+    REQUIRE(result[3][1] == Coordinates{4, 0});
+}
+