Font advance width #4031

mdboom · 2015-01-23T19:47:50Z

This changes the text bounding box calculation to use advance width of the character, rather than the bounding box of the control points.

It also fixes a long-standing bug in kerning in that (a) the conversion from 26.6 fixed point to floating-point was incorrect, and (b) the sign was backwards.

This PR needs a couple of things to push it over the finish line:

Virtually every test with text in it now fails, but from the spot-checking I've done, they've improved for the better. I'd suggest updating every image with any text, even if the difference isn't enough for the test to fail, just to reset the baseline correctly.
The Abel font @sgmentzer used in Align text using advance width, not glyph width #253 is a good one to catch this kind of error because the difference between advance width and control point bounding box is so large and because the kerning is so extreme. It might make sense to make a test or two based on it -- but then we have some technical issues to solve about how to get it. (It's under SIL Open Font License, so we can just incorporate it, but that might be overkill).

WeatherGod · 2015-01-23T20:12:15Z

While I am thinking of this, I should remember to double-check the results
from mplot3d, especially with offset text and other things that do
non-traditional anchoring. Most (all?) mplot3d test have text removed.
Also, the tests may not capture what one might see when interacting with
the 3d axes.

On Fri, Jan 23, 2015 at 2:47 PM, Michael Droettboom <
notifications@github.com> wrote:

Fix #253 #253.

This changes the text bounding box calculation to use advance width of the
character, rather than the bounding box of the control points.

It also fixes a long-standing bug in kerning in that (a) the conversion
from 26.6 fixed point to floating-point was incorrect, and (b) the sign was
backwards.

This PR needs a couple of things to push it over the finish line:

Virtually every test with text in it now fails, but from the
spot-checking I've done, they've improved for the better. I'd suggest
updating every image with any text, even if the difference isn't enough for
the test to fail, just to reset the baseline correctly.

The Abel font @sgmentzer https://github.com/sgmentzer used in #253
#253 is a good one to
catch this kind of error because the difference between advance width and
control point bounding box is so large and because the kerning is so
extreme. It might make sense to make a test or two based on it -- but then
we have some technical issues to solve about how to get it. (It's under SIL
Open Font License, so we can just incorporate it, but that might be
overkill).

You can view, comment on, or merge this pull request online at:

#4031
Commit Summary

Fix Align text using advance width, not glyph width #253. Use advance width to calculate bounding box.

Add CHANGELOG

File Changes

M CHANGELOG
https://github.com/matplotlib/matplotlib/pull/4031/files#diff-0 (3)

M lib/matplotlib/backends/backend_pdf.py
https://github.com/matplotlib/matplotlib/pull/4031/files#diff-1 (2)

M lib/matplotlib/backends/backend_ps.py
https://github.com/matplotlib/matplotlib/pull/4031/files#diff-2 (2)

M src/ft2font.cpp
https://github.com/matplotlib/matplotlib/pull/4031/files#diff-3 (16)

Patch Links:

https://github.com/matplotlib/matplotlib/pull/4031.patch

https://github.com/matplotlib/matplotlib/pull/4031.diff

—
Reply to this email directly or view it on GitHub
#4031.

mdboom · 2015-01-25T14:46:46Z

Note: There are some problems discovered with this approach. Working on a better solution now.

mdboom · 2015-01-26T18:30:12Z

All of the baseline images with text (all 7.7MB of them) have been updated so this passes. I'm not crazy about the increase in size in the repo, but I don't have any better ideas...

jenshnielsen · 2015-01-27T09:40:50Z

@mdboom The only thing that I can think of is to put the test images in a git submodule similar to what IPython does with it's javascript dependencies. Not great since it will involve another setup step.

tacaswell · 2015-01-27T14:29:13Z

That won't really help us with historical problem. The repo is currently almost 300MB, another 8 doesn't really matter.

tcaswell@eowyn:tmp$ git clone matplotlib
Cloning into 'matplotlib'...
remote: Counting objects: 101713, done.
remote: Compressing objects: 100% (29291/29291), done.
remote: Total 101713 (delta 72112), reused 101687 (delta 72095)
Receiving objects: 100% (101713/101713), 294.75 MiB | 9.67 MiB/s, done.
Resolving deltas: 100% (72112/72112), done.
Checking connectivity... done.

What we need is a way to hijack git and only pull the large objects when needed or get people to start doing shallow clones.

