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

Default drawing behaviour different after merging agg2.4 #61

Closed
a-hurst opened this issue Aug 14, 2019 · 14 comments · Fixed by #62
Closed

Default drawing behaviour different after merging agg2.4 #61

a-hurst opened this issue Aug 14, 2019 · 14 comments · Fixed by #62

Comments

@a-hurst
Copy link
Contributor

a-hurst commented Aug 14, 2019

As I brought up in #50, the default line-end and line-cap arguments are different after merging the agg 2.4 update, unexpectedly changing the way various aggdraw-generated shapes look. Here are two screenshots from the same aggdraw-rendered experiment program, the first pre-2.4 and the second post-2.4:

Screen Shot 2019-08-13 at 7 21 10 PM

Screen Shot 2019-08-13 at 7 19 48 PM

If you download both the images and switch back and forth between them with Quick Look or something (ignoring the missing left line, that's just trial-to-trial variation), you can see that:

  1. The corners of the boxes are now rounded, whereas before they were completely square.
  2. The line in the middle of the box (created with the line() method of the aggdraw.Draw(), not the .rectangle() method) is slightly longer for some reason.
  3. The asterisk in the middle is slightly thicker.

I'm able to revert the drawing behaviour for both 1 and 2 to the pre-2.4 defaults by adding linejoin = 0 and linecap = 0 to the arguments when creating the Pen object, but this breaks backwards compatibility with old aggdraw versions, which lack these arguments. My personal preference would be to revert the defaults here to the old defaults before the next release, since a lot of the use of aggdraw in my field (cognitive science research) is in older Python experiments for generating stimuli, and people trying to revive/replicate one of those experiments (largely one-off scripts with no real maintainer) are highly unlikely to realize the shapes look different than they did 5-10 years ago and add the new arguments accordingly.

The thicker asterisk remains unchanged with the above overrides, however, which appears to be a regression because the thickness/size of the asterisk is exactly as specified pre-2.4 but appears to be an extra 1 pixel thicker/larger post-2.4. I'll see if I can figure out a minimal example that showcases this bug clearly, since what I have here is all wrapped in my own drawing/rendering code.

EDIT: To revert to the old defaults in aggdraw itself, I believe all that's required is replacing the agg::round_join with agg::miter_join and agg::round_cap with agg::butt_cap in this section here:

aggdraw/aggdraw.cxx

Lines 1823 to 1833 in e9d7bcb

static PyObject*
pen_new(PyObject* self_, PyObject* args, PyObject* kw)
{
PenObject* self;
PyObject* color;
float width = 1.0;
agg::line_join_e line_join = agg::round_join;
agg::line_cap_e line_cap = agg::round_cap;
float miter_limit = 4.0; // Like default in agg_math_stroke.h
int opacity = 255;

@djhoese
Copy link
Member

djhoese commented Aug 14, 2019

I'm ok setting the defaults to maintain behavior. Is it possible for you to provide the code that created these images?

@a-hurst
Copy link
Contributor Author

a-hurst commented Aug 14, 2019

Great to hear! As for the code, you can find it here but I'm not sure it would do you much good as it uses aggdraw somewhat indirectly: it's meant for use with the klibs experiment runtime environment, which draws shapes using aggdraw internally and then draws them to the screen using OpenGL (this is my primary use case for aggdraw). I can probably come up with a more direct reproducible example with just aggdraw and Pillow's .show().

@djhoese
Copy link
Member

djhoese commented Aug 14, 2019

Oh interesting. FYI I'm the maintainer of vispy (python wrapper around OpenGL). A more direct example would be great and could even be used to verify this type of appearance between versions.

@a-hurst
Copy link
Contributor Author

a-hurst commented Aug 14, 2019

Okay, here we go, this should be sufficient to reproduce the differences above:

from PIL import Image
from aggdraw import Draw, Pen, Brush
from math import sin, cos, radians


# Functions needed for drawing the asterisk

def rotate_points(points, origin, angle, clockwise=True):
	rad_angle = radians((angle if clockwise else -angle) % 360)
	rotated = []
	for i in range(0, len(points), 2):
		dx = points[i] - origin[0]
		dy = points[i+1] - origin[1]
		rx = origin[0] + cos(rad_angle) * dx - sin(rad_angle) * dy
		ry = origin[1] + sin(rad_angle) * dx + cos(rad_angle) * dy
		rotated += [round(rx, 12), round(ry, 12)]
	return rotated

def translate_points(points, delta):
	translated = []
	for i in range(0, len(points), 2):
		dx = points[i] + delta[0]
		dy = points[i+1] + delta[1]
		translated += [dx, dy]
	return translated


# Actual drawing starts here

size = [1024, 600]
col = (0, 0, 0) # shape colour

# Initialize canvas and pen/brush objects
img = Image.new("RGBA", size, (col[0], col[1], col[2], 0))
canvas = Draw(img) # aggdraw surface
canvas.setantialias(True)
stroke = Pen(col, 16, 255)
fill = Brush(col, 255)

# Draw rectangle & line
canvas.rectangle([56, 100, 456, 500], stroke)
canvas.line([100, 300, 412, 300], stroke)

# Draw asterisk
sz = 70
thickness = 13
ht = thickness / 2.0 # half of the asterisk's thickness
hs = sz / 2.0 # half of the asterisk's size
pts = []
for s in range(0, 6):
	spoke = [-ht, -ht, -ht, -hs, ht, -hs, ht, -ht]
	pts += rotate_points(spoke, (0, 0), s*(360.0/6))
pts = translate_points(pts, delta=(768, 300))
canvas.polygon(pts, None, fill)

# Render and show image
canvas.flush()
img.show()

With the old aggdraw, the above code produces this:

tmp1g8SCC

With the latest commit, it produces this:

tmpqQGMcQ

@djhoese
Copy link
Member

djhoese commented Nov 6, 2019

The linejoin and linecap defaults should now be fixed. However, I'm still seeing some major differences for some of my own projects. They still seem related to the ends/joins of lines.

@djhoese
Copy link
Member

djhoese commented Nov 6, 2019

Old:

test_issue_61_old

New:

test_issue_61_new

If you switch between them the "asterisk" is still slight different. The new one is slightly wider. That's what I'm seeing with our pycoast package too (CC @mraspaud).

@a-hurst
Copy link
Contributor Author

a-hurst commented Dec 24, 2019

@djhoese Think I just tracked this down! So the agg "contour" functions that the Brush object uses have a "width" attribute that makes shapes expand outwards by a certain amount (default is 1.0 pixel in both 2.2 and 2.4). However, either by fixing an old bug or introducing a new one, the width of 1.0 pixel in 2.2 is equivalent to a width of 0.5 pixels in 2.4, resulting in minor size differences in Brush-filled shapes. By setting m_width to 0.5 near the top of agg_vcgen_contour.cpp, the asterisk looks identical to old aggdraw in my A/B testing.

That said, is this actually desired/expected behaviour? I personally would not expect a shape I drew that's 70 pixels high to have an extra anti-aliased quarter-pixel on all sides. I guess that's a matter for a new issue, though.

@djhoese
Copy link
Member

djhoese commented Dec 27, 2019

Wow, you're amazing! Thanks for tracking this down. As for whether or not this is expected, I can't remember. Every time this comes up I think I know what I should expect, but am never confident.

Maybe @rougier could help us out. @rougier If you have an anti-aliased line/shape, should you expect it to have more than "width" pixels due to the anti-aliased calculations? I figured you'd either know the answer and know someone who knows. Thanks for any info.

@rougier
Copy link

rougier commented Dec 28, 2019

Yes and now. It depends if you consider the antialiased area (usually 1 or 1.5 pixels) to be part of the shape or not. For example, from what I rememeber, in vispy/glumpy, a line of 1 pixel ends up being 3 pixels wide with the core being 1 pixel full color and the shell is two pixels (1 pixel left, 1 pixel right). You could also decide to "share" the antialiased area between the core and the shell (let's say 0.5 pixel in the core and 0.5 pixel in the shell, ending with 2 pixels wide line). More at http://jcgt.org/published/0002/02/08/ (see figure 3).

@djhoese
Copy link
Member

djhoese commented Dec 28, 2019

Thanks @rougier. So the answer is it "depends", but this is most likely a backwards incompatible change in the antigrain/agg library.

For the record, I'd really like avoid patching the vendor agg library that we include in aggdraw. I'd also like to avoid some "magic" width manipulation. For example, if the user enters "70" pixels wide, I don't want aggdraw to say "oh this is actually an agg width of 68.5 to adjust for the new antialias border". Those things said, it seems we have a few choices:

  1. Leave as is and document as an upstream change that is out of our control. We can give users a workaround of "adjust your width by X% to reproduce old images" if that's even possible.
  2. Patch agg in aggdraw to reproduce the old values. We'd need to make sure to document this.
  3. Do the width "X%" adjustment inside aggdraw. This is some how the best and the worst solution at the same time.
  4. Submit a bug report to agg about this change and see what the current maintainers have to say and possible get this changed upstream. From what I've seen though the maintainers don't seem to have time for the project any more. This might be good regardless so others can see what @a-hurst has found.

I'm leaning towards 2, but with more official documentation about what we've changed.

I'd really like to avoid 1 because of how much of a difference this can have in various use cases. I have a package (pycoast) that uses aggdraw to draw coastlines on images of the Earth. Having every one of these lines have another 1.5 pixels (or whatever) makes the images look really bad. I should see if @a-hurst's change fixes what I've seen so far.

@djhoese
Copy link
Member

djhoese commented Dec 28, 2019

Example from the pycoast tests, expected:

grid_nh_agg

Actual (after 0.5 width change):

grid_nh_agg_test

If you go back and forth between these you'll see the coastlines are wider in the new agg.

@djhoese
Copy link
Member

djhoese commented Dec 28, 2019

I must be doing something wrong. Even with your test code @a-hurst with the square and asterisk and the width change, I don't see anything different. I changed m_width(1) to m_width(0.5).

Edit: Or maybe I do? It is so hard to tell.

@a-hurst
Copy link
Contributor Author

a-hurst commented Dec 28, 2019

Maybe try installing/running it in a virtualenv? The old version might be taking precedence somehow. Here's what the asterisk demo produces on my end (I also added a Brush-filled square, which shows the anti-alias padding more clearly):

Aggdraw 1.3.10 from PyPi (agg 2.2):

aggdraw_agg22

Aggdraw (git HEAD, m_width(1.0)):

aggdraw_agg24old

Aggdraw (git HEAD, m_width(0.5)):

aggdraw_agg24new

A/B testing with Quick Look on my end shows that m_width(0.5) produces the same size asterisk/rectangle as the old aggdraw, and that the m_width(1.0) Brush-filled shapes are noticeably bigger than in the other images.

That said, in testing this out I've noticed that this still doesn't make agg 2.4 output match 2.2 images exactly: there appear to be some very slight blending differences that make the anti-aliased regions a little different. It's extremely subtle though (I only noticed after running them through a pixel-by-pixel comparison tool).

@a-hurst
Copy link
Contributor Author

a-hurst commented Dec 28, 2019

As for how to patch, I think both options 2 and 3 sound reasonable! IIRC option 4 isn't possible, given that agg 2.5+ is GPL'd and thus incompatible with the aggdraw licence. Also re: option 3 there's already some overriding of default agg settings in the Brush/Pen drawing code, so adding a width override in aggdraw.cxx wouldn't be doing anything uniquely unclean.

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 a pull request may close this issue.

3 participants