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 haddock of Graphics.Implicit #287

Merged
merged 12 commits into from
Nov 14, 2020
Merged

Improve haddock of Graphics.Implicit #287

merged 12 commits into from
Nov 14, 2020

Conversation

isovector
Copy link
Contributor

@isovector isovector commented Nov 14, 2020

This PR documents most of the top-level functions, insofar as I could figure out what they do. The argument descriptions were deduced from their names in Graphics.Implicit.ObjectUtil.GetImplicit3, but could use a second pair of eyes.

In addition, this PR fixes up some of the weird whitespace across the codebase (some trailing spaces, and a few intermingling tabs).

Potentially breaking change: I've stopped exporting extrudeRotateR, since as @raptortech-js pointed out, it's implemented as error and as such has no business existing.

Additionally: I've now exported the Object class, giving haddock users some indication of what types on which they can use translate et al. Also, R2' and R3` are now exported.

isovector referenced this pull request Nov 14, 2020
Add additional documentation for Haskell eDSL
Copy link
Member

@julialongtin julialongtin left a comment

Choose a reason for hiding this comment

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

Perfect. looks nice.

@julialongtin julialongtin merged commit 5ddd750 into Haskell-Things:master Nov 14, 2020
@jackie-scholl
Copy link
Contributor

This is awesome, thanks for adding so much documentation!

I'm pretty sure rotation is in degrees, though, not radians. You can test with main = writeSVG 10 "out/test1.svg" $ rotate 45 $ rectR 0 (0, 0) (100, 200).

Also, I'm not convined it's fair to say that the distance unit is universally millimeters; in the STL and SVG, for example, the output is at native, unspecified scale.

@isovector
Copy link
Contributor Author

@raptortech-js interesting. I've been using rads in all of my models, and things seem to work out. 45 is a bad test number I think, because 45 rad is equivalent to 58 deg, which is pretty close.

Indeed, the code seems to agree that it's in rads:

getImplicit3 (Rotate3 (yz, zx, xy) symbObj) =
    let
        obj = getImplicit3 symbObj
        rotateYZ :: ℝ -> (ℝ3 -> ℝ) -> (ℝ3 -> ℝ)
        rotateYZ θ obj' (x,y,z) = obj' ( x, y*cos θ + z*sin θ, z*cos θ - y*sin θ)
        rotateZX :: ℝ -> (ℝ3 -> ℝ) -> (ℝ3 -> ℝ)
        rotateZX θ obj' (x,y,z) = obj' ( x*cos θ - z*sin θ, y, z*cos θ + x*sin θ)
        rotateXY :: ℝ -> (ℝ3 -> ℝ) -> (ℝ3 -> ℝ)
        rotateXY θ obj' (x,y,z) = obj' ( x*cos θ + y*sin θ, y*cos θ - x*sin θ, z)
    in
        rotateXY xy $ rotateZX zx $ rotateYZ yz obj

where the sin and cos functions come from prelude.

You make an excellent point about the distances. My major point of confusion going through this was wondering if the rounding numbers were normalized to [0,1], and learning that they measure the radius was helpful. I'll send a followup PR.

@jackie-scholl
Copy link
Contributor

you're right, I should have tested more carefully! I brought it up because in my use case, I had noticed that RotateExtrude uses degrees: https://github.com/colah/ImplicitCAD/blob/5ddd75006cc8f1d6a0e184f56c3714a9097ce6b0/Graphics/Implicit/ObjectUtil/GetImplicit3.hs#L175-L176
It appears to be the only implicit to do that, so I think we should change how RotateExtrude works to use radians like the rest. I'll open an issue.

@isovector
Copy link
Contributor Author

😱

I spent a few hours yesterday attempting to quickspec this library. Sounds like a high-value thing to throw more time into!

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.

3 participants