jenshnielsen · 2015-01-27T14:48:03Z

Yes moving the images to a submodule will of cause only stop the growth of the repository.

There are ways of removing files entirely from the history https://help.github.com/articles/remove-sensitive-data/ but I don't really thing that it is a good idea and I would be very scared about doing such a thing to the matplotlib repository.

tacaswell · 2015-01-27T15:01:06Z

That would change sha1s of every commit back to the first image we added which is a complete non-starter.

This is a feature of git, you can't tamper with the development history with out destroying all of it.

jenshnielsen · 2015-01-27T15:28:07Z

Yes I agree which was what I tried to say above. I don't think there is going to be a good way of only hijacking git to not pull large objects for the same reason.

I think the only thing that can be done is to advice on using shallow clones and try to figure out a solution to stop making it worse.

BTW I tried doing something similar to what Fernando did to the IPython repository here on a local copy but that only gives me a total save from 279 to 271 Mb

✔ /tmp/matplotlib [master|✔] du -d 1 -h
198M    ./.git
4.0K    ./.travis
6.9M    ./doc
2.1M    ./examples
3.9M    ./extern
 67M    ./lib
 36K    ./LICENSE
8.0K    ./release
712K    ./src
 24K    ./tools
168K    ./unit
279M    .
✔ /tmp/matplotlib [master|✔] du -d 1 -h
190M    ./.git
4.0K    ./.travis
6.9M    ./doc
2.1M    ./examples
3.9M    ./extern
 67M    ./lib
 36K    ./LICENSE
8.0K    ./release
712K    ./src
 24K    ./tools
168K    ./unit
271M    .
✔ /tmp/matplotlib [master|✔]

jenshnielsen · 2015-01-27T15:32:39Z

For some reason I only had to download ~195 MB Strange

✘-1 /tmp/test git clone https://github.com/matplotlib/matplotlib.git
Cloning into 'matplotlib'...
remote: Counting objects: 96987, done.
remote: Compressing objects: 100% (115/115), done.
remote: Total 96987 (delta 79), reused 32 (delta 15)
Receiving objects: 100% (96987/96987), 195.26 MiB | 1.75 MiB/s, done.
Resolving deltas: 100% (70659/70659), done.
Checking connectivity... done.

tacaswell · 2015-01-27T16:55:31Z

Oh, I know what the problem is, the mpl fork on my gh has a gh-pages branch, your number is ~~more~~ accurate.

mdboom · 2015-01-27T17:01:49Z

Ok -- well let's not worry about repo size for this PR, then, but address it in some way in the future.

I think this PR is ready to merge, then.

tacaswell · 2015-01-27T17:07:52Z

lib/matplotlib/backends/backend_ps.py

@@ -802,7 +802,7 @@ def draw_text(self, gc, x, y, s, prop, angle, ismath=False, mtext=None):
                else:
                    kern = 0
                lastgind = gind
-                thisx += kern/64.0
+                thisx -= kern/64.0


why did the sign of the kern adjustment change? This no longer matches the sign in the pdf backend...

I'm looking into this further... I may have been mistaken about this change... (But in any event the sign does match because the analogous change was made in backend_pdf.py)

This also may be an issue of me reading badly as this looked fine before the pep8 fixes

tacaswell · 2015-01-27T17:33:39Z

For reference to see just the code changes: mdboom/matplotlib@matplotlib:master...b52589b

tacaswell · 2015-01-27T17:37:11Z

@mdboom Can you also document which FreeType and font version you used for this so that we can (eventually) pin that on travis?

mdboom · 2015-01-27T19:19:03Z

Thanks to @tacaswell's urging, it turns out the kerning wasn't as broken as I thought. I have a new approach that I hope is now correct, and I've rewritten the history so the updated test images are only in here once. I'll leave a few inline comments to @tacaswell's earlier comments.

mdboom · 2015-01-27T20:02:47Z

These images were generated with freetype 2.5.3-21 on Fedora 21. The fonts used are all the ones that ship with matplotlib.

WeatherGod · 2015-01-31T16:32:50Z

I have manually checked for any negative effects in mplot3d. I didn't see any regressions.

API : Font advance width This resets _all_ of the images with text.

tacaswell · 2015-02-19T01:54:02Z

Merged this before we accumulate too many more test images which will need to be reset (I think we already have one or two).

Fall out from matplotlib#4031

mdboom · 2015-02-19T13:18:57Z

