Michael-F-Bryan / arcs

A Rust CAD System
https://michael-f-bryan.github.io/arcs
Apache License 2.0
249 stars 23 forks source link

Implement matrix for transformations #5

Closed DerLando closed 4 years ago

DerLando commented 4 years ago

Hi @Michael-F-Bryan,

I read your very interesting blog post and got interested in the project. This is my first contribution to a rust project, so I would be glad to receive some criticism.

Pull request is for a transformation framework based on 3x3 matrices. Implemented in the branch is the scale trait from the wishlist. Some further thoughts:

Michael-F-Bryan commented 4 years ago

Thanks for adding a uniform Scale trait! It looks almost identical to how I'd implement it.

How would you see this Transformation matrix being used? Could we just use kurbo::Affine directly?

DerLando commented 4 years ago

Yes! I had a look at the kurbo source and the change to implement it directly is quite small, as you can see in the commit.

There are still some design decisions to be had around transformations, maybe it would be good to have them in a seperate issue:

We already have a BoundingBox algorithm and transforming with reference points is basically just a composition:

What would be a good interface for implementing this? Having 3 traits for each transformation, f.e.

  1. ScaleOrigin
  2. ScaleReference
  3. ScaleInPlace ?
Michael-F-Bryan commented 4 years ago

The main way I write Scale (and Rotate and friends) is to explicitly provide some sort of "base" point that the transformation is done relative to.

trait Scale {
  fn scale(&mut self, scale_factor: f64, base: Vector):
}

Then you can pass in Vector::zero() or self.bounding_box().centre() to get ScaleReference and ScaleOrigin for free... Thoughts?

I'm more than happy to upstream things like shear and mirror. They're easy enough to implement using the coefficients constructor, I'm guessing they were just never needed in piet.

DerLando commented 4 years ago

Sure enough.

I change the function signature and do a commit.

DerLando commented 4 years ago

Sometimes waiting is good :)

I removed the unneeded using and the branch should be ready to be merged.

Michael-F-Bryan commented 4 years ago

Excellent! Thanks for all the hard work @DerLando :+1: