Fix compatibility with Pillow 10 (Draw.textsize versus Draw.textbbox)

[avalentino]

This PR fixes one of the compatibility issues with Pillow 10.0.0.
It addresses the use of the removed method draw.textsize

Never the less, some tests still fail.

See also #107.

• Closes Incompatibility with Pillow 10 #107
• Tests added
• Tests passed
• Passes git diff origin/main **/*py | flake8 --diff

[djhoese]

Thanks for fixing this. We had just noticed this last week. The textbbox method was added in Pillow 8.0.0 which was released in October 2020. I think I'd be OK removing compatibility for older version by only using the bbox method and not the textsize at all.

For the remaining failures, any idea if those are caused by the changes/differences in this textbbox usage? Or are they some other issue?

[avalentino]

Thanks for fixing this. We had just noticed this last week. The textbbox method was added in Pillow 8.0.0 which was released in October 2020. I think I'd be OK removing compatibility for older version by only using the bbox method and not the textsize at all.

OK I will update the patch

For the remaining failures, any idea if those are caused by the changes/differences in this textbbox usage? Or are they some other issue?

Actually I have always had problems with those tests in debian, but with Pillow10 they start failing also in github CI.
IMHO it could be something related the font management, but I'm not 100% sure.

[avalentino]

Thanks for fixing this. We had just noticed this last week. The textbbox method was added in Pillow 8.0.0 which was released in October 2020. I think I'd be OK removing compatibility for older version by only using the bbox method and not the textsize at all.

OK I will update the patch

Sorry, apparently some of the Draw objects around do not have the textbbox method and this sometimes results in a segfault.

[djhoese]

Sorry, apparently some of the Draw objects around do not have the textbbox method and this sometimes results in a segfault.

Ah I suppose this means the aggdraw version of Draw needs to have the bbox and textlength methods added to it to reflect the changes in pillow.

[mraspaud]

Crazy idea: should we integrate aggdraw into pillow?

[djhoese]

We'd have to talk to the PIL maintainers. Right now there is a third-part developer who is rewriting aggdraw to be proper classes and fix various backwards incompatibilities. I think given the licensing complexities of including the C++ agg library I can't imagine pillow being open to the idea.

[djhoese]

Let's restart CI and see what happens.

[djhoese]

@avalentino if we were seg faulting before it doesn't look like that's happening, but it doesn't look like the results are equivalent either.

[avalentino]

Thanks @djhoese for looking into it.
Yes tests do not pass, but when I wrote the patch I checked the images and they were apparently fine.
I assumed that that there was some slightly different behaviour in Pillow 10 and a regeneration of the reference images was needed.
But I let to you to judge.

[djhoese]

Any chance you have screenshots from before? Oh wait, you're saying they looked fine before so something else must have changed.

[avalentino]

No sorry I do not have screenshots or faved files.

[djhoese]

After breaking my compilers in my environment and trying to install aggdraw from source, I was able to reproduce the failures locally. I'm out of time for now, but I'll try to look at this tonight.

[djhoese]

It looks like the lower "10E" disappears in the new version:

[djhoese]

Ok so the main issue seems to be that the text height with the new textbbox was producing a negative height. That is, in the sense that bottom was larger than top so top - bottom was negative. If I do abs around these then I at least get the lower label "10E" again, but now it is too low. I looked at the old API docs and saw this:

https://github.com/python-pillow/Pillow/blob/204590600c6a8c5246d0564d3312abd6e6b0ed70/src/PIL/ImageFont.py#L435-L438

So it says to please use anchor="lt", but I take that as what I should do, but not what preserves past behavior. So I tried "la" and "lb" just to see if there was a difference (based on https://pillow.readthedocs.io/en/stable/handbook/text-anchors.html#text-anchors) but I'm not seeing a difference in the result so I need to figure that out tomorrow.

[djhoese]

Hm I was able to get the "germ" test to pass with the abs change and a hardcoded 3 added to the height. From what I can tell an offset is/was added to textsize to deal with other C changes:

python-pillow/Pillow#4910 (comment)

But I can't find a way to get that offset.

[djhoese]

Ok I am able to get the offset and do this calculation to reproduce the old results, but it requires using what I think is a deprecated and/or hidden API by accessing font.font.getsize(...). The new images have the bottom "10E" almost at the bottom of the image though which to me is not as good of an image.

I think this is one of those cases of Pillow fixing a result that had been that way for a long time. @mraspaud @avalentino opinions on this?

[djhoese]

I chose to replace the expected images. It seems like the best solution.

[djhoese]

I think there are problems with Python 3.8 pyproj on conda-forge. I'm just dropping 3.7 and 3.8 python support for pycoast and bumping the CI environments accordingly.

[coveralls]

coverage: 95.327% (-0.2%) from 95.535%
when pulling d36ab30 on avalentino:feature/pillow-10
into 00c32cc on pytroll:main.

[djhoese]

@mraspaud I kind of want to also include this:

Subject: [PATCH] Replace old string formatting with f-strings
---
Index: pycoast/cw_base.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/pycoast/cw_base.py b/pycoast/cw_base.py
--- a/pycoast/cw_base.py	(revision ab950195f45375fc0f95aa879db089e81246b81a)
+++ b/pycoast/cw_base.py	(date 1699042204841)
@@ -1695,19 +1695,14 @@
             yield from index_arrays
 
     def _grid_lon_label(self, lon):
-        # FIXME: Use f-strings or just pass the direction
-        if lon > 0.0:
-            txt = "%.2dE" % (lon)
-        else:
-            txt = "%.2dW" % (-lon)
-        return txt
+        direction = "E" if lon > 0.0 else "W"
+        lon = int(abs(round(lon)))
+        return f"{lon:02d}{direction}"
 
     def _grid_lat_label(self, lat):
-        if lat >= 0.0:
-            txt = "%.2dN" % (lat)
-        else:
-            txt = "%.2dS" % (-lat)
-        return txt
+        direction = "N" if lat >= 0.0 else "S"
+        lat = int(abs(round(lat)))
+        return f"{lat:02d}{direction}"
 
     def _draw_grid_labels(self, draw, xys, placement_def, txt, font, **kwargs):
         """Draw text with default PIL module."""

As far as I can tell the old "%.2d" formatting rounds floats to the nearest integer and always shows 2 digits. So a latitude of "1" becomes "01" which sounds like an odd way of formatting these labels, but I live in Wisconsin so I've never had latitude or longitudes that were single digit in my images.

[mraspaud]

Looks good code-wise, but the lower lon labels are kind of bothering. I think we should fix it.

[mraspaud]

I think the lower longitude labels are problematic here as part of the text disappears...

[mraspaud]

Is it a matter of changing the vertical aligment of the text?

[djhoese]

Oh yeah it is especially bad in this one. Good catch. I'll see if I can play with the alignment. When I tried for the original/old images it wasn't producing the same result and I didn't notice any movement in the test image labels. In this new images it looks like it is basing label location based on the center (vertical middle) or something.

[mraspaud]

Nice, very useful!

[djhoese]

@mraspaud @pnuu and anyone else, I think I've found a bug that has existed since the lon/lat labeling was added 11 years ago. Check out:

pycoast/pycoast/__init__.py

Lines 61 to 64 in 9fe2dcf

	if ay == 'b':
	y_pos = y_pos - txt_height
	elif ay == 'c':
	y_pos = y_pos - txt_width / 2

Shouldn't the ay == "c" case be using txt_height / 2 not txt_width / 2? Changing this to txt_height / 2 puts all latitude labels on the center of the lines of latitude instead of just above them.

So besides me wanting to fix that, I found that the above conversation with @mraspaud about the "grid_from_dict_pil.png" test result change can be resolved by multiplying the txt_height by 1.25 for the font used during testing. With this change all old versions of the test files pass. See conversation and image comparison here (hopefully the link works but otherwise go to the "Files" and look for the conversation):

https://github.com/pytroll/pycoast/pull/108/files#diff-4441bdecdee683c71dc7be87ee2c8b213c03f08689643e9e9c4c8057bf610d69

But that may only work for the particular font being used in the tests. Any other font may have other ratios/metrics. From what I can tell the main difference/change in Pillow was that text drawing is now done from the "top" anchor instead of the "ascender" anchor (I think):

https://pillow.readthedocs.io/en/stable/reference/ImageDraw.html#PIL.ImageDraw.ImageDraw.text

https://pillow.readthedocs.io/en/stable/handbook/text-anchors.html#text-anchors

I think text size was also changed similarly, but there may have also been an offset removed on the Y size. Also note that textbbox which is now used to get the width and height of the text seems to ignore anchor for non-multiline strings as far as I can tell.

I'll see what I can do to produce nearly the same results regarding the bottom labels, but so far it isn't really making sense.

[mraspaud]

Shouldn't the ay == "c" case be using txt_height / 2 not txt_width / 2? Changing this to txt_height / 2 puts all latitude labels on the center of the lines of latitude instead of just above them.

Smells like a bug indeed.

[djhoese]

I figured it out! Even though the documentation says that textbbox and text default to an anchor of "lt" (left-top) it turns out that is only for vertical text. For "normal" horizontal text it is actually "la" (left-ascender). I was putting together an example for a pillow bug report when I noticed the difference:

'lt' anchor bbox:  (400, 400, 426, 428)
'la' anchor bbox:  (400, 411, 426, 439)
Default anchor bbox:  (400, 411, 426, 439)

So even though I was wrong about anchor not being recognized for single line text, this shows the (x1, y1, x2, y2) bounding box of the text. This is for the same position of (400, 400) but we can see that the "la" case has the bbox starting at 411 in the Y axis. So while you'd get the same height, it ignores the 11 pixels that the text is shifted due to the anchor being the "ascender".

The fix is to make the txt_height be bottom - position[1] instead of bottom - top. I have to go offline now, but will make commits for both of these bugs and update the images with the new vertical positions of the latitude lines. There shouldn't be an update due to the longitude lines at the bottom any more.

[djhoese]

python-pillow/Pillow#7586

[djhoese]

@mraspaud This should be ready now. I'll re-review the test changes, but it should be good to go. This fixes both pillow 10 compatibility and the long standing (since it was added) bug for "vertically centered" lon/lat labels. They now appear on the line instead of just above it.

[djhoese]

There are a couple of the new test images where I don't like the new label placement, but I think these are special cases and seem to be limited to projections/areas with lon/lat lines being at extreme angles.

I think this is good to go.

[mraspaud]

Looks good to me! Thanks for taking the time, looks great now.