I see master is passing on Travis. I assume that means all of our images are currently good, then?

tacaswell · 2015-02-19T13:20:10Z

I fixed one last night after I merged this

On Thu, Feb 19, 2015, 08:19 Michael Droettboom notifications@github.com
wrote:

I see master is passing on Travis. I assume that means all of our images
are currently good, then?

—
Reply to this email directly or view it on GitHub
#4031 (comment)
.

Document what version of freetype was used to generate baseline images in PR matplotlib#4031 / commit 005cfde and which version is (currently) being run on travis.ci

mdboom added status: needs review topic: text labels Jan 23, 2015

mdboom self-assigned this Jan 23, 2015

mdboom added this to the v1.5.x milestone Jan 23, 2015

tacaswell reviewed Jan 27, 2015
View reviewed changes

Fix matplotlib#253: Use advance width to calculate text bounding box

c6d0183

mdboom force-pushed the font-advance-width branch from 14a8da6 to d5284f5 Compare January 27, 2015 19:17

Update baseline images with text

3afdaf0

mdboom force-pushed the font-advance-width branch from d5284f5 to 3afdaf0 Compare January 27, 2015 19:57

Update docstring

f8dc710

tacaswell modified the milestones: proposed next point release, next point release Feb 19, 2015

tacaswell added a commit that referenced this pull request Feb 19, 2015

Merge pull request #4031 from mdboom/font-advance-width

005cfde

API : Font advance width This resets _all_ of the images with text.

tacaswell merged commit 005cfde into matplotlib:master Feb 19, 2015

tacaswell removed the status: needs review label Feb 19, 2015

tacaswell mentioned this pull request Feb 19, 2015

Add better test for text aligment issues #4127

Closed

tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Feb 19, 2015

MNT : fix text-based text with new advance-width

4dc9824

Fall out from matplotlib#4031

tacaswell mentioned this pull request Feb 19, 2015

MNT : fix text-based text with new advance-width #4129

Merged

mdboom deleted the font-advance-width branch March 3, 2015 18:43

tacaswell mentioned this pull request Apr 16, 2015

Implementation of Issue #3418 - Auto-wrapping text #4342

Merged

tacaswell mentioned this pull request Apr 16, 2015

DOC : document exact freetype versions for tests #4345

Merged

astrofrog mentioned this pull request Feb 2, 2016

Update baseline images for Matplotlib 1.5.x astrofrog/wcsaxes#188

Closed

Uh oh!

Font advance width #4031

Font advance width #4031

Uh oh!

Conversation

mdboom commented Jan 23, 2015

Uh oh!

WeatherGod commented Jan 23, 2015

Virtually every test with text in it now fails, but from the spot-checking I've done, they've improved for the better. I'd suggest updating every image with any text, even if the difference isn't enough for the test to fail, just to reset the baseline correctly.

Uh oh!

mdboom commented Jan 25, 2015

Uh oh!

mdboom commented Jan 26, 2015

Uh oh!

jenshnielsen commented Jan 27, 2015

Uh oh!

tacaswell commented Jan 27, 2015

Uh oh!

jenshnielsen commented Jan 27, 2015

Uh oh!

tacaswell commented Jan 27, 2015

Uh oh!

jenshnielsen commented Jan 27, 2015

Uh oh!

jenshnielsen commented Jan 27, 2015

Uh oh!

tacaswell commented Jan 27, 2015

Uh oh!

mdboom commented Jan 27, 2015

Uh oh!

tacaswell Jan 27, 2015

Choose a reason for hiding this comment

Uh oh!

mdboom Jan 27, 2015

Choose a reason for hiding this comment

Uh oh!

tacaswell Jan 27, 2015

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Jan 27, 2015

Uh oh!

tacaswell commented Jan 27, 2015

Uh oh!

mdboom commented Jan 27, 2015

Uh oh!

mdboom commented Jan 27, 2015

Uh oh!

WeatherGod commented Jan 31, 2015

Uh oh!

tacaswell commented Feb 19, 2015

Uh oh!

mdboom commented Feb 19, 2015

Uh oh!

tacaswell commented Feb 19, 2015

Uh oh!

Uh oh!

Virtually every test with text in it now fails, but from the
spot-checking I've done, they've improved for the better. I'd suggest
updating every image with any text, even if the difference isn't enough for
the test to fail, just to reset the baseline correctly.