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

Improve Angle #641

Merged
merged 3 commits into from
May 27, 2022
Merged

Improve Angle #641

merged 3 commits into from
May 27, 2022

Conversation

T0mstone
Copy link
Contributor

This is a suggestion for a slight improvement to the Angle type.

  1. It replaces the usage of 2. * PI with TAU: This constant exists, so why not use it?
  2. It adds two new units that an angle can be converted to/from. I think gon is used in engineering, so it could prove useful; Revolutions is just good to have in any case (say you want to rotate something 1/3 of a turn, now it's super easy)
  3. It removes the multiplication of two angles. Multiplying two angles is mathematically only useful if talking about solid angles, which this definetely wasn't. I don't see a context in which you would multiply an angle by another angle. Rather, you'll probably want to multiply an angle by a simple factor (something like "2 times that angle" or "1/3 of that angle"), so I implemented that. Dividing an angle by an angle is well-defined, so I kept it.

I don't think the changed API was used anywhere yet, but I'm not familiar with the codebase. This just jumped out at me when reading the PR that introduces angles, so I thought to just try a PR of my own.

* Add two new units
* Replace usage of `2 * PI` by `TAU`
* Replace angle-angle mul./div. with angle-scalar mul./div.
@T0mstone T0mstone requested a review from hannobraun as a code owner May 26, 2022 12:24
@T0mstone T0mstone changed the title Improve angle Improve Angle May 26, 2022
Copy link
Owner

@hannobraun hannobraun left a comment

Choose a reason for hiding this comment

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

Thank you for the pull request, @T0mstone, these are some great changes!

@hannobraun hannobraun merged commit c85c096 into hannobraun:main May 27, 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