pytroll / aggdraw

Python package wrapping AGG2 drawing functionality
https://aggdraw.readthedocs.io/en/latest/
Other
98 stars 48 forks source link

Default drawing behaviour different after merging agg2.4 #61

Closed a-hurst closed 4 years ago

a-hurst commented 4 years ago

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:

https://github.com/pytroll/aggdraw/blob/e9d7bcb49faf688e29b3013bde6621bde82f110a/aggdraw.cxx#L1823-L1833

djhoese commented 4 years ago

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

a-hurst commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.