HarryStevens / geometric

A JavaScript library for doing geometry.
https://www.npmjs.com/package/geometric
MIT License
976 stars 49 forks source link

polygonWind uses the wrong convention #48

Open kimchirichie opened 1 month ago

kimchirichie commented 1 month ago

I was looking to use polygonWind to reorient my polygons to be right hand oriented. However there appears to be a mistake in the implementation which incorrectly interprets positive area as clockwise orientation.

https://github.com/HarryStevens/geometric/blob/124de1eca175d4c7e315ef9909ed2269375f9e84/src/polygons/polygonWind.js#L10

The implementation here should be corrected to:

  var isClockwise = polygonArea(polygon, true) < 0;

Conventionally, counterclockwise nets a positive area & polygonArea is correctly implemented.

HarryStevens commented 2 weeks ago

Hi @kimchirichie. Take a look at the demo here: https://observablehq.com/@harrystevens/geometric-polygonwind.

First, draw a polygon in the gray field. On the polygon below, the vertices are numbered in order. If you toggle back and forth between "Clockwise" and "Counter-clockwise", you will see that the vertices, as displayed on the screen, are numbered in the correct order corresponding to your selection.

It is true that in plane geometry, the convention is that counter-clockwise winding nets a positive area. The reason it is reversed here is that computer screens reverse the y-axis — 0 is on top rather than on bottom. I recognize that defying the convention can be confusing, but I think it would be even more confusing if a user asked the function to return a polygon whose vertices were in clockwise order, but then they appeared counter-clockwise on the screen.

That's why I leaned towards implementing polygonWind in this way, but I am open to counter-arguments. For instance, maybe it would be better to do const isClockwise = polygonArea(polygon, true) < 0;, and then I would update the documentation to explain that the polygon's vertices will appear in the opposite order on your screen. If I were to do this, it would be a breaking change, so I would only want to implement it along with a major update to V3.