Replacing jsts for turf/difference

[rowanwins]

To-Do

• Add Complex geometries to test fixtures

Gday @DenisCarriere

I thought perhaps we could make a start replacing JSTS. I'm somewhat familiar with the martinez module so I thought that might be a good starting point and would help reduce a lot of the jsts dependencies.

So I've replaced jsts with martinez, it's gone pretty smoothly, a couple of notes

1. The tests aren't passing because there seems to be some minor differences in coordinates, im not sure which library would be handling this the best (eg jsts or martinez), it might be easiest to come up with some arbitrary more readable test cases. I did include one very simple one.
2. Im not sure if I've handled the Polygon/MultiPolygon/Holes output in the best way possible, in fact Im sure there are errors, so if you could review and if you have any clever ideas that would be awesome.
3. The latest version of martinez isn't available via npm yet so I've just included it directly but Im sure that'll be easy to get a new release of if we're happy with this approach.

Anyway just getting the ball rolling on this long over-due task :)

[DenisCarriere]

👍 I'll have a crack at it, I scanned briefly the source code of the martinez library and it looks pretty solid. I can help out to include some test cases for your PR.

Also another library that looks solid is polygon-offset which also uses martinez as it's only dependency, this could replace @turf/buffer which also has jsts as a dependency.

[rowanwins]

Thanks for taking a look. Let me know if you want me to do anything additional.

Yeah the polygon offset library does look good, hopefully between that and martinez I think we could do away with jsts.

[DenisCarriere]

@rowanwins Ok I've added a significant amounts of tests to this library.

Things I've noticed/modified:

@returns {FeatureCollection<Polygon>}

Having undefined as possible @returns can be VERY annoying, it breaks any forEach or featureEach loops and adds a layer of complexity whenever you can't predict if you are going to receive undefined or a Feature.

Having only Polygon in the results also makes it easier for the end users since they don't have to deal with complex shapes such as a MultiPolygon.

Polygon/MultiPolygon (Malformed geometry from martinez library)

More information w8r/martinez#5
Once we can properly handle MultiPolygons we could iterate over those features using @turf/flatten and add them to the FeatureCollection (results).

Positive Remarks

This martinez library is REALLY good! It's surprisingly fast for the complex operations it's doing, looking forward for their next releases, this change will make it way better than jsts.

Benchmark Results

completely-overlapped x 70,903 ops/sec ±4.07% (80 runs sampled)
multi-polygon-input x 14,147 ops/sec ±1.50% (86 runs sampled)
multi-polygon-target x 9,252 ops/sec ±1.25% (88 runs sampled)
polygons-with-holes x 5,673 ops/sec ±2.98% (84 runs sampled)
simple x 26,264 ops/sec ±9.35% (74 runs sampled)
split-to-multipolygon-with-holes x 13,678 ops/sec ±5.52% (79 runs sampled)
split-to-multipolygon x 22,073 ops/sec ±1.61% (85 runs sampled)

[rowanwins]

awesome work as always @DenisCarriere !

Yeah happy to ditch the undefined, that's just how it was previously so I kept it.

Yep the polygon/multipolygon thing is a bit complicated but hopefully with a bit of thought between here and martinez it shouldn't be a deal breaker.

And Im glad to see the performance is holding up. I flagged performance over in the martinez repo a while ago and it may be something we can look at further, Im not sure they've had much time to do any optimising (or at least checking that it's as optimised as possible!)

[DenisCarriere]

👍 Sounds good, lets keep these PR's open and revamp all the modules that have jsts dependencies. Also lets re-think their input/output, I never really liked the jsts outputs, they always seemed to be completely random.

Since these will be breaking changes, lets keep the PR's open until we are ready to do the next major release. That way we can still add patches & minor updates while we continue working on this.

🛌 now

[rowanwins]

yep agreed re holding off on the release until all the other modules are ready as well

[rowanwins]

Just thinking @DenisCarriere - is there anything stopping us from starting to implement the other modules (eg union, clip, xor). Do we need to wait on anything downstream in martinez etc, or should we just get cracking?

[DenisCarriere]

@rowanwins Had another look at this library and it's 95% done (very good so far!), we just need to solve that issue with Polygon/MultiPolygon outputs.

Example - Split polygon with hole

https://github.com/Turfjs/turf/blob/replace-jsts-difference/packages/turf-difference/test/out/split-to-multipolygon-with-holes.geojson

Current Output:

• Single Polygon with 3 linestrings (2 outers & 1 inner)

Expected Output:

• 1 Polygon with a hole (2 linestrings) & 1 poly without (1 linestring)
• Option 1: FeatureCollection of two Polygons.
• Option 2: Multi Polygon with two Polygons.

@rowanwins Which option would you prefer? FeatureCollection or MultiPolygon (both are fine with me)?

[rowanwins]

For comparison here are the results using the polybooljs library which offers an alternate martinez implementation. Most the results appear to be marginally faster compared to the martinez repo

completely-overlapped x 87,020 ops/sec ±0.97% (89 runs sampled)
multi-polygon-input x 16,263 ops/sec ±0.97% (90 runs sampled)
multi-polygon-target x 10,995 ops/sec ±0.84% (92 runs sampled)
polygons-with-holes x 6,616 ops/sec ±1.02% (90 runs sampled)
simple x 34,918 ops/sec ±0.92% (88 runs sampled)
split-to-multipolygon-with-holes x 17,797 ops/sec ±1.01% (88 runs sampled)
split-to-multipolygon x 24,850 ops/sec ±1.86% (87 runs sampled)

It has the same shortcomings in terms of the output not adequately distinguishing between poly with holes and multi-poly.

[DenisCarriere]

@rowanwins I'd say the performance is pretty much the same between the two libraries. Both libraries seem to be in their early stages of development (<40 commits and only a few issues opened/closed).

