Added regular_polygon draw method by comhar · Pull Request #4846 · python-pillow/Pillow

comhar · 2020-08-09T21:04:13Z

Changes proposed in this pull request:

Add a regular_polygon method to ImageDraw

Usage

Draw a red hexagon, rotated by 90 degrees, inscribed by the following bounding box = [(50,50), (150, 150)]

img = Image.new('RGBA', size=(200, 200), color=(255, 0, 0, 0))
draw = ImageDraw.Draw(img)
bbox=[(50,50), (150, 150)]
n_sides=6
draw.regular_polygon(bbox, n_sides, rotation=90, fill='red')

~~- Add a compute_polygon_coordinates method to ImageDraw.~~

~~#### What does compute_polygon_coordinates do?~~

~~compute_polygon_coordinates creates a list of coordinates for a polygon of n sides with a given length.~~

~~### Usage~~
~~Draw a regular hexagon with side of length 10, starting at the position (50, 50)~~

~~```python~~
~~# 1. Compute Co-ordinates~~
~~coordinates = compute_polygon_coordinates(~~
~~nb_sides=6,~~
~~side_length=10,~~
~~starting_point=(50,50)~~
)
~~# 2. Draw hexagon~~
~~img = Image.new('RGBA', size=(256, 256), color=(255, 0, 0, 0))~~
~~draw = ImageDraw.Draw(img)~~
~~draw.polygon(coordinates, fill=colour)~~

radarhere · 2020-08-10T07:28:30Z

Hmm. What is your use case for this?

Our drawing methods of arc, chord, ellipse, etc, accept a bounding box as an argument, and work within that. So I'm wondering if it would be helpful here to have a regular_polygon method, that accepts a bounding box, and the number of sides, and then draws a regular polygon centered within that? Not entirely sure if that's helpful, but I wanted to put the idea out there.

nulano · 2020-08-10T09:29:04Z

It seems to me that either this or @radarhere's suggestion would be a lot more useful if they allowed an arbitrary rotation. It looks like this PR always returns a polygon with a horizontal edge at the bottom.

comhar · 2020-08-10T20:14:20Z

Hmm. What is your use case for this?

I needed to generate regular polygons for a machine learning project. Although drawing regular polygons isn't as common as drawing squares or circles, regular_polygon would be a nice method to have.

@radarhere @nulano, I like both of your suggestions. I'll include these changes in the PR.

comhar · 2020-08-16T12:43:24Z

Here's the updated method

# Draw a red hexagon, rotated by 90 degrees, inscribed by the following bounding box = [(50,50), (150, 150)]
img = Image.new('RGBA', size=(200, 200), color=(255, 0, 0, 0))
draw = ImageDraw.Draw(img)
bbox = [(50,50), (150, 150)
n_sides=6
draw.regular_polygon(bbox, n_sides, rotation=90, fill='red')

nulano · 2020-08-16T17:42:26Z

Note that other functions accept both a [(x0, y0), (x1, y1)] and [x0, y0, x1, y1] bbox in a positional parameter named xy, see e.g. https://pillow.readthedocs.io/en/stable/reference/ImageDraw.html#PIL.ImageDraw.ImageDraw.arc

Have you considered what happens if the bbox is not a square? I can't see any tests concerning that. I think the current version draws a polygon bounded by the height of the bbox, possibly spilling over horizontally. I'm wondering if it would be useful to first compute a polygon bounded by the height and then stretch the width to match the bbox (similar to shapes in MS Paint). It would no longer be a regular polygon (not sure how this would affect the function name), but perhaps it would be useful to draw axis-stretched shapes. I'm not sure how common this would be, but in such a rare situation, I imagine it could be quite useful.

hugovk · 2020-08-16T18:56:15Z

    :param fill: Color to use for the fill.

+
+.. py:method:: ImageDraw.regular_polygon(*, b_box, nb_sides, rotation=0, fill=None, outline=None)


Nits:

Let's call it bbox throughout, that's what the rest of the codebase uses.

Also, n_things is used quite a lot, and there's very little nb_things:

Suggested change

.. py:method:: ImageDraw.regular_polygon(*, b_box, nb_sides, rotation=0, fill=None, outline=None)

.. py:method:: ImageDraw.regular_polygon(*, bbox, n_sides, rotation=0, fill=None, outline=None)

I would go with xy here, as while most of the code base uses bbox, the ImageDraw functions use xy: https://pillow.readthedocs.io/en/stable/reference/ImageDraw.html#PIL.ImageDraw.ImageDraw.rectangle

Suggested change

.. py:method:: ImageDraw.regular_polygon(*, b_box, nb_sides, rotation=0, fill=None, outline=None)

.. py:method:: ImageDraw.regular_polygon(*, xy, n_sides, rotation=0, fill=None, outline=None)

Summary of changes - Allow positional args in `regular_polygon` method - Allow multiple bounding box formats - (e.g. bbox = [(x0, y0), (x1, y1)] or [x0, y0, x1, y1]) - Check if bounding box is square - Update var names - b_box => bbox - nb_sides => n_sides

comhar · 2020-08-16T20:26:34Z

@nulano , thanks for your review.

Note that other functions accept both a [(x0, y0), (x1, y1)] and [x0, y0, x1, y1] bbox in a positional parameter named xy, see e.g. https://pillow.readthedocs.io/en/stable/reference/ImageDraw.html#PIL.ImageDraw.ImageDraw.arc

Both formats are now accepted.

Have you considered what happens if the bbox is not a square? I can't see any tests concerning that. I think the current version draws a polygon bounded by the height of the bbox, possibly spilling over horizontally. I'm wondering if it would be useful to first compute a polygon bounded by the height and then stretch the width to match the bbox (similar to shapes in MS Paint). It would no longer be a regular polygon (not sure how this would affect the function name), but perhaps it would be useful to draw axis-stretched shapes. I'm not sure how common this would be, but in such a rare situation, I imagine it could be quite useful.

I've added a check to ensure the bounding box is square.
axis-stretched shapes would be a nice addition. As you mentioned, axis stretching would mean the polygon is no longer regular. Perhaps axis-stretching would be better in another PR.

comhar · 2020-08-19T18:26:05Z

@radarhere , @nulano let me know if there's anything I can do to help with the review.

hugovk · 2020-08-20T09:51:21Z

Minor thing, maybe make the filenames a bit shorter? For example:

-Tests/images/imagedraw_regular_polygon__octagon_with_0_degree_rotation.png
+Tests/images/imagedraw_regular_octagon.png

-Tests/images/imagedraw_regular_polygon__square_with_0_degree_rotation.png
+Tests/images/imagedraw_square.png

-Tests/images/imagedraw_regular_polygon__square_with_45_degree_rotation.png
+Tests/images/imagedraw_square_rotate_45.png

hugovk

Some consistency, nits and simplification!

nulano · 2020-08-20T14:54:01Z

Hmm. The current version draws a polygon inscribed into a circle inscribed into the bounding box. While this is fine for e.g. hexagons, it is not great for squares or triangles:

I implemented my stretching idea, but that is instead not great for hexagons, where it would require users to compute the height for a regular hexagon:

Maybe it would be best to pass the center point in xy and a radius instead? That would also solve the problem of what to do for a non-square bbox.

For reference, I used the following code:

def test(*args, **kwargs):
     im = Image.new("RGB", (100,100), "yellow")
     d = ImageDraw.Draw(im)
     d.regular_polygon(*args, **kwargs)
     im.show()
test((1,1,98,98), 3, 0, "black")
test((1,1,98,98), 4, 0, "black")
test((1,1,98,98), 6, 0, "black")

Summary of changes: - `ImageDraw.regular_polygon` now accepts a bounding circle which inscribes the polygon. A bounding circle is defined by a center point (x0, y0) and a radius. A bounding box is no longer accepted. - All keyword args have been replaced with positional args. Misc - Test image file renaming, minor variable name changes

comhar · 2020-08-20T19:57:01Z

@nulano, thanks for your review.

Maybe it would be best to pass the center point in xy and a radius instead? That would also solve the problem of what to do for a non-square bbox.

Agreed. A bounding circle is a good compromise + it significantly simplifies the code. I've included this change in the latest version.

comhar · 2020-08-20T20:01:47Z

@hugovk, thanks the review. I've implemented your suggestions. Small detail, but what are your thoughts on b_circle vs bcircle?

hugovk · 2020-08-20T20:13:18Z

Thanks for the updates!

@hugovk, thanks the review. I've implemented your suggestions. Small detail, but what are your thoughts on b_circle vs bcircle?

Well, bbox is a well-used term, and googling I find helpful results on Wikipedia, OpenStreetMap and R Documentation. Not so with bcircle, so I think b_circle is better. I'm tempted to suggest bounding_circle too (or even just circle?), but I don't have a strong opinion.

hugovk · 2020-08-20T20:15:03Z

+    :param b_circle: A bounding circle which inscribes the polygon
+        (e.g. b_circle=[50, 50, 25]).
+    :param n_sides: Number of sides
+        (e.g. n_sides=3 for a triangle, 6 for a hexagon).
+    :param rotation: Apply an arbitrary rotation to the polygon
+        (e.g. rotation=90, applies a 90 degree rotation).


Suggested change

:param b_circle: A bounding circle which inscribes the polygon

(e.g. b_circle=[50, 50, 25]).

:param n_sides: Number of sides

(e.g. n_sides=3 for a triangle, 6 for a hexagon).

:param rotation: Apply an arbitrary rotation to the polygon

(e.g. rotation=90, applies a 90 degree rotation).

:param b_circle: A bounding circle which inscribes the polygon

(e.g. ``b_circle=[50, 50, 25]``).

:param n_sides: Number of sides

(e.g. ``n_sides=3`` for a triangle, ``6`` for a hexagon).

:param rotation: Apply an arbitrary rotation to the polygon

(e.g. ``rotation=90``, applies a 90 degree rotation).

https://pillow--4846.org.readthedocs.build/en/4846/reference/ImageDraw.html#PIL.ImageDraw.ImageDraw.regular_polygon

nulano

I think bounding_circle is slightly better than b_circle, but both are much better than bcircle. Either way, it should accept a point specified as a tuple.

nulano · 2020-08-20T20:57:32Z

+    if not len(b_circle) == 3:
+        raise ValueError(
+            "b_circle should contain 2D coordinates and a radius (e.g. [x0, y0, r])"
+        )
+
+    if not all(isinstance(i, (int, float)) for i in b_circle):
+        raise ValueError("b_circle should only contain numeric data")


Suggested change

if not len(b_circle) == 3:

raise ValueError(

"b_circle should contain 2D coordinates and a radius (e.g. [x0, y0, r])"

)

if not all(isinstance(i, (int, float)) for i in b_circle):

raise ValueError("b_circle should only contain numeric data")

if len(b_circle) == 3:

*centroid, polygon_radius = b_circle

elif len(b_circle) == 2:

centroid, polygon_radius = b_circle

else:

raise ValueError(

"b_circle should contain 2D coordinates and a radius (e.g. [x0, y0, r])"

)

if not all(isinstance(i, (int, float)) for i in (*centroid, polygon_radius)):

raise ValueError("b_circle should only contain numeric data")

nulano · 2020-08-20T20:57:45Z

+
+    # 3. Variable Declarations
+    vertices = []
+    *centroid, polygon_radius = b_circle


Suggested change

*centroid, polygon_radius = b_circle

nulano · 2020-08-20T20:59:04Z

+    Generate a list of vertices for a 2D regular polygon.
+
+    :param b_circle: A bounding circle which inscribes the polygon
+        (e.g. b_circle = [x0, y0, r])


Suggested change

(e.g. b_circle = [x0, y0, r])

(e.g. ``b_circle = [x0, y0, r]`` or ``b_circle = [(x0, y0), r]``)

hugovk · 2020-08-21T14:27:15Z

Actually, on tuples vs. lists:

    :param bounding_circle: A bounding circle which inscribes the polygon
        (e.g. ``bounding_circle=[x0, y0, r]`` or ``bounding_circle=[(x0, y0), r]``)

Generally, a tuple is better for a set number of things, and lists for a mutable number of things. (See also #3738 (comment).)

Here, we're taking three and only three values, so should we use a 3-tuple?

We generally do the same elsewhere. For example, ImageDraw.polygon:

    :param xy: Sequence of either 2-tuples like ``[(x, y), (x, y), ...]`` or
               numeric values like ``[x, y, x, y, ...]``.

So there's a mutable list of immutable 2-tuple x, y coords. (Or a flat, mutable list of single coords.)

Whether we actively disallow a list is another question, but I think at least show tuples in the docs.

(I think Image.new can take both tuple and list for the w, h but some other ImageDraw things only take tuples for the non-flat x, y sequences.)

What do you think?

nulano · 2020-08-21T14:41:42Z

For ImageDraw.rectangle it is:

    :param xy: Two points to define the bounding box. Sequence of either
            ``[(x0, y0), (x1, y1)]`` or ``[x0, y0, x1, y1]``. The second point
            is just outside the drawn rectangle.

Other ImageDraw functions are similar. This PR adds an ImageDraw function, so I think it should have the same semantics, i.e. accept a sequence of a point and a radius (or a flat sequence of the coords and a radius.

What do you think about the following suggestion (i.e. swapping the two examples)?

    :param bounding_circle: A point and a radius to define the bounding circle.
        Sequence of either ``[(x, y), r]`` or ``[x, y, r]``. The polygon is drawn
        inscribed in this circle.

hugovk · 2020-08-21T15:04:03Z

For ImageDraw.rectangle it is:

    :param xy: Two points to define the bounding box. Sequence of either
            ``[(x0, y0), (x1, y1)]`` or ``[x0, y0, x1, y1]``. The second point
            is just outside the drawn rectangle.

rectangle's xy can be:

[(x0, y0), (x1, y1)] or [x0, y0, x1, y1]

So can be:

[(x0, y0), (x1, y1)]
[(x0, y0), (x1, y1), (x2, y2)]
[(x0, y0), (x1, y1), (x2, y2), more stuff here...]
[x0, y0, x1, y1]
[x0, y0, x1, y1, x2, y2]
[x0, y0, x1, y1, x2, y2, more stuff here...]

What do you think about the following suggestion (i.e. swapping the two examples)?
    :param bounding_circle: A point and a radius to define the bounding circle.
        Sequence of either ``[(x, y), r]`` or ``[x, y, r]``. The polygon is drawn
        inscribed in this circle.
Other ImageDraw functions are similar. This PR adds an ImageDraw function, so I think it should have the same semantics, i.e. accept a sequence of a point and a radius (or a flat sequence of the coords and a radius.

These lists cannot be meaningfully extended:

[(x, y), r]
[(x, y), r, more stuff here...]
[x, y, r]
[x, y, r, more stuff here...]

nulano · 2020-08-21T15:17:50Z

Rectangle can only accept two points:

Pillow/src/_imaging.c

Lines 3217 to 3221 in 8deaebd

    
           if (n != 2) { 
        
               PyErr_SetString(PyExc_TypeError, must_be_two_coordinates); 
        
               free(xy); 
        
               return NULL; 
        
           }

But I think I see what you mean, a circle is never a subsequence of another (longer) list.

So perhaps go with the following and silently accept a list as well?

    :param bounding_circle: A point and a radius to define the bounding circle.
        Tuple of either ``(x, y, r)`` or ``((x, y), r)``. The polygon is drawn
        inscribed in this circle.

comhar · 2020-08-21T16:44:28Z

So perhaps go with the following and silently accept a list as well?

    :param bounding_circle: A point and a radius to define the bounding circle.
        Tuple of either ``(x, y, r)`` or ``((x, y), r)``. The polygon is drawn
        inscribed in this circle.

I like this approach. A tuple is a better fit for the bounding_circle. I've updated the docs, tests so that all examples and error messages refer to tuples. However, lists are still silently accepted.

hugovk · 2020-08-21T17:38:43Z

Thanks! Though I think ((x, y), r) is a bit of an unnecessary complication and (x, y, r) is enough.

comhar · 2020-08-22T16:14:20Z

@hugovk @nulano , what are your thoughts on the latest iteration? Are there any outstanding issues that still need to be resolved?

nulano

Just one small thing.

nulano · 2020-08-22T16:46:08Z

-    :param b_circle: A bounding circle which inscribes the polygon
-        (e.g. b_circle=[50, 50, 25]).
+    :param bounding_circle: A bounding circle is defined by a point and radius.
+        (e.g. ``bounding_circle=(x, y, r)`` or ``((x, y), r)``).


Suggested change

(e.g. ``bounding_circle=(x, y, r)`` or ``((x, y), r)``).

(e.g. ``bounding_circle=(x, y, r)`` or ``((x, y), r)``).

The polygon is inscribed in this circle.

Docs for ImageDraw.regular_polygon should probably also mention this, not just the _compute_regular_polygon_vertices function (which is not shown in the docs and can only be accessed with the __docs__ attribute).

Summary of changes - Rename `b_circle` and `bounding_circle` -`bounding_circle` now accepts both formats below: - (x0, y0, r) - ((x0, y0), r)

hugovk

Still not keen on ((x, y), r)), but I'll defer to others on that.

Otherwise 👍

comhar · 2020-08-23T17:19:37Z

Still not keen on ((x, y), r)), but I'll defer to others on that.

Otherwise 👍

Thanks @hugovk , I don't have a strong opinion on this. @nulano, what are your thoughts on removing the ((x, y), r)) formatting option?

nulano · 2020-08-24T18:02:48Z

To me a circle is (center, radius), not (x, y, r).

I can't find other examples online that take a circle parameter, but there doesn't seem to be a strong preference for either:

separate parameters xy and radius: https://matplotlib.org/3.1.1/api/_as_gen/matplotlib.patches.Circle.html
separate parameters r, c, radius (deprecated): https://scikit-image.org/docs/dev/api/skimage.draw.html#skimage.draw.circle
replaced with parameters center and radius: https://scikit-image.org/docs/dev/api/skimage.draw.html#skimage.draw.disk
a class with Point and int parameters: http://anh.cs.luc.edu/handsonPythonTutorial/graphics.html
a named tuple of x y r: https://books.google.sk/books?id=H9emM_LGFDEC&pg=PA234&lpg=PA234&dq=python+circle+tuple&source=bl&ots=ZbcRbPF8lR&sig=ACfU3U013mMTBOCH_OB7j75t6WB1Uhim0Q&hl=en&sa=X&ved=2ahUKEwjz1fOWsLTrAhUJyqQKHRGvDHUQ6AEwCHoECAEQAQ#v=onepage&q=python circle tuple&f=false

So it is not clear to me that one is better than the other.

If only (x, y, r) is supported and someone needs to draw polygons with centres stored in a list, they would have to either use the somewhat obscure

for point in points:
    draw.regular_polygon((*point, 5), ...)

or pollute the locals with

for x, y in points:
   draw.regular_polygon((x, y, 5), ...)

Therefore I think it would be nice to support

for point in points:
    draw.regular_polygon((point, 5), ...)

since it doesn't seem to add too much complexity to the function as far as I can tell.

hugovk · 2020-09-03T16:13:56Z

Okay, there's been no comments in a while, so if there's no further suggestions by tomorrow will merge!

hugovk · 2020-09-04T20:28:38Z

@comhar Thank you for your patience and for all your updates, and thank you @nulano for the reviews!

Merging now! Please could you also add release notes in https://github.com/python-pillow/Pillow/blob/master/docs/releasenotes/8.0.0.rst? This can be another PR :)

comhar · 2020-09-05T19:14:38Z

No problem @hugovk , here's the PR for the release notes.

radarhere added the Enhancement label Aug 10, 2020

Add ImageDraw.regular_polygon

0ed01dd

hugovk reviewed Aug 16, 2020

View reviewed changes

hugovk reviewed Aug 20, 2020

View reviewed changes

Comment thread docs/reference/ImageDraw.rst Outdated

hugovk reviewed Aug 20, 2020

View reviewed changes

Comment thread src/PIL/ImageDraw.py Outdated

hugovk reviewed Aug 20, 2020

View reviewed changes

nulano reviewed Aug 20, 2020

View reviewed changes

nulano approved these changes Aug 22, 2020

View reviewed changes

Rename b_circle and bounding_circle + accept ((x0, y0), r)

b142560

Summary of changes - Rename `b_circle` and `bounding_circle` -`bounding_circle` now accepts both formats below: - (x0, y0, r) - ((x0, y0), r)

hugovk approved these changes Aug 23, 2020

View reviewed changes

hugovk merged commit 3dba4ee into python-pillow:master Sep 4, 2020

radarhere changed the title ~~Compute polygon coordinates~~ Added regular_polygon draw method Sep 5, 2020

comhar mentioned this pull request Sep 5, 2020

Add ImageDraw.regular_polygon to release notes #4902

Merged

		:param fill: Color to use for the fill.


		.. py:method:: ImageDraw.regular_polygon(*, b_box, nb_sides, rotation=0, fill=None, outline=None)

	(e.g. b_circle = [x0, y0, r])
	(e.g. ``b_circle = [x0, y0, r]`` or ``b_circle = [(x0, y0), r]``)

	(e.g. ``bounding_circle=(x, y, r)`` or ``((x, y), r)``).
	(e.g. ``bounding_circle=(x, y, r)`` or ``((x, y), r)``).
	The polygon is inscribed in this circle.

Conversation

comhar commented Aug 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Usage

Draw a red hexagon, rotated by 90 degrees, inscribed by the following bounding box = [(50,50), (150, 150)]

Uh oh!

radarhere commented Aug 10, 2020

Uh oh!

nulano commented Aug 10, 2020

Uh oh!

comhar commented Aug 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

comhar commented Aug 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nulano commented Aug 16, 2020

Uh oh!

Uh oh!

hugovk Aug 16, 2020

Choose a reason for hiding this comment

Uh oh!

nulano Aug 16, 2020

Choose a reason for hiding this comment

Uh oh!

comhar commented Aug 16, 2020

Uh oh!

comhar commented Aug 19, 2020

Uh oh!

hugovk commented Aug 20, 2020

Uh oh!

hugovk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nulano commented Aug 20, 2020

Uh oh!

comhar commented Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

comhar commented Aug 20, 2020

Uh oh!

hugovk commented Aug 20, 2020

Uh oh!

hugovk Aug 20, 2020

Choose a reason for hiding this comment

Uh oh!

nulano left a comment

Choose a reason for hiding this comment

Uh oh!

nulano Aug 20, 2020

Choose a reason for hiding this comment

Uh oh!

nulano Aug 20, 2020

Choose a reason for hiding this comment

Uh oh!

nulano Aug 20, 2020

Choose a reason for hiding this comment

Uh oh!

hugovk commented Aug 21, 2020

Uh oh!

nulano commented Aug 21, 2020

Uh oh!

hugovk commented Aug 21, 2020

Uh oh!

nulano commented Aug 21, 2020

Uh oh!

comhar commented Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

comhar commented Aug 9, 2020 •

edited

Loading

comhar commented Aug 10, 2020 •

edited

Loading

comhar commented Aug 16, 2020 •

edited

Loading

comhar commented Aug 20, 2020 •

edited

Loading

comhar commented Aug 21, 2020 •

edited

Loading