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

Added bothSize function, lineHead and lineTail #165

Merged
merged 4 commits into from Mar 12, 2014
Merged

Added bothSize function, lineHead and lineTail #165

merged 4 commits into from Mar 12, 2014

Conversation

jeffreyrosenbluth
Copy link
Member

I think this is ready to merge but there are two caveats.

  • Setting arrowHead and arrowTail after using bothSize instead of before might give unintuitive results.
  • bothSize is still asymmetric since we use the arrowhead to calculate the width for both the head and tail. The only ways around it would be 1) to specify the width instead of the size, which is inconsistent with the rest of the API. Or 2) to provide 2 functions bothSizeHead and bothSizeTail such that bothSizeHead is the current bothSize and bothSizeTail bases the width on the arrow tail, which I think is unnecessarily complicated.

@bergey
Copy link
Member

bergey commented Mar 4, 2014

I'm concerned that this gives counterintuitive results all around, because the argument to bothSize can't be interpreted at all without knowing most of the ArrowOpts.

I actually think it would make sense to specify the xWidth (which is the length of the arrowhead + gap along the direction it points) instead of the circumcircle size. If that's the measurement we think users want to control for good visual results, let's make it easy. Maybe we should redefine the semantics of headSize and tailSize to be xWidth rather than circumcircle radius, while we're at it.

Second, bothSize doesn't do what the docs imply when used with *~ and different initial arrow sizes. (Bottom arrow) I think the current behavior is correct; can the docs be clarified?

aStyle = (with & arrowTail .~ dart' & tailSize .~ 0.6)

example = centerY $ vcat' (with & sep .~ 1)
          [ arrowBetween' aStyle ((-1)^&0) (1^&0)
          , arrowBetween' (aStyle & bothSize .~ 0.6) ((-1)^&0) (1^&0)
          , arrowBetween' (aStyle & bothSize *~ 2) ((-1)^&0) (1^&0)
          ]

main = renderSVG "bothSize-test.svg" (Width 400) $ example <> square 3

@jeffreyrosenbluth
Copy link
Member Author

The argument to bothSize can be interpreted the same way as the argument to headSize which also depends on the arrowhead. That said, I had the same thought about redefining the semantics of of headSize to xWidth. We actually choose the current semantics somewhat arbitrarily when the arrowheads where first being designed. Let me look in to what it would take to do this. I won't bother fixing the documentation until we decide on the semantic change. Also, I'm heading to France tonight (back on Sunday) so I will have spotty internet and very little hacking time. Except perhaps on the flight.

@jeffreyrosenbluth
Copy link
Member Author

@bergey There is a problem with redefining the semantics of headSize to be xWidth which is the xWidth of the joint which is the part of the arrowhead that connects to the shaft depends on the shaft line width which we don't know until the ArrowOpt, shaftStyle is set. So we are basically back in the same situation. I think I should try to clarify the documentation and then merge this branch as I think solving issues like the ushaft problem easily is important for users.

@byorgey
Copy link
Member

byorgey commented Mar 6, 2014

Personally I would rather leave this until after the 1.1 release. I am not yet convinced we have settled on the best design, and it just creates a lot more work sticking in a new feature this late. We're getting really close with 1.1 and I'd like to get it out the door.

@jeffreyrosenbluth
Copy link
Member Author

That makes sense, especially since I'm out of town and won't be able to
work on it until next week. I have also had some other thoughts about how
best to handle the design. Lets revisit it after 1.1

On Thu, Mar 6, 2014 at 5:09 PM, Brent Yorgey notifications@github.comwrote:

Personally I would rather leave this until after the 1.1 release. I am not
yet convinced we have settled on the best design, and it just creates a lot
more work sticking in a new feature this late. We're getting really close
with 1.1 and I'd like to get it out the door.

Reply to this email directly or view it on GitHubhttps://github.com//pull/165#issuecomment-36903670
.

@byorgey
Copy link
Member

byorgey commented Mar 12, 2014

Looks good to me. We should be sure to add a description & examples to the arrow tutorial.

@byorgey
Copy link
Member

byorgey commented Mar 12, 2014

Oh, the travis build is failing because of the redundant import of sets --- can you fix that first?

byorgey added a commit that referenced this pull request Mar 12, 2014
Added bothSize function, lineHead and lineTail
@byorgey byorgey merged commit 0193b0e into master Mar 12, 2014
@byorgey byorgey deleted the tail branch March 12, 2014 11:03
@byorgey
Copy link
Member

byorgey commented Mar 12, 2014

Never mind, I just removed the redundant import myself.

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

3 participants