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

Initial commit turf buffer no jsts #876

Closed
wants to merge 9 commits into from
Closed

Initial commit turf buffer no jsts #876

wants to merge 9 commits into from

Conversation

rowanwins
Copy link
Member

Alright @DenisCarriere @stebogit

Here is a pretty rough commit of a dependency free buffer module

Positives

  • Supports polys and lines
  • Superfast compared to jsts
  • Supports negative buffers on polys

Needs work

  • No support for polys with holes yet
    • It would be great if we had a meta module for looping interior and exterior rings!
  • Add support for points
  • Add other end cap strategies, you'll see i've put some placeholders and we just need a switch statement or something.
  • The projection thing needs doing as per the current module
  • support multi features & looping
  • Lines that result in a buffer with overlap need some thought,,,

Areas for thought

  • The number are arguments might get quite long, we could pass through a single options object although this might break turf tradition
  • Comparing buffers against other providers is really hard but what I've produced is pretty close to esri and jsts, every buffer from every provider seems to differ minutely
  • I've purposefully not dissolved the polygons, I'd prefer we let people do this post buffer rather than pass another option

Lots more work required but thought I'd throw it up while I had time.

@DenisCarriere
Copy link
Member

The number are arguments might get quite long, we could pass through a single options object although this might break turf tradition

I'm good if we start adding options for optional params, since this would be a Major release type of module, we might want to consider doing this to a few other modules, that way we can easily expand the module with additional params without making it so complicated to use.

Would look something like this:

/**
 * @param {Object} [options={}] Optional params
 * @param {string} [options.units="kilometers"] can be degrees, radians, miles, kilometers, inches, yards, meters
 * @param {number} [options.steps=64] number of steps on a rounded corner
 */
module.exports = function (feature, distance, options) {
    options = options || {};
    var units = options.units;
    var steps = options.steps;

@stebogit @rowanwins Feel free to look back at some previous modules and we can make a list of modules that would need this type of refactoring (modules with 2 or greater optional params)

@stebogit
Copy link
Collaborator

It would be great if we had a meta module for looping interior and exterior rings!

Yes! Something that would avoid this, quite used:

var coords = getCoords(geojson);
switch (type) {
case 'LineString':
    // do stuff with line coords
    break;
case 'MultiLineString':
case 'Polygon':
    // do stuff with lines or polygons coords
    break;
case 'MultiPolygon':
    coords.forEach(function (polygons) {
        // do stuff with polygons coords
        // add to result array of polygons
    });
    break;
case 'Point':
        // do stuff with point coord
case 'MultiPoint':
        // do stuff with points coord
    break;
default:
    throw new Error(type + ' geometry not supported');
}

pass through a single options object

Love this!
Couldn't we adopt this pattern in all modules? It would make the transaction to it much easier to the user having a new consistent standard.

@DenisCarriere
Copy link
Member

DenisCarriere commented Aug 1, 2017

It would be great if we had a meta module for looping interior and exterior rings!

👍 That would be pretty cool! We could maybe add this to @turf/meta:

  • interiorRingEach &interiorRingReduce
  • exteriorRingEach & exteriorRingReduce

@rowanwins rowanwins mentioned this pull request Aug 2, 2017
@DenisCarriere
Copy link
Member

@rowanwins not that I'm giving hope... but there's no way we should be implementing this Buffer module internally, this seems like a massive undertaking.

@rowanwins You can use this branch as "play" to help learn about Buffer operations, but I doubt this will ever get stable enough to release.

One good take away from this would be to learn what is missing as smaller internal modules, such as ringEach / lineEach or other missing modules we didn't think about yet.

@DenisCarriere
Copy link
Member

I've purposefully not dissolved the polygons, I'd prefer we let people do this post buffer rather than pass another option

🤔 Seems like this should be done internally, or at least an option in the param.

The projection thing needs doing as per the current module

👍 Agreed, we need a solid projection module, still haven't cracked that one yet, I'm sure we can come up with a good solution using one of d3's arsenal of tools.

@DenisCarriere
Copy link
Member

The number are arguments might get quite long, we could pass through a single options object although this might break turf tradition

We should start using options={} for modules with 2 or greater optional parameters, that way it's easier to expand the module. All required parameters should not be provided as an Object.

The JSDocs will look something like this:

/**
 * Takes a {@link Feature and returns a {@link Feature} at offset by the specified distance.
 *
 * @name buffer
 * @param {Geometry|Feature<LineString>} geojson input
 * @param {number} distance distance to buffer the feature (can be of negative value)
 * @param {Object} [options={}] Optional parameters
 * @param {string} [options.units=kilometers] can be degrees, radians, miles, kilometers, inches, yards, meters
 * @param {number} [options.steps] Number of steps
 * @returns {Feature} Feature buffered from the input feature
 */
module.exports = function (geojson, distance, options) {

@rowanwins
Copy link
Member Author

ye of little faith @DenisCarriere :)

Yep it's a big job but having said that I've got the following working

  • points
  • lines
  • polygons
    • inc with reverse buffers

Haven't tackled multipart or polygons with holes but they both should be relatively simple.

Performance is currently beating jsts hands down. And I've made a start on catering for different join types and end types.

So as far as Im concerned it's coming along it, it's just taking time, and if we don't do it who will?

@rowanwins
Copy link
Member Author

rowanwins commented Aug 14, 2017

Just to prove some progress @DenisCarriere :)
Polygons with multiple holes buffering

