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

Implement anchor for truetype text functions #4724

Closed
wants to merge 18 commits into from

Conversation

nulano
Copy link
Contributor

@nulano nulano commented Jun 23, 2020

I am splitting this PR into multiple parts. I have marked this as draft until all parts are finished and will close this afterwards.

  1. Refactor font_getsize and font_render (Refactor font_getsize and font_render #4910)
  2. Implement anchor for TrueType fonts (Implement anchor for TrueType fonts #4930)
  3. Add getlength and getbbox functions for TrueType fonts (Add getlength and getbbox functions for TrueType fonts #4959)

The following is the original text of this PR:


  • For ImageFont module: getsize is slow? #4651, Font spacing changes #3977, font.getsize() changes after installing libraqm #4483 (comment), Obtain Character-Level Bounding Boxes of Generated Text #3921, likely others.
    Add new function getlength and getbbox which compute the advance length and rendered bounding box of text respectively.

    I did not update getsize (and getoffset) and the related ImageDraw functions, as I don't think this is possible while maintaining backwards compatibility. The issue is that while getoffset is correct, getsize returns the width and vertical distance to the last drawn pixel (i.e. the bottom parameter of the bounding box). The horizontal is sometimes equal to the advance length (now in getlength), but breaks with e.g. slanted fonts or odd diacritics (see test_combine for double breve). This is because the bbox is always extended at least up to the advance length. The returned vertical distance is from the ascender line to the bottom-most pixel, which is mostly useless without looking at getoffset as well, which is confusing.

    getbbox is suggested by @wiredfool in Fix return value of FreeTypeFont.textsize() does not include font offsets #784 (comment), getlength is inspired by my thoughts in Font spacing changes #3977 (comment) where I also described the problems mentioned above.
    I will gladly improve the API if someone can give a good suggestion.

    • TODO ImageDraw.Draw.textsize docs should at least point to the new functions, or they should also be added to ImageDraw. I have added a note summarising the paragraph above to Draw.textsize.
    • TODO Multiline text equivalent to getbbox. I would like to get feedback for the above before adding a multiline version.

    `getbbox` is merely a better public api for the *private version* of `getsize`, while `getlength` is 3x faster for basic layout (2x for raqm with simple text) because it only performs the layout step and sums the advance, without the need to reload all glyphs (it would be nice to modify the layout to cache the loaded glyhs to speed up internal `getsize` as well). Testing with the following code, I get these times (Windows 10 laptop):
    layout_engine getsize (7.1.2 from PyPI) getsize (master) getsize (PR) getlength (PR)
    LAYOUT_BASIC 0.092 0.080 0.082 0.027
    LAYOUT_RAQM 0.107 0.108 0.108 0.053
    Code (click to expand)

      from timeit import repeat
      print(
          'getsize(L) (x100): ',
          min(repeat(
              'font.getsize("Hello world")',
              'from PIL import ImageFont; font = ImageFont.truetype("Tests/fonts/FreeMono.ttf", 20, layout_engine=ImageFont.LAYOUT_BASIC)',
              number=100, repeat=30
          )),
      )
      if hasattr(FreeTypeFont, 'getlength'):
          print(
              'getlength(L) (x100): ',
              min(repeat(
                  'font.getlength("Hello world")',
                  'from PIL import ImageFont; font = ImageFont.truetype("Tests/fonts/FreeMono.ttf", 20, layout_engine=ImageFont.LAYOUT_BASIC)',
                  number=100, repeat=30
              )),
          )



  • Remove the additional offset caused by stroking text.
    This was incompatible with the anchor implementation and I believe it to be a bug; I don't think users should need to offset text just to stroke a single word in the middle of a line.

  1. Remove kerning for basic layout, since it has been broken for years anyway.
  2. Add support for -kern as the only feature supported by basic layout.

(The problem is that kerning is scaled twice, see below.)

Pillow/src/_imagingft.c

Lines 578 to 585 in c9745f4

if (kerning && last_index && (*glyph_info)[i].index) {
FT_Vector delta;
if (FT_Get_Kerning(self->face, last_index, (*glyph_info)[i].index,
ft_kerning_default,&delta) == 0) {
(*glyph_info)[i-1].x_advance += PIXEL(delta.x);
(*glyph_info)[i-1].y_advance += PIXEL(delta.y);
}
}

@nulano nulano force-pushed the anchor branch 3 times, most recently from 4642275 to fcfc1e3 Compare July 22, 2020 22:18
@nulano
Copy link
Contributor Author

nulano commented Aug 30, 2020

Can I change something here to make this easier to review? I'm hoping this could be merged in time for 8.0.0. See also #3346 (comment).

@hugovk
Copy link
Member

hugovk commented Sep 1, 2020

Thanks for the PR, it's a big one! That also makes it harder to review. Would it be possible to split it into smaller PRs?

@nulano
Copy link
Contributor Author

nulano commented Sep 1, 2020

I think I can split this into three incremental steps (PR2 depends on PR1 changes etc.):

  1. Refactor and fix some text rendering issues (clipping, rounding issues, etc.)
  2. Implement anchor parameter
  3. Add getbbox and getlength

However, I don't think I can write tests for (1) specifically. Would it be OK to merge that under the assumption that the next PR in the sequence adds tests? For example, the test for #4553 makes heavy use of the anchor parameter, see here. I can also add this test to (1) with the anchor test cases commented out, but I think this will only partially test the (1) changes.

@hugovk
Copy link
Member

hugovk commented Sep 1, 2020

Yeah, that'll probably be okay. At least for refactoring, you don't always need new tests because generally the idea is not to change what is done, but how it's done. Thank you!

@nulano nulano marked this pull request as draft September 1, 2020 23:43
@nulano
Copy link
Contributor Author

nulano commented Sep 9, 2020

I have now rebased the first part in PR #4910. While writing the changes comment, I realized I had misread the FreeType documentation for FT_Set_Transform, so it took me a bit more time than expected to fix and test a different approach.

self.font.getlength(text, mode == "1", direction, features, language) / 64
)

def getbbox(
Copy link
Contributor

@gofr gofr Sep 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be pedantic to say this should be called getcbox, since this uses FreeType's FT_Glyph_Get_CBox, not FT_Outline_Get_BBox, if some future version of Pillow would ever add a separate function for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is to return the bounding box of the drawn text, i.e. the rectangle of pixels that may be modified by draw.text, not just the union of glyph CBoxes. In fact, this function doesn't even return the tightest possible bound on the glyph CBoxes, since it also includes the pen line (i.e. the origin and glyph advance points).

I think the reason for calling it getbbox is that this is what it is most often called in Pillow code (see https://pillow.readthedocs.io/en/latest/search.html?q=bbox). Another possible name would be getbounds to avoid confusion with the similarly named concept in FreeType? However, this doesn't seem to be used anywhere else in code yet (see https://pillow.readthedocs.io/en/latest/search.html?q=bound).

@nulano
Copy link
Contributor Author

nulano commented Sep 22, 2020

Part two, implementing the anchor parameter, is now rebased in #4930. I think part three can actually be split into two PRs for the two functions, once part two is merged.

@nulano
Copy link
Contributor Author

nulano commented Oct 9, 2020

Part three, the finale, is now rebased in #4959, also adding ImageDraw versions of the two functions.

@nulano
Copy link
Contributor Author

nulano commented Oct 12, 2020

All parts of this PR have now been merged, closing.

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 this pull request may close these issues.

Vertical text alignment ignores yOffset
4 participants