Skip to content

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

Merged
smallsaucepan merged 4 commits intoTurfjs:masterfrom
smallsaucepan:random-poly-fix
Sep 21, 2024
Merged

Reversed the order in which random polygon points are generated to satisfy the right hand rule#2715
smallsaucepan merged 4 commits intoTurfjs:masterfrom
smallsaucepan:random-poly-fix

Conversation

@smallsaucepan
Copy link
Member

Resolves #2605.

…tisfy to the right hand rule for polygons. Resolves Turfjs#2605
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.

@smallsaucepan smallsaucepan merged commit f4a00b2 into Turfjs:master Sep 21, 2024
@smallsaucepan smallsaucepan deleted the random-poly-fix branch November 22, 2024 01:22
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.

Randomly generated polygons do not comply with the right-hand rule.

2 participants