GEOS: Wrap Make Valid #433
Conversation
|
Hmm, CI build is failing with |
|
I think I've got a decent solution for this in c8e5fa0 It basically checks the current GEOS version, and defines a C stub if it doesn't exist. It then checks the version and gives a friendly error if it's about to call the stub. |
| @@ -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.
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).
| }, | ||
| } { | ||
| t.Run(strconv.Itoa(i), func(t *testing.T) { | ||
| in := geomFromWKT(t, tt.input, geom.DisableAllValidations) |
There was a problem hiding this comment.
Is there a way to assert that the input geometry is invalid?
There was a problem hiding this comment.
Yes, that's possible. Added in 65c2837
| @@ -30,6 +30,9 @@ __Special thanks to Albert Teoh and Sameera Perera for contributing to this rele | |||
| - Fixes a bug in Simplify where invalid interior rings would be omitted rather | |||
| than producing an error. | |||
|
|
|||
| - Adds a wrapper in the `geos` package for the `GEOSMakeValid_r` function | |||
There was a problem hiding this comment.
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.
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.
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.
Good call, it's definitely in scope. I've created a ticket here: #435
| ) | ||
| #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.
Is this needed to prevent a compile-time error?
There was a problem hiding this comment.
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).
| @@ -30,6 +30,9 @@ __Special thanks to Albert Teoh and Sameera Perera for contributing to this rele | |||
| - Fixes a bug in Simplify where invalid interior rings would be omitted rather | |||
| than producing an error. | |||
|
|
|||
| - Adds a wrapper in the `geos` package for the `GEOSMakeValid_r` function | |||
There was a problem hiding this comment.
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.
|
Thanks for the review, I really appreciate it! 🥳 |
Description
GEOSMakeValid_rfunction from GEOS.Check List
Have you:
Added unit tests? Yes, just to confirm that the wrapping is working. Doesn't actually do extensive tests of the functionality.
Add cmprefimpl tests? (if appropriate?) N/A
Updated release notes? (if appropriate?) Yes.
Related Issue
Benchmark Results
N/A