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<Point>, AddAssign<Point>, and SubAssign<Point> for Point. #185

Open
JAicewizard opened this issue Jul 4, 2021 · 4 comments
Labels
enhancement New feature or request needs discussion This change requires some more discussion before we decide we definitely want it

Comments

@JAicewizard
Copy link
Contributor

This is really inconsistent ATM. Only Sub<Point> exists, not any of the other.

@raphlinus
Copy link
Contributor

This is intentional, as Point+Point has no real meaning in a strict mathematical sense, while Point-Point is a vector. That said, we're getting consistent feedback that all this confusing - #182 is a similar thread, and I have comments on that as well.

What I propose, and am leaning toward, is relaxing the mathematical correctness and making Point and Vec2 behave generally quite similarly; the distinction between the two is largely documentation. That would make, for example, 0.5 * (p1 + p2) valid without having to interconvert with Vec2 as now. There is some risk that it would let slip through some errors that would be caught at compile time, but I'm not seeing a lot of evidence these are real.

@JAicewizard
Copy link
Contributor Author

What I really wanted to do mathematically, is change the origin of a point from (0,0) to p. The Point type is kind of incompatible with the idea of changing origin, since the origin is also a constant (0,0). Something like translate or something like that, similarly for midpoint in your example.

I would be more inclined to keep the separation, like colin said in that issue, and just add usefull methods on points that are usefull and mathematically relevant. Like translate, scale, midpoint, angle, and length.

Angle is debatable, but I think that it should definitely be considered a property of a point in the polar coordinate system, as should length for the same reason.

@derekdreery
Copy link
Collaborator

derekdreery commented Mar 18, 2023

I agree with @JAicewizard here. Once you've got your head around the Point/Vec2 model, it is really nice and logical Perhaps there should be a doc note on Point to explain how it interacts with Vec2. For midpoint, there is lerp which you could use with the fixed parameter of 0.5, but perhaps it makes sense to add this as well, with a note pointing out that it as a specialization of lerp. For the standard affine transformations, I currently would create a Affine and then just do Affine * Point, but it might be that this has significantly worse performance than just applying the operation, especially in the translation case where the operation doesn't need the linear part at all (would need to experiment). As soon as you start composing transformations, though, I'm pretty sure the affine approach would win. Again, I haven't measured so might be totally wrong.

@derekdreery
Copy link
Collaborator

derekdreery commented Mar 18, 2023

So for me, definite actions are

  • add a midpoint function to Point that takes another point and returns point.lerp(other_point, 0.5)
  • add an alias for Vec2::hypot called length, either as an extra method or a doc alias. I'd probably favor adding another method (and a length_squared) because you wouldn't search for the name hypot if you didn't know the standard libm function names.
  • add an alias for Vec2::atan2 called angle, with the same spiel about doc alias or extra fn. Document as "the angle this vector makes with the vector pointing in the positive x direction, in the range (-pi, pi]".
  • consider adding some methods for common transformations, like translate(Vec2), scale(f64), scale_non_uniform(f64, f64), rotate(f64), rotate_about(f64, Point) and implement them directly rather than through Affine. I'd probably also use a Vec2 for scale_non_uniform, but since other functions take 2 arguments I think that ship has sailed.

What do people think?

@derekdreery derekdreery added enhancement New feature or request needs discussion This change requires some more discussion before we decide we definitely want it labels Mar 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs discussion This change requires some more discussion before we decide we definitely want it
Projects
None yet
Development

No branches or pull requests

3 participants