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.

Sign up for GitHub

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

Jump to bottom

Reversed the order in which random polygon points are generated to satisfy the right hand rule #2715

Merged
merged 4 commits into from
Sep 21, 2024
Merged

Reversed the order in which random polygon points are generated to satisfy the right hand rule #2715

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7047abb
Reversed the order in which random polygon points are generated to sa…
smallsaucepan Sep 15, 2024
3e2badd
Removing overlooked console.log()
smallsaucepan Sep 15, 2024
2038910
Merge branch 'Turfjs:master' into random-poly-fix
smallsaucepan Sep 21, 2024
1b3a2ce
Adding a simple benchmark test for turf-random.
smallsaucepan Sep 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions packages/turf-random/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,16 +145,16 @@ function randomPolygon(

const features = [];
for (let i = 0; i < count; i++) {
let vertices: any[] = [];
let vertices: number[][] = [];
const circleOffsets = [...Array(options.num_vertices + 1)].map(Math.random);

// Sum Offsets
circleOffsets.forEach((cur: any, index: number, arr: any[]) => {
circleOffsets.forEach((cur, index, arr) => {
arr[index] = index > 0 ? cur + arr[index - 1] : cur;
});

// scaleOffsets
circleOffsets.forEach((cur: any) => {
circleOffsets.forEach((cur) => {
cur = (cur * 2 * Math.PI) / circleOffsets[circleOffsets.length - 1];
const radialScaler = Math.random();
vertices.push([
Expand All @@ -165,9 +165,9 @@ function randomPolygon(
vertices[vertices.length - 1] = vertices[0]; // close the ring

// center the polygon around something
vertices = vertices.map(
vertexToCoordinate(randomPositionUnchecked(paddedBbox))
);
vertices = vertices
.reverse() // Make counter-clockwise to adhere to right hand rule.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seem fine to do in the module's current state, but the performance of the module with all of the array.map() calls seems like it'd probably be easy to make quite a bit faster. If we went to that trouble we'd probably want to just make vertices just populate in the reverse order so that it was correct without the reversing step here.

Happy to merge this as is, because I haven't heard of any performance-sensitive random polygon generation hitting issues here, and this reads clearly in its current form.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me take a look and see if there's a way to avoid reverse() ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Some performance results. Ops/sec for a 100000 vertex random polygon, average of three runs:

Original reverse()
47.98 46.75

Agree with you - the added code complexity isn't worth it. Will merge.

.map(vertexToCoordinate(randomPositionUnchecked(paddedBbox)));
features.push(polygon([vertices]));
}
return featureCollection(features);
Expand Down