Skip to content
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

Add ImplHelper::ValidOp Module to Provide Uniform Validity Checks #271

Merged
merged 7 commits into from
Oct 5, 2021
16 changes: 8 additions & 8 deletions doc/Factory-Compatibility.md
Original file line number Diff line number Diff line change
Expand Up @@ -159,8 +159,8 @@ _Note: This list is not exhaustive of all the methods defined by each factory. T
| `LineString#disjoint?(MultiLineString)` | ✅ | ✅ | ✅ | ✅ | ❌ | ✅ | ❌ |
| `LineString#disjoint?(MultiPolygon)` | ✅ | ✅ | ✅ | ✅ | ❌ | ✅ | ❌ |
| `LineString#intersects?(Point)` | ✅ | ✅ | ✅ | ✅ | ❌ | ✅ | ❌ |
| `LineString#intersects?(LineString)` | ✅ | ✅ | ✅ | ✅ | ❌ | ✅ | |
| `LineString#intersects?(LinearRing)` | ✅ | ✅ | ✅ | ✅ | ❌ | ✅ | |
| `LineString#intersects?(LineString)` | ✅ | ✅ | ✅ | ✅ | ❌ | ✅ | |
| `LineString#intersects?(LinearRing)` | ✅ | ✅ | ✅ | ✅ | ❌ | ✅ | |
| `LineString#intersects?(Polygon)` | ✅ | ✅ | ✅ | ✅ | ❌ | ✅ | ❌ |
| `LineString#intersects?(Collection)` | ✅ | ✅ | ✅ | ✅ | ❌ | ✅ | ❌ |
| `LineString#intersects?(MultiPoint)` | ✅ | ✅ | ✅ | ✅ | ❌ | ✅ | ❌ |
Expand All @@ -175,8 +175,8 @@ _Note: This list is not exhaustive of all the methods defined by each factory. T
| `LineString#touches?(MultiLineString)` | ✅ | ✅ | ✅ | ✅ | ❌ | ✅ | ❌ |
| `LineString#touches?(MultiPolygon)` | ✅ | ✅ | ✅ | ✅ | ❌ | ✅ | ❌ |
| `LineString#crosses?(Point)` | ✅ | ✅ | ✅ | ✅ | ❌ | ✅ | ❌ |
| `LineString#crosses?(LineString)` | ✅ | ✅ | ✅ | ✅ | | ✅ | |
| `LineString#crosses?(LinearRing)` | ✅ | ✅ | ✅ | ✅ | | ✅ | |
| `LineString#crosses?(LineString)` | ✅ | ✅ | ✅ | ✅ | | ✅ | |
| `LineString#crosses?(LinearRing)` | ✅ | ✅ | ✅ | ✅ | | ✅ | |
| `LineString#crosses?(Polygon)` | ✅ | ✅ | ✅ | ✅ | ❌ | ✅ | ❌ |
| `LineString#crosses?(Collection)` | ✅ | ✅ | ✅ | ✅ | ❌ | ✅ | ❌ |
| `LineString#crosses?(MultiPoint)` | ✅ | ✅ | ✅ | ✅ | ❌ | ✅ | ❌ |
Expand Down Expand Up @@ -251,8 +251,8 @@ _Note: This list is not exhaustive of all the methods defined by each factory. T
| `LinearRing#disjoint?(MultiLineString)` | ✅ | ✅ | ✅ | ✅ | ❌ | ✅ | ❌ |
| `LinearRing#disjoint?(MultiPolygon)` | ✅ | ✅ | ✅ | ✅ | ❌ | ✅ | ❌ |
| `LinearRing#intersects?(Point)` | ✅ | ✅ | ✅ | ✅ | ❌ | ✅ | ❌ |
| `LinearRing#intersects?(LineString)` | ✅ | ✅ | ✅ | ✅ | ❌ | ✅ | |
| `LinearRing#intersects?(LinearRing)` | ✅ | ✅ | ✅ | ✅ | ❌ | ✅ | |
| `LinearRing#intersects?(LineString)` | ✅ | ✅ | ✅ | ✅ | ❌ | ✅ | |
| `LinearRing#intersects?(LinearRing)` | ✅ | ✅ | ✅ | ✅ | ❌ | ✅ | |
| `LinearRing#intersects?(Polygon)` | ✅ | ✅ | ✅ | ✅ | ❌ | ✅ | ❌ |
| `LinearRing#intersects?(Collection)` | ✅ | ✅ | ✅ | ✅ | ❌ | ✅ | ❌ |
| `LinearRing#intersects?(MultiPoint)` | ✅ | ✅ | ✅ | ✅ | ❌ | ✅ | ❌ |
Expand All @@ -267,8 +267,8 @@ _Note: This list is not exhaustive of all the methods defined by each factory. T
| `LinearRing#touches?(MultiLineString)` | ✅ | ✅ | ✅ | ✅ | ❌ | ✅ | ❌ |
| `LinearRing#touches?(MultiPolygon)` | ✅ | ✅ | ✅ | ✅ | ❌ | ✅ | ❌ |
| `LinearRing#crosses?(Point)` | ✅ | ✅ | ✅ | ✅ | ❌ | ✅ | ❌ |
| `LinearRing#crosses?(LineString)` | ✅ | ✅ | ✅ | ✅ | | ✅ | |
| `LinearRing#crosses?(LinearRing)` | ✅ | ✅ | ✅ | ✅ | | ✅ | |
| `LinearRing#crosses?(LineString)` | ✅ | ✅ | ✅ | ✅ | | ✅ | |
| `LinearRing#crosses?(LinearRing)` | ✅ | ✅ | ✅ | ✅ | | ✅ | |
| `LinearRing#crosses?(Polygon)` | ✅ | ✅ | ✅ | ✅ | ❌ | ✅ | ❌ |
| `LinearRing#crosses?(Collection)` | ✅ | ✅ | ✅ | ✅ | ❌ | ✅ | ❌ |
| `LinearRing#crosses?(MultiPoint)` | ✅ | ✅ | ✅ | ✅ | ❌ | ✅ | ❌ |
Expand Down
1 change: 1 addition & 0 deletions lib/rgeo/cartesian.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

