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

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
21 changes: 11 additions & 10 deletions packages/turf-random/bench.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
// I don't think this bench test does anything? There is no entry point into
// the module called random() that takes a string?
import Benchmark, { Event } from "benchmark";
import { randomPolygon } from "./index.js";

import { random } from "./index.js";
import Benchmark from "benchmark";
let totalTime = 0.0;
const suite = new Benchmark.Suite("turf-random");

var suite = new Benchmark.Suite("turf-random");
suite
.add("turf-random", function () {
random("point");
})
.on("cycle", function (event) {
console.log(String(event.target));
.add("turf-random", () => randomPolygon(1, { num_vertices: 100000 }), {
onComplete: (e: Event) =>
(totalTime = totalTime += e.target.times?.elapsed),
})
.on("cycle", (e: Event) => console.log(String(e.target)))
.on("complete", () =>
console.log(`completed in ${totalTime.toFixed(2)} seconds`)
)
.run();
12 changes: 6 additions & 6 deletions packages/turf-random/index.ts
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