Skip to content
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

Separate Point and Vector types #159

Merged
merged 1 commit into from
May 26, 2017
Merged

Conversation

nical
Copy link
Contributor

@nical nical commented Aug 19, 2016

This changeset implements my proposal from issue #151:

  • TypedVector2D and TypedVector3D types are added (no 4D vector)
  • Matrix types implement separate transformations for points and vectors
  • the 4D vector is removed, and along with it the bug-prone behavior of ignoring its w component half of the time.
  • Point arithmetic is changed so that:
    • Point + Point is not implemented
    • Point + Vector = Point
    • Point - Point = Vector

With points and vectors as separate types, we don't need the 4D types anymore because the homogeneous coordinate is part type itself: for vectors it is always zero, and for points it is always one. Matrix transformations always divide the resulting points by w. It avoids the confusion that motivated the PR #157, where w was ignored in half of euclid because the code expected it to be equal to one.


This change is Reviewable

@nical
Copy link
Contributor Author

nical commented Aug 19, 2016

Once again, this is a pretty invasive breaking change. I think that it is worth it, as I have plenty of bug stories about confusing positions and offsets in space, but we should make sure the interested parties all agree about this before landing anything.

I am pretty tempted to rename Matrix4D into Matrix3D at the same time. It makes more sense since it expresses 3D transformations, just like Matrix2D expresses 2D transformation and is not a 2x2 matrix.
As the matrix name is purely a cosmetic change I'm hesitant to mix it with the potential point vs vector bikeshed.

Also, about the added square_norm and norm methods: I am personally more used to "length", but I anticipated that the naming would collide with length::Length. If you prefer length, I'll be happy to change the name.

cc @nox @glennw @pcwalton @SimonSapin

@nical
Copy link
Contributor Author

nical commented Aug 19, 2016

A motivating example of where this could be useful is implementing SVG. The latter is full of relative and absolute coordinates, see this SVG paths building interface for example. In this library I have Point and Vec2 aliases which are the same underlying euclid Point2D. Having the the aliases helped clarify the API a bit, but didn't prevent me from forgetting to do the right thing when working with relative coordinates in a lot of places.

@peterjoel
Copy link

Renaming Matrix4D toMatrix3D seems like it would be confusing. But I see the benefit of preventing users from setting m14, m24, m34, m44 to anything except 0, 0, 0, 1.

Is there a better name, along the lines of AffineTransform3D or similar?

@nox
Copy link
Contributor

nox commented Aug 19, 2016

Renaming Matrix4D toMatrix3D seems like it would be confusing.

How is it more confusing than Matrix2D not being a 2x2 matrix?

@nical
Copy link
Contributor Author

nical commented Aug 19, 2016

I didn't mean to change the structure of Matrix4D (no change to m14 m24 m34 and m44), just the name to reflect that it is a matrix for 3D transformations. Also, my PR removes the Point4D type so Matrix4D ends up the only thing labelled as 4D.
For reference, gecko uses the name Matrix4x4 which is explicit about the layout of the matrix, but does not make it sound like it is meant for 4D transformations (whatever that means in our context).
To me it is confusing that the "4D" of Matrix4D here really means "4 by 4" rather than 4-dimensions, while the matrix is in fact used for 3D transformations.

AffineTransform3D is very accurate name but it's a bit of a mouthful and not a name I would think about searching for if I discovered the crate. Tranform3D would be fine, though.

@nical
Copy link
Contributor Author

nical commented Aug 19, 2016

Since there isn't a clear consensus on matrix names, we should probably continue this part of the discussion in a separate issue.

@peterjoel
Copy link

How is it more confusing than Matrix2D not being a 2x2 matrix?

Fair enough. I hadn't used the 2D objects and didn't notice!

@nical
Copy link
Contributor Author

nical commented Aug 19, 2016

cc @mrobinson

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #165) made this pull request unmergeable. Please resolve the merge conflicts.

@brendanzab
Copy link
Sponsor Contributor

This is exactly what cgmath and nalgebra do. 👍

@nical
Copy link
Contributor Author

nical commented Nov 28, 2016

