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

3D utility functions #107

Merged
merged 12 commits into from
Sep 13, 2013
Merged

3D utility functions #107

merged 12 commits into from
Sep 13, 2013

Conversation

bergey
Copy link
Member

@bergey bergey commented Sep 12, 2013

Some basic conveniences for working in 3D.

  • unit vectors
  • a few angle functions
  • rotation transforms

I'd like feedback on:

  • Is overlapping with 2D names a mistake? I think the parallels are mnemonic, even though we'll need to import parts of Diagrams qualified. But I'm open to persuasion.
  • The choice of rotation functions
  • For some reason I used unicode θ φ as variable names. Is this a terrible idea, or only a bad idea? I usually assume that people can read unicode but can only type ASCII. Should I fix it?

@bergey
Copy link
Member Author

bergey commented Sep 12, 2013

Having tried to work with this a bit more today, I think I'll add a Direction class, like the Angle class. That will make type signatures more useful Otherwise I think we'll end up with lots of comments saying that this or that R3 will be normalized. So spherical & fromSpherical will go away. Do you want to hold off merging until I have an implementation?

@byorgey
Copy link
Member

byorgey commented Sep 12, 2013

OK, sounds good.

@byorgey
Copy link
Member

byorgey commented Sep 12, 2013

Also,

  1. I think overlapping with 2D names is fine.
  2. Rotation functions look good to me.
  3. I fully endorse all uses of non-ASCII characters so long as users do not have to type them to use the library. We are finally living in the Age of Unicode, let's enjoy it.

@bergey
Copy link
Member Author

bergey commented Sep 12, 2013

Yes, I certainly did forget to update the documentation. Thank you!

@bergey
Copy link
Member Author

bergey commented Sep 13, 2013

I added the new Direction type class, which is specific to 3D. Polar is the only instance for now. I also stuck angleRatio in this branch, even though that commit doesn't touch any 3D files. I think this is good to go now.

--
-- The angle can be expressed using any type which is an
-- instance of 'Angle'. For example, @aboutZ (1\/4 ::
-- 'Turn')@, @aboutZ (tau\/4 :: 'Rad')@, and @rotate (90 ::
Copy link
Member

Choose a reason for hiding this comment

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

Should be aboutZ instead of rotate

fromPolar (Polar θ φ) = Polar (convertAngle θ) (convertAngle φ)

-- | A direction expressed as a pair of polar coordinates
data Angle a => Polar a = Polar a a
Copy link
Member

Choose a reason for hiding this comment

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

The semantics of Polar are not clear to me. Is it supposed to represent spherical coordinate angles? In that case might Spherical be a better name? Though I guess "spherical coordinates" usually refers to two angles paired with a distance rather than just the bare angles. Or is it supposed to be more general somehow?

Copy link
Member Author

Choose a reason for hiding this comment

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

I meant spherical coordinates. Renaming it Spherical makes sense to me. I think the Direction instance makes it clear enough that it doesn't include the distance. I'll try to improve the docstring while I'm at it. I took out the old version because it said that the angles were normalized (to [0,2pi), [-pi/2,pi/2]) which isn't true.

byorgey added a commit that referenced this pull request Sep 13, 2013
@byorgey byorgey merged commit 38003ae into master Sep 13, 2013
@bergey bergey deleted the threeD-util branch December 9, 2013 03:36
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.

None yet

2 participants