-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GEOS: Wrap Make Valid #433
Changes from 4 commits
b9aa15b
88cd41b
c8e5fa0
1fa5b98
65c2837
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,17 @@ package geos | |
#cgo LDFLAGS: -lgeos_c | ||
#cgo CFLAGS: -Wall | ||
#include "geos_c.h" | ||
|
||
#define MAKE_VALID_MIN_VERSION "3.8.0" | ||
#define MAKE_VALID_MISSING ( \ | ||
GEOS_VERSION_MAJOR < 3 || \ | ||
(GEOS_VERSION_MAJOR == 3 && GEOS_VERSION_MINOR < 8) \ | ||
) | ||
#if MAKE_VALID_MISSING | ||
// This stub implementation always fails: | ||
GEOSGeometry *GEOSMakeValid_r(GEOSContextHandle_t handle, const GEOSGeometry* g) { return NULL; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this needed to prevent a compile-time error? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's correct. Prior versions of the GEOS library don't define the symbol at all, so we can define a stub version conditionally if the GEOS library doesn't do it for us (of course, that stub implementation wouldn't actually do anything, but we can check for that condition before invoking it). |
||
#endif | ||
|
||
*/ | ||
import "C" | ||
|
||
|
@@ -309,3 +320,19 @@ func SymmetricDifference(a, b geom.Geometry, opts ...geom.ConstructorOption) (ge | |
}) | ||
return result, wrap(err, "executing GEOSSymDifference_r") | ||
} | ||
|
||
// MakeValid can be used to convert an invalid geometry into a valid geometry. | ||
// It does this by keeping the original control points and constructing a new | ||
// geometry that is valid and similar (but not the same as) the original | ||
// invalid geometry. If the input geometry is valid, then it is returned | ||
// unaltered. | ||
func MakeValid(g geom.Geometry, opts ...geom.ConstructorOption) (geom.Geometry, error) { | ||
if C.MAKE_VALID_MISSING != 0 { | ||
return geom.Geometry{}, unsupportedGEOSVersionError{ | ||
C.MAKE_VALID_MIN_VERSION, "MakeValid"} | ||
} | ||
result, err := unaryOpG(g, opts, func(ctx C.GEOSContextHandle_t, g *C.GEOSGeometry) *C.GEOSGeometry { | ||
return C.GEOSMakeValid_r(ctx, g) | ||
}) | ||
return result, wrap(err, "executing GEOSMakeValid_r") | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,9 +7,9 @@ import ( | |
"github.com/peterstace/simplefeatures/geom" | ||
) | ||
|
||
func geomFromWKT(t *testing.T, wkt string) geom.Geometry { | ||
func geomFromWKT(t *testing.T, wkt string, opts ...geom.ConstructorOption) geom.Geometry { | ||
t.Helper() | ||
geom, err := geom.UnmarshalWKT(wkt) | ||
geom, err := geom.UnmarshalWKT(wkt, opts...) | ||
if err != nil { | ||
t.Fatalf("could not unmarshal WKT:\n wkt: %s\n err: %v", wkt, err) | ||
} | ||
|
@@ -767,3 +767,26 @@ func TestSymmetricDifference(t *testing.T) { | |
|
||
expectGeomEq(t, got, want, geom.IgnoreOrder) | ||
} | ||
|
||
func TestMakeValid(t *testing.T) { | ||
for i, tt := range []struct { | ||
input string | ||
wantOutput string | ||
}{ | ||
{ | ||
"POLYGON((0 0,2 2,2 0,0 2,0 0))", | ||
"MULTIPOLYGON(((0 0,0 2,1 1,0 0)),((2 0,1 1,2 2,2 0)))", | ||
}, | ||
} { | ||
t.Run(strconv.Itoa(i), func(t *testing.T) { | ||
in := geomFromWKT(t, tt.input, geom.DisableAllValidations) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a way to assert that the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's possible. Added in 65c2837 |
||
gotGeom, err := MakeValid(in) | ||
if _, ok := err.(unsupportedGEOSVersionError); ok { | ||
t.Skip(err) | ||
} | ||
expectNoErr(t, err) | ||
wantGeom := geomFromWKT(t, tt.wantOutput) | ||
expectGeomEq(t, gotGeom, wantGeom) | ||
}) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,7 +22,6 @@ char *marshal(GEOSContextHandle_t handle, const GEOSGeometry *g, size_t *size, c | |
GEOSGeometry const *noop(GEOSContextHandle_t handle, const GEOSGeometry *g) { | ||
return g; | ||
} | ||
|
||
*/ | ||
import "C" | ||
|
||
|
@@ -62,6 +61,7 @@ func newHandle() (*handle, error) { | |
h.release() | ||
return nil, errors.New("malloc failed") | ||
} | ||
C.memset((unsafe.Pointer)(h.errBuf), 0, 1024) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was a bug I noticed while I was here. Malloc is not going to actually zero out the memory content... (I've been spoilt by Go). |
||
|
||
h.context = C.sf_init(unsafe.Pointer(h.errBuf)) | ||
if h.context == nil { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the motivation for wrapping the
GEOSMakeValid_r
function? What challenges are there to implementing this natively in Go?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's something that Ken may find useful in his current work (might be better to take that discussion offline for further details).
The main challenges involved there would be coming up with an actual algorithm to do it (possibly that's the hard part) and the implementing it. It would most likely involve a DCEL data structure (which we have already implemented). I'd ballpark estimate 50hrs to 100hrs of effort would be required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there are plans to do this, maybe it's worth creating a ticket for this work; but if it's not in scope of the goals of simplefeatures, then all good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, it's definitely in scope. I've created a ticket here: #435