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

first steps to removal of Point #506

Closed
wants to merge 1 commit into from
Closed

first steps to removal of Point #506

wants to merge 1 commit into from

Conversation

InnocentusLime
Copy link

@InnocentusLime InnocentusLime commented Jun 6, 2020

I did the first steps to remove the Point (solving issue #502)
As result all the PointN structs were removed (and the point.rs file was removed too) and replaced with VectorN. VectorN had EucledeanSpace implemented too. Some associated types and methods were removed for simplification

  1. Diff, Scalar in EuclideanSpace
  2. dot, from_vec and to_vec for EucledeanSpace
  3. to_homogenous and from_homogenous were added to Vector3 for convenienve

The library seems to compile in my case. Most likely the change is too radical and depreacating should be considered instead

I wasn't sure about some methods in Transform, so I left them intact

@InnocentusLime InnocentusLime changed the title first steps to removal of first steps to removal of Point Jun 6, 2020
Copy link
Contributor

@nstoddard nstoddard left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I still think we should postpone this until 0.19 and maybe mark Point etc as deprecated in 0.18. Unfortunately this will require some significant changes to pretty much every dependent crate, so I'm not sure whether it's feasible to remove Point entirely.

If we're going to simplify the API like this, we might also consider removing some of the traits like VectorSpace and EuclideanSpace, and moving the methods on those traits into the types that implement them. I'm not sure how many breaking changes are acceptable, though.

@@ -426,6 +433,14 @@ impl<S: BaseNum> Vector2<S> {
}

impl<S: BaseNum> Vector3<S> {
#[inline]
pub fn to_homogeneous(self) -> Vector4<S> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Points and vectors have different representations in homogeneous coordinates (vectors have a w coordinate of 0 so they're not affected by translations, points have a w coordinate other than 0 (often 1)), so these methods should probably be called point_to_homogeneous etc or something similar, or made private.

}
#[inline]
pub fn from_homogeneous(v : Vector4<S>) -> Vector3<S> {
v.truncate()
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to divide by the w coordinate.

@InnocentusLime InnocentusLime closed this by deleting the head repository Dec 4, 2022
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