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

Add some useful methods to Angle. #247

Merged
merged 3 commits into from Dec 12, 2017
Merged

Add some useful methods to Angle. #247

merged 3 commits into from Dec 12, 2017

Conversation

@nical
Copy link
Collaborator

nical commented Dec 8, 2017

Part of the motivation for a dedicated angle type was to have this type of goodies.

  • Angle::positive returns the angle in the [0 .. 2*PI[ range
  • Angle::signed returns the angle in the ]-PI .. PI] range
  • a bunch of constructors with common constants (zero, pi, etc.)
  • sin_cos

This change is Reviewable

@nical nical force-pushed the nical:angle-wrap branch from 412409e to 329c9c7 Dec 8, 2017
@kvark
kvark approved these changes Dec 12, 2017

pub fn two_pi() -> Self { Angle::radians(T::PI() + T::PI()) }

pub fn frac_pi_2() -> Self { Angle::radians(T::FRAC_PI_2()) }

This comment has been minimized.

@kvark

kvark Dec 12, 2017

Member

Does the Angle have a semantic of radians? I thought of it as an abstracted opaque angle, in which case having pi in the name here is leaking that abstraction.

This comment has been minimized.

@nical

nical Dec 12, 2017

Author Collaborator

Angle exposes its internals as radians, I didn't try to abstract out the unit as much as just make the type more convenient (and explicit), while keeping some helpers to deal with degrees without too much hassle. Kinda like Transform3D being a matrix, Rotation3D a quaternion, etc.

I think that it's best if radians to are the first class citizen and I would rather be transparent about it than build an abstraction. But it doesn't go against picking names that are more "units agnostic" when convenient. Not sure what those could be for these constants, though. Do you have ideas?

@kvark
Copy link
Member

kvark commented Dec 12, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Dec 12, 2017

📌 Commit 68cc2be has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented Dec 12, 2017

Testing commit 68cc2be with merge ea042ad...

bors-servo added a commit that referenced this pull request Dec 12, 2017
Add some useful methods to Angle.

Part of the motivation for a dedicated angle type was to have this type of goodies.
 - `Angle::positive` returns the angle in the `[0 .. 2*PI[` range
 - `Angle::signed` returns the angle in the `]-PI .. PI]` range
 - a bunch of constructors with common constants (zero, pi, etc.)
 - sin_cos

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/euclid/247)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Dec 12, 2017

☀️ Test successful - status-travis
Approved by: kvark
Pushing ea042ad to master...

@bors-servo bors-servo merged commit 68cc2be into servo:master Dec 12, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@nical nical deleted the nical:angle-wrap branch Dec 13, 2017
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.