From 2d184012d7153063c3e517d1f8bed00dcf075abd Mon Sep 17 00:00:00 2001 From: Gautam Dey Date: Mon, 6 Aug 2018 15:50:45 -0700 Subject: [PATCH 1/2] Cleanup: removed unneeded constrained package We are no longer using the columnization for makevalid; no longer need the constrained package. --- .../tegola/internal/constrained/point.go | 35 ------- .../tegola/internal/constrained/point_test.go | 93 ------------------- 2 files changed, 128 deletions(-) delete mode 100644 planar/makevalid/tegola/internal/constrained/point.go delete mode 100644 planar/makevalid/tegola/internal/constrained/point_test.go diff --git a/planar/makevalid/tegola/internal/constrained/point.go b/planar/makevalid/tegola/internal/constrained/point.go deleted file mode 100644 index 321b4bba..00000000 --- a/planar/makevalid/tegola/internal/constrained/point.go +++ /dev/null @@ -1,35 +0,0 @@ -package constrained - -import "github.com/go-spatial/geom/cmp" - -type Point struct { - Pt [2]float64 - IsConstrained bool -} - -func (pt Point) XY() [2]float64 { - return pt.Pt -} - -func RotatePointsToPos(pts []Point, pos int) { - lp := len(pts) - if pos == 0 || pos >= lp { - return - } - is := make([]Point, lp) - copy(is, pts) - copy(pts, is[pos:]) - copy(pts[lp-pos:], is[:pos]) -} -func RotatePointsToLowestFirst(pts []Point) { - if len(pts) < 2 { - return - } - var fi int - for i := 1; i < len(pts); i++ { - if cmp.PointLess(pts[i].Pt, pts[fi].Pt) { - fi = i - } - } - RotatePointsToPos(pts, fi) -} diff --git a/planar/makevalid/tegola/internal/constrained/point_test.go b/planar/makevalid/tegola/internal/constrained/point_test.go deleted file mode 100644 index cfbae7f6..00000000 --- a/planar/makevalid/tegola/internal/constrained/point_test.go +++ /dev/null @@ -1,93 +0,0 @@ -package constrained - -import ( - "reflect" - "strconv" - "testing" -) - -func pt(x, y float64) Point { - return Point{Pt: [2]float64{x, y}} -} -func cpt(x, y float64) Point { - return Point{Pt: [2]float64{x, y}, IsConstrained: true} -} - -func TestRotatePointsToPos(t *testing.T) { - - type tcase struct { - pts []Point - pos int - epts []Point - } - - fn := func(t *testing.T, tc tcase) { - RotatePointsToPos(tc.pts, tc.pos) - if !reflect.DeepEqual(tc.pts, tc.epts) { - t.Errorf("point seq, expected %v got %v", tc.epts, tc.pts) - } - } - tests := [...]tcase{ - { - pts: []Point{pt(5, 4), pt(2, 2), pt(1, 1), pt(7, 2), pt(5, 3)}, - pos: 0, - epts: []Point{pt(5, 4), pt(2, 2), pt(1, 1), pt(7, 2), pt(5, 3)}, - }, - { - pts: []Point{pt(5, 4), pt(2, 2), pt(1, 1), pt(7, 2), pt(5, 3)}, - pos: 6, - epts: []Point{pt(5, 4), pt(2, 2), pt(1, 1), pt(7, 2), pt(5, 3)}, - }, - { - pts: []Point{pt(5, 4), pt(2, 2), pt(1, 1), pt(7, 2), pt(5, 3)}, - pos: 3, - epts: []Point{pt(7, 2), pt(5, 3), pt(5, 4), pt(2, 2), pt(1, 1)}, - }, - { - pts: []Point{pt(5, 4), pt(2, 2), pt(1, 1), pt(7, 2), pt(5, 3)}, - pos: 2, - epts: []Point{pt(1, 1), pt(7, 2), pt(5, 3), pt(5, 4), pt(2, 2)}, - }, - } - for i := range tests { - tc := tests[i] - t.Run(strconv.Itoa(i), func(t *testing.T) { fn(t, tc) }) - } -} - -func TestRotatePointsToLowestFirst(t *testing.T) { - - type tcase struct { - pts []Point - epts []Point - } - - fn := func(t *testing.T, tc tcase) { - RotatePointsToLowestFirst(tc.pts) - if !reflect.DeepEqual(tc.pts, tc.epts) { - t.Errorf("point seq, expected %v got %v", tc.epts, tc.pts) - } - } - tests := [...]tcase{ - { - pts: []Point{pt(5, 4), pt(2, 2), pt(1, 1), pt(7, 2), pt(5, 3)}, - epts: []Point{pt(1, 1), pt(7, 2), pt(5, 3), pt(5, 4), pt(2, 2)}, - }, - { - pts: []Point{pt(5, 4), pt(2, 2), pt(1, 1), pt(7, 2), pt(0, 0)}, - epts: []Point{pt(0, 0), pt(5, 4), pt(2, 2), pt(1, 1), pt(7, 2)}, - }, - { - pts: []Point{pt(5, 4)}, - epts: []Point{pt(5, 4)}, - }, - { - pts: []Point{}, - epts: []Point{}, - }, - } - for i := range tests { - tc := tests[i] - t.Run(strconv.Itoa(i), func(t *testing.T) { fn(t, tc) }) - } -} From f591a27d0e419b744afcded3f0f8ed923ab9c262 Mon Sep 17 00:00:00 2001 From: Gautam Dey Date: Fri, 3 Aug 2018 09:36:21 -0700 Subject: [PATCH 2/2] [issue-18] Found the issue in polygonForRing * Added testcase * issue had to do with calculating the correct i value. closes #18 --- planar/makevalid/tegola/cut.go | 6 +- planar/makevalid/tegola/cut_test.go | 22 +++ planar/makevalid/tegola/edge_triangle.go | 120 ++++++---------- planar/makevalid/tegola/edge_triangle_test.go | 50 +++++++ planar/makevalid/tegola/makevalid.go | 136 ------------------ 5 files changed, 118 insertions(+), 216 deletions(-) diff --git a/planar/makevalid/tegola/cut.go b/planar/makevalid/tegola/cut.go index 62a24ea7..3558e927 100644 --- a/planar/makevalid/tegola/cut.go +++ b/planar/makevalid/tegola/cut.go @@ -1,8 +1,10 @@ package tegola +import "fmt" + func cut(rng *[][2]float64, start, end int) (sliver [][2]float64) { - if start < 0 || end < 0 || start >= len(*rng) || end >= len(*rng) { - panic("Index out of bounds.") + if start < 0 || end < 0 || start >= len(*rng) || end > len(*rng) { + panic(fmt.Sprintf("index out of bounds[0 - %v], start %v end %v", len(*rng), start, end)) } switch { case end < start: diff --git a/planar/makevalid/tegola/cut_test.go b/planar/makevalid/tegola/cut_test.go index bd37b46e..f3fa54f9 100644 --- a/planar/makevalid/tegola/cut_test.go +++ b/planar/makevalid/tegola/cut_test.go @@ -6,6 +6,28 @@ import ( "testing" ) +//TestCutPanic test the panic conditions of the cut function. +func TestCutPanic(t *testing.T) { + test := func(start, end int) func(*testing.T) { + return func(t *testing.T) { + defer func() { + if r := recover(); r == nil { + t.Error("panic, expected panic got nil") + } + }() + // len for rng is 10 + rng := [][2]float64{{0, 0}, {0, 1}, {0, 2}, {0, 3}, {0, 4}, {0, 5}, {0, 6}, {0, 7}, {0, 8}, {0, 9}} + cut(&rng, start, end) + } + } + + t.Run("bad start", test(-1, 9)) + t.Run("bad end", test(5, -1)) + t.Run("bad start bigger then ring", test(11, 7)) + t.Run("bad end bigger then ring", test(1, 11)) + +} + func TestCut(t *testing.T) { type tcase struct { ring [][2]float64 diff --git a/planar/makevalid/tegola/edge_triangle.go b/planar/makevalid/tegola/edge_triangle.go index 0493db23..de656cc1 100644 --- a/planar/makevalid/tegola/edge_triangle.go +++ b/planar/makevalid/tegola/edge_triangle.go @@ -43,7 +43,7 @@ func triangulateGeometry(g geom.Geometry) (geom.MultiPolygon, error) { } } // TODO(gdey): We need to insure that GetTriangles does not dup the first point to the - // last point. + // last point. It may be better if it returned triangles and we moved triangles to Geom. return uut.GetTriangles() } @@ -68,14 +68,12 @@ func newEdgeIndexTriangles(ctx context.Context, hm planar.HitMapper, g geom.Geom } triangles = append(triangles, tri) } - if debug { - log.Printf("Got the following triangles: %v", triangles) - } t := edgeIndexTriangles{ triangles: make([]triangle, len(triangles)), edgeMap: edgeMapFromTriangles(triangles...), } + copy(t.triangles, triangles) return &t, nil } @@ -123,7 +121,7 @@ func (eit *edgeIndexTriangles) ringForTriangle(ctx context.Context, idx int, see var ok bool if debug { - log.Printf("Getting ring for Triangle %v", idx) + log.Printf("getting ring for triangle %v", idx) } seen[idx] = true @@ -137,17 +135,6 @@ func (eit *edgeIndexTriangles) ringForTriangle(ctx context.Context, idx int, see cidxs := []int{idx, idx, idx} cidx := cidxs[len(cidxs)-1] - if debug { - log.Printf("Triangles: Starting at %v", idx) - for i := range eit.triangles { - log.Printf("Triangle(%v) %v", i, eit.triangles[i]) - } - log.Printf("EdgeMap") - for k, v := range eit.edgeMap { - log.Printf("Edge %v => %v", k, v) - } - } - RING_LOOP: for { // A few sanity checks, were we cancelled, or reached the end of our walk. @@ -178,7 +165,7 @@ RING_LOOP: } if debug { - log.Printf("Check to see if we have seen the triangle we are going to jump to.") + log.Printf("check to see if we have seen the triangle we are going to jump to.") } // Check to see if we have reached the triangle before. @@ -187,7 +174,7 @@ RING_LOOP: continue } if debug { - log.Printf("We have encountered idx (%v) before at %v", cidx, i) + log.Printf("we have encountered idx (%v) before at %v", cidx, i) } // need to move all the points over tlen := len(rng) - (i + 1) @@ -212,19 +199,17 @@ RING_LOOP: } // polygonForRing returns a polygon for the given ring, this will destroy the ring. -func (eit *edgeIndexTriangles) polygonForRing(ctx context.Context, rng [][2]float64) (plyg [][][2]float64) { +func polygonForRing(ctx context.Context, rng [][2]float64) (plyg [][][2]float64) { if debug { - log.Printf("Turning ring into polygon.") + log.Printf("turn ring into polygon.") } - if len(rng) == 0 { + + if len(rng) <= 2 { return nil } // normalize ring cmp.RotateToLeftMostPoint(rng) - if len(rng) <= 3 { - return [][][2]float64{rng} - } pIdx := func(i int) int { if i == 0 { @@ -251,10 +236,8 @@ func (eit *edgeIndexTriangles) polygonForRing(ctx context.Context, rng [][2]floa // let's build an index of where the points that we are walking are. That way when we encounter the same // point we are able to “jump” to that point. ptIndex := map[[2]float64]int{} - var ( - ok bool - pidx, idx, nidx int - ) + var ok bool + var idx int // Let's walk the points for i := 0; i < len(rng); i++ { @@ -262,103 +245,84 @@ func (eit *edgeIndexTriangles) polygonForRing(ctx context.Context, rng [][2]floa if ctx.Err() != nil { return nil } - pt := rng[i] // check to see if we have already seen this point. - if idx, ok = ptIndex[pt]; !ok { + if idx, ok = ptIndex[rng[i]]; !ok { ptIndex[rng[i]] = i continue } - if debug { - log.Printf("Ring %v", rng) - log.Printf("Found Bubble at %v", i) - } - // We need to figure out which type of bubble this is. - pidx, nidx = pIdx(idx), nIdx(i) - - if debug { - log.Printf("Check to see what kind of bubble: %v : %v -- %v : %v ", pidx, idx, i, nidx) - log.Printf("Prev Pt: %v Next Pt: %v", rng[pidx], rng[nidx]) + pidx, nidx := pIdx(idx), nIdx(i) + // Clear out ptIndex of the values we are going to cut. + for j := idx; j <= i; j++ { + delete(ptIndex, rng[j]) } // ab…ba ring. So we need to remove all the way to a. if nidx != pidx && cmp.PointEqual(rng[pidx], rng[nidx]) { if debug { - log.Printf("bubble type ab … ba") - log.Printf("pidx: %v idx: %v i: %v nidx: %v", pidx, idx, i, nidx) + log.Printf("bubble type ab…ba: (% 5v)(% 5v) … (% 5v)(% 5v)", pidx, idx, i, nidx) } - // Cuts are in counter-clockwise order. - for j := idx; j <= i; j++ { - if debug { - log.Printf("Adding Point(%v) to cut %v", j, rng[j]) - } - delete(ptIndex, rng[j]) - } - // These may have wrapped; so don't range with them. - delete(ptIndex, rng[nidx]) + // Delete the a points as well. delete(ptIndex, rng[pidx]) sliver := cut(&rng, pidx, nidx) // remove ther start ab sliver = sliver[2:] - { // make a copy to free up memory. + if len(sliver) >= 3 { // make a copy to free up memory. ps := make([][2]float64, len(sliver)) copy(ps, sliver) cmp.RotateToLeftMostPoint(ps) plyg = append(plyg, ps) } - i = pidx - if i >= len(rng) { + if i = idx - 1; i < 0 { i = 0 } - ptIndex[rng[i]] = i continue } - // ab … bc - if debug { - log.Printf("bubble type ab…bc") - } - // Do a quick check to see if b is on ac + // do a quick check to see if b is on ac removeB := planar.IsPointOnLine(rng[i], rng[pidx], rng[nidx]) - // Cuts are in counter-clockwise order. - for j := idx; j <= i-1; j++ { - delete(ptIndex, rng[j]) + // ab … bc + if debug { + log.Printf("bubble type ab…bc: (% 5v)(% 5v) … (% 5v)(% 5v) == %t", pidx, idx, i, nidx, removeB) } sliver := cut(&rng, idx, i) - cmp.RotateToLeftMostPoint(sliver) - plyg = append(plyg, sliver) + if len(sliver) >= 3 { + cmp.RotateToLeftMostPoint(sliver) + plyg = append(plyg, sliver) + } if removeB { cut(&rng, idx, idx+1) + if idx == 0 { + break + } + i = idx - 1 } + } - i = idx - ptIndex[rng[i]] = i + if len(rng) <= 2 { + if debug { + log.Println("rng:", rng) + log.Println("plyg:", plyg) + panic("main ring is not correct!") + } + return nil } + plyg[0] = make([][2]float64, len(rng)) copy(plyg[0], rng) return plyg } func (eit *edgeIndexTriangles) PolygonForTriangle(ctx context.Context, idx int, seen map[int]bool) (plyg [][][2]float64) { - - if debug { - log.Printf("Polygon For Triangle: Started at %v", idx) - } // Get the external ring for the given triangle. - rng := eit.ringForTriangle(ctx, idx, seen) - if debug { - log.Printf("Got back ring: %v", rng) - } - plyg = eit.polygonForRing(ctx, rng) - - return plyg + return polygonForRing(ctx, eit.ringForTriangle(ctx, idx, seen)) } diff --git a/planar/makevalid/tegola/edge_triangle_test.go b/planar/makevalid/tegola/edge_triangle_test.go index e8ce13c5..1dc2d723 100644 --- a/planar/makevalid/tegola/edge_triangle_test.go +++ b/planar/makevalid/tegola/edge_triangle_test.go @@ -2,16 +2,22 @@ package tegola import ( "context" + "log" "reflect" "sort" "strconv" "testing" "github.com/go-spatial/geom" + "github.com/go-spatial/geom/cmp" "github.com/go-spatial/geom/planar" "github.com/go-spatial/geom/planar/makevalid/hitmap" ) +func init() { + log.SetFlags(log.Lshortfile | log.LstdFlags) +} + func TestNewEdgeIndexTriangles(t *testing.T) { type tcase struct { g geom.Geometry @@ -284,3 +290,47 @@ func TestPolygonForTriangle(t *testing.T) { }) } } + +func TestPolygonForRing(t *testing.T) { + type tcase struct { + rng [][2]float64 + plyg [][][2]float64 + } + + fn := func(t *testing.T, tc tcase) { + defer func() { + if r := recover(); r != nil { + t.Errorf("panic'd, expected nil, got %v", r) + } + }() + + plyg := polygonForRing(context.Background(), tc.rng) + if !cmp.PolygonEqual(tc.plyg, plyg) { + t.Errorf("polygon, expected %v got %v", tc.plyg, plyg) + } + } + + tests := [...]tcase{ + + { // issue-12 + rng: [][2]float64{ + {985, 1485}, {986, 1484}, {986, 1483}, {989, 1483}, {991, 1482}, + {993, 1483}, {993, 1485}, {992, 1487}, {989, 1489}, {988, 1490}, + {987, 1489}, {986, 1487}, {988, 1485}, {986, 1487}, + }, + plyg: [][][2]float64{ + { + {985, 1485}, {986, 1484}, {986, 1483}, {989, 1483}, {991, 1482}, + {993, 1483}, {993, 1485}, {992, 1487}, {989, 1489}, {988, 1490}, + {987, 1489}, + }, + }, + }, + } + + for i, tc := range tests { + tc := tc + t.Run(strconv.Itoa(i), func(t *testing.T) { fn(t, tc) }) + } + +} diff --git a/planar/makevalid/tegola/makevalid.go b/planar/makevalid/tegola/makevalid.go index 35ebffb8..e7365dd0 100644 --- a/planar/makevalid/tegola/makevalid.go +++ b/planar/makevalid/tegola/makevalid.go @@ -4,7 +4,6 @@ import ( "context" "errors" "log" - "math" "sort" "github.com/go-spatial/geom" @@ -134,141 +133,6 @@ func destructure(ctx context.Context, clipbox *geom.Extent, multipolygon *geom.M return nsegs, nil } -func snapToGrid(tolerance float64, segments []geom.Line) []geom.Line { - // We first need to get all the unque x values, and track how they relate to the segments and their points. - type pt struct { - ptidx int - // index to the segment - index int - // left point of the segment, otherwise it's the right point - isLeft bool - modified bool - } - - xs := make(map[float64][]*pt) - ys := make(map[float64][]*pt) - - shiftX := func(from, to float64) { - for j := range xs[from] { - idx, ptidx := xs[from][j].index, xs[from][j].ptidx - segments[idx][ptidx][0] = to - xs[from][j].modified = true - xs[to] = append(xs[to], xs[from][j]) - } - delete(xs, from) - } - shiftY := func(from, to float64) { - for j := range ys[from] { - idx, ptidx := xs[from][j].index, xs[from][j].ptidx - segments[idx][ptidx][1] = to - ys[from][j].modified = true - ys[to] = append(ys[to], ys[from][j]) - } - delete(ys, from) - } - - for i := range segments { - pt1, pt2 := segments[i][0], segments[i][1] - isLeft := cmp.PointEqual(pt1, pt2) - ppt1 := &pt{ - ptidx: 0, - index: i, - isLeft: isLeft, - } - xs[pt1[0]] = append(xs[pt1[0]], ppt1) - ys[pt1[1]] = append(ys[pt1[1]], ppt1) - ppt2 := &pt{ - ptidx: 1, - index: i, - isLeft: !isLeft, - } - xs[pt2[0]] = append(xs[pt2[0]], ppt2) - ys[pt2[1]] = append(ys[pt2[1]], ppt2) - - } - xkeys, ykeys := make([]float64, 0, len(xs)), make([]float64, 0, len(ys)) - for k, _ := range xs { - xkeys = append(xkeys, k) - } - sort.Float64s(xkeys) - - for i := 1; i < len(xkeys)-1; { - dist1 := math.Abs(xkeys[i] - xkeys[i-1]) - dist2 := math.Abs(xkeys[i+1] - xkeys[i]) - - if dist1 >= tolerance && dist2 >= tolerance { - // no change. - i++ - continue - } - - switch { - case dist1 < tolerance && dist2 < tolerance: - if dist1 <= dist2 { - shiftX(xkeys[i], xkeys[i-1]) - } else { - shiftX(xkeys[i], xkeys[i+1]) - } - case dist1 < tolerance: - shiftX(xkeys[i], xkeys[i-1]) - default: - shiftX(xkeys[i], xkeys[i+1]) - } - i = i + 2 - } - - sort.Float64s(ykeys) - for i := 1; i < len(ykeys)-1; { - dist1 := math.Abs(ykeys[i] - ykeys[i-1]) - dist2 := math.Abs(ykeys[i+1] - ykeys[i]) - - if dist1 >= tolerance && dist2 >= tolerance { - // no change. - i++ - continue - } - - switch { - case dist1 < tolerance && dist2 < tolerance: - if dist1 <= dist2 { - shiftY(ykeys[i], ykeys[i-1]) - } else { - shiftY(ykeys[i], ykeys[i+1]) - } - case dist1 < tolerance: - shiftY(ykeys[i], ykeys[i-1]) - default: - shiftY(ykeys[i], ykeys[i+1]) - } - i = i + 2 - } - - segmap := make(map[geom.Line]int) - - for i := range segments { - if cmp.PointEqual(segments[i][0], segments[i][1]) { - // Both points are equal, we can ignore it. - continue - } - // We have not seen this line, so we can add it to our map. - if _, ok := segmap[segments[i]]; !ok { - segmap[segments[i]] = i - } - } - - idxs := make([]int, 0, len(segmap)) - for _, v := range segmap { - idxs = append(idxs, v) - } - sort.Ints(idxs) - segs := make([]geom.Line, len(idxs)) - for i := range idxs { - segs[i] = segments[idxs[i]] - } - return segs - -} - func (mv *Makevalid) makevalidPolygon(ctx context.Context, clipbox *geom.Extent, multipolygon *geom.MultiPolygon) (*geom.MultiPolygon, error) { if debug { log.Printf("*Step 1 : Destructure the geometry into segments w/ the clipbox applied.")