servo / euclid

Geometry primitives (basic linear algebra) for Rust
Other
458 stars 102 forks source link

Add Measure and Measure2D traits #383

Closed pbor closed 4 years ago

pbor commented 4 years ago

To get the width of a Rect, today you need to do rect.size.width. To get the width of a Box2D, today you need to do rect.size().width.

Both are not very convenient (I miss having a simple rect.width() and apparently I am not the only one, see https://github.com/servo/euclid/issues/282) and they are not consistent.

That got me thinking that we could have a trait like

trait Measure2D<T> {
    fn width(&self) -> T;
    fn height(&self) -> T;
}

one we have that we can also have a default implementation of area()

trait Measure2D<T> {
    fn width(&self) -> T;
    fn height(&self) -> T;
    fn area(&self) -> T {
         self.width() * self.height()
    }
}

However I am now wondering if the width() and height() method should return Length<T, U> instead of a plain T... But if we do that, what would area() return?

Additionally, we could also have a Measure trait for uni-dimensional things... once again we would have to think of what to return: Length would make sense for Vector but less so for Angle etc

nical commented 4 years ago

I'm not super fond of adding traits unless there's a need for abstraction and/or genericity, but adding fn width(&self) -> T and fn height(&self) -> T to rectangle and box types sounds good to me.

As far as I know, adding typed units has paid off for vector, matrix types etc, but Length isn't used much in the wild, so I would go for using T by default in parameter and return values of common functions since this is what people will reach for the most.

We can also add an extra set of functions that return lengths if we can come up with a good naming scheme that is both consistent and not cumbersome. We used to have *_typed() -> Lenth<T, U> variants of a lot of methods like vector.x_typed() which weren't used so they were removed. Since that's a bit of a bikeshedy topic I propose having the Length discussion separately if you want to add width() and height() (bonus point if you add depth() to Box3D!).

pbor commented 4 years ago

Sounds good. I will send out a PR that adds the simple methods directly to the objects.

Just out of curiosity, is there any performance or ergonomics drawbacks in having this methods on a shared trait? I am still new to rust and the idea of using traits to enforce a consistent interface among objects sounded appealing, so if more experienced developers prefer to avoid them unless needed, I would like to learn the best practices :)

nical commented 4 years ago

For me it's mostly ergonomics in the sense that:

Now that's just my opinion and you'll find that some people like to use extension traits such as the one you proposed.

About traits and abstractions in general, my humble opinion is that as programmers, we tend to enjoy organizing code in abstractions because it feels right, regardless of whether or not these abstractions are useful in practice (sometimes they are!). Abstractions add a bit of weight to your code, in the sense that they are extra code that you need to maintain, are extra API surface that you expose (and thus try not to change too much), and an extra source of friction when you need to make changes to your code.

That's why in general my stance is to only add abstractions when they are needed or when I have evidence that they provide substantial value (sometimes they do of course).

I don't expect trait methods to incur a performance penalty if the method is called via static dispatch (the compiler knows the type of the object).

pbor commented 4 years ago

Thanks for the feedback!

The fact that you have to import the trait is indeed annoying... Documentaion seems less of a problem in the long term: given how much these are used in rust, we should probably just fix the docs viewer to make extensions easily discoverable.

Anyway, closing since you merged the PR (thanks for the super quick review!)