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

Implement Add/Sub of Translation* for Point* #507

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ids1024
Copy link
Contributor

@ids1024 ids1024 commented Sep 25, 2023

Like #506, these exist for the vector types. There doesn't seem to be any reason there shouldn't also work for translation types.

The existing translate methods could be used for subtraction with inv, but that requires a complicated bound involving Inv and Add instead of a Sub bound.

Like servo#506, these exist for the
vector types. There doesn't seem to be any reason there shouldn't also
work for translation types.

The existing `translate` methods could be used for subtraction with
`inv`, but that requires a complicated bound involving `Inv` and `Add`
instead of a `Sub` bound.
Copy link
Contributor

@nical nical left a comment

Choose a reason for hiding this comment

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

Why not. If you are adding these then please also implement the equivalent Add/Sub Rect and Box2D

@@ -220,6 +220,15 @@ impl<T: Copy, Src, Dst> Translation2D<T, Src, Dst> {
point2(p.x + self.x, p.y + self.y)
}

/// Translate a point by inverse and cast its unit.
#[inline]
pub(crate) fn transform_point_inv(&self, p: Point2D<T, Dst>) -> Point2D<T::Output, Src>
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not add _inv methods like this. You could relax the requirements on inverse or simply do the math manually in sub.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These functions are pub(crate) since I wasn't sure they made sense in the public API or what to name them. Having them alongside the transformations in the opposite direction make it easier to see that they're correct, but the body of the function could definitely be moved directly into the Sub implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

but the body of the function could definitely be moved directly into the Sub implementation.

This would be my preference.

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.

2 participants