-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Implemented another ellipse drawing algorithm #4523
Conversation
Co-Authored-By: Andrew Murray <3112309+radarhere@users.noreply.github.com>
Adjusted symmetry test
src/libImaging/Draw.c
Outdated
@@ -988,7 +1165,7 @@ int | |||
ImagingDrawEllipse(Imaging im, int x0, int y0, int x1, int y1, | |||
const void* ink, int fill, int width, int op) | |||
{ | |||
return ellipse(im, x0, y0, x1, y1, 0, 360, ink, fill, width, CHORD, op); | |||
return ellipseNew(im, x0, y0, x1, y1, ink, fill, width, op); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any chance new implementation is used for other functions? At least for ImagingDrawChord?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but it would involve another layer of processing that clips ellipse down to arc.
I've got it more or less done locally, maybe I'll commit that later. The code is gonna look funky though due to all the floating point geometry and special cases processing...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arcs, chords and pies are kinda working now.
At least compared to whatever was in master before.
I also added a test for various size ellipses. I added it long ago but forgot to push.
All test images are circles, actually, not ellipses. Could you add ones? |
@xtsm Little reminder for ^ this, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see elliptical images have been added (e.g. 5830a64), thanks! Any more review comments?
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Thanks! Please also update the release notes if you think it's necessary: https://github.com/python-pillow/Pillow/blob/master/docs/releasenotes/8.0.0.rst |
Release notes PR: #4976 |
Add #4523 ellipse-drawing algorithm changes to release notes
…s to no longer be polygons
This PR changes ellipse drawing algorithm from simply drawing 360-gon to something that resembles Bresenham's algo for circles. It works like 100x faster (on my machine, according to randomized test) and produces nicer results, especially for smaller ellipse sizes.
Arcs, pies, etc. are left untouched for now.
Upd: I've also changed some tests (e.g. ellipse_symmetric and ellipse_edge) according to new drawing behavior. If now they're not checking what they were supposed to check, please tell me.