Linestring.intersection method #167
Conversation
| zip(coordinates.dropLast(), coordinates.dropFirst()).forEach { segment1 in | ||
| zip(line.coordinates.dropLast(), line.coordinates.dropFirst()).forEach { segment2 in |
There was a problem hiding this comment.
I don't see how this method complexity could be reduced. Original implementation has tree search for corresponding segments, but this one just relies on denominator check in intersection(_:, _:).
There was a problem hiding this comment.
Original implementation has tree search for corresponding segments
Turf.js’s use of an optimized R-tree from RBush is key to the method’s high performance. There is a similar GKRTree in GameplayKit, and apparently some support in Core Data as well, but I’m not sure we’d want to maintain a separate codepath for non-Apple platforms. I also haven’t found any good RTree implementations in pure Swift yet; this package stores the tree inefficiently on disk. Let’s ticket out a port of RBush separately as a performance optimization.
There was a problem hiding this comment.
Added a ticket to track porting.
| zip(coordinates.dropLast(), coordinates.dropFirst()).forEach { segment1 in | ||
| zip(line.coordinates.dropLast(), line.coordinates.dropFirst()).forEach { segment2 in |
There was a problem hiding this comment.
Original implementation has tree search for corresponding segments
Turf.js’s use of an optimized R-tree from RBush is key to the method’s high performance. There is a similar GKRTree in GameplayKit, and apparently some support in Core Data as well, but I’m not sure we’d want to maintain a separate codepath for non-Apple platforms. I also haven’t found any good RTree implementations in pure Swift yet; this package stores the tree inefficiently on disk. Let’s ticket out a port of RBush separately as a performance optimization.
| zip(line.coordinates.dropLast(), line.coordinates.dropFirst()).forEach { segment2 in | ||
| if let intersection = Turf.intersection(LineSegment(segment1.0, segment1.1), | ||
| LineSegment(segment2.0, segment2.1)) { | ||
| let key = "\(intersection.latitude),\(intersection.longitude)" |
There was a problem hiding this comment.
A string representation is kind of weird, but it’s probably the most straightforward approach until swiftlang/swift#37398 lands, since we wouldn’t be able to have LocationCoordinate2D privately conform to Hashable.
There was a problem hiding this comment.
I'm a bit concerned about floating-point accuracy here. And about time complexity constant.
What if we implement a small struct like
private struct HashableCoordinate: Hashable {
var latitude: Double
var longitude: Double
}And use Set<HashableCoordinate> that is then converted to an array of coordinates.
Or is it too much? May it be useful somewhere else in Turf?
There was a problem hiding this comment.
Yes, that would work. Pedantically, Turf should represent GeoJSON Point objects as its own hashable Point type instead of reusing CLLocationCoordinate2D. We stuck to the latter type as a practical matter, to avoid excessive conversions to and from a very common type. This intersection method’s performance would be dominated by the non-RTree-based intersection search, but you have a point that the struct representation would give us more certainty.
There was a problem hiding this comment.
Implemented a Hashable wrapper
| zip(line.coordinates.dropLast(), line.coordinates.dropFirst()).forEach { segment2 in | ||
| if let intersection = Turf.intersection(LineSegment(segment1.0, segment1.1), | ||
| LineSegment(segment2.0, segment2.1)) { | ||
| let key = "\(intersection.latitude),\(intersection.longitude)" |
There was a problem hiding this comment.
I'm a bit concerned about floating-point accuracy here. And about time complexity constant.
What if we implement a small struct like
private struct HashableCoordinate: Hashable {
var latitude: Double
var longitude: Double
}And use Set<HashableCoordinate> that is then converted to an array of coordinates.
Or is it too much? May it be useful somewhere else in Turf?
| @@ -35,9 +37,9 @@ public func intersection(_ line1: LineSegment, _ line2: LineSegment) -> Location | |||
| longitude: line1.0.longitude + a * (line1.1.longitude - line1.0.longitude)) | |||
|
|
|||
| /// True if line 1 is finite and line 2 is infinite. | |||
| let intersectsWithLine1 = a > 0 && a < 1 | |||
| let intersectsWithLine1 = a >= 0 && a <= 1 | |||
There was a problem hiding this comment.
I am not sure if this was intended or not, but strict comparison here will not detect an intersection by line's ending points (for example when 2 LineStrings share the same coordinate). turf-line-intersect has such detection, but Turfjs's point-on-line implementation (which seems to be used as base reference for Swift port) for some reason does not. It seems like a bug to me, as when I seek for intersections between lines I usually want to include end points, especially if it can happen even with LineString's intermediate points, so I decided to fix that aswell.
| .init(latitude: 0, longitude: 5), | ||
| .init(latitude: 2, longitude: 9)]) | ||
|
|
||
| let intersections = lineString.intersections(with: intersectingLineString) |
There was a problem hiding this comment.
Should we test edge cases? Like "empty LineString" and "no intersections"?
…reference; refactored `LineString.intersections` method to explicify line segments.
… use hashable struct; added Unit tests; fixed(?) a bug where intersections were not found on end points.
Udumft
force-pushed
the
vk/141-linestring-intersection
branch
from
0f4b821 to
16049b2
Compare
|
LGTM |
Resolves #141
PR adds
Linestring.intersection(with:)method.