Implement GeometryFixer #433
Conversation
|
Also see RFC for JTS implementation. |
rouault
left a comment
rouault
left a comment
There was a problem hiding this comment.
Sorry for the very boring review, mostly/only on stylistic things. Feel free to keep or ditch :-) I didn't try to review the alg itself.
|
The major difference between Polygon with self-overlap
Polygon with overlapping holes
MultiPolygon with overlapping elements
|
|
Thoughts on just having |
|
I'm interested in hearing from our friends on postgis-devel, the old MakeValid behaviour was very specific due to requirements from a funder, and I'm not sure if a change in behaviour is acceptable to some. Same goes for all of downstream, though I do in general think the new results are (a) faster and (b) more aesthetically pleasing by and large. Past a certain level of awfulness the question "what is the valid form" becomes very open to interpretation. |
| { | ||
| if (holes == nullptr || holes->isEmpty()) | ||
| return shell->clone(); | ||
| return operation::overlayng::OverlayNGRobust::Overlay(shell, holes, |
There was a problem hiding this comment.
Why call a specific implementation instead of shell->difference(holes) ?
There was a problem hiding this comment.
Just following the JTS implementation... I guess this one has the fallbacks for sure (which the default does now too!)
There was a problem hiding this comment.
This is because in JTS OverlayNG is not (yet) the default. In GEOS it's fine to use the Geometry.difference() method.
| * @param poly1 a polygonal geometry (which may be empty) | ||
| * @return a combined polygonal geometry | ||
| */ | ||
| static geom::Geometry* combine( |
There was a problem hiding this comment.
No, because the combine method also contains logic to restrict to polygons and handle empty results appropriately for this context.
Anyway it looks like @pramsey changed this code to be inline. So this declaration should be removed?
How about two signatures: The second one is analogous to
That said, I've no real horse in this race -- my geometries are always valid 😉 |
|
I like the idea of one signature with parameters to make the old behaviour still available, unfortunately that doesn't change the fact that the default behaviour will alter (surprise!) which for some folks is a no-go. I'll poke this up on postgis-devel so we can get a read on it. |
|
How does this fixing method handle z values - are they discarded? (I can't see any examples in the unit tests with z) |
Yes, Z handling needs to be looked at. It probably mostly works, but that needs to be tested and any shortcomings fixed. |
…n-geometry-fixer
…n-geometry-fixer
…SMakeValidWithOptions() signature
capi/geos_c.h.in
Outdated
| * Options structure to fill out when passing options | ||
| * to GEOSMakeValidWithOptions. | ||
| */ | ||
| typedef struct { |
There was a problem hiding this comment.
Does this need to be defined in the implementation file only so that the layout is defined by the library we link against rather than the header we built against?
There was a problem hiding this comment.
I think it needs to be in the header, or the user can't form the right struct to pass in. I think I may have over-thought this though, copying the GDAL api for grid interpolation, which has 5 different algorithms each of which is several distinct options. But here I have two algorithms and only one currently has an option. A single struct with all the possible options makes more sense, and can avoid the void* in the call.
There was a problem hiding this comment.
Is GEOSBufferParams a pattern that can be copied? Pretty verbose tho...
There was a problem hiding this comment.
There's a level of conditionality in this (if algorithm A then options A, if B then options for B) that doesn't exist in the buffer case (which might have been better handled in named parameters, frankly, precisely because the options are fixed, but named parameters didn't exist at the time). I'm probably going to write up something a little more general for this form of options string like the GDAL CSList so at least they can share some utility code.
There was a problem hiding this comment.
That's a point. Although all the optionality exists on the GeoemetryFixer side, and at the moment MakeValid gets the no-option signature. So the options can just apply to GeometryFixer. If and when the default is changed, then we can add an option that indicates "use the MakeValid algorithm".
There was a problem hiding this comment.
I tried to keep structs out of that header as much as possible. Opaque void pointers are what made it possible so far to avoid breaking the ABI
There was a problem hiding this comment.
Ug. Makes me want to just have two function end points, less cluttered than having a whole new options object with creators/destructors/setters/getters for what is, currently, two options (algorithm and keepCollapsed).
There was a problem hiding this comment.
It's the price of a C API, I think.
capi/geos_c.h.in
Outdated
| * \param makeValidOptions to control the behaviour or the repair method. | ||
| * \return A repaired geometry. Caller must free with GEOSGeom_destroy(). | ||
| */ | ||
| extern GEOSGeometry GEOS_DLL *GEOSMakeValidWithOptions( |
There was a problem hiding this comment.
I'd call this GEOSMakeValidWithParams for similarity to GEOSBufferWithParams, unless there is some distinction between "options" and "params"
…n opaque params object instead of a header struct, following BufferParams model.
6 participants
Port of locationtech/jts#704
Adds a new approach to making valid geometry.