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

calculate arrow shaftScale using projection of ends onto shaft offset #125

Merged
merged 1 commit into from
Oct 10, 2013

Conversation

bergey
Copy link
Member

@bergey bergey commented Oct 10, 2013

I think this fixes the bug described in 56b703f#commitcomment-4294528. The output looks like this

@byorgey
Copy link
Member

byorgey commented Oct 10, 2013

Ah, this is a much more sensible way to do it.

byorgey added a commit that referenced this pull request Oct 10, 2013
calculate arrow shaftScale using projection of ends onto shaft offset
@byorgey byorgey merged commit a8befce into master Oct 10, 2013
@byorgey
Copy link
Member

byorgey commented Oct 10, 2013

Now we just need to fix the warning, and we can also do away with endTangent and startTangent, simply calling tangentAtStart and tangentAtEnd (from Diagrams.Tangent) directly on the Trail.

@jeffreyrosenbluth
Copy link
Member

Puzzling. If you look closely at http://bergey.s3.amazonaws.com/diagrams/Arrowtest-fixed.svg you will notice that the tip of the orange arrow head is not in the exact center of the origin. This occurs in both @bergey 's fix and my alternative, but did not happen when I calculated the shaft scale iteratively (i.e. using binomial search)?

For comparison:

https://github.com/jeffreyrosenbluth/diagrams-gallery/blob/master/arrowtest.svg

@byorgey
Copy link
Member

byorgey commented Oct 10, 2013

Are you sure it's not just a rendering artifact? Can you make a bigger version which clearly shows the discrepancy? It is too small for me to really see.

@jeffreyrosenbluth
Copy link
Member

I'm not 100% sure but if you look at the diagram I made with old code you
can see that it's off.

Take a look, I put a link in the comment to an svg that runs the same test
using the iterative technique

On Thu, Oct 10, 2013 at 10:29 AM, Brent Yorgey notifications@github.comwrote:

Are you sure it's not just a rendering artifact? Can you make a bigger
version which clearly shows the discrepancy?


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

@jeffreyrosenbluth
Copy link
Member

I actually, just dropped my old shaftScale function (using fInverse) into
my newest lens code and the diagram renders correctly!

On Thu, Oct 10, 2013 at 10:31 AM, Jeffrey Rosenbluth <
jeffrey.rosenbluth@gmail.com> wrote:

I'm not 100% sure but if you look at the diagram I made with old code you
can see that it's off.

Take a look, I put a link in the comment to an svg that runs the same test
using the iterative technique

On Thu, Oct 10, 2013 at 10:29 AM, Brent Yorgey notifications@github.comwrote:

Are you sure it's not just a rendering artifact? Can you make a bigger
version which clearly shows the discrepancy?


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

@byorgey
Copy link
Member

byorgey commented Oct 10, 2013

I still cannot see the difference. How about this: can you (1) open a new ticket for this issue, and (2) make two very large (like, a width of at least 1000) images which show the discrepancy, preferably using a cut-down version of the diagram that includes only the first two circles and the arrow with the orange head.

@bergey
Copy link
Member Author

bergey commented Oct 10, 2013

Last night I spent some time in ghci comparing the outputs of various shaftScale functions. The project-and-divide version agrees with the fInverse version to 3 decimal places, with differences beyond that. I'm not sure why that is; I'm pretty sure that they're the same algebraicly.

(Incidentally, @jeffreyrosenbluth I agree that the code you pasted above is equivalent to the code I commited. I just wrote the one that occurred to me first.)

@jeffreyrosenbluth
Copy link
Member

When I get a chance I will

  1. Open a ticket
  2. Try to create a more obvious example
  3. Try to compare the actual svg generated.

Sent from my iPad

On Oct 10, 2013, at 10:48 AM, Daniel Bergey notifications@github.com wrote:

Last night I spent some time in ghci comparing the outputs of various shaftScale functions. The project-and-divide version agrees with the fInverse version to 3 decimal places, with differences beyond that. I'm not sure why that is; I'm pretty sure that they're the same algebraicly.

(Incidentally, @jeffreyrosenbluth I agree that the code you pasted above is equivalent to the code I commited. I just wrote the one that occurred to me first.)


Reply to this email directly or view it on GitHub.

@jeffreyrosenbluth
Copy link
Member

1 and 2 are done.
I'm not sure if 3 is worthwhile - too many small rounding errors to find
the relevant info.
But I'll give it a try.

On Thu, Oct 10, 2013 at 11:04 AM, Jeffrey.rosenbluth <
jeffrey.rosenbluth@gmail.com> wrote:

When I get a chance I will

  1. Open a ticket
  2. Try to create a more obvious example
  3. Try to compare the actual svg generated.

Sent from my iPad

On Oct 10, 2013, at 10:48 AM, Daniel Bergey notifications@github.com
wrote:

Last night I spent some time in ghci comparing the outputs of various
shaftScale functions. The project-and-divide version agrees with the
fInverse version to 3 decimal places, with differences beyond that. I'm not
sure why that is; I'm pretty sure that they're the same algebraicly.

(Incidentally, @jeffreyrosenbluth https://github.com/jeffreyrosenbluthI agree that the code you pasted above is equivalent to the code I
commited. I just wrote the one that occurred to me first.)


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

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