From 9db90dddd1e5d4460e56452b8b9475047d8aec19 Mon Sep 17 00:00:00 2001
From: Oliver Tan <otan@cockroachlabs.com>
Date: Mon, 31 Aug 2020 08:42:22 -0700
Subject: [PATCH] geo: account for duplicate points in IsLinearRingCCW

Rings can have duplicate adjacent points, so account for this by
scanning potentially more to determing CCW-ness.

Release justification: low risk changes to new functionality

Release note: None
---
 pkg/geo/geo.go      |  41 +++++++++++----
 pkg/geo/geo_test.go | 120 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 152 insertions(+), 9 deletions(-)

diff --git a/pkg/geo/geo.go b/pkg/geo/geo.go
index cc6a0e33dc2b..27bf5ad8e5c1 100644
--- a/pkg/geo/geo.go
+++ b/pkg/geo/geo.go
@@ -621,17 +621,36 @@ func IsLinearRingCCW(linearRing *geom.LinearRing) bool {
 		}
 	}
 
-	// prevIdx is the previous point. If we are at the 0th point, the last coordinate
-	// is also the 0th point, so take the second last point.
-	// Note we don't have to apply this for "nextIdx" as we cap the search above at the
-	// second last vertex.
+	// Find the previous point in the ring that is not the same as smallest.
 	prevIdx := smallestIdx - 1
-	if smallestIdx == 0 {
-		prevIdx = linearRing.NumCoords() - 2
+	if prevIdx < 0 {
+		prevIdx = linearRing.NumCoords() - 1
+	}
+	for prevIdx != smallestIdx {
+		a := linearRing.Coord(prevIdx)
+		if a.X() != smallest.X() || a.Y() != smallest.Y() {
+			break
+		}
+		prevIdx--
+		if prevIdx < 0 {
+			prevIdx = linearRing.NumCoords() - 1
+		}
+	}
+	// Find the next point in the ring that is not the same as smallest.
+	nextIdx := smallestIdx + 1
+	if nextIdx >= linearRing.NumCoords() {
+		nextIdx = 0
+	}
+	for nextIdx != smallestIdx {
+		c := linearRing.Coord(nextIdx)
+		if c.X() != smallest.X() || c.Y() != smallest.Y() {
+			break
+		}
+		nextIdx++
+		if nextIdx >= linearRing.NumCoords() {
+			nextIdx = 0
+		}
 	}
-	a := linearRing.Coord(prevIdx)
-	b := smallest
-	c := linearRing.Coord(smallestIdx + 1)
 
 	// We could do the cross product, but we are only interested in the sign.
 	// To find the sign, reorganize into the orientation matrix:
@@ -640,6 +659,10 @@ func IsLinearRingCCW(linearRing *geom.LinearRing) bool {
 	//  1 x_c y_c
 	// and find the determinant.
 	// https://en.wikipedia.org/wiki/Curve_orientation#Orientation_of_a_simple_polygon
+	a := linearRing.Coord(prevIdx)
+	b := smallest
+	c := linearRing.Coord(nextIdx)
+
 	areaSign := a.X()*b.Y() - a.Y()*b.X() +
 		a.Y()*c.X() - a.X()*c.Y() +
 		b.X()*c.Y() - c.X()*b.Y()
diff --git a/pkg/geo/geo_test.go b/pkg/geo/geo_test.go
index ddeadf0c88aa..efd36ebe3c7d 100644
--- a/pkg/geo/geo_test.go
+++ b/pkg/geo/geo_test.go
@@ -859,3 +859,123 @@ func TestValidateGeomT(t *testing.T) {
 		})
 	}
 }
+
+func TestIsLinearRingCCW(t *testing.T) {
+	testCases := []struct {
+		desc     string
+		ring     *geom.LinearRing
+		expected bool
+	}{
+		{
+			desc: "flat linear ring",
+			ring: geom.NewLinearRingFlat(geom.XY, []float64{
+				0, 0,
+				0, 0,
+				0, 0,
+				0, 0,
+			}),
+			expected: false,
+		},
+		{
+			desc: "invalid linear ring with duplicate points at end deemed CW",
+			ring: geom.NewLinearRingFlat(geom.XY, []float64{
+				0, 0,
+				0, 0,
+				0, 0,
+				0, 0,
+				1, 0,
+				1, 0,
+				1, 0,
+			}),
+			expected: false,
+		},
+		{
+			desc: "invalid linear ring with duplicate points at start deemed CW",
+			ring: geom.NewLinearRingFlat(geom.XY, []float64{
+				0, 0,
+				0, 0,
+				0, 0,
+				0, 0,
+				-1, 1,
+				-1, 1,
+				-1, 1,
+				0, 0,
+			}),
+			expected: false,
+		},
+
+		{
+			desc: "CW linear ring",
+			ring: geom.NewLinearRingFlat(geom.XY, []float64{
+				0, 0,
+				1, 1,
+				1, 0,
+				0, 0,
+			}),
+			expected: false,
+		},
+		{
+			desc: "CW linear ring, first point is bottom right",
+			ring: geom.NewLinearRingFlat(geom.XY, []float64{
+				1, 0,
+				0, 0,
+				1, 1,
+				1, 0,
+			}),
+			expected: false,
+		},
+		{
+			desc: "CW linear ring, duplicate points for bottom right",
+			ring: geom.NewLinearRingFlat(geom.XY, []float64{
+				0, 0,
+				1, 1,
+				1, 0,
+				1, 0,
+				1, 0,
+				0, 0,
+			}),
+			expected: false,
+		},
+		{
+			desc: "CCW linear ring",
+			ring: geom.NewLinearRingFlat(geom.XY, []float64{
+				0, 0,
+				1, 0,
+				1, 1,
+				0, 0,
+			}),
+			expected: true,
+		},
+		{
+			desc: "CCW linear ring, duplicate points for bottom right",
+			ring: geom.NewLinearRingFlat(geom.XY, []float64{
+				0, 0,
+				1, 0,
+				1, 0,
+				1, 0,
+				1, 0,
+				1, 0,
+				1, 1,
+				0, 0,
+			}),
+			expected: true,
+		},
+		{
+			desc: "CCW linear ring, first point is bottom right",
+			ring: geom.NewLinearRingFlat(geom.XY, []float64{
+				1, 0,
+				1, 0,
+				1, 1,
+				0, 0,
+				1, 0,
+			}),
+			expected: true,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			require.Equal(t, tc.expected, IsLinearRingCCW(tc.ring))
+		})
+	}
+}