georust / geo

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

Expose fields in AffineTransform struct (or have a way to access the internal values)? #1158

Closed weiji14 closed 4 months ago

weiji14 commented 6 months ago

Currently as of geo=0.28.0, the AffineTransform struct's fields (implemented in #866) are currently not publicly exposed:

https://github.com/georust/geo/blob/08d3f394a9552e90474b4dacd508927cba84a6d2/geo/src/algorithm/affine_ops.rs#L118-L119

The nine values in the tuple struct are:

[[a, b, xoff],
 [d, e, yoff],
 [0, 0, 1]]

where a/e are the x/y resolution, b/d are x/y rotation (typically 0), and xoff/yoff are the x/y origin coordinates. See https://www.perrygeo.com/python-affine-transforms.html which is a good reference on this.

Personally, I'd like to have a way to access these individual fields to build up an array of x and y coordinates for pixels in a raster image, e.g. when reading from GeoTIFFs (currently implementing this in https://github.com/weiji14/cog3pio/pull/8, but will do further work that will require access to a/xoff/e/yoff). I'm aware of https://docs.rs/gdal/0.16.0/gdal/type.GeoTransform.html which implements something similar, but would prefer to not depend on GDAL if possible :slightly_smiling_face:

Specifically, it would be nice to be able to do something like:

use geo::AffineTransform;

let transform = AffineTransform::new(200.0, 0.0, 499980.0, 0.0, -200.0, 5300040.0);

let x_scale: f64 = transform.a;
let x_rotation: f64 = transform.b;
let x_origin: f64 = transform.c;
let y_rotation: f64 = transform.d;
let y_scale: f64 = transform.e;
let y_origin: f64 = transform.f;

Yes, the affine transformation field names could possibly be renamed to something more easily interpretable.

Would be happy to open a PR for this if this sounds ok. Alternatively, if making the fields public would cause breaking changes, I'd also be ok with implementing a trait that can convert AffineTransform to a HashMap or some other type that allows for variable-based indexing.

kylebarron commented 6 months ago

I'm +1 on this. It doesn't look like there's any way to currently access those fields, even with a From impl. I don't think Rust has a concept of a getter so those attributes would have to become methods. I'm ok with using a-f letters for naming, since that seems pretty standard and we can additionally link to perrygeo's blog (I also go back to that blog post often!)

lnicola commented 6 months ago

Of course you can access the coefficients, you can apply the transform to (0, 0), (1, 0) and (0, 1) and compute them :smile:.

TBH they could even be public, but it's fine either way.

urschrei commented 6 months ago

I'm happy to have the fields be public, but I'm pretty sure the current field names are correct and meaningful in terms of affine transforms generally (see e.g. Shapely's affinity module); xoff and yoff are calculated offsets and I'd rather not rename them just to conform to Matt's terminology (it is a great blog post, btw).

weiji14 commented 6 months ago

It doesn't look like there's any way to currently access those fields, even with a From impl. I don't think Rust has a concept of a getter so those attributes would have to become methods.

~Found out at https://users.rust-lang.org/t/access-tuple-struct-with-one-element-more-ergonomically/27236 that there's an Index trait in std (see https://doc.rust-lang.org/std/ops/trait.Index.html) which seems to allow indexing into the array/matrix. Have started a draft PR at #1159 as a proof of concept. This enables indexing using transform["a"] or transform.index("a")~. I could also rewrite things so that accessing the elements happens via transform.a() if that's more ergonomic. Edit: refactored implementation to use getter methods like transform.a().

Next point is to decide on the naming. To be clear, the current AffineTransform tuple struct doesn't set any names like c or xoff, it is only in the documentation. So we can go with either a,b,c,d,e,f naming or a,b,xoff,d,e,yoff.