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

Buffer - Observation #849

Closed
flightsurvey opened this issue Jul 19, 2017 · 15 comments
Closed

Buffer - Observation #849

flightsurvey opened this issue Jul 19, 2017 · 15 comments
Labels

Comments

@flightsurvey
Copy link

flightsurvey commented Jul 19, 2017

This is not an issue per se, but an observation and I'm raising on behalf of a customer as I do not have an adequate enough understanding of how this module works.

There are screen captures of two scenarios below. The first shows a buffer (in orange) set at +25m and the second shows a buffer set at -25m. The first smooths out the buffer polygon (and I can just about understand why) whilst the second makes the buffer much the same shape as the object polygon. Is this in line with what everyone expects to happen?

+25 metre buffer:

image

-25 metre buffer:

image

It does beg the question that if I set the object polygon to be the same size as the second buffer and then apply a +25 metre buffer then what would I expect to see? A buffer with the same proportions or one that is smoothed out?

To be honest, I find both results to be acceptable from an operational point of view but I still think it's worth raising the point.

But otherwise, thank you very much for such a useful utility.

Cheers.

@DenisCarriere
Copy link
Member

DenisCarriere commented Jul 19, 2017

Thanks for sharing 👍

Those outcomes are expected, the smoothing edges is for normal for positive buffers.

There has been some discussions (#639) about having different joining strategies (miter & flat).

Those screen shots look good! Good use of high resolution imagery + TurfJS.

@stebogit
Copy link
Collaborator

stebogit commented Jul 20, 2017

As just an alternative, if useful, I'd suggest the use of @turf/transform-scale, which would scale the polygon keeping the original shape.
In this case you would need to pass the scaling factor instead of a length parameter:

var scaledPoly = turf.transformScale(poly, factor);

factor = 1.24:
screen shot 2017-07-19 at 6 24 56 pm

factor = 0.82:
screen shot 2017-07-19 at 6 31 23 pm

@DenisCarriere
Copy link
Member

Oh nice! @turf/transform-scale is another great option for this

@rowanwins
Copy link
Member

Hey @stebogit

A thought on this, is there any way that we could get transformScale to accept something other than a factor, eg it would be great to pass in a regular turf distance like kilometers...

@stebogit
Copy link
Collaborator

stebogit commented Jul 26, 2017

👍 @rowanwins we should be able to calculate the factor internally based on a bbox on the geojson input I guess, but let me think about it

@stebogit
Copy link
Collaborator

stebogit commented Jul 29, 2017

@rowanwins I think we need to define how to apply a distance as factor. I can think about two approaches: we can apply it to the bbox, or add it to the distance of each point from the origin.
The actual factor used for the scaling would be the ratio between bboxes widths or distances from origin.

Example:

const scaled = turf.transformScale(polygon, 10, null, false, 'miles');

triangle scale 1

The left picture is actually misleading because the two polygons could still be "centered" (if the chosen origin is the centroid), but I wanted to highlight where the distance factor would be applied.

I'm not sure if the result is what you would expect, however we can definitely add this feature.

We can add an optional units parameter that, if defined as a string, implies the factor has to be considered as a distance of that unit.

Any other idea?
cc: @DenisCarriere

@rowanwins
Copy link
Member

Hmm yeah maybe it's not such a smart idea, might be wisest to keep that functionality in the buffer module....

@DenisCarriere
Copy link
Member

@stebogit Can't we just expand the origin parameter to include the center-of-mass & center?

@rowanwins
Copy link
Member

That's alright I think I've got it sorted @DenisCarriere & @stebogit . I have a fully functional polygon buffer module with 0 dependencies (other than turf deps). I'm just adding the linestring buffer before I put in an initial pull request for your review (perhaps tonight...). Currently 5 times faster than jsts, gone from 3,000 ops/second to 18,000. Will support holes, inverse buffers (eg with a negative value) etc. I think I can even get it support endcap strategies!

@DenisCarriere
Copy link
Member

@rowanwins 👍 Awesome! 🎉 Looking forward to it

@stebogit
Copy link
Collaborator

stebogit commented Jul 31, 2017

Can't we just expand the origin parameter to include the center-of-mass & center?

@DenisCarriere the centroid is currently the default origin for scale 👍

Great @rowanwins!

@stebogit stebogit reopened this Jul 31, 2017
@DenisCarriere
Copy link
Member

DenisCarriere commented Jul 31, 2017

Agreed, but couldn't we also add center-of-mass? wouldn't that make the output different (compared to centroid/center)?

@stebogit
Copy link
Collaborator

Sorry didn't mean to close this... 😅

@DenisCarriere I remember we defined centroid as default origin following @morganherlocker suggestion. The user can anyway pass any point as origin.

However, here the issue is not really the origin, but how you would apply a distance parameter to then be able to calculate the scaling factor.

I'd actually agree with @rowanwins here, that maybe it's not the best way of think about @turf/transform-scale, in terms of distance increase I mean. @turf/buffer is definitely the best approach in this case.

@DenisCarriere
Copy link
Member

Ooh yea! that's true... The user can just pass his own point 👍 Gotcha

@DenisCarriere
Copy link
Member

DenisCarriere commented Jul 31, 2017

@turf/buffer is definitely the best approach in this case.

👍 Yes this issue topic seems more of a buffer operation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants