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

Documentation of Transform2D does not reflect the actual calculation #523

Closed
paxbun opened this issue Apr 4, 2024 · 1 comment · Fixed by #524
Closed

Documentation of Transform2D does not reflect the actual calculation #523

paxbun opened this issue Apr 4, 2024 · 1 comment · Fixed by #524

Comments

@paxbun
Copy link

paxbun commented Apr 4, 2024

In the doc-comment of Transform2D, there is a matrix formula that describes the transformation performed by Transform2D:

/// ```text
/// | m11 m12 0 | |x| |x'|
/// | m21 m22 0 | x |y| = |y'|
/// | m31 m32 1 | |1| |w |
/// ```

and this does not reflect the actual calculation, because Transform2D does not transform Point3D or Vector3D, so the w component in the result is always 1.

Furthermore, all matrices are transposed in the Transform2D or Transform3D documentation. Although it is mentioned that Transform2D or Transform3D use column-major order, I think putting transposed matrices without the transpose symbol is counter-intuitive for readers.

At least the following code must be represented as below.

euclid/src/transform2d.rs

Lines 546 to 554 in 6b2aece

/// Returns the given point transformed by this transform.
#[inline]
#[must_use]
pub fn transform_point(&self, point: Point2D<T, Src>) -> Point2D<T, Dst> {
Point2D::new(
point.x * self.m11 + point.y * self.m21 + self.m31,
point.x * self.m12 + point.y * self.m22 + self.m32,
)
}

 | m11 m12 0 |T   |x|   |x'|
 | m21 m22 0 |  x |y| = |y'|
 | m31 m32 1 |    |1|   |1 |
@nical
Copy link
Contributor

nical commented Apr 8, 2024

Good points. I think that the doc should read:

| m11 m21 m31 |   |x|   |x'|
| m12 m22 m32 | x |y| = |y'|
|   0   0   1 |   |1|   |1 |

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 a pull request may close this issue.

2 participants