Initial commit of boolean DE-9IM module #707

rowanwins · 2017-05-02T11:46:05Z

As discussed attached is my current approach on a boolean style module for DE-9IM. I've been using the doco on this page as my guide so far.

DE-9IM modules

Extra Testing

donut polygons
lines [[0,0], [2,0]] and [[0,0], [1,0], [2,0]] are equal but don't have identical coordinates.

There are a few helper style functions within the helpers.js file.

Anyway happy for any feedback. I'm sure some of the algorithms could be improved, they are pretty brute force at the moment.
Cheers
Rowan

DenisCarriere · 2017-05-02T12:44:23Z

😄 Oh my! Looks great so far (quickly browsed the repo), I'll dive into further when I have hours and hours to dedicate to this 👍

dpmcmlxxvi · 2017-05-03T00:14:23Z

Looks good. Just a thought, the motivation behind bringing up the flattenEach issue in #692 was that I was implementing some of the DE-9IM functions in another project and found that it gets much simpler if you flatten the features first. That way you really only have to deal with 3 geometries (i.e., point, line, and polygon) and a 3x3 decision matrix. Otherwise, you have to deal with all 6 and a 6x6 decision matrix.

DenisCarriere · 2017-05-03T05:17:32Z

@dpmcmlxxvi Only having to deal with 3x3 makes a lot sense!

rowanwins · 2017-05-03T09:53:04Z

Thanks for the comment/idea @dpmcmlxxvi , I wasn't sure how much overhead flattening the features would add in terms of performance so it's probably worth setting up some benchmark tests, it would be nice to cater for fewer scenarios :)

rowanwins · 2017-05-03T13:04:10Z

Cross now mostly done...

DenisCarriere · 2017-05-03T13:43:53Z

👍 Keep them coming! Next steps would be isolate the modules to be in their own repo @turf/boolean-contains. It's already starting to be confusing to test them.

DenisCarriere · 2017-05-03T14:22:01Z

@rowanwins you might want to look into switch cases instead of a ton of if statements.

For example (using @dpmcmlxxvi's flatten approach):

var multiFeature1 = flatten(feature1);
var multiFeature2 = flatten(feature2);

geomEach(multiFeature1, function (geom1) {
    geomEach(multiFeature2, function (geom2) {
        switch (geom1.type) {
        case 'Point':
            switch (geom1.type) {
            case 'Point':
            case 'LineString':
            case 'Polygon':
            }
        case 'LineString':
            switch (geom1.type) {
            case 'Point':
            case 'LineString':
            case 'Polygon':
            }
        case 'Polygon':
            switch (geom1.type) {
            case 'Point':
            case 'LineString':
            case 'Polygon':
            }
        }
    });
});

Depending on which boolean operations, you might want to use geomReduce instead of geomEach. Example from commit 93a3400

DenisCarriere · 2017-05-03T14:26:43Z

@rowanwins Just noticed checkIfCoordArraysMatch & checkIfDeepCoordArraysMatch can be replaced using deep-equal.

var deepEqual = require('deep-equal');

deepEqual(MultiPoint.geometry.coordinates[i], Point.geometry.coordinates)

@rowanwins

- Support Geometry Objects - Use deep-equal - Use Switches CC: @rowanwins

DenisCarriere · 2017-05-04T00:42:46Z

packages/turf-boolean-contains/index.js

+
+// TO DO - Work out how to check if line is in line (can potentially use line overlap module)
+// Also need to make sure lines are exactly the same, eg the second must be smaller than the first
+function isLineOnLine(LineString1, LineString2) {


t.equal(contains(line1, line3), false, 'A line fails that lies partially outside the other line');

Test fail because line should be completely on line (returns true because of partial match).

Might be worth adding a isLineCompletelyOnLine(line1, line2) method.

Gday @DenisCarriere

As a general comment I feel like the line booleans are the trickiest to get right at this stage, acknowledge that there is some work needed there

Never said it was easy 👍 Keep it up!

rowanwins · 2017-05-04T12:18:14Z

Just on the tests @DenisCarriere I dont think they are really too bad, if you want to test a individual boolean simply cd into the relevant directory such as test/contains and run node test,

I think it would actually be a fair bit more overhead to split them all out.

DenisCarriere · 2017-05-04T17:08:48Z

Just on the tests @DenisCarriere I dont think they are really too bad

I disagree, it's going to be a nightmare when there's 9 different test cases all in one repo.

Especially when we start adding fixtures to each repo (larger GeoJSON).

Also, we might end up including a test fixture folder for true/false.

test/true/<filename>.geojson
test/false/<filename>.geojson

That way it's easier for someone to submit a random GeoJSON that has the wrong expected output (true/false).

DenisCarriere · 2017-05-04T17:10:30Z

Also for modules that depend on other modules, we would simply just add it as a dependency like @turf/boolean-within would depend on @turf/boolean-contains.

dpmcmlxxvi · 2017-05-07T00:02:38Z

packages/turf-boolean-contains/index.js

+            return false;
+        }
+    }
+    return foundAMatch > 0 && foundAMatch < MultiPoint1.coordinates.length;


The second condition here excludes the two geometries being equal. Not sure if that is actually in the definition of contains. According to DE-9IM the definition is:

No point of MultiPoint2 must be in the exterior of MultiPoint1

At least 1 point in the interior of MultiPoint2 must be in the interior of MultiPoint1

Since the points themselves are their own interior, the two being equal would seem to meet that criteria. Could be wrong, the DE-9IM definitions are sometimes tricky to parse.

dpmcmlxxvi · 2017-05-07T00:06:10Z

packages/turf-boolean-contains/index.js

+}
+
+// http://stackoverflow.com/a/11908158/1979085
+function isPointOnLine(LineString, Point) {


This seems to be allowing the point to land on the line end points. That would place the point on the line boundary and therefore no point lies on the interior of the line (see above definition of contains). I think the end points should be excluded.

dpmcmlxxvi · 2017-05-07T00:08:44Z

packages/turf-boolean-contains/index.js

+    return output;
+}
+
+function isMultiPointOnLine(LineString, MultiPoint) {


Same issue as above. No check that any point is in the interior of line. As you probably have noticed already the edge cases are the real pain when implementing these booleans functions.

dpmcmlxxvi · 2017-05-07T00:09:46Z

packages/turf-boolean-contains/index.js

+    return inside(Point, Polygon);
+}
+
+function isMultiPointInPoly(Polygon, MultiPoint) {


Same interior point issue as above.

dpmcmlxxvi · 2017-05-07T00:14:40Z

packages/turf-boolean-contains/index.js

+    return output;
+}
+
+function isLineInPoly(Polygon, Linestring) {


I think this approach will only work for convex polygons. Concave or polygons with holes can fail this. Think of a donut with a line that only crosses the hole.

I think it would be safer to first check if the line intersects the polygon boundaries (including holes). If it doesn't then check that at least one point is in polygon interior.

👍 Polygons with holes will be hard to implement, @rowanwins I think at first lets just get it working with only outer rings and afterwards we work on the inner ring validation (donuts).

dpmcmlxxvi · 2017-05-07T00:16:10Z

packages/turf-boolean-contains/index.js

+
+// See http://stackoverflow.com/a/4833823/1979085
+// TO DO - Need to handle if the polys are the same, eg there must be some outer coordinates different
+function isPolyInPoly(Polygon1, Polygon2) {


I think this has the same issue as isLineInPoly above.

dpmcmlxxvi · 2017-05-07T00:19:54Z

packages/turf-boolean/equal.js

+*/
+
+module.exports = function (feature1, feature2) {
+    return checkIfDeepCoordArraysMatch(feature1.geometry.coordinates, feature2.geometry.coordinates) && feature1.geometry.type === feature2.geometry.type;


Might want to add a warning that this is only testing that the coordinates are identical. It is not testing that the geometries are equal. For example, the lines [[0,0], [2,0]] and [[0,0], [1,0], [2,0]] are equal but don't have identical coordinates.

This one sounds like it would be real tricky to write. Would be worth including as a test case or add it to the JSDocs description.

dpmcmlxxvi · 2017-05-07T00:22:02Z

packages/turf-boolean/index.js

+* var along = turf.contains(line, point);
+*/
+
+module.exports = {


@DenisCarriere Just wondering what the policy is on namespacing functionality. I thought I recalled reading in some other thread that Turf enforced a flat structure and namespacing functionality was discouraged. May be mis-remembering.

Agree, that's why I suggested to have all the boolean modules within their own unique repos. I've only converted @turf/boolean-contains so far from this PR.

@rowanwins Lets stick with 1 PR per boolean operation & each operation should be within their own unique repo, @turf/boolean can group them all up afterwards.

dpmcmlxxvi · 2017-05-07T00:28:55Z

packages/turf-boolean/touch.js

+            }
+        }
+    }
+    return onStartOrEnd === 1 && pointIsSomewhereOnLine === 0;


Just a nit-pick. You can probably return inside the loop if the second criteria is broken. That way you don't iterate through all the points if you don't need to. Also, I think the first criteria should read onStartOrEnd > 0. It could be both ends that are touched.

dpmcmlxxvi · 2017-05-13T17:57:19Z

@rowanwins @DenisCarriere Just a quick note on turf-overlaps, it checks for overlaps by testing for intersections. However, you can have polygons with edges that intersect without actually overlapping.

rowanwins · 2017-05-15T11:55:34Z

Just as a heads up on this @DenisCarriere I've created a turf-boolean-helpers module where I'm storing lots of the common code,

For example isPointOnLineSegment gets used in pretty much every module. As a further FYI I've now adjusted it to include a flag for isPointOnInteriorOnly as per @dpmcmlxxvi suggestion.

Any qualms with this approach? I'm not sure how many functions will end up in here but probably half a dozen I'd guess.

DenisCarriere · 2017-05-15T20:49:10Z

👍 I like the idea of having @turf/boolean-helpers, it makes sense having a collection of methods that are shared between many modules. If one of those boolean-helpers methods because too elaborate then we can always convert into it's own repo.

DenisCarriere · 2017-05-15T21:37:01Z

@rowanwins I would refrain from adding any eslint-disable since most of these warning message are valuable.

[eslint] Unnecessary use of boolean literals in conditional expression (no-unneeded-ternary)

Example:

return !isPointOnLine(geom2, geom1) ? true : false; //eslint-disable-line no-unneeded-ternary

Refactored to:

return !isPointOnLine(geom2, geom1) || false;

Since isPointOnLine should only returns a boolean, you also be dropping the false.

return !isPointOnLine(geom2, geom1);

rowanwins · 2017-05-15T23:00:22Z

ah that's how you get rid of that stupid error, thanks @DenisCarriere , learn something new everyday!

dpmcmlxxvi · 2017-05-15T23:52:14Z

packages/turf-boolean-cross/test.js

@@ -17,7 +17,8 @@ test('turf-boolean-cross', t => {

    t.equal(cross(line1, line2), true, 'True if lines cross');
    t.equal(cross(line1, line3), false, 'False if lines cross');
-    t.equal(cross(line1, line4), false, 'False if lines only touch');
+    // **== ERROR ==**
+    // t.equal(cross(line1, line4), false, 'False if lines only touch');


Just wondering if the test library provides a skip method to gracefully skip a test so that it still shows up listed but just not executed. That way it doesn't get forgotten but won't cause a failure. I know mocha lets you do that by just throwing in .skip( before the test. According to the tape docs it looks like they do though I haven't tried it.

That's most likely a better idea, there's still tons of tests still missing, lots of work left to do

Prevents accidental dotfiles from being read by `fs`

- Update JSDocs - Add bench.js - Add index.d.ts - Add LICENSE

DenisCarriere · 2017-06-06T14:01:14Z

@rowanwins have you tried this library? geojson-equality which was used in turf-equal

DenisCarriere · 2017-06-06T16:14:43Z

@rowanwins Added a few extra params to equal.

/**
 * @name equal
 * @param {Geometry|Feature<any>} feature1 GeoJSON Feature or Geometry
 * @param {Geometry|Feature<any>} feature2 GeoJSON Feature or Geometry
 * @param {Boolean} [direction=false] direction of LineString or Polygon (orientation) is ignored if false
 * @param {number} [precision] coordinate decimal precision

Tests

test('turf-boolean-equal -- reduce coordinate precision', t => {
    const pt1 = point([30.2, 10]);
    const pt2 = point([30.22233, 10]);

    t.true(equal(pt1, pt2, false, 1));
    t.false(equal(pt1, pt2, false, 3));
    t.end();
});


test('turf-boolean-equal -- apply direction (orientation) rule', t => {
    const line1 = lineString([[30, 10], [10, 30], [40, 40]]);
    const line2 = lineString([[40, 40], [10, 30], [30, 10]]);

    t.true(equal(line1, line2, true));
    t.false(equal(line1, line2, false));
    t.end();
});

@stebogit

* added fiji + resolute-bay tests to translate * added fiji + resolute-bay test to rotate * Add new modules to Turf core * v4.4.0 * Fix tests for blank dependencies * Add inside tests * New @turf/isolines based on MarchingSquares.js (#781) * replaced conrec with marchingsquare * fixed script and tests * updated bench * updated readme * updated package.json * updated index.d.ts * introduced suggested changes * added zProperty to index.d.ts * minor doc change * Updates to Isolines - Divide properties={} to two params - Simplify Catch error logic (index.js) - Update Typescript definition - Add types.ts to test Typescript definition - Add single task to benchmark - Update Yarn.lock - Simplify bench & test logic - Save fixtures out to GeoJSON CC: @stebogit * perIsoline overlaps properties from toAllIsolines * Update Readme * Buffer (#786) Buffer - Drop circle buffer operation * removed turf-isolines old test files * copied files from repo * refactored index and test * updated README * updated index.d.ts and bench * added yarn.lock * removed Polygon as accepted input; added array test; * added masking feature to turf-grid * added throws tests * updated yarn.lock * added contributor * Resolves #785 * Add test fixture * Update complex unkink-polygon * Update name to boolean-clockwise * Declare input as line instead of Feature

DenisCarriere · 2017-06-30T17:31:32Z

@rowanwins Don't mind if we close this PR, I'm pretty sure you have this stored locally and we can leave the branch active until we port over everything.

Initial commit of boolean DE-9IM module

09abe34

DenisCarriere assigned rowanwins May 2, 2017

DenisCarriere added the new-module label May 2, 2017

DenisCarriere mentioned this pull request May 2, 2017

removing jsts dependency #88

Open

4 tasks

DenisCarriere added 4 commits May 3, 2017 10:42

Update contains

93a3400

- Support Geometry Objects - Use deep-equal - Use Switches CC: @rowanwins

Create boolean-contains own module

6a9b1ba

Add benchmark boolean-contains

727c80a

Add GeometryObject support

ede82d6

DenisCarriere mentioned this pull request May 3, 2017

Improve Modules to support Geometry Object/Collection #710

Closed

5 tasks

DenisCarriere reviewed May 4, 2017

View reviewed changes

dpmcmlxxvi reviewed May 7, 2017

View reviewed changes

dpmcmlxxvi mentioned this pull request May 15, 2017

Extended turf-inside input to any feature #744

Closed

Update Readmes & minor refactoring

87500ef

Exclude failing tests

f4f8a18

dpmcmlxxvi reviewed May 15, 2017

View reviewed changes

DenisCarriere and others added 9 commits May 27, 2017 22:02

Add yarn locks

cb32dd5

add Skip to tests

d772b9a

boolean-overlap improvements

78a0af8

Support geometries and features

ba618ad

Change test approach. Fix multipoint check.

3d40ec8

Using Glob in tests

7d715e6

Prevents accidental dotfiles from being read by `fs`

Skip polygonsShareBoundary

c1c898c

Add required files

300208b

- Update JSDocs - Add bench.js - Add index.d.ts - Add LICENSE

Merge branch 'master' into booleans

2ab6a2b

DenisCarriere modified the milestones: 4.5.0, 4.4.0 Jun 5, 2017

Add precision & direction params

0827683

DenisCarriere modified the milestones: 4.6.0, 4.5.0 Jun 30, 2017

DenisCarriere closed this Jun 30, 2017

DenisCarriere mentioned this pull request Jul 22, 2017

New module @turf/boolean-overlaps #856

Merged

5 tasks

DenisCarriere deleted the booleans branch September 5, 2017 03:45

Initial commit of boolean DE-9IM module #707

Initial commit of boolean DE-9IM module #707

Conversation

rowanwins commented May 2, 2017 • edited by DenisCarriere Loading

DE-9IM modules

Extra Testing

DenisCarriere commented May 2, 2017

dpmcmlxxvi commented May 3, 2017

DenisCarriere commented May 3, 2017

rowanwins commented May 3, 2017

rowanwins commented May 3, 2017

DenisCarriere commented May 3, 2017 • edited Loading

DenisCarriere commented May 3, 2017 • edited Loading

DenisCarriere commented May 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rowanwins commented May 4, 2017

DenisCarriere commented May 4, 2017

DenisCarriere commented May 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpmcmlxxvi May 7, 2017 • edited Loading

Choose a reason for hiding this comment

DenisCarriere May 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpmcmlxxvi commented May 13, 2017

rowanwins commented May 15, 2017

DenisCarriere commented May 15, 2017 • edited Loading

DenisCarriere commented May 15, 2017 • edited Loading

rowanwins commented May 15, 2017

dpmcmlxxvi May 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DenisCarriere commented Jun 6, 2017

DenisCarriere commented Jun 6, 2017

DenisCarriere commented Jun 30, 2017

rowanwins commented May 2, 2017 •

edited by DenisCarriere

Loading

DenisCarriere commented May 3, 2017 •

edited

Loading

DenisCarriere commented May 3, 2017 •

edited

Loading

dpmcmlxxvi May 7, 2017 •

edited

Loading

DenisCarriere May 7, 2017 •

edited

Loading

DenisCarriere commented May 15, 2017 •

edited

Loading

DenisCarriere commented May 15, 2017 •

edited

Loading

dpmcmlxxvi May 15, 2017 •

edited

Loading