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

Add arcCCW and friends. Fix offset joins. #221

Merged
merged 6 commits into from
Nov 8, 2014
Merged

Add arcCCW and friends. Fix offset joins. #221

merged 6 commits into from
Nov 8, 2014

Conversation

fryguybob
Copy link
Member

These variants for arc creation hide the complexity of angles and
orderings between them and instead always gives an arc in a fixed
direction (clockwise "CW" or counterclockwise "CCW") from some start
angle or direction to an end angle or direction.

There is a possibility that arcDCCW and arcDCW could be constrained by HasTheta instead of a direction, but that broke inference for me and I didn't play around with it after that.

These variants for arc creation hide the complexity of angles and
orderings between them and instead always gives an arc in a fixed
direction (clockwise "CW" or counterclockwise "CCW") from some start
angle or direction to an end angle or direction.
@cchalmers
Copy link
Member

I think we should only use the Direction versions. I reduces the number of special cases to deal with and is consistent with the idea that a Direction is a "fixed angle" and an Angle is the angle between two directions. (I'm tempted to write an Affine instance for Direction V2 with Diff = Angle)

Also I think a directionBetween function would be nice in Diagrams.Direction.

@bergey
Copy link
Member

bergey commented Oct 24, 2014

What would directionBetween do?

@cchalmers
Copy link
Member

directionBetween p q = direction $ p .-. q

@byorgey
Copy link
Member

byorgey commented Oct 24, 2014

The Affine instance for Direction and Angle has been discussed before, but it doesn't quite satisfy the usual axioms for an affine space. In particular, there is a uniqueness axiom that essentially says that designating an origin gives you an isomorphism between points and vectors. But in this case the mapping from angles to directions, given some fixed reference direction as the base, is not an injection.

@fryguybob
Copy link
Member Author

Perhaps it would make sense to make Angle an instance of HasTheta then that would cover all cases. But I'm too tired now to figure out the inference issues I was having to even judge if this is a workable idea. We should also be careful not to accumulate rotation errors from arcs due to our use of bezierFromSweep. The idea would be, if we know the desired offset exactly, we should assign that exact (or close to it) offset to the generated Segment rather than having the offset filtered through half the precision and errors from trig functions.

@cchalmers
Copy link
Member

HasTheta can be a convent hack but I don't like the idea of using it for library functions. There's just too much it can take. For instance you could give it two P2 Doubles (or even P3) and the result probably isn't what the user expects

For such a low level function that's prone to bugs/confusion I'd prefer it was limited to Direction. Then it's clear that only the direction is considered.

@fryguybob
Copy link
Member Author

That's fine with me, but I don't really have time right now to do anything. I'm happy with just using the names arcCCW and arcCW for what is in this patch as arcDCCW and arcDCW and only exporting those. The easy thing would be just to change the names and exports or we could consider how to simplify. I can post the code I used to generate a test animation if we do end up simplifying things.

@cchalmers
Copy link
Member

Cool. I'll just change the names and merge for now. We can simplify later.

@fryguybob
Copy link
Member Author

Great. The code for my tests is here: https://gist.github.com/fryguybob/93e71378d5868338f2d1

@cchalmers
Copy link
Member

Thanks. I made it simpler and made some changes to Direction. It passes your tests and the original problem so I hope it's alright.

cchalmers added a commit that referenced this pull request Nov 8, 2014
Add arcCCW and friends.  Fix offset joins.
@cchalmers cchalmers merged commit e80d885 into master Nov 8, 2014
@byorgey byorgey deleted the arcCCW branch February 9, 2017 14:18
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

4 participants