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

New module @turf/boolean-overlaps #856

Merged
merged 12 commits into from
Jul 26, 2017
Merged

New module @turf/boolean-overlaps #856

merged 12 commits into from
Jul 26, 2017

Conversation

stebogit
Copy link
Collaborator

@stebogit stebogit commented Jul 22, 2017

Solves #286.
@DenisCarriere can you please take a look at the Typescript file?

Thanks @tcql for the great job! 👍

To-Do:

  • add support for Point & MultiPoint
  • update index.d.ts
  • refactor using @turf/line-intersects, segmentEach, flattenEach
  • rename module to @turf/boolean-overlap
  • add/test geometry support
/**
 * Takes two features and returns true or false whether or not they overlap, i.e. whether any pair 
 * of edges on the two polygons intersect. If there are any edge intersections, the polygons 
 * overlap.
 *
 * @name booleanOverlaps
 * @param  {Geometry|Feature<LineString|MultiLineString|Polygon|MultiPolygon>} feature1 input
 * @param  {Geometry|Feature<LineString|MultiLineString|Polygon|MultiPolygon>} feature2 input
 * @returns {boolean} true/false
 * @example
 * var poly1 = turf.polygon([[[18.70,-34.19],[18.93,-34.19],[18.93,-34],[18.70,-34],[18.70,-34.19]]]);
 * var poly2 = turf.polygon([[[18.52,-34.36],[18.79,-34.36],[18.79,-34.10],[18.52,-34.10],[18.52,-34.36]]]);
 * var line = turf.lineString([[18.62,-34.39],[18.87,-34.21]]);
 *
 * turf.booleanOverlaps(poly1, poly2)
 * //=true
 * turf.booleanOverlaps(poly2, line)
 * //=false
 */
module.exports = function (feature1, feature2) {

@stebogit stebogit added this to the 4.6.0 milestone Jul 22, 2017
@DenisCarriere
Copy link
Member

@stebogit Couldn't we also support Point & MultiPoint? Sounds like it would be an easy one to do and that way we could use this module for Every GeoJSON type.

@DenisCarriere
Copy link
Member

The definition looks good, however if we did support all Geometry Types then it would look something like this:

/// <reference types="geojson" />

type Feature = GeoJSON.Feature<any> | GeoJSON.GeometryObject

/**
 * http://turfjs.org/docs/#boolean-overlaps
 */
declare function overlaps(feature1: Feature, feature2: Feature): boolean;
declare namespace overlaps { }
export = overlaps;

* @param {Array<Array<number>>} line2 array of a segment coordinates
* @returns {boolean} segments intersect
*/
function doSegmentsIntersect(line1, line2) {
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't this be replaced with @turf/line-intersects? The module goes really fast if you only provide 2 vertex line segments.
https://github.com/Turfjs/turf/blob/master/packages/turf-line-intersect/index.js#L85

* @param {Array<Array<number|Array<number>>>} ring2 array of a LineString coordinates
* @returns {boolean} lines intersect
*/
function doLinesIntersect(ring1, ring2) {
Copy link
Member

Choose a reason for hiding this comment

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

We should build a segmentEach & segmentReduce module in @turf/meta for iterating over line segments. (most logic can be pulled from @turf/line-segment).

Copy link
Member

Choose a reason for hiding this comment

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

Done, segmentEach & segmentReduce is now available in PR #863.

You can use it right now (after PR merged) by using lerna bootstrap on an updated branch.


// This looks completely stupid ridiculous to have so many nested loops, but it supports
// multipolygons nicely. In the case of polygons or linestrings, the outer loops are only one iteration.
return coords1.some(function (rings1) {
Copy link
Member

Choose a reason for hiding this comment

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

We have flattenEach which solves this "stupid ridiculous" pattern 😝 (pulling directly from comment above).

Copy link
Member

@DenisCarriere DenisCarriere left a comment

Choose a reason for hiding this comment

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

Lots of this source code could be replaced with existing modules:

  • @turf/line-intersect

  • @turf/meta => flattenEach

  • Also to include Point & MultiPoint support would be ideal.

  • Change name to @turf/boolean-overlap (drop the S)

@DenisCarriere
Copy link
Member

Since this is one of the DE-9IM modules, we should use the same naming convention overlap instead of overlaps.

http://edndoc.esri.com/arcsde/9.0/general_topics/understand_spatial_relations.htm

@rowanwins had already started working on this in the past, might be worth including some of his test cases (PR #707 & turf/packages/boolean-overlap).

@stebogit
Copy link
Collaborator Author

stebogit commented Jul 22, 2017

Couldn't we also support Point & MultiPoint?

👍 great idea!
so, just to be all on the same page:

  • true if aPoint overlaps (i.e. equals coordinates) another Point
  • true if aPoint overlaps one point of a MultiPoint
  • true if ANY Point of a MultiPoint overlaps one point of a MultiPoint feature
  • false otherwise

correct?

I'll implement the changes you suggested using the available modules.
I'll also try to define the segmentEach & segmentReduce modules, I like the idea!

Since this is one of the DE-9IM modules, we should use the same naming convention overlap instead of overlaps.

Sure thing

@DenisCarriere
Copy link
Member

@stebogit You are spot on #856 (comment) 👍

@rowanwins
Copy link
Member

rowanwins commented Jul 24, 2017

Gday @stebogit

For verifying scenarios it's also worth using @DenisCarriere handy boolean-shapely module, you can see it in action here. Basically it allows us to check our definitions against a widely used python library, you'll just need to make sure you have python installed and the shapely lib.

@DenisCarriere
Copy link
Member

Thanks @rowanwins for sharing, that boolean-shapely module is pretty handy for testing purposes, overlaps is supported.

@stebogit
Copy link
Collaborator Author

@DenisCarriere @rowanwins
looking at the spatial relations article, it seems to me that the current implementation of @turf/boolean-overlap is actually kind of a mix of overlap and cross.
In fact it should only

compares two geometries of the same dimension and returns t (TRUE) if their intersection set results in a geometry different from both but of the same dimension.

Shall I implement this behavior?
It would only apply then to (Multi)Point, (Multi)LineString and (Multi)Polygon; following @turf/boolean-crosses example I would throw an error on Point input(s).

@rowanwins
Copy link
Member

Gday @stebogit

Yep to aim is to follow the implementation provided by those guidelines. If in doubt about how the rules play out check the scenarios against shapely.

Copy link
Member

@DenisCarriere DenisCarriere left a comment

Choose a reason for hiding this comment

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

👍 Looks good

@DenisCarriere DenisCarriere merged commit 1325e7d into master Jul 26, 2017
@DenisCarriere DenisCarriere deleted the overlaps branch July 26, 2017 18:28
This was referenced Jul 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants