-
Notifications
You must be signed in to change notification settings - Fork 983
turf-oriented-envelope: add new function to calculate rotated envelope, fix #2900 #2901
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
base: master
Are you sure you want to change the base?
Conversation
2ea5cdf
to
daebc18
Compare
WIP, still missing tests and has compilation errors fix bench.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to mention my implementation as the basis somewhere in the docs :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moin / hej! Thanks for your great work on the original code :)
I wanted to credit you in the License file but that is enforced to be the same in all packages. As the Readme is auto-generated from index.ts, I added you in the “docstring” of the function. Are you fine with the current attribution? I can of course also add your GitHub username or link there if you want.
/**
* Takes any number of features and returns a rotated rectangular {@link Polygon} that encompasses all vertices.
*
* Based on the [geojson-minimum-bounding-rectangle](https://www.npmjs.com/package/geojson-minimum-bounding-rectangle)
* package by Matthias Feist.
*/
## orientedEnvelope
Takes any number of features and returns a rotated rectangular [Polygon][1] that encompasses all vertices.
Based on the [geojson-minimum-bounding-rectangle][2]
package by Matthias Feist.
.github/workflows/turf.yml
Outdated
@@ -19,7 +19,7 @@ jobs: | |||
|
|||
strategy: | |||
matrix: | |||
node-version: [18.x, 20.x, 22.x] | |||
node-version: [18.x, 20.x, 22.x, 24.x] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice addition. We can drop 18.x while we're here as it's end of life.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can do. Is it okay to have in the same PR or would you prefer a separate one for adding 24 and removing 18?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bundled here will be fine. Maybe make a note of it in the PR description for anyone who ends up reviewing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. If Matthias is fine with the current attribution, then from my side the PR is ready for review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with it :)
Enhancement: Provide a package and method directly in turf for calculating the oriented envelope by area or optionally when passing options.minimizeWidth=true by width for a given GeoJSON feature or feature collection.
Adapted from geojson-minimum-bounding-rectangle by @matthiasfeist.
As mentioned in the original readme file, this problem it is solving has many names: Minimum Bounding Rectangle, Minimum Bounding Box, Smallest Surrounding Rectangle, Minimum Area Rectangle...
Fix #2900. Successfully builds on Node 18–24.
Breaking: Drop support for EOL Node 18, build for Node 20 by default, add Node 24 to build matrix.
I have read the steps for preparing a pull request.