[DenisCarriere]

This PR is ready to be merged 👍 (waiting for other replace-jsts PR's).

Benefits of replacing jsts with martinez

• 8-14X performance increase
• Supports Polygons with Holes (jsts breaks)
• Same geometry results as jsts
• Smaller source code footprint

Nice to haves

• Use npm martinez-polygon-clipping as dependency (latest version 0.1.5)
• Handling of Polygon & Multi-Polygons

CC: @rowanwins

[rowanwins]

Gday @DenisCarriere

Good news, glad you're happy with where it's at.

I reckon we hold off on creating the others and merging I/we take a peak upstream in martinez to see if we can get improved handling for the multipolygon/holes issue, if we can get that going then implementing the remainder should be pretty easy.

Cheers
Rowan

[DenisCarriere]

Agreed, commented here: w8r/martinez#16 (comment)

[rowanwins]

With my latest changes to martinez I'm getting the following results running the benchmark tests, so some improvements.

completely-overlapped x 79,765 ops/sec ±0.56% (90 runs sampled)
multi-polygon-input x 15,087 ops/sec ±1.33% (90 runs sampled)
multi-polygon-target x 10,198 ops/sec ±3.95% (87 runs sampled)
polygons-with-holes x 6,251 ops/sec ±2.11% (89 runs sampled)
simple x 32,014 ops/sec ±1.56% (91 runs sampled)
split-to-multipolygon-with-holes x 16,411 ops/sec ±0.64% (93 runs sampled)
split-to-multipolygon x 22,968 ops/sec ±0.37% (90 runs sampled)

It would be good to get some comparable benchmark results with jsts.

[DenisCarriere]

Awesome stuff! 👍

I had already ran some benchmark tests with jsts, here are the results:

turf/packages/turf-difference/bench.js

Lines 16 to 35 in 261295a

	/**
	* Benchmark Results
	*
	* completely-overlapped x 70,903 ops/sec ±4.07% (80 runs sampled)
	* multi-polygon-input x 14,147 ops/sec ±1.50% (86 runs sampled)
	* multi-polygon-target x 9,252 ops/sec ±1.25% (88 runs sampled)
	* polygons-with-holes x 5,673 ops/sec ±2.98% (84 runs sampled)
	* simple x 26,264 ops/sec ±9.35% (74 runs sampled)
	* split-to-multipolygon-with-holes x 13,678 ops/sec ±5.52% (79 runs sampled)
	* split-to-multipolygon x 22,073 ops/sec ±1.61% (85 runs sampled)
	*
	* ==using jsts==
	* completely-overlapped x 4,841 ops/sec ±22.20% (69 runs sampled)
	* multi-polygon-input x 1,794 ops/sec ±13.80% (69 runs sampled)
	* multi-polygon-target x 1,368 ops/sec ±7.53% (71 runs sampled)
	* polygons-with-holes: ERROR
	* simple x 2,918 ops/sec ±7.49% (68 runs sampled)
	* split-to-multipolygon-with-holes x 1,684 ops/sec ±10.29% (62 runs sampled)
	* split-to-multipolygon x 2,676 ops/sec ±5.12% (79 runs sampled)
	*/

completely-overlapped x 4,841 ops/sec ±22.20% (69 runs sampled)
multi-polygon-input x 1,794 ops/sec ±13.80% (69 runs sampled)
multi-polygon-target x 1,368 ops/sec ±7.53% (71 runs sampled)
polygons-with-holes: ERROR
simple x 2,918 ops/sec ±7.49% (68 runs sampled)
split-to-multipolygon-with-holes x 1,684 ops/sec ±10.29% (62 runs sampled)
split-to-multipolygon x 2,676 ops/sec ±5.12% (79 runs sampled)

[rowanwins]

Thanks @DenisCarriere . It might also be worth getting one or two really complex polys (eg lots of vertices, not complex in terms of holes and multi-ness) jut to check that performance doesn't degrade as the count of vertices goes up.

[DenisCarriere]

👍 Agreed, the current fixtures are the "simple" ones, we get those perfect first, then we investigate in more complex geometries (not including invalid geometries of course).

[rowanwins]

Gday @DenisCarriere

I've pushed up the latest version which I think now matches the output that will come from martinez. I've also added a complex poly test although for some reason npm run bench isn't working for me (it does that sometimes - just doesn't show any results...) so I can't compare it to jsts properly.

Anyway Im running out of puff this evening but another step closer I think...

[DenisCarriere]

👍 Awesome work @rowanwins , I'll have a look at those bench results and see why they are not showing up.

[DenisCarriere]

@rowanwins Coordinates looks messed up with true

[DenisCarriere]

@rowanwins Might need to merge the changes from #725 (might be easier to close this PR and start a new one).

Seems like jsts was able to handle Holes & MultiPolygons just fine and the output result was a "proper" MultiPolygon unlike Martinez which still had issues outputting the correct geometry (at this very moment).

[DenisCarriere]

Going to close this PR due to so many Conflicts, this PR will simply be used as reference.

[xaviergonz]

@rowanwins I just released a asm.js / webassembly port of Angus Johnson's Clipper (https://sourceforge.net/projects/polyclipping/) library named js-angusj-clipper (https://www.npmjs.com/package/js-angusj-clipper). While the port is pretty new, the library it is based on is pretty solid on itself.

PS: It supports both clipping (Union, Difference, Xor, Intersection) as well as offsetting.

[DenisCarriere]

Woot it's made in Typescript & C++ (rarely see those two languages together), it must be fast! 🏎 💨

@xaviergonz Feel free to send a PR to get a rough draft up and @rowanwins or myself can have a look at it afterwards.

👍 Definitely looking forward to using this.