georust / geo

Geospatial primitives and algorithms for Rust
https://crates.io/crates/geo
Other
1.47k stars 190 forks source link

Semantics for AffineTransform composition is reversed #1195

Closed adampowerdeciphex closed 1 week ago

adampowerdeciphex commented 1 week ago

When composing multiple AffineTransforms, one would expect that the composed transform is the equivalent of applying the individual transforms from left to right. Instead it is the same as applying right to left.

The solution would be to reverse the matrix multiplication of the compose() function. Alternatively this could be made clear in the docs as there may be people that depend on this right to left behaviour.

Sample code

use geo::{AffineOps, AffineTransform, Point};

fn main() {
    let point = Point::new(1.,0.);

    let translate = AffineTransform::translate(1., 0.);
    let scale = AffineTransform::scale(4., 1., [0., 0.]);
    let composed = translate.compose(&scale);

    assert_eq!(point.affine_transform(&translate), Point::new(2., 0.));
    assert_eq!(point.affine_transform(&scale), Point::new(4., 0.));
    assert_eq!(point.affine_transform(&translate).affine_transform(&scale), Point::new(8., 0.));

    // This assert fails, as the composed transform instead produces Point::new(5., 0.)
    assert_eq!(point.affine_transform(&composed), Point::new(8., 0.));
}
lnicola commented 1 week ago

For prior art, see https://gdal.org/api/raster_c_api.html#_CPPv424GDALComposeGeoTransformsPKdPKdPd.

lnicola commented 1 week ago

But I don't understand, why do you say it's right-to-left? It looks left-to-right to me (composed is equivalent to translate, then scale).

adampowerdeciphex commented 1 week ago

Sorry I should have mentioned that the final assert here fails. The expected result would be Point::new(8., 0.), but instead produces Point::new(5., 0.)

michaelkirk commented 1 week ago

Fixed in #1196 - please take a look. Thank you for catching this and providing a test!

adampowerdeciphex commented 1 week ago

Fantastic, thanks for the quick fix! 😃