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

Raise an error if ImageDraw co-ordinates are incorrectly ordered #6978

Merged
merged 3 commits into from
Mar 1, 2023

Conversation

radarhere
Copy link
Member

Alternative to #6956

For ImageDraw methods that take (x0, y0, x1, y1), raise an error if x1 < x0 or y0 < y1.

I've also added this requirement to the docs for rectangle() and rounded_rectangle().

@hugovk
Copy link
Member

hugovk commented Feb 28, 2023

Shall we use ValueError instead of TypeError?

Passing arguments of the wrong type (e.g. passing a list when an int is expected) should result in a TypeError, but passing arguments with the wrong value (e.g. a number outside expected boundaries) should result in a ValueError.

https://docs.python.org/3/library/exceptions.html#TypeError

We're getting a tuple as expected, but of the wrong value order.

@radarhere
Copy link
Member Author

Sure, I've updated my last commit to use ValueError instead.

src/PIL/ImageDraw.py Outdated Show resolved Hide resolved
@hugovk hugovk added the automerge Automatically merge PRs that are ready label Mar 1, 2023
@mergify mergify bot merged commit 1c24fa5 into python-pillow:main Mar 1, 2023
@radarhere radarhere deleted the coordinates branch March 1, 2023 12:02
Schamschula referenced this pull request in macports/macports-ports Apr 16, 2023
@wlritchi
Copy link

I think choosing this approach over just ordering the points isn't a great idea - or at least, not without a major version bump. There are plenty of applications using Pillow that rely on the old behaviour, and the fact that they don't sort their points doesn't necessarily imply that they've got a typo or a bug in their calculations. Maybe sorting would be the better option, with the ordering requirement scheduled for 10.x?

@radarhere
Copy link
Member Author

the fact that they don't sort their points doesn't necessarily imply that they've got a typo or a bug in their calculations

My main thinking behind this change was trying to help if they do have a bug.

I don't know exactly which method is a problem for you, but we've had notes in the ImageDraw documentation that x1 >= x0 and y1 >= y0 since 5.2.0 (#3080) for arc(), chord(), ellipse() and pieslice(). For the general case, we've had notes at https://pillow.readthedocs.io/en/latest/handbook/concepts.html#coordinate-system that

Rectangles are represented as 4-tuples, with the upper left corner given first.

since 2.3.0, 9 years ago. So I don't think this is a sudden change.

If many users say that this is a problem, sure, we can revisit it, but otherwise, I'm reluctant to rollback decisions that we've made.

@wlritchi
Copy link

Alright, fair enough - 9 years seems like plenty of notice to me. I guess Hyrum's Law applies here.

I'll mention for context: the application where I encountered the problem is Overviewer, a Minecraft map viewer with 3.2k stars that recently became unmaintained (hence no official fix and no update to require pillow<9.5.0). The offending line is

ImageDraw.Draw(torch).rectangle((0,24,10,15),outline=(0,0,0,0),fill=(0,0,0,0))

which I suspect doesn't hit any of the snags mentioned in the earlier PR because it lacks rounding, outline, or fill.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Automatically merge PRs that are ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants