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

Replace jsts @turf/buffer #674

Closed
wants to merge 6 commits into from
Closed

Conversation

DenisCarriere
Copy link
Member

@DenisCarriere DenisCarriere commented Apr 16, 2017

2nd attempt to replace jsts from @turf/buffer using polygon-offset.

To-Do

  • Performance increase
  • Improve & test arcSegments/steps > 32

Good 👍

  • Very similar output results as jsts
  • Not jsts 🥇
  • Handles complex Multi-Polygons
  • Handles polygons with holes (jsts breaks)

Needs improvement 👎

  • Polygons start having issues with arcSegments > 32 (need to tests this behavior more).
  • Performance loss for the following geometries:
    • Multi-Polygon (50x slower)
    • LineString (40x slower)
    • Multi-LineString (20x slower)

Neutral

Benchmark Results

  • Point Operations (no change, same source code)
  • LineString Operations (20x-30x slower) 👎
  • Polygon Operations (60x-90x slower) 👎
/**
 * Benchmark Results
 *
 * ==polygon-offset:after==
 * feature-collection-points x 9,876 ops/sec ±0.88% (89 runs sampled)
 * geometry-collection-points x 9,919 ops/sec ±0.90% (91 runs sampled)
 * linestring x 219 ops/sec ±1.37% (83 runs sampled)
 * multi-linestring x 61.91 ops/sec ±4.88% (64 runs sampled)
 * multi-point x 995 ops/sec ±24.51% (82 runs sampled)
 * multi-polygon x 42.77 ops/sec ±0.92% (56 runs sampled)
 * north-latitude-points x 842 ops/sec ±1.23% (91 runs sampled)
 * northern-polygon x 236 ops/sec ±1.83% (83 runs sampled)
 * point x 43,814 ops/sec ±1.19% (94 runs sampled)
 * polygon-with-holes x 79.33 ops/sec ±16.13% (65 runs sampled)
 *
 * ==jsts:before ==
 * feature-collection-points x 9,335 ops/sec ±1.37% (88 runs sampled)
 * geometry-collection-points x 9,505 ops/sec ±1.57% (86 runs sampled)
 * linestring x 7,977 ops/sec ±17.93% (75 runs sampled)
 * multi-linestring x 1,371 ops/sec ±2.00% (88 runs sampled)
 * multi-point x 515 ops/sec ±3.35% (85 runs sampled)
 * multi-polygon x 2,549 ops/sec ±3.03% (88 runs sampled)
 * north-latitude-points x 812 ops/sec ±2.31% (88 runs sampled)
 * point x 42,867 ops/sec ±1.38% (89 runs sampled)
 * polygon-with-holes x 7,397 ops/sec ±1.93% (88 runs sampled)
 */

Example1 - Complex Multi-Polygon

image

Example1.a - Smooth edges (using polygon-offset)

image

Example2 - With Holes

image

Ref: #656
CC: @rowanwins

@flightsurvey
Copy link

This looks much better. Is there a version of the amended code that we can access for try-out?

@rowanwins
Copy link
Member

Hi @flightsurvey

This code is still very much in prototype but if you really want to test it you can check out the replace-jsts-buffer branch of this repo.

Fingers crossed we'll be a bit closer in another week or two if you can hold on.

@flightsurvey
Copy link

OK - I'd best leave well alone until you're happy with the modifications.

@DenisCarriere
Copy link
Member Author

Is there a version of the amended code that we can access for try-out?

Like @rowanwins was saying, you can always try it out using the replace-jsts-buffer branch.

$ git clone git@github.com:Turfjs/turf.git -b replace-jsts-buffer

However, there's still a few more things we need to work on before we merge this, so might be best to wait a until we officially release/merge this PR.

Expect these changes to be in the next major release Turf v5.0
CC: @flightsurvey

@DenisCarriere DenisCarriere mentioned this pull request May 2, 2017
4 tasks
@DenisCarriere
Copy link
Member Author

Having a really hard time using polygon-offset vs. jsts.

Going to close this PR since a lot of changes occurred in PR #718

@DenisCarriere DenisCarriere removed this from the 5.0.0 milestone May 7, 2017
@rowanwins
Copy link
Member

Gday @DenisCarriere

Just wondering on what the hard part is with polygon-offset?

@DenisCarriere DenisCarriere deleted the replace-jsts-buffer branch May 7, 2017 23:50
@DenisCarriere
Copy link
Member Author

Will attempt to use polygon-offset again, however with the new Equidistance change caused the polygon-offset library to break.

@mclaeysb
Copy link

mclaeysb commented Jun 18, 2017

Hi all. Very glad that turf buffer is being incrementally improved. Good job with the reprojection @DenisCarriere - I'm glad the buffers are looking more natural now!

I've just had some time to check out polygon-offset and @w8r 's other work. Very impressive! Glad to see some other people tried to implement buffering from scratch :)

In testing it I also experienced it to be much slower in its current implementation as you pointed out, and showing some unpleasant artefacts near boundaries of complex polygons.

I just wanted to point out that my one-year-old no-jsts pull request for turf-buffer is still online (is there a way to port it to the new, modular version of turf?). With the new reprojections taking up some additional computation time, I've found it to as performant as the current turf buffer, especially for small, everyday geojson features.

Is there any interest in continuing from that PR?

@DenisCarriere
Copy link
Member Author

@mclaeysb porting over that PR would be difficult, might be better to dissect which component is actually improving the performance and simply add that.

Buffer is such a complex operation that we should stick with existing libraries (jsts or polygon-offset) since they are robust and already tested.

I don't think we should be adding all that extra logic in @turf/buffer, we should only focus on implementing an existing robust buffer library.

Also mixing different buffer libraries seemed to have backfired (ref: #786)

@mclaeysb
Copy link

@DenisCarriere Indeed, buffering and union/intersect/... are very complex and error-prone. Sticking with jsts is definitely a good way to try to ensure a robust result.

I just thought I'd mention that PR because 1) I thought removing jstsdependancy was still on the turf wish list, and 2) my impression is that the PR I mentioned, which is based on a similar technique as polygon-offset, is faster and more robust than the current state of polygon-offset. So its there if you want to use it :) .

PS: thanks for all the work you're putting into turf.

PSS: your note on #786 gave me an insight on #801. Indeed proves that mixing libraries is tricky, especially on a sphere..

@DenisCarriere
Copy link
Member Author

@mclaeysb 👍 It's been hard to solve every buffer scenario, especially including equidistance buffer PR #718.

@DenisCarriere
Copy link
Member Author

@mclaeysb correct me if I'm wrong, but I don't think turf-buffer correctly handles Multi Geometries. The overlapping geometries should be dissolved like what @turf/buffer is doing.

Examples

MultiPoint (using @turf/buffer)

image

MultiPolygon (using @turf/buffer)

image

MultiPoint (using turf-buffer)

image

Multi LineString (using turf-buffer)

image

@mclaeysb
Copy link

mclaeysb commented Jun 19, 2017

Indeed! That's one of the things that should be implemented. Either by passing it through @turf-dissolve, or by implementing dissolving in its internal code (possibly within simplepolygon).

@DenisCarriere
Copy link
Member Author

After adding @turf/dissolve and/or simplepolygon then the module becomes slower than jsts which defeats the purpose of replacing jsts in the first place.

@mclaeysb
Copy link

Indeed, adding @turf/dissolve would not be a good idea wrt speed.simplepolygon, however, is already used in turf-buffer (and does most of the work there now, but isn't implemented to handle multi geometries).

I don't wanted to push anything here, just wanted to point out its there if you'd want to look for alternatives.

@DenisCarriere
Copy link
Member Author

👍 OK thanks for the reference.

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