Implement non uniform scale #6

DerLando · 2020-01-16T16:42:08Z

I implemented non-uniform-scale mostly by copying over the structure of scale.

To implement the trait for primitives::Arc we would need either an ElipticalArc or NursbArc primitive. Similar to how a circle would turn into an ellipse with two distinct focii when scaled non-uniform.

Also, the transformation traits are all returning Self right now, which might have to change for transforms like non-uniform or shear for example.

Michael-F-Bryan · 2020-01-16T16:50:29Z

arcs/src/algorithms/scale_non_uniform.rs

+
+/// Something which can be scaled **non-uniform** in x and y directions in *Drawing Space*
+pub trait ScaleNonUniform {
+    fn scale_nu(&mut self, factor_x: f64, factor_y: f64, base: Vector);


Instead of _nu, what about using the full _non_uniform to improve readability?

Otherwise here are some synonyms for uniform:

constant

consistent

steady

invariable

unvarying

unfluctuating

unvaried

unchanging

unwavering

undeviating

stable

static

sustained

regular

fixed

even

equal

equable

monotonous

identical

matching

similar

same

alike

like

selfsame

homogeneous (scale_non_homogeneous() sounds fairly accurate, but it's pretty long...)

corresponding

equivalent

reading through this list i lean towards non-constant, although non-uniform comes from another CAD package i frequently use.

Definitely agree on the readability.

Could the word "constant" be confused with Rust's concept of mutability?

I'm leaning towards scale_non_uniform(). The thinking being that trait scales something, but the scaling doesn't happen uniformly in all directions.

One could argue that it would be scale_mutable then 😉
I'm totally fine with scale_non_uniform, lets name it that.

arcs/src/algorithms/scale_non_uniform.rs

Michael-F-Bryan · 2020-01-16T16:52:40Z

arcs/src/algorithms/scale_non_uniform.rs

+
+        assert_eq!(actual, expected);
+
+        let base = Vector::new(2.0, 0.0);


What about moving this into its own test (e.g. scale_vector_around_base).

Michael-F-Bryan · 2020-01-16T16:53:23Z

arcs/src/algorithms/scale_non_uniform.rs

+
+        let actual = original.scaled_nu(factor_x, factor_y, Vector::zero());
+        // known value
+        let expected = Vector::new(-2.0, 12.5);


How about Vector::new(-1.0 * factor_x, 5.0 * factor_y)? That way you can drop the comment and it's obvious where the numbers came from.

Michael-F-Bryan · 2020-01-16T16:54:10Z

arcs/src/algorithms/scale_non_uniform.rs

+
+    #[test]
+    fn line() {
+        let start = Vector::new(2.0, 4.0);


(Same comments as the vector test)

Michael-F-Bryan · 2020-01-16T16:58:01Z

arcs/src/algorithms/scale_non_uniform.rs

+    }
+}
+
+impl ScaleNonUniform for Vector {


Are there any other types that could implement ScaleNonUniform and Scale? BoundingBox would be a good candidate.

It almost feels like we could use a more general AffineTransformable trait which accepts a kurbo::Affine to transform itself with. That way we could automatically implement Scale, ScaleNonUniform, Translate, and Rotate for any T: AffineTransformable... Thoughts?

BoundingBox could also implement Translate then.

Using a AffineTransformable trait we could avoid writing all those seperate files.
So should we still use a layer of abstraction, e.g. a Transformation struct wich has methods for returning Affine instances?

We still would need to think about how to handle Transformations which are possibly destructive e.g. shear and non-uniform.

Maybe implementing a Primitive enum and a return an instance of it from the transform function when using destuctive transforms.

So should we still use a layer of abstraction, e.g. a Transformation struct wich has methods for returning Affine instances?

Isn't that just the various constructor functions on kurbo::Affine?

What is your definition of destructive? My thoughts are that if you can apply an affine transform and the end product is still the same type of shape, it's okay.

So Line could implement AffineTransformation because stretching and scaling a Line still gives you a Line, but if you shear a BoundingBox it turns into a rhombus and is no longer an axis-aligned bounding box, so it wouldn't make sense for it to implement AffineTransformable and you'd need to implement Scale, ScaleNonUniform, and Translate manually. Same with Arc.

Ok that makes a lot of sense, so we define one base trait AffineTransformable with all the transforms auto-implemented and if primitives need other transforms they get it implemented manually.

First draft for AffineTransformable:

pub trait AffineTransformable { fn transform(&mut self, affine: Affine, base: Vector); fn transformed(&self, affine: Affine, base: Vector) -> Self where Self: Sized + Clone, { let mut clone = self.clone(); clone.transform(affine, base); clone } fn translate(&mut self, displacement: Vector) where Self: Sized, { let affine = Affine::translate(displacement); self.transform(affine, Vector::zero()); } fn translated(&mut self, displacement: Vector) -> Self where Self: Sized + Clone, { let mut clone = self.clone(); clone.translate(displacement); clone } // TODO: implement other transforms, e.g. scale, scale-non-uniform, etc... }

I was thinking more along the lines of this.

My thoughts are as follows:

Make each transformation its own trait, so each type only implements the things that make sense for them (i.e. translate() doesn't belong in the AffineTransformable trait)

Keep each trait as small as possible, preferably with just one core method

Where some transformation can be implemented exactly using a more general transformation (Scale is just a ScaleNonUniform where both factors are the same, Translate is just a AffineTransformable with an Affine::translate()) add a generic impl so you don't need to type out code twice (i.e. impl <S: ScaleNonUniform> Scale for S { ... })

Methods like translated() and scaled() which take an immutable reference and return a mutated copy are purely for convenience, so it's okay for them to have a where Self: ... clause and default implementation

... Thoughts?

I will have time to really look at this starting tomorrow.

My first impressions:

Thank you for the detailed write-up

Keeping traits (interfaces) small sounds logical and good

I didn't know this pattern of writing traits and it looks like a real good foundation for the transformation system here

…Michael-F-Bryan#6 (comment) .

…wn test

…adable

DerLando · 2020-01-22T19:05:58Z

This should be all good now, if you still see things let me know.

After this gets merged i would like to change the implementation of translate and scale like this branch does with scale_non_uniform

DerLando · 2020-01-22T19:07:52Z

arcs/src/algorithms/scale_non_uniform.rs

-        (*self).scale_nu(factor_x, factor_y, base);
+impl<A: AffineTransformable> ScaleNonUniform for A {
+    fn scale_non_uniform(&mut self, factor_x: f64, factor_y: f64, base: Vector){
+        // TODO: Change to `Affine::scale_non_uniform()` after crates.io update


After a kurbo version update we can change this to Affine::scale_non_uniform(factor_x, factor_y)

Michael-F-Bryan

Other than my question about base I'm happy to merge this.

Michael-F-Bryan · 2020-01-23T15:17:03Z

arcs/src/algorithms/affine_transform.rs

+use kurbo::Affine;
+
+pub trait AffineTransformable {
+    fn transform(&mut self, affine: Affine, base: Vector);


Do we actually need the base parameter here? I would have thought if you need an operation to be relative to a particular point you'd incorporate that into your transform matrix.

Something else to keep in mind is this function will be the simplest building block for most geometric operations so it'll be really, really hot. If we push the translate_to_base * affine * translate_back dance up to the caller it means they can reuse the same Affine for multiple calls to transform() and there's less code to get in the way of inlining or auto-vectorisation.

We don't need the base parameter, it's just convenient.

Removing it for better performance is something I can get behind, although I would -at some point- implement some convenience wrappers, as in my experience, composing low-level transforms can be quite error-prone for users.

But de-coupling this from the most basic implementation seems right to me.

DerLando · 2020-01-23T17:40:11Z

I ran all builds again with stable and nightly to double check but it all just works on my machine ©.
Dou you have an idea why travis might fail on nightly while succeeding on stable?

Michael-F-Bryan · 2020-01-24T00:16:52Z

Looks like it's timing out while trying to download and extract the cache, and has nothing to do with your PR per-se.

I'm going to blow away the caches and restart the build for now, if it comes up again I'll find a proper solution.

DerLando added 3 commits January 16, 2020 17:07

Implements non uniform scaling via trait for Vector with test

2c0057f

Implements non uniform scaling via trait for Point

fb2d98f

Implements non uniform scaling via trait for Line with test

232fbb1

Michael-F-Bryan reviewed Jan 16, 2020

View reviewed changes

DerLando added 4 commits January 21, 2020 18:54

Refactores AffineTransformable according to strategies discussed here: …

b8320f2

…Michael-F-Bryan#6 (comment) .

Moves test for scaling around an arbitrary base for Vector into its o…

5b8b4d1

…wn test

Changes variables inside of scale_vector test to be more obviously re…

e2ffa6e

…adable

Splits test for Line primitive scaling into scale and scale_around_base

0b5306e

DerLando commented Jan 22, 2020

View reviewed changes

Michael-F-Bryan requested changes Jan 23, 2020

View reviewed changes

DerLando added 2 commits January 23, 2020 17:25

Fixes accidental version change of kurbo in cargo.toml

199875a

Removes �ase requirement for AffineTransformable

675f23f

Michael-F-Bryan merged commit a9fd038 into Michael-F-Bryan:master Jan 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement non uniform scale #6

Implement non uniform scale #6

DerLando commented Jan 16, 2020

Michael-F-Bryan Jan 16, 2020

DerLando Jan 16, 2020

Michael-F-Bryan Jan 17, 2020

DerLando Jan 17, 2020

Michael-F-Bryan Jan 16, 2020

Michael-F-Bryan Jan 16, 2020

Michael-F-Bryan Jan 16, 2020

Michael-F-Bryan Jan 16, 2020

DerLando Jan 16, 2020

Michael-F-Bryan Jan 17, 2020

DerLando Jan 17, 2020 •

edited

Loading

Michael-F-Bryan Jan 17, 2020 •

edited

Loading

DerLando Jan 20, 2020

DerLando commented Jan 22, 2020

DerLando Jan 22, 2020

Michael-F-Bryan left a comment

Michael-F-Bryan Jan 23, 2020

DerLando Jan 23, 2020

DerLando commented Jan 23, 2020

Michael-F-Bryan commented Jan 24, 2020


		assert_eq!(actual, expected);

		let base = Vector::new(2.0, 0.0);

Implement non uniform scale #6

Implement non uniform scale #6

Conversation

DerLando commented Jan 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DerLando Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

Michael-F-Bryan Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DerLando commented Jan 22, 2020

Choose a reason for hiding this comment

Michael-F-Bryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DerLando commented Jan 23, 2020

Michael-F-Bryan commented Jan 24, 2020

DerLando Jan 17, 2020 •

edited

Loading

Michael-F-Bryan Jan 17, 2020 •

edited

Loading