Validity Handling #275
Merged
Validity Handling #275
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7 tasks
|
GEOS 3.10 reworked their validity handling somewhat and the |
…int error on buffer_with_style test
Replace a manual validity check on MultiPolygon, which was iterating on each polygon combination, by a direct call of GEOSisValid.
…created SweeplineIntersector class to compute intersections on groups of segments. Segment#segment_intersection sligtly modifies the code from intersects_segment? and actually computes the intersection point (or nil if none). Edge cases for colinear segments are handled by return a single point from the intersection. intersects_segment? was modified to just check if segment_intersection is not nil. A SweeplineIntersector class was added that uses a basic sweepline algorithm to compute the intersections in a set of segments. This should be faster or as fast as the current validate_geometry intersection algorithm being used (more testing needed), since it only compares segments where their y-range includes the y value of the event the sweepline is handling. There are options to return all intersections or proper intersections that will filter out the connections in LineStrings. In the future, it is possible to make a Sweepline class and have the SweeplineIntersector inherit from it. This would allow us to use the sweepline more generically in other applications (some polygon clipping algos use a sweepline).
…rved_segments with Set, condense event creation branching.
Adds a PlanarGraph implementation based on a Doubly Connected Edge List (DCEL). A DCEL is a graph data structure of vertices and half-edges. Half-edges are directed edges where one segment can be represented by a pair of half-edges. Half-edges are linked so that faces can be traversed (although faces are not stored). The PlanarGraph accepts an array of segments to build it and additional edges can be added via the add_edge and add_edges methods. When edges are added, intersections are automatically recomputed and handled. When possible, existing half-edges are preserved so that geometries can reference those half-edges. Also adds GeometryGraph which is a convenient way to interface geometries to a planar graph and provides ways to find important half-edges in each geometry. I haven't done a full performance test yet, but one notable area of improvement is that everytime an edge or edges is added, all of the half-edges are re-linked when we likely only need to re-link the half-edges that originate at new vertices.
…d class in GeometryGraph
…alfEdge and Cartesian::GeometryGraph
… if no block is given
…idity checking of geometries. The ValidOp module provides methods based on simple geometric predicates to determine validity for a geometry and can be used as a fallback or overridden. The module can be overriden fully (in the case of Cartesian polygons) or just by implementing #invalid_reason (FFI and CAPI factories). Removes #validate_geometry from all classes and replaces them with #prepare_geometry. Any required validations were moved to the constructors. Adds the ValidityTest module to test/common to ensure that all classes of geometries validate in the same way.
ValidOp is to be included in feature classes and has the invalid_reason and valid? methods, as well as feature type specific methods that walk through the validity flow for each geometry type. ValidOpHelpers is a collection of functions where an object is passed to them. These perform the specific checks and return the invalid reason if applicable. Also added overrides for both modules for cartesian functions.
…nd fix tests, ValidOp so the spherical factory passes validity checks
Co-authored-by: Keith Doggett <keith.doggett887@gmail.com>
BuonOmo
force-pushed
the
validity-handling
branch
from
fcde425
to
23d64d2
Compare
BuonOmo
reviewed
Jan 14, 2022
ext/geos_c_impl/geometry.c
Outdated
| if (str) result = (str[0] == '\0' || !strcmp(str, "Valid Geometry")) ? Qnil : rb_str_new2(str); | ||
| else result = rb_str_new2("Exception"); | ||
| GEOSFree_r(self_data->geos_context, str); | ||
| // TODO: should we consider using the flag GEOSVALID_ALLOW_SELFTOUCHING_RING_FORMING_HOLE? |
There was a problem hiding this comment.
@keithdoggett since you implemented validity on the ruby part, may I have your opinion on that one?
There was a problem hiding this comment.
That would make it inconsistent with ours, but if you're already using GEOS you likely don't care about the ruby implementation, so I think it's ok to let the user pick if they want to allow that.
There was a problem hiding this comment.
Well since users never had this option and no one requested it I'm not sure I'm in favor of that. The question was more : we have to pick one option (either always or never allow), which is the ruby counterpart? I'll try and find an example in the GEOS test suite to answer that question myself
There was a problem hiding this comment.
def ring(pts, factory: RGeo::Geos.factory)
pts = pts.each_slice(2) if pts.first.is_a?(Numeric) && pts.size.even?
factory.linear_ring(
pts.map { factory.point(_1, _2) }
)
end
[RGeo::Geos.factory, RGeo::Cartesian.simple_factory].each do |f|
puts ring([0,0, 5,5, 0,10, 10,10, 5,5, 10,0, 0,0], factory: f).invalid_reason
endHence I'd be in favor of not adding the flag and sticking to SFS.
Another thing I found however that we may want to fix before releasing v3 is that when calling make_valid on the above ring we get nil. As we want to harmonize and always raise on error, I'd we should raise in that particular case. What do you say?
There was a problem hiding this comment.
Ahh, gotcha yes, I agree that we should not use that flag and the ring self-intersections be treated as invalid since that is not standard.
Good catch with make_valid return nil. Should definitely raise if it can't make it valid.
BuonOmo
reviewed
Jan 14, 2022
BuonOmo
reviewed
Jan 14, 2022
BuonOmo
reviewed
Jan 14, 2022
BuonOmo
force-pushed
the
validity-handling
branch
from
8938a72
to
e2f103f
Compare
I've added our `.yardopts` to make sure our options are used by rubydoc.info I've made examples point images to github to be sure we always see an image, wether we're on rubydoc or github. Completed TODOs. Added Klaxit to the gallery :)
BuonOmo
force-pushed
the
validity-handling
branch
from
e2f103f
to
41228d5
Compare
|
@BuonOmo thanks for writing that doc. I added a section at the end that clarifies which types of methods require a validity check first. |
(1) `make_valid` should raise when it cannot produce a geometry, not
return nil.
(2) +, -, * operator methods would generate uncallable `unsafe_+`
methods, hence we alias those with text.
|
Merging into v3-dev now since the workflow is satisfying, issues left will be adressed per PR |
BuonOmo
added a commit
that referenced
this pull request
Mar 17, 2022
Get rid of `lenient_assertions` in favor of another way of handling validity: never fail fast, fail when impossible to do otherwise. Co-authored-by: Keith Doggett <keith.doggett887@gmail.com> Co-authored-by: Quentin Wentzler <quentin.wentzler@klaxit.com> Co-authored-by: Ulysse Buonomo <buonomo.ulysse@gmail.com>
Closed
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Combination of #262 #269 and #271