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

Add available end cap style options from turf-jsts to @turf/buffer #1994

Closed
wants to merge 7 commits into from
Closed

Add available end cap style options from turf-jsts to @turf/buffer #1994

wants to merge 7 commits into from

Conversation

chrispahm
Copy link
Contributor

This PR adds an option to @turf/buffer to specify different end cap styles (round = default, flat, square) for (line) buffers (solving #1257 and #639).

The change to the codebase is minimal, as the turf-jsts dependency already incorporates this feature and we would just be passing the option along.

I am fully aware that the issues #1257 and #639 have been originally closed because of ongoing efforts to remove the JSTS dependency entirely (e.g. #88 and the V7 branch). However, following the discussion around #1499, it seems like there is no viable alternative to JSTS regarding the buffer operation yet. In the meantime, I would argue it is fair to users to expose the full functionality of turf-jsts and its buffer operation since the relevant code is included anyway.

Demo

https://observablehq.com/@chrispahm/turf-js-buffer-end-cap-styles

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Have read How To Contribute.
  • Run npm test at the sub modules where changes have occurred.
  • Run npm run lint to ensure code style at the turf module level.

@aaronkyle
Copy link

Nice. These endcaps really make things look prettier!

@throberto
Copy link

Good! I need it!

@pork1977
Copy link

Great addition this is, I guess it's not quite ready for release yet?

@shenzhuxi
Copy link

It's a great feature to have.
But with this fork, if I use flat end and 1 step on a 2-points line, it will return 5 points.

line = turf.lineString([start, end]);
buffered = turf.buffer(line, 2, {units: 'meters', endCapStyle: 'flat', steps: 1});
console.log(buffered.geometry.coordinates); //[Array(5)]

Shouldn't it return a rectangle?

@chrispahm
Copy link
Contributor Author

Yup, that's due to the way LinearRings are defined in the GeoJSON spec:

A LinearRing is closed LineString with 4 or more positions. The first and last positions are equivalent (they represent equivalent points).

Here's a great blog post explaining the GeoJSON spec with great examples: https://macwright.com/2015/03/23/geojson-second-bite.html

@shenzhuxi
Copy link

Thanks @chrispahm !
The code seems very straight forward. Why hasn't it been merged yet?

@chrispahm
Copy link
Contributor Author

Hey @shenzhuxi,

Thanks for your approval, however the PR requires at least 1 approving review by one of the maintainers (@mfedderly and @rowanwins) or anyone with write access to the repo in order to be considered for merging.

Anyways, thanks for having a look!
Best
Chris

@JamesLMilner
Copy link
Collaborator

Hi @chrispahm, I can see that it is a simple and useful change so just want to update here; I think the reason this hasn't been merged yet is because we were looking at removing JSTS completely from the turfjs - I think @rowanwins has been rewriting the buffer library which would mean if we merge this we would have to maintain compatibility which might slow us getting the new buffer implementation in. JSTS is a huge pain for us because it creates very big bundle sizes because of it's size, hence wanting to remove it.

@chrispahm
Copy link
Contributor Author

Hi @JamesLMilner,

Absolutely understanding your point and the issues with JSTS:

I am fully aware that the issues #1257 and #639 have been originally closed because of ongoing efforts to remove the JSTS dependency entirely (e.g. #88 and the V7 branch). However, following the discussion around #1499, it seems like there is no viable alternative to JSTS regarding the buffer operation yet. In the meantime, I would argue it is fair to users to expose the full functionality of turf-jsts and its buffer operation since the relevant code is included anyway.

I'm just wondering if the switch to geojson-buffer (or any other library) will happen anytime soon, especially considering the discussion in rowanwins/geojson-buffer#4 (comment).
Therefore, might it not just be (more) sensible to just switch to upstream jsts as a start or even use @rycgar's BufferOp port instead? Thought this was a similar to what you were proposing #88 (comment) 😊

@GetecHarry
Copy link

Hi just wanted to drop in and say that this PR should definitely be approved in my opinion, this is exactly what we needed and Chris was super helpful in providing the minified JS file when emailing him. Thanks again!

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

Successfully merging this pull request may close these issues.

7 participants