Add winding order for linestrings #132
Conversation
amandasaurus
force-pushed
the
winding-order
branch
from
d79a68b
to
a80c194
Compare
| } else if shoelace > T::zero() { | ||
| WindingOrder::CounterClockwise | ||
| } else if shoelace == T::zero() { | ||
| // what should be done here? |
There was a problem hiding this comment.
It looks like shoelace == 0 when the line is degenerate (empty, single point, multiple coincident points) which brings us back to the validity conversation ( #123 #127 ).
It will also be zero for linestrings co-incident with the line y = x or either of the axes (and perhaps some other cases?).
I think changing the return type to Option<WindingOrder> would be a reasonable choice here.
There was a problem hiding this comment.
I was considering making WindingOrder have 3 enum values: Clockwise, CounterClockwise" and Open` (as in 'this linestring is open, so there is no winding order'), since in theory this method can be called on every linestring, including linestrings which are not a closed ring.
However I didn't know about the other failure cases. So maybe a None would be better then.
| /// Returns the winding order of this line | ||
| pub fn winding_order(&self) -> WindingOrder { | ||
| let shoelace = twice_area(self); | ||
| if shoelace < T::zero() { |
There was a problem hiding this comment.
imo I think a match here instead would be a little easier to read.
| } | ||
| let mut tmp = T::zero(); | ||
| for ps in linestring.0.windows(2) { | ||
| tmp = tmp + (ps[0].x() * ps[1].y() - ps[1].x() * ps[0].y()); |
There was a problem hiding this comment.
maybe for another PR, but does it make sense to extract the cross product into a trait?
There was a problem hiding this comment.
@mbattifarano we're also already computing it in convexhull.rs although in a slightly different way, and it's used in a couple of places in the Polygon Distance PR, so it's probably worth thinking about how to unify them.
| assert_eq!(new_line2.winding_order(), WindingOrder::CounterClockwise); | ||
| assert_ne!(new_line2, cw_line); | ||
| assert_eq!(new_line2, ccw_line); | ||
|
|
There was a problem hiding this comment.
I think it would be worth testing a few cases where we should expect a panic.
|
This seems relevant to the Orient trait, although I'm not sure how best to combine them… |
🤦 Gah! I didn't know about that! I saw the file and just thought it was something to do with rotations. That'll teach me for redoing work. Yes, there shouldn't be both, and hence it makes sense to "merge" them somehow. |
|
I'm not tied to |
|
I've redid this as #134, so I'm closing this. |
For some data formats (e.g. ESRI Shapefiles), rings in a polygon are stored in a specific winding order (clockwise for exterior ring, anti-clockwise for interior rings IIRC). So I added support to detect this, to make a linestring clockwise/counterclockwise, and to iterate over the points in that order.
The code is already there for the Area calculation. I split the shoelace formula code into it's own file, since it felt weird importing something from the
areamodule intotypes. But I'm open to suggestions on this, because I'm not 100% sure it's the best approach.