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

Order ImageDraw.rounded_rectangle() points #6956

Closed

Conversation

Yay295
Copy link
Contributor

@Yay295 Yay295 commented Feb 16, 2023

If the points given to ImageDraw.rounded_rectangle() aren't in the correct order, the rounded rectangle won't be drawn correctly. This change orders the coordinates before they're used.

Here's what the images from my new test look like without my change:
(10, 20, 190, 180) test (10, 20, 190, 180)
(10, 180, 190, 20) test (10, 180, 190, 20)
(190, 20, 10, 180) test (190, 20, 10, 180)
(190, 180, 10, 20) test (190, 180, 10, 20)

@radarhere
Copy link
Member

Your concern applies not just to rounded_rectangle(), but also to rectangle().

from PIL import Image, ImageDraw
xy = (10, 20, 190, 180)
im = Image.new("RGB", (200, 200))
draw = ImageDraw.Draw(im)
draw.rectangle(xy, fill="red", outline="green", width=5)

out

from PIL import Image, ImageDraw
xy = (190, 20, 10, 180)
im = Image.new("RGB", (200, 200))
draw = ImageDraw.Draw(im)
draw.rectangle(xy, fill="red", outline="green", width=5)

out

It also applies to pieslice(), and perhaps other methods.

But I'm unsure about automatically correcting user's mistakes like this. If we silently handle this bug, it might lead to other problems in the user's code when they don't realise that they have their co-ordinates the wrong way around.

@hugovk any thoughts?

@Yay295
Copy link
Contributor Author

Yay295 commented Feb 17, 2023

I though it looked like rectangle already had C code to handle this, but I hadn't tested it. Apparently it doesn't completely work. The hline functions do reorder their x coordinates, but the line functions don't.

if (x0 > x1) {
tmp = x0, x0 = x1, x1 = tmp;
}

I hadn't noticed pieslice(); it says it's the same as arc() though (with extra lines between the start/end and middle), which does specify "where x1 >= x0 and y1 >= y0". chord() is also the same as arc(), but with a line between the start and end. ellipse() also takes two points and specifies "where x1 >= x0 and y1 >= y0". Neither rounded_rectangle() nor rectangle() specify that the points have to be in a specific order. A lot of the drawing functions take a single point, and some take a list of points, so this doesn't affect them.

@hugovk
Copy link
Member

hugovk commented Feb 26, 2023

Thinking about what might cause points to be out of order:

  • user types in coordinates, got them in the wrong order. For this, it might be nice to fix it.

  • coordinates are data passed from somewhere else. This might indicate there's another problem in their code, and better not hide it (as @radarhere suggested).

I expect it's had this behaviour for a long time.

We could update the docs, and/or raise a ValueError for invalid input.

@radarhere
Copy link
Member

Ok, I've created PR #6978, to update the docs for rectangle() and rounded_rectangle(), and to raise an error if the co-ordinates are the wrong way around.

@radarhere
Copy link
Member

#6978 has been merged instead.

@radarhere radarhere closed this Mar 1, 2023
@Yay295 Yay295 deleted the rounded_rectangle_alt_points branch March 1, 2023 14:21
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.

None yet

3 participants