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

@turf/buffer attempt to replace jsts #656

Closed
wants to merge 3 commits into from

Conversation

DenisCarriere
Copy link
Member

@DenisCarriere DenisCarriere commented Apr 10, 2017

Attempting to replace jsts from @turf/buffer, my findings are good & bad:

Good

  • Using @turf/circle to create (Multi)Points buffers is 2-4x faster and has very minimal source code.

Bad

  • Handling (Multi)Polygons is 8-12x SLOWER using polygon-offset.
  • the [martinez] library has a hard time determining the differences between Polygon & MultiPolygon, the output coordinates are always formatted for a Polygon.
  • Edges of new buffered polygons are jagged (jsts has smooth edges)
    image

Benchmark results

==before==
feature-collection-points x 3,792 ops/sec ±10.41% (82 runs sampled)
geometry-collection-points x 4,346 ops/sec ±2.05% (90 runs sampled)
linestring x 9,087 ops/sec ±2.14% (89 runs sampled)
multi-linestring x 1,145 ops/sec ±9.46% (80 runs sampled)
multi-point x 4,898 ops/sec ±4.73% (78 runs sampled)
multi-polygon x 1,737 ops/sec ±9.31% (66 runs sampled)
point x 11,907 ops/sec ±8.10% (72 runs sampled)
polygon-with-holes x 6,417 ops/sec ±6.16% (79 runs sampled)

==after==
feature-collection-points x 6,198 ops/sec ±13.12% (58 runs sampled)
geometry-collection-points x 6,518 ops/sec ±13.37% (67 runs sampled)
linestring x 1,694 ops/sec ±4.85% (71 runs sampled)
multi-linestring x 466 ops/sec ±4.26% (77 runs sampled)
multi-point x 12,055 ops/sec ±5.63% (77 runs sampled)
multi-polygon x 202 ops/sec ±1.73% (83 runs sampled)
point x 43,109 ops/sec ±1.58% (89 runs sampled)
polygon-with-holes x 438 ops/sec ±1.70% (86 runs sampled)

Summary

So far, looks like jsts is here to stay until we find a better replacement solution.

CC: @morganherlocker @rowanwins

@DenisCarriere DenisCarriere self-assigned this Apr 10, 2017
@DenisCarriere DenisCarriere requested a review from rowanwins April 10, 2017 05:33
@DenisCarriere DenisCarriere added this to the 5.0.0 milestone Apr 10, 2017
@rowanwins
Copy link
Member

Thanks for posting the results of your investigation @DenisCarriere , a bit disappointing but let's not lose all hope just yet. I suggest we a look upstream in poylgon-offset and see if the is anything we can contribute the that may aid the cause

@DenisCarriere
Copy link
Member Author

I still have hope! 👍 I'm sure we can find something that's equal or better than jsts.

@DenisCarriere
Copy link
Member Author

Going to close this PR, the martinez library doesn't seem like a solid replacement for jsts (at the moment).

@DenisCarriere DenisCarriere deleted the replace-jsts-buffer branch April 16, 2017 01:24
@rowanwins
Copy link
Member

Gday @DenisCarriere

I'd leave the pull request open while we explore things just so it's here for reference.

Another thing I noticed in the polygon-offset library is that there is an arcSegments option that you can pass through. It defaults to 5 (I suspect as we're seeing in your image above). I guess you could use this to generate different corner types (eg a mitre corner vs a curved corner) which may actually be handy.

Still need to look at the performance thing further (perhaps it's worth trying and seeing what happens speed wise if you set arcSegments = 1).

@DenisCarriere
Copy link
Member Author

Agreed, however all the no-breaking changes from this PR was translated to: #667.

I will be opening a new replace jsts for turf-buffer PR starting from the most recent master branch.

👍 Yes I didn't know about those arcSegments, using 64 steps looks the same as jsts, unfortunately the performance is still significantly slower than jsts.

@DenisCarriere DenisCarriere mentioned this pull request Apr 16, 2017
2 tasks
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.

2 participants