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

angle: provide conversion trait from Vector2D #382

Closed
wants to merge 1 commit into from
Closed

Conversation

@pbor
Copy link
Contributor

pbor commented Nov 25, 2019

No description provided.

T: Trig,
{
fn from(vec: Vector2D<T, U>) -> Self {
Angle::radians(Trig::fast_atan2(vec.y, vec.x))

This comment has been minimized.

@nical

nical Nov 25, 2019

Collaborator

Note that there is Vector2D::angle_from_x_axis which does this.

@kvark made a good case against implicit things in euclid in general. Let's hear what he has to say.

This comment has been minimized.

@kvark

kvark Nov 25, 2019

Member

If you have this From implementation, you are getting two things:

  1. Angle::from(), which is fairly straightforward, and looks good
  2. Vector2D::into, which is by now way is straightforward. When looking at xx.into() passed into a function, you have no idea what it's converted into.

It's similar, in a way, to bool arguments: calling site is useless and forces you to look at the function definition.

Saying that, I think it's cleaner to just have a specific constructor aka Angle::from_vector()

This comment has been minimized.

@pbor

pbor Nov 25, 2019

Author Contributor

In general I agree that using .into() can be confusing, but at the same time I find using the From trait very ergonimic instead of hunting for the correct constructor... I guess it boils down to defining a best practice where Foo::from(bar) is preferred.

Anyway, if the convention in euclid is to avoid From that is ok with me.
Let me know if you want a PR with the constructor or if I should just stick to the Vector2D method at that point (which is ok with me)

This comment has been minimized.

@nical

nical Nov 26, 2019

Collaborator

Since there are mixed feelings and the overall sentiment is that it is fine to use the Vector2D method, I propose that we close the PR for now. We can always revisit in the future if people feel strongly about it.

This comment has been minimized.

@pbor

pbor Nov 26, 2019

Author Contributor

👍

@nical nical closed this Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.