Skip to content

Commit

Permalink
Remove error and ctor options from direct ctors
Browse files Browse the repository at this point in the history
Modify direct constructors (`NewPoint` etc.) to no longer perform
validation at all. The direct constructors are for more advanced use
cases where geometries are being constructed from other geometries or
raw sequences of floats. Things like custom geometric algorithms come to
mind. Because these are for advanced use cases, making users validate
manually is reasonable.

Modifying direct constructors removes error handling mismatch, with
users no longer having to handle errors that can never occur (when
validation is disabled). It also eliminates the situation where
constructor options would sometimes be ignored, and simplifies the rules
around what validation occurs during direct construction (i.e. now none
is performed).
  • Loading branch information
peterstace committed Sep 20, 2023
1 parent 71e0813 commit 622929f
Show file tree
Hide file tree
Showing 27 changed files with 231 additions and 411 deletions.
6 changes: 2 additions & 4 deletions geom/accessor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,11 @@ func TestLineStringAccessor(t *testing.T) {
pt56 := xyCoords(5, 6)

t.Run("start", func(t *testing.T) {
want, err := NewPoint(pt12)
expectNoErr(t, err)
want := NewPoint(pt12)
expectGeomEq(t, ls.StartPoint().AsGeometry(), want.AsGeometry())
})
t.Run("end", func(t *testing.T) {
want, err := NewPoint(pt56)
expectNoErr(t, err)
want := NewPoint(pt56)
expectGeomEq(t, ls.EndPoint().AsGeometry(), want.AsGeometry())
})
t.Run("num points", func(t *testing.T) {
Expand Down
9 changes: 3 additions & 6 deletions geom/alg_convex_hull.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,9 @@ func convexHull(g Geometry) Geometry {
floats[2*i+1] = hull[i].Y
}
seq := NewSequence(floats, DimXY)
ring, err := NewLineString(seq)
if err != nil {
panic(fmt.Errorf("bug in monotoneChain routine - didn't produce a valid ring: %w", err))
}
poly, err := NewPolygon([]LineString{ring})
if err != nil {
ring := NewLineString(seq)
poly := NewPolygon([]LineString{ring})
if err := poly.Validate(); err != nil {
panic(fmt.Errorf("bug in monotoneChain routine - didn't produce a valid polygon: %w", err))
}
return poly.AsGeometry()
Expand Down
3 changes: 3 additions & 0 deletions geom/alg_set_op.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,9 @@ func setOp(a Geometry, include func([2]bool) bool, b Geometry) (Geometry, error)
if err != nil {
return Geometry{}, wrap(err, "internal error extracting geometry")
}
if err := g.Validate(); err != nil {
return Geometry{}, wrap(err, "invalid geometry produced by overlay")
}
return g, nil
}

Expand Down
8 changes: 8 additions & 0 deletions geom/ctor_options.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,3 +37,11 @@ func newOptionSet(opts []ConstructorOption) ctorOptionSet {
}
return cos
}

func validate(opts []ConstructorOption, g interface{ Validate() error }) error {
os := newOptionSet(opts)
if os.skipValidations {
return nil
}
return g.Validate()
}
24 changes: 4 additions & 20 deletions geom/dcel_extract_geometry.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,7 @@ func (d *doublyConnectedEdgeList) extractGeometry(include func([2]bool) bool) (G
if len(areals) == 1 {
return areals[0].AsGeometry(), nil
}
mp, err := NewMultiPolygon(areals)
if err != nil {
return Geometry{}, wrap(err, "could not extract areal geometry from DCEL")
}
return mp.AsGeometry(), nil
return NewMultiPolygon(areals).AsGeometry(), nil
case len(areals) == 0 && len(linears) > 0 && len(points) == 0:
if len(linears) == 1 {
return linears[0].AsGeometry(), nil
Expand Down Expand Up @@ -100,11 +96,7 @@ func (d *doublyConnectedEdgeList) extractPolygons(include func([2]bool) bool) ([

// Construct the polygon.
orderPolygonRings(rings)
poly, err := NewPolygon(rings)
if err != nil {
return nil, err
}
polys = append(polys, poly)
polys = append(polys, NewPolygon(rings))
}

sort.Slice(polys, func(i, j int) bool {
Expand Down Expand Up @@ -146,11 +138,7 @@ func extractPolygonRing(faceSet map[*faceRecord]bool, start *halfEdgeRecord, see
}
rotateSeqs(seqs, len(seqs)-minI)

ring, err := NewLineString(buildRingSequence(seqs))
if err != nil {
panic(fmt.Sprintf("could not create LineString: %v", err))
}
return ring
return NewLineString(buildRingSequence(seqs))
}

func buildRingSequence(seqs []Sequence) Sequence {
Expand Down Expand Up @@ -260,11 +248,7 @@ func (d *doublyConnectedEdgeList) extractLineStrings(include func([2]bool) bool)
e.origin.extracted = true
e.twin.origin.extracted = true

ls, err := NewLineString(e.seq)
if err != nil {
return nil, err
}
lss = append(lss, ls)
lss = append(lss, NewLineString(e.seq))
}
}
sort.Slice(lss, func(i, j int) bool {
Expand Down
25 changes: 8 additions & 17 deletions geom/dcel_re_noding.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ func ulpSizeForLine(ln line) float64 {
// such that when the two geometries are overlaid the only interactions
// (including self-interactions) between geometries are at nodes. Nodes that
// are close to each other are also snapped together.
//
// TODO: should be able to remove the error returns from this function and its
// descendants.
func reNodeGeometries(g1, g2 Geometry, mls MultiLineString) (Geometry, Geometry, MultiLineString, error) {
// Calculate the maximum ULP size over all control points in the input
// geometries. This size is a good indication of the precision that we
Expand Down Expand Up @@ -245,11 +248,7 @@ func reNodeLineString(ls LineString, cut cutSet, nodes nodeSet) (LineString, err
newCoords = append(newCoords, last.X, last.Y)
}

newLS, err := NewLineString(NewSequence(newCoords, DimXY), DisableAllValidations)
if err != nil {
return LineString{}, err
}
return newLS, nil
return NewLineString(NewSequence(newCoords, DimXY)), nil
}

func reNodeMultiLineString(mls MultiLineString, cut cutSet, nodes nodeSet) (MultiLineString, error) {
Expand All @@ -262,7 +261,7 @@ func reNodeMultiLineString(mls MultiLineString, cut cutSet, nodes nodeSet) (Mult
return MultiLineString{}, err
}
}
return NewMultiLineString(lss, DisableAllValidations), nil
return NewMultiLineString(lss), nil
}

func reNodePolygon(poly Polygon, cut cutSet, nodes nodeSet) (Polygon, error) {
Expand All @@ -275,11 +274,7 @@ func reNodePolygon(poly Polygon, cut cutSet, nodes nodeSet) (Polygon, error) {
for i := 0; i < n; i++ {
rings[i] = reNodedBoundary.LineStringN(i)
}
reNodedPoly, err := NewPolygon(rings, DisableAllValidations)
if err != nil {
return Polygon{}, err
}
return reNodedPoly, nil
return NewPolygon(rings), nil
}

func reNodeMultiPolygonString(mp MultiPolygon, cut cutSet, nodes nodeSet) (MultiPolygon, error) {
Expand All @@ -292,11 +287,7 @@ func reNodeMultiPolygonString(mp MultiPolygon, cut cutSet, nodes nodeSet) (Multi
return MultiPolygon{}, err
}
}
reNodedMP, err := NewMultiPolygon(polys, DisableAllValidations)
if err != nil {
return MultiPolygon{}, err
}
return reNodedMP, nil
return NewMultiPolygon(polys), nil
}

func reNodeGeometryCollection(gc GeometryCollection, cut cutSet, nodes nodeSet) (GeometryCollection, error) {
Expand All @@ -309,5 +300,5 @@ func reNodeGeometryCollection(gc GeometryCollection, cut cutSet, nodes nodeSet)
return GeometryCollection{}, err
}
}
return NewGeometryCollection(geoms, DisableAllValidations), nil
return NewGeometryCollection(geoms), nil
}
57 changes: 20 additions & 37 deletions geom/geojson_unmarshal.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,14 @@ func UnmarshalGeoJSON(input []byte, opts ...ConstructorOption) (Geometry, error)
ctype = DimXYZ
}

return geojsonNodeToGeometry(rootObj, ctype, opts)
g, err := geojsonNodeToGeometry(rootObj, ctype, opts)
if err != nil {
return Geometry{}, err
}
if err := validate(opts, g); err != nil {
return Geometry{}, err
}
return g, nil
}

type geojsonNode struct {
Expand Down Expand Up @@ -234,34 +241,28 @@ func detectCoordinatesLengths(node interface{}, hasLength map[int]bool) error {
}
}

// TODO: remove error handling and opts from this function
func geojsonNodeToGeometry(node interface{}, ctype CoordinatesType, opts []ConstructorOption) (Geometry, error) {
switch node := node.(type) {
case geojsonPoint:
coords, ok := oneDimFloat64sToCoordinates(node.coords, ctype)
if ok {
pt, err := NewPoint(coords, opts...)
return pt.AsGeometry(), err
return NewPoint(coords).AsGeometry(), nil
}
return NewEmptyPoint(ctype).AsGeometry(), nil
case geojsonLineString:
seq := twoDimFloat64sToSequence(node.coords, ctype)
ls, err := NewLineString(seq, opts...)
return ls.AsGeometry(), err
return NewLineString(seq).AsGeometry(), nil
case geojsonPolygon:
if len(node.coords) == 0 {
return Polygon{}.ForceCoordinatesType(ctype).AsGeometry(), nil
}
rings := make([]LineString, len(node.coords))
for i, coords := range node.coords {
seq := twoDimFloat64sToSequence(coords, ctype)
var err error
rings[i], err = NewLineString(seq, opts...)
if err != nil {
return Geometry{}, err
}
rings[i] = NewLineString(seq)
}
poly, err := NewPolygon(rings, opts...)
return poly.AsGeometry(), err
return NewPolygon(rings).AsGeometry(), nil
case geojsonMultiPoint:
// GeoJSON MultiPoints cannot contain empty Points.
if len(node.coords) == 0 {
Expand All @@ -271,11 +272,7 @@ func geojsonNodeToGeometry(node interface{}, ctype CoordinatesType, opts []Const
for i, coords := range node.coords {
coords, ok := oneDimFloat64sToCoordinates(coords, ctype)
if ok {
var err error
points[i], err = NewPoint(coords, opts...)
if err != nil {
return Geometry{}, err
}
points[i] = NewPoint(coords)
} else {
points[i] = NewEmptyPoint(ctype)
}
Expand All @@ -288,13 +285,9 @@ func geojsonNodeToGeometry(node interface{}, ctype CoordinatesType, opts []Const
lss := make([]LineString, len(node.coords))
for i, coords := range node.coords {
seq := twoDimFloat64sToSequence(coords, ctype)
var err error
lss[i], err = NewLineString(seq, opts...)
if err != nil {
return Geometry{}, err
}
lss[i] = NewLineString(seq)
}
return NewMultiLineString(lss, opts...).AsGeometry(), nil
return NewMultiLineString(lss).AsGeometry(), nil
case geojsonMultiPolygon:
if len(node.coords) == 0 {
return MultiPolygon{}.ForceCoordinatesType(ctype).AsGeometry(), nil
Expand All @@ -304,21 +297,11 @@ func geojsonNodeToGeometry(node interface{}, ctype CoordinatesType, opts []Const
rings := make([]LineString, len(coords))
for j, coords := range coords {
seq := twoDimFloat64sToSequence(coords, ctype)
var err error
rings[j], err = NewLineString(seq, opts...)
if err != nil {
return Geometry{}, err
}
}
var err error
polys[i], err = NewPolygon(rings, opts...)
if err != nil {
return Geometry{}, err
rings[j] = NewLineString(seq)
}
polys[i] = polys[i].ForceCoordinatesType(ctype)
polys[i] = NewPolygon(rings).ForceCoordinatesType(ctype)
}
mp, err := NewMultiPolygon(polys, opts...)
return mp.AsGeometry(), err
return NewMultiPolygon(polys).AsGeometry(), nil
case geojsonGeometryCollection:
if len(node.geoms) == 0 {
return GeometryCollection{}.ForceCoordinatesType(ctype).AsGeometry(), nil
Expand All @@ -331,7 +314,7 @@ func geojsonNodeToGeometry(node interface{}, ctype CoordinatesType, opts []Const
return Geometry{}, err
}
}
return NewGeometryCollection(children, opts...).AsGeometry(), nil
return NewGeometryCollection(children).AsGeometry(), nil
default:
panic(fmt.Sprintf("unexpected node: %#v", node))
}
Expand Down
8 changes: 1 addition & 7 deletions geom/line.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package geom

import (
"fmt"
"math"

"github.com/peterstace/simplefeatures/rtree"
Expand Down Expand Up @@ -49,15 +48,10 @@ func (ln line) centroid() XY {
}

func (ln line) asLineString() LineString {
ls, err := NewLineString(NewSequence([]float64{
return NewLineString(NewSequence([]float64{
ln.a.X, ln.a.Y,
ln.b.X, ln.b.Y,
}, DimXY))
if err != nil {
// Should not occur, because we know that a and b are distinct.
panic(fmt.Sprintf("could not create line string: %v", err))
}
return ls
}

func (ln line) intersectsXY(xy XY) bool {
Expand Down

0 comments on commit 622929f

Please sign in to comment.