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

Preliminary arc support #42

Merged
merged 6 commits into from
Mar 30, 2018
Merged

Preliminary arc support #42

merged 6 commits into from
Mar 30, 2018

Conversation

raboof
Copy link
Member

@raboof raboof commented Mar 27, 2018

Only non-rotated arcs for now.

cx and cy seem accurate, but the arc and sweep appear to have a pretty
large (rounding?) errors - do you see where these might be coming from?

Only non-rotated arcs for now.

cx and cy seem accurate, but the arc and sweep appear to have a pretty
large (rounding?) errors - do you see where these might be coming from?
@frmdstryr
Copy link
Collaborator

It seems pretty close to me.

image

Not yet completely right nor do I understand exactly what this means
yet, but this gets rid of our 'rounding error'.

Still some work to do since there's some sign errors left to be worked out
This covers drawing non-rotated arcs, which I suspect are the common ones.

I noticed relatively-positioned arcs are not handled correctly, but that might
be a separate issue altogether.
@raboof raboof mentioned this pull request Mar 29, 2018
@raboof raboof changed the title Start on arc support Preliminary arc support Mar 29, 2018
@raboof
Copy link
Member Author

raboof commented Mar 29, 2018

@frmdstryr in that picture you had an arc that started at angle 0 and spanned the full 360 degrees - that indeed worked, but my algorithm was incorrect for most other angles since I mixed up 'phi' and 'theta'. I think it should be fine now though!

@raboof
Copy link
Member Author

raboof commented Mar 29, 2018

(please squash when merging)

Avoid drawing a straight line from start to end or interfering with the
'current' position.
@frmdstryr frmdstryr merged commit c975d7b into inkcut:master Mar 30, 2018
y1 = self.currentPosition().y()
(rx, ry, x_axis_rotation, large_arc_flag, sweep_flag, x2, y2) = params

self.arc(x1, y1, rx, ry, x_axis_rotation, large_arc_flag, sweep_flag, x2, y2)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this again... does x_axis_rotation need to be converted to radians here?

https://www.w3.org/TR/SVG/paths.html#PathDataEllipticalArcCommands "and an x-axis-rotation, which indicates how the ellipse as a whole is rotated, in degrees, relative to the current coordinate system".

All of python's trig functions take radians https://docs.python.org/2/library/math.html#trigonometric-functions.

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.

2 participants