Merge pull request #2771 from smallsaucepan/clean-coords-fixes

Fixed cleanCoords to remove points with appropriate tenacity

packages/turf-clean-coords/index.ts

@@ -1,6 +1,8 @@
 import { Position } from "geojson";
 import { feature } from "@turf/helpers";
 import { getCoords, getType } from "@turf/invariant";
+import { booleanPointOnLine } from "@turf/boolean-point-on-line";
+import { lineString } from "@turf/helpers";
 
 // To-Do => Improve Typescript GeoJSON handling
 
@@ -99,62 +101,71 @@ function cleanCoords(
  * @returns {Array<number>} Cleaned coordinates
  */
 function cleanLine(line: Position[], type: string) {
-  var points = getCoords(line);
+  const points = getCoords(line);
   // handle "clean" segment
   if (points.length === 2 && !equals(points[0], points[1])) return points;
 
-  var newPoints = [];
-  var secondToLast = points.length - 1;
-  var newPointsLength = newPoints.length;
-
-  newPoints.push(points[0]);
-  for (var i = 1; i < secondToLast; i++) {
-    var prevAddedPoint = newPoints[newPoints.length - 1];
-    if (
-      points[i][0] === prevAddedPoint[0] &&
-      points[i][1] === prevAddedPoint[1]
-    )
-      continue;
-    else {
-      newPoints.push(points[i]);
-      newPointsLength = newPoints.length;
-      if (newPointsLength > 2) {
-        if (
-          isPointOnLineSegment(
-            newPoints[newPointsLength - 3],
-            newPoints[newPointsLength - 1],
-            newPoints[newPointsLength - 2]
-          )
-        )
-          newPoints.splice(newPoints.length - 2, 1);
-      }
+  const newPoints = [];
+
+  // Segments based approach. With initial segment a-b, keep comparing to a
+  // longer segment a-c and as long as b is still on a-c, b is a redundant
+  // point.
+  let a = 0,
+    b = 1,
+    c = 2;
+
+  // Guaranteed we'll use the first point.
+  newPoints.push(points[a]);
+  // While there is still room to extend the segment ...
+  while (c < points.length) {
+    if (booleanPointOnLine(points[b], lineString([points[a], points[c]]))) {
+      // b is on a-c, so we can discard point b, and extend a-b to be the same
+      // as a-c as the basis for comparison during the next iteration.
+      b = c;
+    } else {
+      // b is NOT on a-c, suggesting a-c is not an extension of a-b. Commit a-b
+      // as a necessary segment.
+      newPoints.push(points[b]);
+
+      // Make our a-b for the next iteration start from the end of the segment
+      // that was just locked in i.e. next a-b should be the current b-(b+1).
+      a = b;
+      b++;
+      c = b;
     }
+    // Plan to look at the next point during the next iteration.
+    c++;
   }
-  newPoints.push(points[points.length - 1]);
-  newPointsLength = newPoints.length;
-
-  // (Multi)Polygons must have at least 4 points, but a closed LineString with only 3 points is acceptable
-  if (
-    (type === "Polygon" || type === "MultiPolygon") &&
-    equals(points[0], points[points.length - 1]) &&
-    newPointsLength < 4
-  ) {
-    throw new Error("invalid polygon");
-  }
+  // No remaining points, so commit the current a-b segment.
+  newPoints.push(points[b]);
+
+  if (type === "Polygon" || type === "MultiPolygon") {
+    // For polygons need to make sure the start / end point wasn't one of the
+    // points that needed to be cleaned.
+    // https://github.com/Turfjs/turf/issues/2406
+    // For points [a, b, c, ..., z, a]
+    // if a is on line b-z, it too can be removed. New array becomes
+    // [b, c, ..., z, b]
+    if (
+      booleanPointOnLine(
+        newPoints[0],
+        lineString([newPoints[1], newPoints[newPoints.length - 2]])
+      )
+    ) {
+      newPoints.shift(); // Discard starting point.
+      newPoints.pop(); // Discard closing point.
+      newPoints.push(newPoints[0]); // Duplicate the new closing point to end of array.
+    }
 
-  if (type === "LineString" && newPointsLength < 3) {
-    return newPoints;
+    // (Multi)Polygons must have at least 4 points and be closed.
+    if (newPoints.length < 4) {
+      throw new Error("invalid polygon, fewer than 4 points");
+    }
+    if (!equals(newPoints[0], newPoints[newPoints.length - 1])) {
+      throw new Error("invalid polygon, first and last points not equal");
+    }
   }
 
-  if (
-    isPointOnLineSegment(
-      newPoints[newPointsLength - 3],
-      newPoints[newPointsLength - 1],
-      newPoints[newPointsLength - 2]
-    )
-  )
-    newPoints.splice(newPoints.length - 2, 1);
-
   return newPoints;
 }
 
@@ -170,35 +181,5 @@ function equals(pt1: Position, pt2: Position) {
   return pt1[0] === pt2[0] && pt1[1] === pt2[1];
 }
 
-/**
- * Returns if `point` is on the segment between `start` and `end`.
- * Borrowed from `@turf/boolean-point-on-line` to speed up the evaluation (instead of using the module as dependency)
- *
- * @private
- * @param {Position} start coord pair of start of line
- * @param {Position} end coord pair of end of line
- * @param {Position} point coord pair of point to check
- * @returns {boolean} true/false
- */
-function isPointOnLineSegment(start: Position, end: Position, point: Position) {
-  var x = point[0],
-    y = point[1];
-  var startX = start[0],
-    startY = start[1];
-  var endX = end[0],
-    endY = end[1];
-
-  var dxc = x - startX;
-  var dyc = y - startY;
-  var dxl = endX - startX;
-  var dyl = endY - startY;
-  var cross = dxc * dyl - dyc * dxl;
-
-  if (cross !== 0) return false;
-  else if (Math.abs(dxl) >= Math.abs(dyl))
-    return dxl > 0 ? startX <= x && x <= endX : endX <= x && x <= startX;
-  else return dyl > 0 ? startY <= y && y <= endY : endY <= y && y <= startY;
-}
-
 export { cleanCoords };
 export default cleanCoords;

packages/turf-clean-coords/package.json

@@ -4,7 +4,8 @@
   "description": "Removes redundant coordinates from a GeoJSON Geometry.",
   "author": "Turf Authors",
   "contributors": [
-    "Stefano Borghi <@stebogit>"
+    "Stefano Borghi <@stebogit>",
+    "James Beard <@smallsaucepan>"
   ],
   "license": "MIT",
   "bugs": {
@@ -58,6 +59,7 @@
     "@types/benchmark": "^2.1.5",
     "@types/tape": "^4.13.4",
     "benchmark": "^2.1.4",
+    "geojson-equality-ts": "^1.0.2",
     "load-json-file": "^7.0.1",
     "npm-run-all": "^4.1.5",
     "tape": "^5.9.0",
@@ -67,6 +69,7 @@
     "write-json-file": "^5.0.0"
   },
   "dependencies": {
+    "@turf/boolean-point-on-line": "workspace:^",
     "@turf/helpers": "workspace:^",
     "@turf/invariant": "workspace:^",
     "@types/geojson": "^7946.0.10",

packages/turf-clean-coords/test.ts

@@ -1,6 +1,7 @@
 import fs from "fs";
 import test from "tape";
 import path from "path";
+import { geojsonEquality } from "geojson-equality-ts";
 import { fileURLToPath } from "url";
 import { loadJsonFileSync } from "load-json-file";
 import { truncate } from "@turf/truncate";
@@ -38,7 +39,10 @@ test("turf-clean-coords", (t) => {
 
     if (process.env.REGEN)
       writeJsonFileSync(directories.out + filename, results);
-    t.deepEqual(results, loadJsonFileSync(directories.out + filename), name);
+    t.true(
+      geojsonEquality(results, loadJsonFileSync(directories.out + filename)),
+      name
+    );
   });
   t.end();
 });
@@ -151,3 +155,166 @@ test("turf-clean-coords -- prevent input mutation", (t) => {
   t.deepEqual(multiPolyBefore, multiPoly, "multiPolygon should NOT be mutated");
   t.end();
 });
+
+test("turf-clean-coords - north south lines - issue 2305", (t) => {
+  // From https://github.com/Turfjs/turf/issues/2305#issue-1287442870
+  t.deepEqual(
+    cleanCoords(
+      lineString([
+        [0, 0],
+        [0, 1],
+        [0, 0],
+      ])
+    ),
+    lineString([
+      [0, 0],
+      [0, 1],
+      [0, 0],
+    ]),
+    "northern turnaround point is kept"
+  );
+
+  // From https://github.com/Turfjs/turf/issues/2305#issue-1287442870
+  t.deepEqual(
+    cleanCoords(
+      lineString([
+        [0, 0],
+        [0, 0],
+        [0, 2],
+        [0, 2],
+        [0, 0],
+      ])
+    ),
+    lineString([
+      [0, 0],
+      [0, 2],
+      [0, 0],
+    ]),
+    "northern turnaround point is kept"
+  );
+
+  t.end();
+});
+
+test("turf-clean-coords - overly aggressive removal - issue 2740", (t) => {
+  // Issue 2740 is cleanCoords was too aggresive at removing points.
+  t.deepEqual(
+    cleanCoords(
+      lineString([
+        [0, 0],
+        [0, 2],
+        [0, 0],
+      ])
+    ),
+    lineString([
+      [0, 0],
+      [0, 2],
+      [0, 0],
+    ]),
+    "north-south retraced line turnaround point kept"
+  );
+
+  t.deepEqual(
+    cleanCoords(
+      lineString([
+        [0, 0],
+        [0, 1],
+        [0, 2],
+        [0, 3],
+        [0, 0],
+      ])
+    ),
+    lineString([
+      [0, 0],
+      [0, 3],
+      [0, 0],
+    ]),
+    "north-south retraced line properly cleaned"
+  );
+
+  t.deepEqual(
+    cleanCoords(
+      lineString([
+        [0, 0],
+        [0, 1],
+        [0, 2],
+        [0, -2],
+        [0, -1],
+        [0, 0],
+      ])
+    ),
+    lineString([
+      [0, 0],
+      [0, 2],
+      [0, -2],
+      [0, 0],
+    ]),
+    "north-south retraced past origin and back to start line properly cleaned"
+  );
+
+  t.end();
+});
+
+test("turf-clean-coords - start point protected - issue 2406", (t) => {
+  t.true(
+    geojsonEquality(
+      cleanCoords(
+        polygon([
+          [
+            [1, 3], // a
+            [3, 3], // b
+            [3, 1], // c
+            [3, -3], // d
+            [-3, -3], // e
+            [-3, 3], // f
+            [1, 3], // a
+          ],
+        ])
+      ),
+      polygon([
+        [
+          [-3, 3], // f
+          [3, 3], // b
+          [3, -3], // d
+          [-3, -3], // e
+          [-3, 3], // f
+        ],
+      ])
+    ),
+    "polygon start point (a) was also removed"
+  );
+
+  t.end();
+});
+
+test("turf-clean-coords - multipolygon - issue #918", (t) => {
+  // Copied from turf-simplify as (at heart) it's cleanCoords that's being
+  // tested here.
+  // simplify hangs on this input #918
+  t.throws(
+    () =>
+      cleanCoords(
+        multiPolygon([
+          [
+            [
+              [0, 90],
+              [0, 90],
+              [0, 90],
+              [0, 90],
+              [0, 90],
+              [0, 90],
+              [0, 90],
+              [0, 90],
+              [0, 90],
+              [0, 90],
+              [0, 90],
+            ],
+          ],
+        ])
+      ),
+    /invalid polygon/,
+    "invalid polygon"
+  );
+
+  t.end();
+});