image

Current benchmarks

complex x 18,434 ops/sec ±1.56% (93 runs sampled)
linestring x 42,968 ops/sec ±5.02% (85 runs sampled)
point x 47,166 ops/sec ±3.80% (90 runs sampled)
rectangleWithHoles x 24,752 ops/sec ±1.04% (92 runs sampled)
rectangle x 34,514 ops/sec ±1.31% (94 runs sampled)

@DenisCarriere
Copy link
Member

DenisCarriere commented Aug 14, 2017

ye of little faith @DenisCarriere :)

lol! 😆 I have faith! I'm just glad you are taking on this challenge, I'm going to test the 💩 out of it!

Polygons with multiple holes buffering

Awesome! 🎉

complex x 18,434 ops/sec ±1.56% (93 runs sampled)
linestring x 42,968 ops/sec ±5.02% (85 runs sampled)
point x 47,166 ops/sec ±3.80% (90 runs sampled)
rectangleWithHoles x 24,752 ops/sec ±1.04% (92 runs sampled)
rectangle x 34,514 ops/sec ±1.31% (94 runs sampled)

Those benchmark results are looking pretty decent!

@DenisCarriere DenisCarriere mentioned this pull request Aug 14, 2017
@@ -1,17 +1,28 @@
<<<<<<< HEAD
Copy link
Member

Choose a reason for hiding this comment

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

@rowanwins Looks like you've got a booboo 🤕 here

@ffflabs
Copy link

ffflabs commented Aug 23, 2017

@DenisCarriere this might be totally unrelated but... What's the use of the steps parameter?

It's not used at all, as seen in

https://github.com/Turfjs/turf/blob/master/packages/turf-buffer/index.js#L81

@rowanwins
Copy link
Member Author

Hi @amenadiel

Steps to used to define how many vertices are used on rounded corners are on the buffer outputs. Eg setting steps to 3 will mean there are 3 vertices on a corner so you'll have a very jagged buffer, having it set at 64 gives nice smooth rounded corners.

@ffflabs
Copy link

ffflabs commented Sep 7, 2017

Steps to used to define how many vertices are used on rounded corners are on the buffer outputs

@rowanwins that's what it's meant to do. However, looking at the code it seems it's currently not used.
Runing this functions with different values for steps yields the same result in my tests.

It seems your PR adds support for real steps manipulation tho 👍

@DenisCarriere DenisCarriere modified the milestones: 5.0.0, 5.1.0 Sep 29, 2017
@DenisCarriere
Copy link
Member

@rowanwins Going to close this PR, but I won't delete the branch for archive purposes (until we completely revamp @turf/buffer).

@DenisCarriere DenisCarriere deleted the buffer-rw branch April 3, 2018 19:41
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.

None yet

4 participants