require_relative "cartesian/calculations"
require_relative "cartesian/feature_methods"
require_relative "cartesian/valid_op"
require_relative "cartesian/feature_classes"
require_relative "cartesian/factory"
require_relative "cartesian/interface"
Expand Down
9 changes: 9 additions & 0 deletions lib/rgeo/cartesian/feature_classes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ class PointImpl # :nodoc:
include RGeo::Feature::Point
include RGeo::ImplHelper::BasicGeometryMethods
include RGeo::ImplHelper::BasicPointMethods
include RGeo::ImplHelper::ValidOp
include RGeo::Cartesian::GeometryMethods
include RGeo::Cartesian::PointMethods
end
Expand All @@ -20,6 +21,7 @@ class LineStringImpl # :nodoc:
include RGeo::Feature::LineString
include RGeo::ImplHelper::BasicGeometryMethods
include RGeo::ImplHelper::BasicLineStringMethods
include RGeo::ImplHelper::ValidOp
include RGeo::Cartesian::GeometryMethods
include RGeo::Cartesian::LineStringMethods
end
Expand All @@ -29,6 +31,7 @@ class LineImpl # :nodoc:
include RGeo::ImplHelper::BasicGeometryMethods
include RGeo::ImplHelper::BasicLineStringMethods
include RGeo::ImplHelper::BasicLineMethods
include RGeo::ImplHelper::ValidOp
include RGeo::Cartesian::GeometryMethods
include RGeo::Cartesian::LineStringMethods
end
Expand All @@ -38,6 +41,7 @@ class LinearRingImpl # :nodoc:
include RGeo::ImplHelper::BasicGeometryMethods
include RGeo::ImplHelper::BasicLineStringMethods
include RGeo::ImplHelper::BasicLinearRingMethods
include RGeo::ImplHelper::ValidOp
include RGeo::Cartesian::GeometryMethods
include RGeo::Cartesian::LineStringMethods
end
Expand All @@ -46,13 +50,15 @@ class PolygonImpl # :nodoc:
include RGeo::Feature::Polygon
include RGeo::ImplHelper::BasicGeometryMethods
include RGeo::ImplHelper::BasicPolygonMethods
include RGeo::Cartesian::ValidOp
include RGeo::Cartesian::GeometryMethods
end

