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

Implement getter methods on AffineTransform to access internal elements #1159

Merged
merged 8 commits into from
May 8, 2024
3 changes: 3 additions & 0 deletions geo/CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

## Unreleased

* Implement getter methods on `AffineTransform` to access internal elements.
* <https://github.com/georust/geo/pull/1159>

## 0.28.0

* BREAKING: The `HasKernel` trait was removed and it's functionality was merged
Expand Down
55 changes: 53 additions & 2 deletions geo/src/algorithm/affine_ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,22 @@ impl<T: CoordNum, M: MapCoordsInPlace<T> + MapCoords<T, T, Output = Self>> Affin
/// (x: -0.5688687, y: 5.5688687)
/// ], max_relative = 1.0);
/// ```
///
/// ## Create affine transform manually, and access elements using getter methods
/// ```
/// use geo::AffineTransform;
///
/// let transform = AffineTransform::new(10.0, 0.0, 400_000.0, 0.0, -10.0, 500_000.0);
///
/// let a: f64 = transform.a();
/// let b: f64 = transform.b();
/// let c: f64 = transform.xoff();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe name this variable binding xoff? Not sure

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, xoff please.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in a6138f1

/// let d: f64 = transform.d();
/// let e: f64 = transform.e();
/// let f: f64 = transform.yoff();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make this an actual doctest, you could create a second transform with these values and assert it's equal?

assert_eq!(transform, AffineTransform::new(a, b, c, d, e, f))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done at 771061c!

/// assert_eq!(transform, AffineTransform::new(a, b, c, d, e, f))
/// ```

#[derive(Copy, Clone, PartialEq, Eq)]
pub struct AffineTransform<T: CoordNum = f64>([[T; 3]; 3]);

Expand Down Expand Up @@ -300,12 +316,37 @@ impl<T: CoordNum> AffineTransform<T> {
/// The argument order matches that of the affine transform matrix:
///```ignore
/// [[a, b, xoff],
/// [d, e, yoff],
/// [0, 0, 1]] <-- not part of the input arguments
/// [d, e, yoff],
/// [0, 0, 1]] <-- not part of the input arguments
/// ```
pub fn new(a: T, b: T, xoff: T, d: T, e: T, yoff: T) -> Self {
Self([[a, b, xoff], [d, e, yoff], [T::zero(), T::zero(), T::one()]])
}

/// Get the x-resolution value.
pub fn a(&self) -> T {
self.0[0][0]
}
/// Get the x-rotation value.
pub fn b(&self) -> T {
self.0[0][1]
}
/// Get the x-offset value.
pub fn xoff(&self) -> T {
self.0[0][2]
}
/// Get the y-rotation value.
pub fn d(&self) -> T {
self.0[1][0]
}
/// Get the y-resolution value.
pub fn e(&self) -> T {
self.0[1][1]
}
/// Get the y-offset value.
pub fn yoff(&self) -> T {
self.0[1][2]
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These doc comments – if they're needed at all – should not refer to the terminology ("resolution", "rotation" etc) used in Matt's blog post, as these methods aren't solely intended for vector <--> raster coordinate transformations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the methods here are only used to retrieve the element values, not for coordinate transformation. So what would be good descriptions for each of these getter methods? Do we simply don't assign any meaning to them, and assume that the user can figure out what the a, b, xoff, c, d, yoff values represent?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That depends on whether it's reasonable to assume that someone who's trying to build a custom affine transform is familiar with its mechanics, and has (or is likely to) look at https://docs.rs/geo/latest/geo/algorithm/affine_ops/struct.AffineTransform.html, where the position and thus meaning of a, b, xoff etc are obvious.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, there should be some description when a user hovers over the method in their IDE, so that they don't have to refer to the online documentation. Having a method with a single letter like .a() or .b() unfortunately isn't very descriptive...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about just a link back to the constructor docs, which has an ascii diagram of the matrix?

// See [new] for this value's role in the affine transformation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about just a link back to the constructor docs, which has an ascii diagram of the matrix?

Tried this locally, and clicking on the AffineTransform::new link in my editor still takes me to the online documentation page.

Screenshot at 2024-04-26 16-42-12

But I'll take this as a good-enough compromise, unless anyone has any other suggestions?

Copy link
Member

@kylebarron kylebarron Apr 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the rendered docs online that link should point to the correct anchor.

}

impl<T: CoordNum + Neg> AffineTransform<T> {
Expand Down Expand Up @@ -585,4 +626,14 @@ mod tests {
poly.affine_transform_mut(&identity);
assert_eq!(expected, poly);
}
#[test]
fn test_affine_transform_getters() {
let transform = AffineTransform::new(10.0, 0.0, 400_000.0, 0.0, -10.0, 500_000.0);
assert_eq!(transform.a(), 10.0);
assert_eq!(transform.b(), 0.0);
assert_eq!(transform.xoff(), 400_000.0);
assert_eq!(transform.d(), 0.0);
assert_eq!(transform.e(), -10.0);
assert_eq!(transform.yoff(), 500_000.0);
}
}
Loading