geo

geo copied to clipboard

They seem pretty similar?

Currently looks like Orient is implemented for (Multi)Polygons, whereas WindingOrder is only for LineString.

Indeed they have overlap, but WindingOrder has a handful of other methods. Do they make sense to implement for (Multi)Polygons?

e.g. does some_poly.points_ccw() make sense? Presumably it'd be equivalent to some_poly.exterior().points_ccw().

What happens when a multi poly has a mix of cw and ccw polys? some_multi_poly.winding_order() == None ?

Do any of those seem too surprising?

I'd be especially interested in @rmanoka's opinion here.

@michaelkirk Yeah, I wasn't sure of the reason for the Orient trait. The doc says:

    //// Orients a Polygon's exterior and interior rings according to convention
    ///
    /// By default, the exterior ring of a Polygon is oriented counter-clockwise, and any interior
    /// rings are oriented clockwise.

I think the Area algorithm used to require this convention, but no longer so. However, if this is a useful convention, we could allow this to exist. I'm not sure about merging the two; like you say points_ccw for Polygon or MultiPolygon is not well-defined and may be confusing.

In general, if we think there are too many algo. traits, we could feature gate them at some point and allow the users to choose whatever subset they like. They are additive anyway as one doesn't preclude any other. The only issue I see is with CI, where we use @frewsxcv 's cargo-build-all-features functionality, which will end up testing out too many different combinations of features. Anyway, I don't think the crate is bloated enough to consider this yet.

I was thinking the primary benefit was less about compile-time/bloat and more so about usability/discoverability of the functionality — It's likely to slow people down when we have two separate-but-similar concepts they need to investigate.

Asked differently - is it likely that anything besides LineString would impl the WindingOrder trait as it's currently expressed?

What if we re-scoped the WindingOrder trait to incorporate only the common things which would apply to multiple geoms - specifically, if we moved the points_cw/points_ccw methods off the trait and implemented them directly onto LineString, at that point maybe it'd be uncontroversial to combine the WindingOrder and Orient trait.

Happy to do the legwork if you're on board.

I was thinking the primary benefit was less about compile-time/bloat and more so about usability/discoverability of the functionality — It's likely to slow people down when we have two separate-but-similar concepts they need to investigate.

Agreed.

Asked differently - is it likely that anything besides LineString would impl the WindingOrder trait as it's currently expressed?

Implementing even a stripped down version of WindingOrder on Polygon feels a bit imposing; the user could as well just use WindingOrder on the exterior or interiors in the manner they want. My opinion is that we should implement important / core algorithms, on the simplest type it works on, and only extend it to larger types if it makes a lot of sense. To me, WindingOrder on Polygon or MultiPolygon is not very well defined, and we would just be imposing some convention with no implications. Right now, that is how I feel about Orient trait too, but that might just be my ignorance.

What if we re-scoped the WindingOrder trait to incorporate only the common things which would apply to multiple geoms - specifically, if we moved the points_cw/points_ccw methods off the trait and implemented them directly onto LineString, at that point maybe it'd be uncontroversial to combine the WindingOrder and Orient trait.

One issue is that the kernels, and thus robust predicates are only available in geo. However, the type and hence it's direct methods are only available in geo-types. This is the same issue as with is_convex, which we are providing as a separate trait in #505 . We could consider moving the kernels into geo-types.

Happy to do the legwork if you're on board.

Very much a drive-by comment, so apologies in advance: the orientation question with regard to Polygon is interesting, and I hadn't considered that it even could be ill-defined, so thank you @rmanoka. That raises a related issue around convention and user expectation: what do JTS and GEOS do here? I ask because we should emulate their functionality. I realise that this might throw a spanner in the works, and I'm not even in a position to do much about it in terms of research or engagement for another ten days or so – again, apologies.