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.

Sign up for GitHub

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

Jump to bottom

Add AffineOps trait to transform an entire geometry (plus some tweaks) #871

Merged

bors merged 1 commit into main from mkirk/affine

Jul 1, 2022

Member

michaelkirk commented Jun 30, 2022

I agree to follow the project's code of conduct.
I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

As discussed in this comment, adds the ability to compose a transformation and apply the thing to an entire geometry (saving users the hassle of having to map_coords themselves:

let center = ls.bounding_rect().unwrap().center();
let mut transform = AffineTransform::skew(40.0, 40.0, center).rotated(45.0, center);
ls.affine_transform(&transform);

I also included some tweaks based on my experience using AffineTransform in another project.

michaelkirk requested a review from urschrei

June 30, 2022 22:20

Member Author

michaelkirk commented Jun 30, 2022

Actually - hold off. I'm going to think about naming a bit more...

michaelkirk closed this

michaelkirk reopened this

michaelkirk commented

View reviewed changes

geo/src/algorithm/affine_ops.rs

+                  /// Apply `transform` immutably, outputting a new geometry.
+                  #[must_use]
+                  fn affine_transform(&self, transform: &AffineTransform<T>) -> Self;

Member Author

michaelkirk Jun 30, 2022 •

edited

Loading

Originally I wanted to do:

mutable: affine_transform
immutable: affine_transformed

But it just conflicts too much with our existing implementation conventions. I'm going to follow up with another PR (update: see #872) to add mutable flavors with consistent naming for Scale/Skew/Rotate/Translate.

michaelkirk mentioned this pull request

Add helpful methods to affine backed traits #872

Merged

2 tasks

michaelkirk commented

View reviewed changes

Member Author

michaelkirk left a comment

Ok - this is ready for review!

geo/src/algorithm/affine_ops.rs

@@ @@ -127,6 +183,10 @@ impl<T: CoordNum> AffineTransform<T> { @@
                       )
                   }
+                  pub fn is_identity(&self) -> bool {

Member Author

michaelkirk Jun 30, 2022

This came in handy while trying to skip application in my code base.

Member

urschrei Jul 1, 2022

Could we get a brief example (or explanation as a doc comment)?

geo/src/algorithm/affine_ops.rs

@@ @@ -141,8 +201,8 @@ impl<T: CoordNum> AffineTransform<T> { @@
                   /// xoff = origin.x - (origin.x * xfact)
                   /// yoff = origin.y - (origin.y * yfact)
                   /// ```
-                  pub fn scale(xfact: T, yfact: T, origin: Point<T>) -> Self {
-                      let (x0, y0) = origin.x_y();
+                  pub fn scale(xfact: T, yfact: T, origin: impl Into<Coordinate<T>>) -> Self {

Member Author

michaelkirk Jun 30, 2022

I'm not sure if this is controversial - but otherwise this is always kind of a papercut for me at the call site.

Member

urschrei Jul 1, 2022

IIRC using Into in function signatures can lead to less informative error messages but I can't find the ref now and I would hope that the signature makes it obvious what's going on, so 👍

Member Author

michaelkirk Jul 1, 2022

Sometimes I daydream about getting rid of one of Point/Coord... 🔥

urschrei approved these changes

View reviewed changes

geo/src/algorithm/affine_ops.rs

+                  fn affine_transform_mut(&mut self, transform: &AffineTransform<T>);
+                  /// Apply `transform` immutably, outputting a new geometry.
+                  #[must_use]

Member

urschrei Jul 1, 2022

TIL about must_use. This is so good.

Member Author

michaelkirk Jul 1, 2022

Yeah, using a builder pattern on something which is Copy makes it very easy to forget that this is wrong:

let mut transform = AffineTransform::identity();
transorm.translated(1.0, 2.0);
geometry.affine_transform(&transform);

It should be:

let mut transform = AffineTransform::identity();
transform = transorm.translated(1.0, 2.0);
geometry.affine_transform(&transform);

And now the user will at least get a warning to clue them in.

geo/src/algorithm/affine_ops.rs

@@ @@ -13,7 +66,7 @@ use std::fmt; @@
               /// `AffineTransform` is a row-major matrix.
               /// 2D affine transforms require six matrix parameters:
               ///
-              /// `[a, b, d, e, xoff, yoff]`
+              /// `[a, b, xoff, d, e, yoff]`

Member

urschrei Jul 1, 2022

Ah, yes. I knew I'd forgotten to update something.

urschrei approved these changes

View reviewed changes

Member

urschrei left a comment

This is great. The only addition I'd like to see is a short note in the docs (maybe using a ## Performance header?) for algorithm::Scale and co., noting that chaining them isn't as efficient as using a chained AffineTransform and affine_transform(_mut). WDYT?


          AffineOps trait to transform entire geometries

2d846cc

- mark AffineTransform builder methods as must_use
- default AffineTransform to f64
- accept coordinates for origin param
- add AffineTransformation.is_identity
- update docs

michaelkirk force-pushed the mkirk/affine branch from e2e17fc to 2d846cc Compare

July 1, 2022 16:45

Member Author

michaelkirk commented Jul 1, 2022

I added the new docs in e2e17fc and then squashed all my commits, which were a bit messy.

Member

urschrei commented Jul 1, 2022

Thanks, LGTM! Merge when you're ready.

Member Author

michaelkirk commented Jul 1, 2022

bors r=urschrei

Thanks for the review!

Contributor

bors bot commented Jul 1, 2022

Build succeeded:

ci result

bors bot merged commit dcaaa9a into main

bors bot deleted the mkirk/affine branch

July 1, 2022 17:09

bors bot added a commit that referenced this pull request


          Merge #872

31eb02f

872: Add helpful methods to affine backed traits r=urschrei a=michaelkirk

- [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/main/CODE_OF_CONDUCT.md).
- [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.
---

Leverages the new `AffineOps::affine_transform` trait to drive our various affine affiliated traits.

This doubles down on the `do_the_thing() -> Self` | `do_the_thing_mut()` naming convention from #871

Also, I was able to generically re-implement the Rotate trait in terms of other traits, which was neat. 

And I added some useful (I think) helper methods to our other Affine driven traits.


FIXES #287 (somewhat incidentally)

Co-authored-by: Michael Kirk <michael.code@endoftheworl.de>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet