-
Notifications
You must be signed in to change notification settings - Fork 206
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
Changes from 2 commits
f4ca06b
a0fdea5
0bd04a5
6431033
771061c
a6138f1
7dc86fd
de84485
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -115,6 +115,19 @@ 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 | ||
/// ``` | ||
/// 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(); | ||
/// let d: f64 = transform.d(); | ||
/// let e: f64 = transform.e(); | ||
/// let f: f64 = transform.yoff(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, done at 771061c!
weiji14 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// ``` | ||
|
||
#[derive(Copy, Clone, PartialEq, Eq)] | ||
pub struct AffineTransform<T: CoordNum = f64>([[T; 3]; 3]); | ||
|
||
|
@@ -300,12 +313,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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CoordNum implements There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, finding out that I need to deref or call Edit: done at 6431033 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. since T is copy, you should be able to just remove the |
||
&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] | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Tried this locally, and clicking on the But I'll take this as a good-enough compromise, unless anyone has any other suggestions? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> { | ||
|
@@ -585,4 +623,14 @@ mod tests { | |
poly.affine_transform_mut(&identity); | ||
assert_eq!(expected, poly); | ||
} | ||
#[test] | ||
fn test_affine_transform_indexing() { | ||
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); | ||
} | ||
} |
There was a problem hiding this comment.
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 sureThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes,
xoff
please.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in a6138f1