[in progress] [ImageDraw] Rewrite of polygon fill algorithm and ImagingDrawWideLine #610
Conversation
The functions `polygon8`, `polygon32` and `polygon32rgba` all have exactly the same logic in code, only changes the hline function called inside. Now all the logic is contained in `polygon_generic` which gets a function pointer to the hline handler, so there is no duplicated code anymore.
The rounds used in the library are towards positive/negative infinity. Since we're in a cartesian plane, rounds around zero are much more convenient, so we can use positive and negative values with consistent results.
The (previously refactored) polygon_generic function didn't draw
consistent polygons (equilateral polygons were not equilateral nor
symmetrical).
The most notable changes are:
* The horizontal edges are searched for when finding the polygon
boundaries, drawn and discarded from the edge list used to detect
intersections.
* The intersections are now checked and calculated from the current value
of the scan line (ymin) instead of in the middle (ymin + 0.5).
* Because the change in the scan line behavior, we should duplicate
the intersections in the maximum Y value of an edge or there will be
draw errors with concave and complex polygons.
* The rounds of the X coordinates in the hline function calls are
switched to draw the inner pixels.
* Removed the ugly micro-optimization of qsort at the end.
This implementation of the scan line algorithm may not be technically
correct, it's not optimized and it have problems with some edge cases,
like a wide line from (x0, y) to (x1, y + 1), therefore it should be
reviewed in the future.
The previous version of the function didn't generate correct wide lines
of even width.
The most notable changes are:
* Make variable names far more descriptive about the process.
* Corrected the width calculation; we should deduct one pixel from the
width because the pixels at the center of the line doesn't count for
the triangles.
* Now we calculate *two* ratios, one for left/top displacement (dxmin)
and one for right/bottom (dxmax), this fix the behavior with lines
of even width.
It can probably be optimized.
The method `create_base_image_draw` will be used in all the new draw tests.
The diagonals of the right angled edges must be perfect and the bottom vertice should be drawn.
Notice that the expansion of the line width depends on the order of the
points:
* If the bigger axis value is provided as the *second* point the line
expand first to the *positive* side of the axis.
* If the bigger axis value is provided as the *first* point the line
expand first to the *negative* side of the axis.
* If the line's width is odd this doesn't matter, as the line will
expand the same amount to both sides.
This behavior should be consistent in both horizontal and vertical lines.
The behavior is consistent with horizontal lines, see previous commit for details.
Now writing in the wall "don't push before compile" 100 times.
Terseus
changed the title
|
I completed the description and added a script that had helped me a lot with the development and testing (sorry for the big wall of text). |
Only the oblique 3 pixels wide lines are defined: * The oblique 2 pixels wide lines are somewhat hard to define. * To define the oblique lines wider than 3 pixels we neet to define first how the oblique lines should expand their width (realistic or exact).
These tests should guarantee that the proportion of the width is maintained with a margin of error < 1%.
This tests are designed to guarantee that the wide lines behave exactly like normal lines drawn with the Bresenham's algorithm. This tests are somewhat subjective since this is non-defined behavior, but I think that mimic the Bresenham's algorithm is reliable enough. Currently the horizontal version of this test **fail**.
|
@Terseus, My apologies for pushing you to this PR, and then not giving feedback for so long. It's in my queue, and I have not forgotten you. |
|
Would love to see this in 2.5.0 |
|
Hello, please accept my apologies for not updating before. Since this is taking too much time in being reviewed, I decided to stop waiting. |
|
Great! In the meantime you've been bumped to the future milestone. Thanks |
|
@Terseus Just give a shout if you need a hand with the merge. |
|
I've done a merge of this to the current master: https://github.com/wiredfool/Pillow/tree/terseus_imagedraw Apart from the change in testing style, it's a clean merge. However, in the meantime, there have been some other tests from @hugovk added to ImageDraw that specify the as then current behavior. In my opinion, the new images look better from test_ellipse, test_polygon, and test_pieslice, where test_line and test_line_horizontal are just different, and are more correct in the new version. I think that we should probably update those test images to reflect the code from this PR. I'm willing to live with blocky ellipses, until the time when someone wants to come in and smooth them with more exact math, or more sides. |
|
@wiredfool thanks a lot for the merge, my local merge was not that clean. Since the @hugovk tests are more focused on testing the different parameter types accepted by the methods, what both of you think about organizing the tests in different classes inside test_imagedraw.py? |
|
@Terseus Sure, go ahead and add/change the classes inside test_imagedraw.py if it makes the tests neater and clearer. And we can have extra test_XXX.py test files if that helps. Generally speaking, test_XXX() functions should test little bits of functionality each, so when one fails it's easier to diagnose what exactly went wrong. |
|
@wiredfool What's next, you want to send a PR from https://github.com/wiredfool/Pillow/tree/terseus_imagedraw ? |
|
And should I assume it's OK to close this PR? |
|
Ok, PR sent, we still need to fixup the failing imagedraw tests prior to merge. |
|
Thanks |
This PR is not finished, do not merge!! Its purpose is just to obtain feedback.
The functions
polygon8|32|32argbof ImageDraw (file libimaging/Draw.c) module don't draw consistent polygons, as can be seen in issues #367 and #463; this is an attempt to solve this problem.Those functions are now refactored into one function,
polygon_generic, to remove duplicated logic code (see deecd8a).polygon_genericdraw polygons using the scanline algorithm, the old implementation were the origin of some inconsistencies in polygons, and while the new implementation it's not technically correct (e.g. it doesn't takes advantage of edge coherency) and have some known bugs (more on this later), it generates more consistent polygons (see cd332fc).The function
ImagingDrawWideLineis responsible of constructing wide lines as a rectangle polygon by obtaining the side ratio of two complementary right triangles, however while the calculations were correct in theory, it doesn't build correct wide lines as seen in #367.The new implementation of this function (see be09da6) construct the line's rectangles with the correct proportion to every side of the line but have some problems with lines of 45º slope, see below.
Some draw tests have been added to
Tests/test_imagedraw.pywith images, check them out to see how the expected behavior is with the new implementations in the covered cases; more tests will be added soon.List (incomplete) of current known bugs and/or to-do items:
This dirty script will allow you to test the ImageDraw module without having to write a program for every case (edit the constant DEV_PILLOW_PATH manually): https://gist.github.com/Terseus/750a02722640ae715f96
It have some basic help, some examples on how to use it:
Edit 1: Completed description with reasoning about
ImagingDrawWideLine, the current test cases and known problems.Edit 2: Added a Python script for easy testing of the ImageDraw module.