Associativity issue because w is not multiplied #157
Conversation
| fn approx_eq(&self, other: &Self) -> bool { | ||
| self.approx_eq_eps(&other, &Self::approx_epsilon()) | ||
| } | ||
| } |
|
☔ The latest upstream changes (presumably #149) made this pull request unmergeable. Please resolve the merge conflicts. |
peterjoel
force-pushed
the
associativity_w
branch
from
05d6263
to
df9487c
Compare
|
@nical What should be done here? Is it intentional that users can't treat a Matrix4D as a general-purpose 4x4 matrix? I feel like m14, m24 & m34 shouldn't be exposed if setting them to non-zero values can cause unexpected results. Likewise for m44 being set to a non-unit value. |
|
I am in favor of fixing the 4D multiplication the way you propose in your patch. |
|
Ok. I think it's worth mentioning that Firefox currently implements it without the multiplying |
|
Yes, I'll handle the gecko side, we should definitely have the same behavior to avoid interop headaches. |
|
☔ The latest upstream changes (presumably #163) made this pull request unmergeable. Please resolve the merge conflicts. |
|
We should merge this. The gecko equivalent has landed in mozilla-central, by the way. |
|
@peterjoel needs to rebase it. |
peterjoel
force-pushed
the
associativity_w
branch
from
df9487c
to
2968443
Compare
|
@bors-servo delegate=nical |
|
✌️ @nical can now approve this pull request |
|
@bors-servo r+ |
|
📌 Commit 2968443 has been approved by |
Associativity issue because w is not multiplied Transforming a point by two matrices in sequence should be the same as premultiplying the matrices and performing one transformation. The issue appears to be because the `w` field of the point is assumed to be always 1. <!-- 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/157) <!-- Reviewable:end -->
|
☀️ Test successful - status-travis |
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 -->
Transforming a point by two matrices in sequence should be the same as premultiplying the matrices and performing one transformation.
The issue appears to be because the
wfield of the point is assumed to be always 1.This change is