class GeometryCollectionImpl # :nodoc:
include RGeo::Feature::GeometryCollection
include RGeo::ImplHelper::BasicGeometryMethods
include RGeo::ImplHelper::BasicGeometryCollectionMethods
include RGeo::ImplHelper::ValidOp
include RGeo::Cartesian::GeometryMethods
end

Expand All @@ -61,6 +67,7 @@ class MultiPointImpl # :nodoc:
include RGeo::ImplHelper::BasicGeometryMethods
include RGeo::ImplHelper::BasicGeometryCollectionMethods
include RGeo::ImplHelper::BasicMultiPointMethods
include RGeo::ImplHelper::ValidOp
include RGeo::Cartesian::GeometryMethods
end

Expand All @@ -69,6 +76,7 @@ class MultiLineStringImpl # :nodoc:
include RGeo::ImplHelper::BasicGeometryMethods
include RGeo::ImplHelper::BasicGeometryCollectionMethods
include RGeo::ImplHelper::BasicMultiLineStringMethods
include RGeo::ImplHelper::ValidOp
include RGeo::Cartesian::GeometryMethods
include RGeo::Cartesian::MultiLineStringMethods
end
Expand All @@ -78,6 +86,7 @@ class MultiPolygonImpl # :nodoc:
include RGeo::ImplHelper::BasicGeometryMethods
include RGeo::ImplHelper::BasicGeometryCollectionMethods
include RGeo::ImplHelper::BasicMultiPolygonMethods
include RGeo::ImplHelper::ValidOp
include RGeo::Cartesian::GeometryMethods
end
end
Expand Down
67 changes: 47 additions & 20 deletions lib/rgeo/cartesian/feature_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,12 @@ def srid
def envelope
BoundingBox.new(factory).add(self).to_geometry
end

private

def graph
@graph ||= GeometryGraph.new(self)
end
end

module PointMethods # :nodoc:
Expand Down Expand Up @@ -50,31 +56,52 @@ def segments
end

def is_simple?
len = segments.length
return false if segments.any?(&:degenerate?)
return true if len == 1
return segments[0].s != segments[1].e if len == 2
segments.each_with_index do |seg, index|
nindex = index + 1
nindex = nil if nindex == len
return false if nindex && seg.contains_point?(segments[nindex].e)
pindex = index - 1
pindex = nil if pindex < 0
return false if pindex && seg.contains_point?(segments[pindex].s)
next unless nindex
oindex = nindex + 1
while oindex < len
oseg = segments[oindex]
return false if !(index == 0 && oindex == len - 1 && seg.s == oseg.e) && seg.intersects_segment?(oseg)
oindex += 1
end
end
true
# TODO: should we replace with graph.incident_edges.length == segments.length
# since graph isn't used elsewhere yet it might be more overhead.
Comment on lines +59 to +60
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not 100% sure I get your point there ^^

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same simplicity check (self-intersection) can be performed by creating the GeometryGraph of the LineString and doing the described comparison. But doing the check with graph will be slower since it has to perform an intersection check and then link all of the nodes whereas the current implementation only does an intersection check.

And since graph is not used elsewhere in any LineString method I figured the additional overhead is not necessary, but I could change it if we think it's better to do so.

li = SweeplineIntersector.new(segments)
li.proper_intersections.empty?
end

def length
segments.inject(0.0) { |sum, seg| sum + seg.length }
end

def crosses?(rhs)
case rhs
when Feature::LineString
crosses_line_string?(rhs)
else
super
end
end

private

# Determines if a cross occurs with another linestring.
# Process is to get the number of proper intersections in each geom
# then overlay and get the number of proper intersections from that.
# If the overlaid number is higher than the sum of individual self-ints
# then there is an intersection. Finally, we need to check the intersection
# to see that it is not a boundary point of either segment.
#
# @param rhs [Feature::LineString]
#
# @return [Boolean]
def crosses_line_string?(rhs)
self_ints = SweeplineIntersector.new(segments).proper_intersections
self_ints += SweeplineIntersector.new(rhs.segments).proper_intersections
overlay_ints = SweeplineIntersector.new(segments + rhs.segments).proper_intersections

(overlay_ints - self_ints).each do |int|
s1s = int.s1.s
s1e = int.s1.e
s2s = int.s2.s
s2e = int.s2.e
return true unless [s1s, s1e, s2s, s2e].include?(int.point)
end

false
end
end

module MultiLineStringMethods # :nodoc:
Expand Down
71 changes: 71 additions & 0 deletions lib/rgeo/cartesian/valid_op.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
# frozen_string_literal: true

module RGeo
module Cartesian
module ValidOp
include ImplHelper::ValidOp

def validity_helper
ValidOpHelpers
end
end

module ValidOpHelpers
include ImplHelper::ValidOpHelpers

module_function(*ImplHelper::ValidOpHelpers.singleton_methods) # rubocop:disable Style/AccessModifierDeclarations

module_function

# Checks that there are no invalid intersections between the components
# of a polygon.
#
# @param [RGeo::Feature::Polygon] poly
#
# @return [String] invalid_reason
def check_consistent_area(poly)
# Get set of unique coords
pts = poly.exterior_ring.coordinates.to_set
poly.interior_rings.each do |ring|
pts += ring.coordinates
end
num_points = pts.size

# if additional nodes were added, there must be an intersection
# through a boundary.
if poly.send(:graph).incident_edges.size > num_points
return ImplHelper::TopologyErrors::SELF_INTERSECTION
end

rings = [poly.exterior_ring] + poly.interior_rings
return ImplHelper::TopologyErrors::DUPLICATE_RINGS if rings.uniq.size != rings.size

nil
end

# Checks that the interior of a polygon is connected.
#
# Process to do this is to walk around an interior cycle of the
# exterior shell in the polygon's geometry graph. It will keep track
# of all the nodes it visited and if that set is a superset of the
# coordinates in the exterior_ring, the interior is connected, otherwise
# it is split somewhere.
#
# @param [RGeo::Feature::Polygon] poly
#
# @return [String] invalid_reason
def check_connected_interiors(poly)
exterior_coords = poly.exterior_ring.coordinates.to_set

visited = Set.new
poly.send(:graph).geom_edges.first.exterior_edge.and_connected do |hedge|
visited << hedge.origin.coordinates
end

return ImplHelper::TopologyErrors::DISCONNECTED_INTERIOR unless exterior_coords.subset?(visited)

nil
end
end
end
end
16 changes: 12 additions & 4 deletions lib/rgeo/geographic/projected_feature_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,14 @@ def is_simple?
projection.is_simple?
end

def valid?
projection.valid?
end

def invalid_reason
projection.invalid_reason
end

def boundary
boundary = projection.boundary
boundary ? factory.unproject(boundary) : nil
Expand Down Expand Up @@ -144,7 +152,7 @@ def self.included(klass)

private

def validate_geometry
def prepare_geometry
@y = 85.0511287 if @y > 85.0511287
@y = -85.0511287 if @y < -85.0511287
super
Expand All @@ -160,7 +168,7 @@ def length
module ProjectedLineStringMethods # :nodoc:
private

def validate_geometry
def prepare_geometry
@points = @points.map(&:canonical_point)
super
end
Expand All @@ -185,7 +193,7 @@ def centroid
module ProjectedPolygonMethods # :nodoc:
private

def validate_geometry
def prepare_geometry
super
unless projection
raise Error::InvalidGeometry, "Polygon failed assertions"
Expand All @@ -196,7 +204,7 @@ def validate_geometry
module ProjectedMultiPolygonMethods # :nodoc:
private

def validate_geometry
def prepare_geometry
super
unless projection
raise Error::InvalidGeometry, "MultiPolygon failed assertions"
Expand Down