-
Notifications
You must be signed in to change notification settings - Fork 1k
Ported union, difference, and intersect to clipper2-ts to improve performance #2997
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?
Changes from 10 commits
e4ac32f
e6c7431
13bc9fc
ba8b43e
00c2074
39035eb
495bc0d
fc80ef3
6b45ae3
f9641d3
61fe366
08b2034
5032695
b4e0728
1ea01aa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| The MIT License (MIT) | ||
|
|
||
| Copyright (c) 2017 TurfJS | ||
|
|
||
| Permission is hereby granted, free of charge, to any person obtaining a copy of | ||
| this software and associated documentation files (the "Software"), to deal in | ||
| the Software without restriction, including without limitation the rights to | ||
| use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of | ||
| the Software, and to permit persons to whom the Software is furnished to do so, | ||
| subject to the following conditions: | ||
|
|
||
| The above copyright notice and this permission notice shall be included in all | ||
| copies or substantial portions of the Software. | ||
|
|
||
| THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR | ||
| IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS | ||
| FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR | ||
| COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER | ||
| IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN | ||
| CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| # @turf/internal | ||
|
|
||
| <!-- Generated by documentation.js. Update this documentation by updating the source code. --> | ||
|
|
||
| <!-- This file is automatically generated. Please don't edit it directly. If you find an error, edit the source file of the module in question (likely index.js or index.ts), and re-run "yarn docs" from the root of the turf project. --> | ||
|
|
||
| --- | ||
|
|
||
| This module is part of the [Turfjs project](https://turfjs.org/), an open source module collection dedicated to geographic algorithms. It is maintained in the [Turfjs/turf](https://github.com/Turfjs/turf) repository, where you can create PRs and issues. | ||
|
|
||
| ### Installation | ||
|
|
||
| Install this single module individually: | ||
|
|
||
| ```sh | ||
| $ npm install @turf/internal | ||
| ``` | ||
|
|
||
| Or install the all-encompassing @turf/turf module that includes all modules as functions: | ||
|
|
||
| ```sh | ||
| $ npm install @turf/turf | ||
| ``` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| import { PluginObj } from "@babel/core"; | ||
|
|
||
| export default function replaceNumberWithJSBIToNumber(): PluginObj { | ||
| return { | ||
| name: "replace-number-with-jsbi-toNumber", | ||
| visitor: { | ||
| CallExpression(path) { | ||
| if (!path.get("callee").isIdentifier({ name: "Number" })) return; | ||
|
|
||
| const arg = path.get("arguments")[0]; | ||
| if (!arg) return; | ||
|
|
||
| let shouldReplace = false; | ||
|
|
||
| // Number(InternalClipper.MaxInt64 / 4n); | ||
| // to | ||
| // JSBI.toNumber(JSBI.divide(InternalClipper.MaxInt64, JSBI.BigInt("4"))); | ||
| if ( | ||
| arg.isBinaryExpression({ operator: "/" }) && | ||
| arg.get("left").isMemberExpression() && | ||
| arg.get("left.object").isIdentifier({ name: "InternalClipper" }) && | ||
| arg.get("left.property").isIdentifier({ name: "MaxInt64" }) && | ||
| arg.get("right").isBigIntLiteral() | ||
| ) { | ||
| shouldReplace = true; | ||
| } | ||
|
|
||
| // InternalClipper.Invalid64 = Number(InternalClipper.MaxInt64); | ||
| // to | ||
| // InternalClipper.Invalid64 = JSBI.toNumber(InternalClipper.MaxInt64); | ||
| if ( | ||
| arg.isMemberExpression() && | ||
| arg.get("object").isIdentifier({ name: "InternalClipper" }) && | ||
| arg.get("property").isIdentifier({ name: "MaxInt64" }) | ||
| ) { | ||
| shouldReplace = true; | ||
| } | ||
|
|
||
| if (!shouldReplace) return; | ||
|
|
||
| path.get("callee").replaceWithSourceString("JSBI.toNumber"); | ||
| }, | ||
| }, | ||
| }; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,165 @@ | ||
| import { PolyPathD, PathD, PathsD, PolyTreeD, areaD } from "clipper2-ts"; | ||
| import { Polygon, MultiPolygon, Position } from "geojson"; | ||
|
|
||
| const DEFAULT_PRECISION = 8; | ||
|
|
||
| /** | ||
| * Converts a multipolygon to a flattened array of clipper2 paths. | ||
| */ | ||
| function multiPolygonToPaths(coords: Position[][][]): PathsD { | ||
| const paths: PathsD = []; | ||
|
|
||
| for (const polygon of coords) { | ||
| paths.push(...polygonToPaths(polygon)); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if polygonToPaths has like 1024 entries or something in it, this may break. Its safer to do a It does create an extra temporary array, but its only O(number of rings) so that's probably fine because its not O(number of coordinates) |
||
| } | ||
|
|
||
| return paths; | ||
| } | ||
|
|
||
| /** | ||
| * Converts a polygon to a flattened array of clipper2 paths. | ||
| */ | ||
| function polygonToPaths(coords: Position[][]): PathsD { | ||
| const paths: PathsD = []; | ||
|
|
||
| for (const [idx, ring] of coords.entries()) { | ||
| // Defensive checking against incorrectly wound Geojson polygons. | ||
| const checkedRing = | ||
| idx === 0 | ||
| ? enforceOuterRing(ringToPath(ring)) | ||
| : enforceInnerRing(ringToPath(ring)); | ||
|
|
||
| paths.push(checkedRing); | ||
| } | ||
|
|
||
| return paths; | ||
| } | ||
|
|
||
| /** | ||
| * Make sure this ring is wound as an outer ring, according to clipper2 | ||
| * expectations. That is, clockwise. | ||
| */ | ||
| function enforceOuterRing(path: PathD): PathD { | ||
| if (areaD(path) < 0) { | ||
| // Leave original array untouched. | ||
| return [...path].reverse(); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should be able to mutate path instead, because ringToPath already creates your own internal representation. |
||
| } | ||
|
|
||
| return path; | ||
| } | ||
|
|
||
| /** | ||
| * Make sure this ring is wound as an inner ring, according to clipper2 | ||
| * expectations. That is, counter clockwise. | ||
| */ | ||
| function enforceInnerRing(path: PathD): PathD { | ||
| if (areaD(path) > 0) { | ||
| // Leave original array untouched. | ||
| return [...path].reverse(); | ||
| } | ||
|
|
||
| return path; | ||
| } | ||
|
|
||
| /** | ||
| * Converts a ring to a clipper2 path. | ||
| */ | ||
| function ringToPath(ring: Position[]): PathD { | ||
| return ring.map(([x, y]) => ({ x, y })); | ||
| } | ||
|
|
||
| /** | ||
| * Construct the output Geojson based on a clipper2 tree. The tree is useful for propertly handing holes. | ||
| */ | ||
| function polyTreeToGeoJSON(polyTree: PolyTreeD): Polygon | MultiPolygon | null { | ||
| const polygons: Position[][][] = []; | ||
|
|
||
| // Process each top-level polygon (outer contours) | ||
| for (let i = 0; i < polyTree.count; i++) { | ||
| const child = polyTree.child(i); | ||
| if (child && !child.isHole) { | ||
| const polygon = processPolyPath(child); | ||
| if (polygon.length > 0) { | ||
| polygons.push(polygon); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (polygons.length === 0) { | ||
| return null; | ||
| } | ||
|
|
||
| // If exactly 1 polygon return as Geojson Polygon | ||
| if (polygons.length === 1) { | ||
| return { | ||
| type: "Polygon", | ||
| coordinates: polygons[0], | ||
| }; | ||
| } | ||
|
|
||
| // If anything else return as MultiPolygon | ||
| return { | ||
| type: "MultiPolygon", | ||
| coordinates: polygons, | ||
| }; | ||
| } | ||
|
|
||
| function processPolyPath(polyPath: PolyPathD): Position[][] { | ||
| const rings: Position[][] = []; | ||
|
|
||
| // Add the outer ring (contour) | ||
| const outerRing = pathToCoordinates(polyPath.poly); | ||
| if (outerRing.length > 0) { | ||
| rings.push(outerRing); | ||
| } | ||
|
|
||
| // Add any holes (children are the holes) | ||
| for (let i = 0; i < polyPath.count; i++) { | ||
| const child = polyPath.child(i); | ||
| if (child && child.isHole) { | ||
| const holeRing = pathToCoordinates(child.poly); | ||
| if (holeRing.length > 0) { | ||
| rings.push(holeRing); | ||
| } | ||
|
|
||
| // Expectation is pools within islands within lakes within continents ... | ||
| // are handled as multipolygons. So further recursion is not required. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you tested this to make sure that it doesn't nest new outer rings inside holes?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had though clearly not well enough. Worked ok for union, but bombed on difference with nested oceans -> continents -> lakes -> islands. Should cover those cases now, with some meatier tests added as well. |
||
| } | ||
| } | ||
|
|
||
| return rings; | ||
| } | ||
|
|
||
| /** | ||
| * Converts a clipper2 integer path to an array of Geojson Positions. | ||
| */ | ||
| function pathToCoordinates(path: PathD | null): Position[] { | ||
| const coords: Position[] = []; | ||
|
|
||
| if (!path || typeof path.length !== "number") { | ||
| return coords; | ||
| } | ||
|
|
||
| for (let i = 0; i < path.length; i++) { | ||
| const pt = path[i]; | ||
| coords.push([pt.x, pt.y]); | ||
| } | ||
|
|
||
| // GeoJSON requires the first and last coordinates to be identical (closed ring) | ||
| if (coords.length > 0) { | ||
| const first = coords[0]; | ||
| const last = coords[coords.length - 1]; | ||
| if (first[0] !== last[0] || first[1] !== last[1]) { | ||
| coords.push([first[0], first[1]]); | ||
| } | ||
| } | ||
|
|
||
| return coords; | ||
| } | ||
|
|
||
| export { | ||
| multiPolygonToPaths, | ||
| polygonToPaths, | ||
| polyTreeToGeoJSON, | ||
| DEFAULT_PRECISION, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| { | ||
| "name": "@turf/internal", | ||
| "version": "7.3.1", | ||
| "private": true, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://docs.npmjs.com/cli/v11/configuring-npm/package-json#private You can't depend on this from other packages if you mark it as private, they won't be able to be work because one of their dependencies will be missing.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ugh. Yes you're right, we can't drop tsup and keep this private. I think there's a valid need for a package where we can store shared code so will make public and document clearly it's not for direct usage, API may change at any time, etc.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes more sense to me to keep it general purpose so we don't have to publish a new package for every bit of common code we want to share between packages. |
||
| "description": "Common functionality used across multiple Turf packages.", | ||
| "author": "Turf Authors", | ||
| "contributors": [ | ||
| "James Beard <@smallsaucepan>" | ||
| ], | ||
| "license": "MIT", | ||
| "bugs": { | ||
| "url": "https://github.com/Turfjs/turf/issues" | ||
| }, | ||
| "homepage": "https://github.com/Turfjs/turf", | ||
| "repository": { | ||
| "type": "git", | ||
| "url": "git://github.com/Turfjs/turf.git" | ||
| }, | ||
| "funding": "https://opencollective.com/turf", | ||
| "type": "module", | ||
| "exports": { | ||
| "./package.json": "./package.json", | ||
| "./clipper2": { | ||
| "import": { | ||
| "types": "./dist/esm/clipper2.d.ts", | ||
| "default": "./dist/esm/clipper2.js" | ||
| }, | ||
| "require": { | ||
| "types": "./dist/cjs/clipper2.d.cts", | ||
| "default": "./dist/cjs/clipper2.cjs" | ||
| } | ||
| }, | ||
| "./babel-plugin-bigint-patch": { | ||
| "import": { | ||
| "types": "./dist/esm/babel-plugin-bigint-patch.d.ts", | ||
| "default": "./dist/esm/babel-plugin-bigint-patch.js" | ||
| }, | ||
| "require": { | ||
| "types": "./dist/cjs/babel-plugin-bigint-patch.d.cts", | ||
| "default": "./dist/cjs/babel-plugin-bigint-patch.cjs" | ||
| } | ||
| } | ||
| }, | ||
| "sideEffects": false, | ||
| "files": [ | ||
| "dist" | ||
| ], | ||
| "scripts": { | ||
| "build": "tsup" | ||
| }, | ||
| "devDependencies": { | ||
| "@babel/core": "^7.26.10", | ||
| "@babel/types": "^7.26.10", | ||
| "@types/babel__core": "^7.20.5", | ||
| "tsup": "^8.4.0" | ||
| }, | ||
| "dependencies": { | ||
| "@types/geojson": "^7946.0.10", | ||
| "clipper2-ts": "^2.0.1", | ||
| "tslib": "^2.8.1" | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| { | ||
| "extends": "../../tsconfig.shared.json" | ||
| } |
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.
This probably needs a rename to make it clear that this is for coordinates in lng, lat. If you project the coords into something with meters as their unit, you probably want 2 instead.
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.
Altered this to
TURF_CLIPPER2_SCALE_FACTORbut still not super happy with it. Scale factor because that's how the clipper2 docs refer to it. What are your thoughts?When you say "if you project the coords ..." I know that's possible, though is that something we would need to do in Turf? Should we maybe abstract that detail away within a TurfClipper class that turf-* packages simply no-arg instantiate and pass decimal degrees to?