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

Bugfix for rotated ellipse #221

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kennyweiss
Copy link

Thanks for this great library!

I was playing with rotated ellipses and noticed a bug in how the rotation angle is extracted from the transform function.
Essentially, arccos wasn't computing the right sign for some angles, so I replaced it with arctan2 and added a bunch of unit tests.

Reproducer:

Before this PR, the following input

<svg width="200" height="200" xmlns="http://www.w3.org/2000/svg" style="fill:green;stroke:black;stroke-width:1.5">
    <ellipse cx="40" cy="80" rx="15" ry="20" transform="rotate(-45 40 80)"/>
</svg>

image

generated these arc paths:

<?xml version="1.0" ?>
<svg:svg xmlns:svg="http://www.w3.org/2000/svg" width="200" height="200" style="fill:red;stroke:black;stroke-width:1.5">
    <path d="M 29.393398282201787,90.60660171779821 
             A 15.0,20.0 45.0 1,0 50.60660171779821,69.39339828220179 
             A 15.0,20.0 45.0 1,0 29.393398282201787,90.60660171779821"/>
</svg:svg>

Note: The angle is 45 degrees instead of -45 degrees, which is rendered as:
image
(note: I changed the fill color for easier comparison).

After this PR, the correct arc paths are produced:

<?xml version="1.0" ?>
<svg:svg xmlns:svg="http://www.w3.org/2000/svg" width="200" height="200" style="fill:red;stroke:black;stroke-width:1.5">
    <path d="M 29.393398282201787,90.60660171779821 
             A 15.0,20.0 -44.99999999999999 1,0 50.60660171779821,69.39339828220179 
             A 15.0,20.0 -44.99999999999999 1,0 29.393398282201787,90.60660171779821"/>
</svg:svg>

image

@kennyweiss
Copy link
Author

Note: This bugfix seems related to the discussion in #98
(specifically, this comment from @Vrroom -- #98 (comment))

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.

1 participant