Direction type #186

bergey · 2014-05-16T16:16:07Z

This branch introduces a new direction type, distinguished from Angles and Vectors. This was discussed a bit in on trello and probably elsewhere.

I changed the semantics of the arc-generating functions substantially, because the old semantics depended on distinguishing the direction 0 from the direction fullTurn. I'm tempted to go farther, and eliminate the CW / CCW functions, since a start direction and a (positive or negative) Angle seems to cover all the options. Or we could go back to taking two Directions, but give up on defining circles. Or interpret arc xDir (xDir.+^fullTurn) specially. @fryguybob what do you think?

I definitely need to test the arrow code more before this can be merged.

Right now, this branch builds, but trying to build diagrams-svg or other packages linking to it fails with linker errors. Is that just for me, or can others reproduce?

# ghc-7.6.3  cabal install core/ lib/ svg/ contrib/
Loading package diagrams-lib-1.1.0.1 ... linking ... ghc: /home/bergey/code/diagrams/.cabal-sandbox/lib/x86_64-linux-ghc-7.6.3/diagrams-lib-1.1.0.1/libHSdiagrams-lib-1.1.0.1.a: unknown symbol `diagramszmlibzm1zi1zi0zi1_DiagramsziDirection_Direction_closure'

# ghc-7.8
Loading package diagrams-lib-1.1.0.1 ... <command line>: can't load .so/.DLL for: /home/bergey/code/diagrams/.cabal-sandbox/lib/x86_64-linux-ghc-7.8.2/diagrams-lib-1.1.0.1/libHSdiagrams-lib-1.1.0.1-ghc7.8.2.so (/home/bergey/code/diagrams/.cabal-sandbox/lib/x86_64-linux-ghc-7.8.2/diagrams-lib-1.1.0.1/libHSdiagrams-lib-1.1.0.1-ghc7.8.2.so: undefined symbol: diagramszmlibzm1zi1zi0zi1_DiagramsziDirection_Direction_con_info)

move some instances around, remove some imports

jeffreyrosenbluth · 2014-05-16T17:20:06Z

src/Diagrams/TwoD/Vector.hs

 e :: Angle -> R2
-e = fromDirection
+e = fromDirection . (xDir .+^)


Can we change the name of this function and perhaps provide e as a synonym. I was really trying to avoid using e in arrowheads, it gives very little info on what the function does. Maybe something like fromAngle?

Actually. I think we should get rid of e, as it's semantics have changed. Since it used to be a synonym for fromDirection.

The behavior of e in unchanged, but the type of fromDirection has changed.

I was torn between

not providing a function Angle -> R2, in favor of the new Direction semantics

keeping one around because there's a lot of code, mostly arrowheads, that uses it

Since e is a pun on e^i\theta, it makes some sense that it takes an Angle. And it isn't exported from Diagrams.Preude, mitigating my concern there. I'd be fine adding a synonym fromAngle. Should it be exported?

I'm also fine removing e, and rewriting code as unitX & _theta .~ foo or similar. Does anyone besides me find that comfortable to read?

I seem to recall that we all agreed several months ago to stop exporting e from prelude (although it is possible I am mistaken). I'm comfortable reading the lens based code, my only concern is that it's a bit verbose. I'm fine with either eliminating an Angler -> R2 function or renaming it to fromAngle.

Why not use rotate theta unitX? That seems simple to write and clearer than lenses or the shorthand e.

I think that's a good idea.

@Mathnerd314 Oh, good point

On May 16, 2014 2:38:57 PM EDT, Mathnerd314 notifications@github.com wrote:

e :: Angle -> R2
-e = fromDirection
+e = fromDirection . (xDir .+^)

Why not use rotate theta unitX? That seems simple to write and
clearer than lenses or the shorthand e.

Reply to this email directly or view it on GitHub:
https://github.com/diagrams/diagrams-lib/pull/186/files#r12753425

Sent from my Android device with K-9 Mail. Please excuse my brevity.

Agreed, we might as well keep this version of e; @jeffreyrosenbluth remembers correctly that we no longer export it from Prelude.

remove extra language extension pragma

bergey · 2014-05-19T15:12:15Z

My linking errors are fixed, after wiping out my entire diagrams tree and cloning all 24 diagrams-related repos anew. Go figure.

bergey · 2014-05-23T19:49:46Z

This will also close #38. It links now; I was missing a cabal "exposed module" line. See also the associated branch of -contrib.

byorgey · 2014-05-26T12:29:22Z

What's the status of this PR? Would it be helpful for me to take a look?

bergey · 2014-05-26T15:20:23Z

@byorgey yes, that would be great. So would feedback from anyone else.

The status is: it builds, but probably shouldn't be merged until I test more and update -doc to match.
But first I'd like feedback:

Is this new type helpful in clarifying the meaning of types? (I think so)
How should the inputs to arc functions be expressed? (see my first comment above)

byorgey · 2014-05-26T16:32:43Z

src/Diagrams/TwoD/Arc.hs

+--   the angle @s@ counterclockwise.
+arcT :: Direction R2 -> Angle -> Trail R2
+arcT start sweep
+    | sweep < zeroV = arcT start (sweep ^-^ (fromIntegral d @@ turn))
    | otherwise     = (if sweep >= fullTurn then glueTrail else id)


I never really thought much about this before. Why do we do this glueTrail? E.g. what if the sweep angle is 3/2 turn? Doing glueTrail is very strange in that case. It seems the most consistent thing to do is to always generate a Line, which may wrap around multiple times in the case of a sweep angle greater than 1 turn (or less than -1 turn). I could imagine actually being able to do useful things with such arcs, e.g. using them to parameterize animations, or applying a transformation that scales along with the parameter in order to create a spiral, etc.

The current behavior (predating this PR) is that any arc which would result in more than a full circle is truncated before gluing. The Haddocks aren't very clear -- it's explained for bezierFromSweep, but you need to know that arcT is implemented in terms of that function. I like your proposal.

byorgey · 2014-05-26T16:47:27Z

I support getting rid of the arcCW and arcCCW functions. As mentioned in some comments above, I think we should also take the gluing out of the arc functions, as this is not really the right place for it. The gluing should go in the definition of unitCircle. A unit circle (a loop) should be different than an arc with an angle of 1 full turn (a line which happens to start + end in the same place). Moreover, I think this should also be different than an arc of 2 full turns, etc. --- that might require rewriting the code a bit.

byorgey · 2014-05-26T16:50:09Z

I should also mention that I like this very much!

Direction R2 is not actually an AffineSpace. for all Directions d, d .+^ fullTurn = d therefore (d .+^ a) .-. d is not a for a >= fullTurn angleBetweenDirs and rotate give nearly the same behavior

bergey · 2014-05-27T00:39:05Z

Removing the AffineSpace instance actually leaves us with few and ugly ways of constructing Directions. I suggested rotate above, but there isn't yet a Transformable instance for Direction, and I'm reluctant to write one because of the weird behavior under non-uniform scaling.

I'm using xDir & _theta <>~ a, which I think is correct (in 2D), but ugly. here's no Num instance on Angles, because Num is too broad, so we can't use +~. Subtraction is even worse -- xDir & _theta <>~ (negateV a).

I'm tempted to add a new function / operator / lens or two to make constructing Directions nicer. Thoughts on what it should be called? What the type should be? Brainstorming:

rotateDir :: Angle -> Direction R2 -> Direction R2
addAngle :: Direction R2 -> Angle -> Direction R2
<+^ :: Direction R2 -> Angle -> Direction R2
^+~ :: AdditiveGroup a => ASetter s t a a -> a -> s -> t

The last three allow the obvious subtraction functions, also.

jeffreyrosenbluth · 2014-05-27T01:39:25Z

Why not create a Transformable instance for Direction R2 that is both scale and translation invariant.

byorgey · 2014-05-27T01:48:53Z

I don't see why we need scale invariant at all. @bergey , what's wrong with non-uniform scaling of Directions? The "obvious" behavior doesn't seem weird to me at all, unless I'm missing something.

bergey · 2014-05-28T00:21:57Z

Note that angleBetween and angleBetweenDirs always give an angle between 0 and tau/2. They are defined in terms of the inner product, and so commutative. Not like - at all! (And that's why arrows look wrong right now.)

It is not right when the intent is to create a rotation between two vectors.

This code is confusing, untested, and poorly named. It will probably change in the future. But this implementation is not obviously wrong, like the last one was.

Conflicts: src/Diagrams/TwoD/Arc.hs

bergey · 2014-06-04T15:55:08Z

I've fixed the bugs of which I am aware. I think this is ready to merge, along with the associated branches of -contrib and -doc

bergey · 2014-06-11T01:55:18Z

I'm assuming folks are just busy. But let me know if you wanted more time to review this, or if I should be more reserved about merging my own code in future.

jeffreyrosenbluth · 2014-06-11T04:24:13Z

I had a chance to read through the code and it looks very nice.
Seems like we use rotate theta unitX and xDir & _theta fairly
often, I wonder if they should have convenience functions like, unitTheta
and dirTheta?

bergey · 2014-06-11T14:30:45Z

Maybe. I'm pretty happy with rotate theta unitX. I'm not sure we can make that shorter without blurring the distinction between angles and directions again. I'm not happy with the frequent code snippet (a ^. _theta) ^-^ (b ^. _theta). We really should provide a better way to do that. Just giving it a name would be OK, but from conversation with @Mathnerd314, I think there may be a nicer polymorphic solution, that gives a Rotation (some type we don't have yet, see eg https://trello.com/c/yMLrUh1o/194-add-special-intensional-representation-for-angle-preserving-transformations-in-r2) in R2 and other spaces. In R3, the trick is mostly explaining why the chosen rotation is better than any of the infinite other rotations between two given directions.

bergey added 7 commits May 16, 2014 16:01

Add Direction type

2d18288

Add Semigroup and Monoid instances for Angle

a7c3b0f

Remove old direction functions operating on Angles

8d365fb

Update arrow code to use Direction

9d4794e

Change Arc code to use a Direction and a sweep Angle

b3c3b8f

Update everything else to use Direction

e6c1433

Wall: add instances for Polar

ddce8f6

move some instances around, remove some imports

jeffreyrosenbluth reviewed May 16, 2014
View reviewed changes

bergey added 3 commits May 16, 2014 21:01

fix header in Direction

b0b39eb

remove extra language extension pragma

avoid use of e in Arrowheads

9f84cdc

Use atan2 instead of atan

fbc97d0

Export xDir, Direction module

d89c025

byorgey reviewed May 26, 2014
View reviewed changes

bergey added 6 commits May 26, 2014 20:31

Export Deform from 3D

c3c6724

Remove law-breaking AffineSpace instance

d4d7398

Direction R2 is not actually an AffineSpace. for all Directions d, d .+^ fullTurn = d therefore (d .+^ a) .-. d is not a for a >= fullTurn angleBetweenDirs and rotate give nearly the same behavior

add atan2A function

42b85ed

atDirection is no longer specific to R2

ed7410c

revise arc functions to respect sign of Angle

e79f568

glue circle into a Loop

6a27bf9

bergey added 2 commits May 27, 2014 02:35

add Transformable instance for Direction

1ac0743

Add UndecidableInstances pragma to Direction

7be96f6

This was referenced May 27, 2014

Direction diagrams/diagrams-contrib#38

Merged

update for new Direction type diagrams/diagrams-doc#55

Merged

bergey added 4 commits June 4, 2014 14:19

angleBetween is commutative

02dd838

It is not right when the intent is to create a rotation between two vectors.

fix implementation of pointAt

d5e265e

This code is confusing, untested, and poorly named. It will probably change in the future. But this implementation is not obviously wrong, like the last one was.

Merge remote-tracking branch 'origin/master' into direction

34c1266

Conflicts: src/Diagrams/TwoD/Arc.hs

fix imports, add some parens

df10acf

bergey merged commit df10acf into master Jun 11, 2014

byorgey deleted the direction branch June 11, 2014 02:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Direction type #186

Direction type #186

bergey commented May 16, 2014

jeffreyrosenbluth May 16, 2014

jeffreyrosenbluth May 16, 2014

bergey May 16, 2014

jeffreyrosenbluth May 16, 2014

Mathnerd314 May 16, 2014

jeffreyrosenbluth May 16, 2014

bergey May 16, 2014

byorgey May 26, 2014

bergey commented May 19, 2014

bergey commented May 23, 2014

byorgey commented May 26, 2014

bergey commented May 26, 2014

byorgey May 26, 2014

bergey May 26, 2014

byorgey commented May 26, 2014

byorgey commented May 26, 2014

bergey commented May 27, 2014

jeffreyrosenbluth commented May 27, 2014

byorgey commented May 27, 2014

bergey commented May 28, 2014

bergey commented Jun 4, 2014

bergey commented Jun 11, 2014

jeffreyrosenbluth commented Jun 11, 2014

bergey commented Jun 11, 2014

Direction type #186

Direction type #186

Conversation

bergey commented May 16, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergey commented May 19, 2014

bergey commented May 23, 2014

byorgey commented May 26, 2014

bergey commented May 26, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

byorgey commented May 26, 2014

byorgey commented May 26, 2014

bergey commented May 27, 2014

jeffreyrosenbluth commented May 27, 2014

byorgey commented May 27, 2014

bergey commented May 28, 2014

bergey commented Jun 4, 2014

bergey commented Jun 11, 2014

jeffreyrosenbluth commented Jun 11, 2014

bergey commented Jun 11, 2014