I'd like to revive this. The PR has been rebased, athough I took out the part about removing Point4D to reduce the amount of churn (I still think we should remove it, but we can talk about that separately).

r? @glennw

@@ -72,7 +72,10 @@ pub use matrix4d::{Matrix4D, TypedMatrix4D};
pub use point::{
Point2D, TypedPoint2D,
Point3D, TypedPoint3D,
Point4D, TypedPoint4D,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You removed the reexport and I don't think that was intended. Was it?

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #170) made this pull request unmergeable. Please resolve the merge conflicts.

@kvark
Copy link
Member

kvark commented Feb 9, 2017

Let me express a different point of view here.

On matrices

How is it more confusing than Matrix2D not being a 2x2 matrix?

Exactly, let's fix this. Instead of renaming Matrix4D to Matrix3D, we could rename Matrix2D to Matrix3D and possibly add a separate Matrix2D class that is a real 2x2 matrix.

The problem here is semantics. Matrix in general is just doing a linear transformation. Matrix by itself doesn't transform points. You can say that it can produce an affine transformation of a point by linearly transforming a homogeneous vector form of that point, but that's thinking on a higher level.

So when naming things, we should be clear about what semantic level is the name in. If we want to operate on the level of the basic algebra, then 4x4 matrix should be called Matrix4D and used for linear transformations of homogeneous vectors. If we operate on the level of 2D/3D transformations, then the 4x4 matrix should be called Transform3D (see cgmath version).

Euclid:
A collection of strongly typed math tools for computer graphics with an inclination towards 2d graphics and layout.

That sounds more like a transform domain (the latter choice with Transform3D) than the algebra one.

On vectors and points

Consequently, the choice of having Vector/Point or just Point can be derived from the chosen domain.

If all you have is Matrix4D, then the distinction between point/vector can be made the user responsibility, and Point3 could just have methods like:

  fn to_point_homogeneous(&self) -> Point4<T> {..}
  fn to_vector_homogeneous(&self) -> Point4<T> {..}  

If you have Transform2D, Transform3D and such, then it totally makes sense to have separate types for Point/Vector, since they are going to be transformed differently.

@nical
Copy link
Contributor Author

nical commented Feb 10, 2017

Euclid tries to be a library to represent and transform geometry. The matrix types happen to be matrices but we really reason about transforms (preferring terminology like transform_point over multiply_point, encoding the unit/space in the types, etc.). Servo and gecko only need to reason about points, vectors and transforms and any higher level mathematical abstraction on top of it is unnecessary complexity for our use case. We don't even need the notion of a 4D point/vector and historically we've handled them poorly (like plainly ignoring the homogeneous coordinate).
Note that the matrix naming discussion moved to issue #160 (where Transform3D/2D seems to be the preferred option).
We don't have a use case for a 2x2 matrix type as far as I know, so I don't think we should add one until the need arises.

I would very much like to revive this (vector/point separation and naming) by the way. I'll happily rebase my PRs if we are willing to land them.

@kvark kvark mentioned this pull request Feb 10, 2017
@kvark
Copy link
Member

kvark commented Feb 10, 2017

@nical Ok, good, looks like we are in agreement then.

@nical
Copy link
Contributor Author

nical commented May 19, 2017

I am reviving this. I ended up re-doing the change so it is still a work in progress although it is almost complete.

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #194) made this pull request unmergeable. Please resolve the merge conflicts.

@nical
Copy link
Contributor Author

nical commented May 21, 2017

I have re-written this PR which had bit-rotted beyond repair.

@kvark and @nox, whoever gets to review it first or both of you if you prefer (considering the size of the change), this PR is a good state now.

@kvark
Copy link
Member

kvark commented May 24, 2017

The more I look at it, the more I become obsessed with an idea that euclid could just be a shim around cgmath with a few typedefs (and maybe Size?) defined. That would reduce the library fragmentation and benefit the community.


Reviewed 7 of 7 files at r2.
Review status: all files reviewed at latest revision, 23 unresolved discussions.


src/point.rs, line 121 at r2 (raw file):

impl<T: Copy + Add<T, Output=T>, U> Add<TypedSize2D<T, U>> for TypedPoint2D<T, U> {
    type Output = TypedPoint2D<T, U>;

type Output = Self;?


src/point.rs, line 386 at r2 (raw file):

    #[inline]
    pub fn to_vector(&self) -> TypedVector3D<T, U> {
        TypedVector3D::new(self.x, self.y, self.y)

vec3?


src/point.rs, line 438 at r2 (raw file):

    // Cross product.
    #[inline]
    pub fn cross(self, other: TypedPoint3D<T, U>) -> Self {

I don't think we should have a cross product between points


src/point.rs, line 440 at r2 (raw file):

    pub fn cross(self, other: TypedPoint3D<T, U>) -> Self {
        point3(self.y * other.z - self.z * other.y,
                          self.z * other.x - self.x * other.z,

indentation is off


src/rect.rs, line 162 at r2 (raw file):

    pub fn translate(&self, by: &TypedVector2D<T, U>) -> TypedRect<T, U> {
        TypedRect::new(
            TypedPoint2D::new(self.origin.x + by.x, self.origin.y + by.y),

self.origin + by ?


src/transform3d.rs, line 391 at r2 (raw file):

    /// The input point must be use the unit Src, and the returned point has the unit Dst.
    #[inline]
    pub fn transform_point2d(&self, p: &TypedPoint2D<T, Src>) -> TypedPoint2D<T, Dst> {

I don't think handling 2D points/vectors by a 3D transformation is useful:

  • same can be achieved by casting to_3d(), then going back to 2d
  • ignoring the post-transform Z is obscure tactic, can lead to logical failures. E.g. someone takes a Point2D, transforms, then transforms it back, and (surprise!) getting a different point. I'd rather leave that to the user.

src/transform3d.rs, line 415 at r2 (raw file):

    /// The input point must be use the unit Src, and the returned point has the unit Dst.
    #[inline]
    pub fn transform_point3d(&self, p: &TypedPoint3D<T, Src>) -> TypedPoint3D<T, Dst> {

correspondingly, this would just be called transform_point


src/transform3d.rs, line 438 at r2 (raw file):

    /// Returns a rectangle that encompasses the result of transforming the given rectangle by this
    /// transform.
    pub fn transform_rect(&self, rect: &TypedRect<T, Src>) -> TypedRect<T, Dst> {

this one got me into trouble once. Perhaps, rename it to transform_rect_lossy?


src/vector.rs, line 1 at r2 (raw file):

// Copyright 2013 The Servo Project Developers. See the COPYRIGHT

let's jump out of the past


src/vector.rs, line 16 at r2 (raw file):

use size::{TypedSize2D, size2};
use scale_factor::ScaleFactor;
use num::*;

I don't think this is idiomatic


src/vector.rs, line 38 at r2 (raw file):

    /// Constructor, setting all components to zero.
    #[inline]
    pub fn zero() -> TypedVector2D<T, U> {

let's use Self more


src/vector.rs, line 39 at r2 (raw file):

    #[inline]
    pub fn zero() -> TypedVector2D<T, U> {
        TypedVector2D::new(Zero::zero(), Zero::zero())

and vec2


src/vector.rs, line 51 at r2 (raw file):

impl<T: fmt::Debug, U> fmt::Debug for TypedVector2D<T, U> {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(f, "({:?},{:?})", self.x, self.y)

Should we put the type name in here as well? As in, Vector(x,y) , Size(w,h), and Point(x,y)


src/vector.rs, line 78 at r2 (raw file):

    /// Equivalent to adding this vector to the origin.
    #[inline]
    pub fn to_point(&self) -> TypedPoint2D<T, U> {

I'd propose to build the type hierarchy bottom-up. Thus, vectors are self-sufficient and don't depend on anything. Points and sizes depend on them and define conversions to/from vectors from within.


src/vector.rs, line 100 at r2 (raw file):

    #[inline]
    pub fn to_untyped(&self) -> Vector2D<T> {
        TypedVector2D::new(self.x, self.y)

vec2 ?


src/vector.rs, line 119 at r2 (raw file):

    /// Dot product.
    #[inline]
    pub fn dot(self, other: TypedVector2D<T, U>) -> T {

Self


src/vector.rs, line 123 at r2 (raw file):

    }

    /// Returns the norm of the cross product [self.x, self.y, 0] x [other.x, other.y, 0]..

Maybe "magnitude" instead of "norm" here?


src/vector.rs, line 140 at r2 (raw file):

    #[inline]
    pub fn square_norm(&self) -> T {

Similarly, I don't recall seeing "norm" used for the length. You can call it "magnitude" (or "mag") or "length", but I don't see "norm" to be appropriate


src/vector.rs, line 174 at r2 (raw file):

    type Output = TypedVector2D<T, U>;
    #[inline]
    fn add(self, other: TypedSize2D<T, U>) -> TypedVector2D<T, U> {

is there a use-case for it? It's not obvious to me that we need it


src/vector.rs, line 181 at r2 (raw file):

impl<T: Copy + Add<T, Output=T>, U> TypedVector2D<T, U> {
    #[inline]
    pub fn add_size(&self, other: &TypedSize2D<T, U>) -> TypedVector2D<T, U> {

why do we need this if we have Add<TypedSize<..>> implemented?


src/vector.rs, line 266 at r2 (raw file):

    /// For example `{ -0.1, -0.8 }.round() == { 0.0, -1.0 }`.
    #[inline]
    pub fn round(&self) -> Self {

Similarly, I'm curious about where do we ever need to round/ceil/floor a vector? It seems to me that these can be limited to points (and maybe sizes?) only


src/vector.rs, line 474 at r2 (raw file):

    pub fn cross(self, other: TypedVector3D<T, U>) -> TypedVector3D<T, U> {
        vec3(self.y * other.z - self.z * other.y,
                          self.z * other.x - self.x * other.z,

indentation


src/vector.rs, line 489 at r2 (raw file):

    #[inline]
    pub fn square_norm(&self) -> T {

same concern about the "norm" here


Comments from Reviewable

@nical
Copy link
Contributor Author

nical commented May 25, 2017

(about the formatting of points and vectors
Should we put the type name in here as well? As in, Vector(x,y) , Size(w,h), and Point(x,y)

I would rather not, because I often print out large buffers of points and vectors and the extra letters make it harder to read (when there's many of them).

The word magnitude rubs me the wrong way (even if it means the same thing). If you prefer we can call it length and square_length it's less of pure mathematical term but it is a more consensual and accessible word.

I have my own use cases for floor/ceil/round in vectors, when I do some of the computation with integers instead of floats. Might not be used by servo but i'd like to keep them.

On the topic of using cgmath, nalgebra or some other established math library, none of them seem interested in treating vector spaces as a first class citizen (the U phantom type in euclid) this feature is very important for us and it alone justifies having a separate library. Anyway, I don't want to revive this topic in this PR, let's talk about it in a separate issue or on irc if you are interested.

@kvark
Copy link
Member

kvark commented May 25, 2017

I would rather not, because I often print out large buffers of points and vectors and the extra letters make it harder to read (when there's many of them).

I see your point. You may use {:#?}, or do other means of debugging large buffers of data. Erasing type information from Debug feels wrong to me.

it's less of pure mathematical term but it is a more consensual and accessible word

Out of curiosity, is norm used anywhere for vector length, other than maybe in functional analysis?
length also sounds better to me than magnitude.

I have my own use cases for floor/ceil/round in vectors, when I do some of the computation with integers instead of floats. Might not be used by servo but i'd like to keep them.

Could you share them? Just to get a bigger picture of the use case space.

Anyway, I don't want to revive this topic in this PR, let's talk about it in a separate issue or on irc if you are interested.

Agreed, moved to #197

@nical
Copy link
Contributor Author

nical commented May 25, 2017

Out of curiosity, is norm used anywhere for vector length, other than maybe in functional analysis?
length also sounds better to me than magnitude.

Norm and magnitude are synonyms. Depending on who your math teacher was you may have run into one more than the other in english (perhaps there are some biases depending on which side of the ocean you are on, I do remember hearing norm in computational geometry courses given in english I took in Denmark and in France). Part of my own bias may come from that no french person will have heard about magnitude while norme is the french word you'll use if you are pedantic about the mathematical lingua. Let's go for length, that's also the one I prefer anyway.

Could you share them? Just to get a bigger picture of the use case space.

Some algorithms rely on being able to reason about precision, and it is very hard to do with floating point numbers. Working with integers (or fixed-point numbers) gives you something where the precision loss is mostly constant or at least something you can reason about (at the cost of having to be very careful with overflows). Lyon's tessellator is an example of this (I don't want to flood this PR with the details but I could go on about this for ever, I'll tell you all about it over a beer in SF if you are interested), I know that working with integer spaces is also very popular for the same reasons when doing things like nav-meshes and lots of geometry processing algorithms in general. When going from floating point to the "fixed precision" space of the algorithm, it's useful to be able to explicitly choose the way you want to go about snapping quantities (be it vectors or points) to the integer space.

@kvark
Copy link
Member

kvark commented May 25, 2017

Norm and magnitude are synonyms.

I just took another stab at googling it and found the confirmation:

A simple example is two dimensional Euclidean space R2 equipped with the Euclidean norm. Elements in this vector space (e.g., (3, 7)) are usually drawn as arrows in a 2-dimensional cartesian coordinate system starting at the origin (0, 0). The Euclidean norm assigns to each vector the length of its arrow. Because of this, the Euclidean norm is often known as the magnitude.

TL;DR: norm() is generally an abstract function in specific space having specific properties. In Euclidean space though, it's clearly defined as what we know as length/magnitude. And since the library happens to be called euclid, it would make sense to have the norm.

This is convincing, I'm fine with either norm/length/magnitude now.

Working with integers (or fixed-point numbers) gives you something where the precision loss is mostly constant or at least something you can reason about

Right, I recall that you've been doing lots of integer work in lyon. My concern with vector rounding is that a lot of operations in vector space are not transitive with regards to rounding, e.g. ceil(a+b) != ceil(a) + ceil(b). Thus, defining it on points, which are more restricted, sounds a little better to me. But I won't lose my sleep over it, I'm fine with having the rounding defined for vectors too, if that's useful to you.

@nox would you want to take a look at the PR as well?

@peterjoel
Copy link

peterjoel commented May 25, 2017 via email

@nical
Copy link
Contributor Author

nical commented May 26, 2017

I don't think being consistent with the terminology used for the size of containers brings any value, and I passionately dislike abbreviating 6 letter words. Plus, maths are full of acronyms, so we I'd rather let names that are not english words be only for those.

@nical
Copy link
Contributor Author

nical commented May 26, 2017

@bors-servo r=@kvark

@bors-servo
Copy link
Contributor

📌 Commit 614eeca has been approved by @kvark

@bors-servo
Copy link
Contributor

🔒 Merge conflict

@nical
Copy link
Contributor Author

nical commented May 26, 2017

Rebased, @bors-servo r=@kvark

@bors-servo
Copy link
Contributor

📌 Commit f33d6e2 has been approved by @kvark

@bors-servo
Copy link
Contributor

⌛ Testing commit f33d6e2 with merge 08ab185...

bors-servo pushed a commit that referenced this pull request May 26, 2017
Separate Point and Vector types

This changeset implements my proposal from issue #151:
- TypedVector2D and TypedVector3D types are added (no 4D vector)
- Matrix types implement separate transformations for points and vectors
- the 4D vector is removed, and along with it the bug-prone behavior of ignoring its w component half of the time.
- Point arithmetic is changed so that:
  - Point + Point is not implemented
  - Point + Vector = Point
  - Point - Point = Vector

With points and vectors as separate types, we don't need the 4D types anymore because the homogeneous coordinate is part type itself: for vectors it is always zero, and for points it is always one. Matrix transformations always divide the resulting points by w. It avoids the confusion that motivated the PR #157, where w was ignored in half of euclid because the code expected it to be equal to one.

<!-- Reviewable:start -->

---

This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/euclid/159)

<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - status-travis
Approved by: @kvark
Pushing 08ab185 to master...

@bors-servo bors-servo merged commit f33d6e2 into servo:master May 26, 2017
@nical nical mentioned this pull request Jun 6, 2017
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants