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

Ellipse rotated incorrectly in test file with ellipse next to a square #2

Closed
fryguybob opened this Issue Aug 4, 2012 · 2 comments

Comments

Projects
None yet
2 participants
@fryguybob
Member

fryguybob commented Aug 4, 2012

(Imported from http://code.google.com/p/diagrams/issues/detail?id=7. Original issue from byor...@gmail.com on December 24, 2010, 10:15:48 PM UTC)

See the attached test file.

The ellipse is rotated the wrong way and intersects with the square. Note that adding a rotation to the square (even a very small one) causes the ellipse to suddenly flip the correct way around and everything seems correct.

@ghost ghost assigned byorgey Aug 4, 2012

@fryguybob

This comment has been minimized.

Show comment
Hide comment
@fryguybob

fryguybob Aug 4, 2012

Member

(Imported. Original comment by byor...@gmail.com on January 15, 2011, 01:03:57 AM UTC)

Oh man, this was a really sneaky one. I'm amazed that we ended up tripping over this bug as soon as we did, and it makes me wonder how many other sneaky, hard-to-trigger bugs are still lurking on the source... basically, the fix was to change

ellipseAngle :: Ellipse -> Angle
ellipseAngle ell
| b == 0 && a <= c = 0
| b == 0 && a > c = pi/2
| a <= c = atan (b/(a-c)) / 2
| otherwise = (pi + atan (b/(a-c))) / 2
where (a,b,c,,,_) = ellipseCoeffs ell

to
ellipseAngle :: Ellipse -> Angle
ellipseAngle ell
| b == 0 && a <= c = 0
| b == 0 && a > c = pi/2
| a < c = atan (b/(a-c)) / 2
| otherwise = (pi + atan (b/(a-c))) / 2
where (a,b,c,,,_) = ellipseCoeffs ell

See the difference? From the darcs patch comment:

To be honest, I don't quite understand what is going on here. But the
problem was clearly only manifesting when an ellipse scaled by exactly
2 was rotated by exactly pi/4 -- change anything the teensiest amount
and everything rendered correctly again! But for those exact values,
the ellipse being rendered was off by an angle of pi/2 from the
correct position. The <= was arbitrary on my part (I thought it
didn't matter which branch was taken when a == c) since the Mathworld
equations began by assuming a /= c. So apparently it did matter and I
guessed wrong.

Member

fryguybob commented Aug 4, 2012

(Imported. Original comment by byor...@gmail.com on January 15, 2011, 01:03:57 AM UTC)

Oh man, this was a really sneaky one. I'm amazed that we ended up tripping over this bug as soon as we did, and it makes me wonder how many other sneaky, hard-to-trigger bugs are still lurking on the source... basically, the fix was to change

ellipseAngle :: Ellipse -> Angle
ellipseAngle ell
| b == 0 && a <= c = 0
| b == 0 && a > c = pi/2
| a <= c = atan (b/(a-c)) / 2
| otherwise = (pi + atan (b/(a-c))) / 2
where (a,b,c,,,_) = ellipseCoeffs ell

to
ellipseAngle :: Ellipse -> Angle
ellipseAngle ell
| b == 0 && a <= c = 0
| b == 0 && a > c = pi/2
| a < c = atan (b/(a-c)) / 2
| otherwise = (pi + atan (b/(a-c))) / 2
where (a,b,c,,,_) = ellipseCoeffs ell

See the difference? From the darcs patch comment:

To be honest, I don't quite understand what is going on here. But the
problem was clearly only manifesting when an ellipse scaled by exactly
2 was rotated by exactly pi/4 -- change anything the teensiest amount
and everything rendered correctly again! But for those exact values,
the ellipse being rendered was off by an angle of pi/2 from the
correct position. The <= was arbitrary on my part (I thought it
didn't matter which branch was taken when a == c) since the Mathworld
equations began by assuming a /= c. So apparently it did matter and I
guessed wrong.

@fryguybob

This comment has been minimized.

Show comment
Hide comment
@fryguybob

fryguybob Aug 4, 2012

Member

(Imported. Original comment by fryguy...@gmail.com on January 15, 2011, 09:49:23 PM UTC)

I made some more changes here and now both ellipseAngle and ellipseScale are written in terms of a new ellipseAxes function. The ellipseAngle in terms of ellipseCoeffs would return negative angles in some cases. It think this was due to differences in the range of acot and atan. I also added some tests in diagrams-cairo/test for these functions.

Member

fryguybob commented Aug 4, 2012

(Imported. Original comment by fryguy...@gmail.com on January 15, 2011, 09:49:23 PM UTC)

I made some more changes here and now both ellipseAngle and ellipseScale are written in terms of a new ellipseAxes function. The ellipseAngle in terms of ellipseCoeffs would return negative angles in some cases. It think this was due to differences in the range of acot and atan. I also added some tests in diagrams-cairo/test for these functions.

@fryguybob fryguybob closed this Aug 4, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment