Draw arc, pieslice and ellipse silently fail with some coordinates

[JamesGKent]

if i try and use the arc, pieslice or ellipse methods of a Draw object with some sets of coordinates no error is raised but nothing is drawn either.
this occurs when one or both axis coordinates decreases between the first and second coord of the bounding box.
eg:

draw.arc((0,0,10,10), 0, 90, 'red') # works
draw.arc((10,0,0,10), 0, 90, 'red') # doesn't work
draw.arc((0,10,10,0), 0, 90, 'red') # doesn't work
draw.arc((10,10,0,0), 0, 90, 'red') # doesn't work

I'm using python3.4 and pillow 5.0.0

I have no issue with the API requiring the coordinates in the correct order, but I couldn't find any documentation stating that it was required, and obviously no error was raised either.
I think it needs documenting and maybe raise an error too?

[radarhere]

https://pillow.readthedocs.io/en/latest/reference/ImageDraw.html?highlight=imagedraw#PIL.ImageDraw.PIL.ImageDraw.ImageDraw.arc

Parameters: xy – Two points to define the bounding box. Sequence of [(x0, y0), (x1, y1)] or [x0, y0, x1, y1].

To check that you've seen this, and to mention it here as part of the discussion, you think this doesn't adequately explain the argument?

[JamesGKent]

it does but it could be a lot clearer, at a quick glance it could easily be interpreted as x and y of first coord and x and y of second, with no clear restrictions as to needing to in ascending order.

[radarhere]

Okay, let me try again. I found this? https://pillow.readthedocs.io/en/latest/handbook/concepts.html#coordinate-system

[hugovk]

Here's the same code, but a fuller example with bigger numbers and different colours. Only the white arc is drawn:

from PIL import Image, ImageDraw

im = Image.new("RGB", (120, 120))
draw = ImageDraw.Draw(im)

draw.arc((0, 0, 100, 100), 0, 90, 'white')  # works
draw.arc((100, 0, 0, 100), 0, 90, 'red')    # doesn't work
draw.arc((0, 100, 100, 0), 0, 90, 'green')  # doesn't work
draw.arc((100, 100, 0, 0), 0, 90, 'blue')   # doesn't work

im.show()

It doesn't matter if you do [(x0, y0), (x1, y1)] or [x0, y0, x1, y1], we get the same result with:

draw.arc(((0, 0), (100, 100)), 0, 90, 'white')  # works
draw.arc(((100, 0), (0, 100)), 0, 90, 'red')    # doesn't work
draw.arc(((0, 100), (100, 0)), 0, 90, 'green')  # doesn't work
draw.arc(((100, 100), (0, 0)), 0, 90, 'blue')   # doesn't work

Here are the four coordinates:

Opposite corners are used in different combinations, and I might expect different arcs, but it's the same square and I might expect some sort of arc drawn for each.

Arcs are drawn using this ellipse C function:

Pillow/src/libImaging/Draw.c

Lines 735 to 838 in 81c88b1

	static int
	ellipse(Imaging im, int x0, int y0, int x1, int y1,
	float start, float end, const void* ink_, int fill,
	int mode, int op)
	{
	float i;
	int n;
	int cx, cy;
	int w, h;
	int x = 0, y = 0;
	int lx = 0, ly = 0;
	int sx = 0, sy = 0;
	DRAW* draw;
	INT32 ink;
	w = x1 - x0;
	h = y1 - y0;
	if (w < 0 || h < 0)
	return 0;
	DRAWINIT();
	cx = (x0 + x1) / 2;
	cy = (y0 + y1) / 2;
	while (end < start)
	end += 360;
	if (end - start > 360) {
	/* no need to go in loops */
	end = start + 361;
	}
	if (mode != ARC && fill) {
	/* Build edge list */
	/* malloc check UNDONE, FLOAT? */
	Edge* e = calloc((end - start + 3), sizeof(Edge));
	if (!e) {
	ImagingError_MemoryError();
	return -1;
	}
	n = 0;
	for (i = start; i < end+1; i++) {
	if (i > end) {
	i = end;
	}
	x = FLOOR((cos(i*M_PI/180) * w/2) + cx + 0.5);
	y = FLOOR((sin(i*M_PI/180) * h/2) + cy + 0.5);
	if (i != start)
	add_edge(&e[n++], lx, ly, x, y);
	else
	sx = x, sy = y;
	lx = x, ly = y;
	}
	if (n > 0) {
	/* close and draw polygon */
	if (mode == PIESLICE) {
	if (x != cx || y != cy) {
	add_edge(&e[n++], x, y, cx, cy);
	add_edge(&e[n++], cx, cy, sx, sy);
	}
	} else {
	if (x != sx || y != sy)
	add_edge(&e[n++], x, y, sx, sy);
	}
	draw->polygon(im, n, e, ink, 0);
	}
	free(e);
	} else {
	for (i = start; i < end+1; i++) {
	if (i > end) {
	i = end;
	}
	x = FLOOR((cos(i*M_PI/180) * w/2) + cx + 0.5);
	y = FLOOR((sin(i*M_PI/180) * h/2) + cy + 0.5);
	if (i != start)
	draw->line(im, lx, ly, x, y, ink);
	else
	sx = x, sy = y;
	lx = x, ly = y;
	}
	if (i != start) {
	if (mode == PIESLICE) {
	if (x != cx || y != cy) {
	draw->line(im, x, y, cx, cy, ink);
	draw->line(im, cx, cy, sx, sy, ink);
	}
	} else if (mode == CHORD) {
	if (x != sx || y != sy)
	draw->line(im, x, y, sx, sy, ink);
	}
	}
	}
	return 0;
	}

Which is also used for ImageDraw.ellipse, chord and pieslice, which similarly only draw the white one.

And right at the top it calculates the width and height, returning 0 if they're negative, therefore assuming (x1, y1) is greater than (x0, y0).

Pillow/src/libImaging/Draw.c

Lines 750 to 753 in 81c88b1

	w = x1 - x0;
	h = y1 - y0;
	if (w < 0 || h < 0)
	return 0;

Commenting out that if gives us this:

We could "fix" it like this, but remember each arc is drawing from 0 to 90 degrees, so it's kind of twisting depending on how the "direction" the bounding box is defined. I think this could be get quite confusing, and might also break existing code that is written for the current design, which has been like this since at least PIL 1.1.1 (released in 2000).

So I propose we better document that the bounding box for these four methods is a "Sequence of [(x0, y0), (x1, y1)] or [x0, y0, x1, y1] where x1 >= x0 and y1 >= y0".

[hugovk]

Re-opening this so it can be closed by PR #3080 :)