MultiPolygon::rotate should use centroid as pivot

[anlumo]

Right now, MultiPolygon::rotate rotates all its polygons individually. However, I'd expect it to rotate them around its own centroid. Otherwise, I'd just iterate over the inner polygons and rotate them myself.

Obviously, this would be a breaking change, so it'd probably be better to add a separate function for this. Ideally, it'd be nice to have a variant where you just pass the pivot explicitly.

[urschrei]

I could have sworn we used the same logic as Shapely when we implemented this. Hmm.

it'd be nice to have a variant where you just pass the pivot explicitly.

Does https://docs.rs/geo/0.12.2/geo/algorithm/rotate/trait.RotatePoint.html work for you?

[anlumo]

Yes, that one does exactly what I need, thank you! So, disregard the second paragraph.

The point about rotate arguably not behaving as you'd expect still stands, though.

[andrei-papou]

Is something like this sufficient?

pub trait RotateCentroid<T>: RotatePoint<T> + Centroid {
    fn rotate_around_centroid(&self, angle: T) -> <Self as Centroid>::Output
    where
        T: Float,
        <Self as Centroid>::Output: Float,
    {
        self.rotate_around_point(angle, self.centroid())
    }
}

Seems like it gives the ability to rotate the multipolygon around the centroid.

[frewsxcv]

@andrei-papou that looks good to me!

a couple relevant links: #151 #287

[martinfrances107]

The code fragment form above does not compile

#412 (comment)

<Self as Centroid>::Output: Float,

Float is a 1-D scalar type, the value is needed is a 2D type like Point .. but that also errors out a as Trait is needed struct type given.

the solution is listed below .. I have bundled this up with a test and I am about to post a PR.

pub trait RotateCentroid<T: CoordinateType>: RotatePoint<T> + Centroid<Output = Point<T>> {
    fn rotate_around_centroid(&self, angle: T) -> Self
    where
        T: Float,
        Self: Sized,
    {
        self.rotate_around_point(angle, self.centroid())
    }
}

[michaelkirk]

I know I'm late to be giving design input - sorry.

Obviously, this would be a breaking change, so it'd probably be better to add a separate function for this. Ideally, it'd be nice to have a variant where you just pass the pivot explicitly.

I appreciate the sensitivity towards "breaking" the existing API, but I actually believe the current behavior is a bug, and we should fix it rather than introduce another trait.

At least one person was surprised by the existing behavior, and more will be in the future.

Having a profusion of options makes things harder for people. Why do I want to think about rotate vs rotate_centroid?

And probably more importantly from my perspective - it seems like other libraries do it this way (rotate everything in the collection around the collections centroid). I think there is a lot of value in being familiar to people coming from other environments.

If there is anyone who thinks the existing behavior is correct and worth keeping, I'm open to more discussion, but otherwise I'd say change the existing implementation rather than add a new trait.

[michaelkirk]

🦗

Unless I hear something to the contrary soon - I'm going to assume people are ok with "fixing" the legacy API to behave like other libs rather than keeping our idiosyncratic version and adding some secondary conventional API.

[urschrei]

Happy for the "fix" to merge for compat / "in line with user expectations" reasons!

[mapoulos]

I'm putting together a PR with a test or two "fixing" of the legacy API.

[michaelkirk]

Fixed in b0a6752