Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Semantics for AffineTransform composition is reversed #1195

Closed
adampowerdeciphex opened this issue Jun 25, 2024 · 5 comments · Fixed by #1196
Closed

Semantics for AffineTransform composition is reversed #1195

adampowerdeciphex opened this issue Jun 25, 2024 · 5 comments · Fixed by #1196

Comments

@adampowerdeciphex
Copy link

adampowerdeciphex commented Jun 25, 2024

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
Copy link
Member

lnicola commented Jun 25, 2024

@lnicola
Copy link
Member

lnicola commented Jun 25, 2024

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
Copy link
Author

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 added a commit that referenced this issue Jun 25, 2024
FIX: #1195

e.g. for "conventional" https://gdal.org/api/raster_c_api.html#_CPPv424GDALComposeGeoTransformsPKdPKdPd

```
/// The resulting geotransform is the equivalent to padfGT1 and then padfGT2 being applied to a point.
/// Parameters:
///     padfGT1 -- the first geotransform, six values.
///     padfGT2 -- the second geotransform, six values.
///     padfGTOut -- the output geotransform, six values, may safely be the same array as padfGT1 or padfGT2.
void GDALComposeGeoTransforms(const double *padfGeoTransform1, const double *padfGeoTransform2, double *padfGeoTransformOut)
```

Note: `padfGT1` **and then** `padfGT2`, previously we were effectively doing `padfGT2` **and then** `padfGT1`.

I also simplified some of the examples to have more understandable
input/output. I'm not good enough at mental matrix math to know what to
expect from a rotated skew.
michaelkirk added a commit that referenced this issue Jun 25, 2024
FIX: #1195

e.g. for "conventional" https://gdal.org/api/raster_c_api.html#_CPPv424GDALComposeGeoTransformsPKdPKdPd

```
/// The resulting geotransform is the equivalent to padfGT1 and then padfGT2 being applied to a point.
/// Parameters:
///     padfGT1 -- the first geotransform, six values.
///     padfGT2 -- the second geotransform, six values.
///     padfGTOut -- the output geotransform, six values, may safely be the same array as padfGT1 or padfGT2.
void GDALComposeGeoTransforms(const double *padfGeoTransform1, const double *padfGeoTransform2, double *padfGeoTransformOut)
```

Note: `padfGT1` **and then** `padfGT2`, previously we were effectively doing `padfGT2` **and then** `padfGT1`.

I also simplified some of the examples to have more understandable
input/output. I'm not good enough at mental matrix math to know what to
expect from a rotated skew.
@michaelkirk
Copy link
Member

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

michaelkirk added a commit that referenced this issue Jun 25, 2024
FIX: #1195

e.g. for "conventional" https://gdal.org/api/raster_c_api.html#_CPPv424GDALComposeGeoTransformsPKdPKdPd

```
/// The resulting geotransform is the equivalent to padfGT1 and then padfGT2 being applied to a point.
/// Parameters:
///     padfGT1 -- the first geotransform, six values.
///     padfGT2 -- the second geotransform, six values.
///     padfGTOut -- the output geotransform, six values, may safely be the same array as padfGT1 or padfGT2.
void GDALComposeGeoTransforms(const double *padfGeoTransform1, const double *padfGeoTransform2, double *padfGeoTransformOut)
```

Note: `padfGT1` **and then** `padfGT2`, previously we were effectively doing `padfGT2` **and then** `padfGT1`.

I also simplified some of the examples to have more understandable
input/output. I'm not good enough at mental matrix math to know what to
expect from a rotated skew.
@adampowerdeciphex
Copy link
Author

Fantastic, thanks for the quick fix! 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants