Added regular_polygon draw method

[comhar]

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]

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]

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]

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]

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]

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]

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)

[nulano]

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)

[comhar]

@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]

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

[hugovk]

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]

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")

[comhar]

@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]

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

[hugovk]

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]

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]

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]

Suggested change

*centroid, polygon_radius = b_circle

[nulano]

Suggested change

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

[hugovk]

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]

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]

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]

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]

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]

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

[comhar]

@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]

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).

[hugovk]

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

Otherwise 👍

[comhar]

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]

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]

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

[hugovk]

@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]

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