Draw glyph

[behdad]

Fixes https://github.com/harfbuzz/uharfbuzz/issues/133 as well.

[behdad]

This changes the API but fixes up confusion in the previous implementation in how it used user_data instead of draw_data.

[justvanrossum]

This breaks FontGoggles:

Traceback for <Task finished name='Task-17' coro=<FGMainWindowController.loadFonts() done, defined at /Users/just/code/git/fontgoggles/Lib/fontgoggles/mac/mainWindow.py:482> exception=TypeError('DrawFuncs.set_move_to_func() takes exactly one argument (2 given)')> (most recent call last):
  File "/Users/just/code/git/fontgoggles/Lib/fontgoggles/mac/mainWindow.py", line 494, in loadFonts
    await asyncio.gather(*coros)
  File "/Users/just/code/git/fontgoggles/Lib/fontgoggles/mac/mainWindow.py", line 527, in _loadFont
    self.setFontItemText(fontItemInfo, fontItem)
  File "/Users/just/code/git/fontgoggles/Lib/fontgoggles/mac/mainWindow.py", line 658, in setFontItemText
    glyphs = font.getGlyphRunFromTextInfo(self.textInfo,
  File "/Users/just/code/git/fontgoggles/Lib/fontgoggles/font/baseFont.py", line 117, in getGlyphRunFromTextInfo
    run = self.getGlyphRun(segmentText,
  File "/Users/just/code/git/fontgoggles/Lib/fontgoggles/font/baseFont.py", line 141, in getGlyphRun
    for glyph, glyphDrawing in zip(glyphInfo, self.getGlyphDrawings(glyphNames, colorLayers)):
  File "/Users/just/code/git/fontgoggles/Lib/fontgoggles/font/baseFont.py", line 159, in getGlyphDrawings
    glyphDrawing = self._getGlyphDrawing(glyphName, colorLayers)
  File "/Users/just/code/git/fontgoggles/Lib/fontgoggles/font/otfFont.py", line 33, in _getGlyphDrawing
    outline = self._getGlyphOutline(glyphName)
  File "/Users/just/code/git/fontgoggles/Lib/fontgoggles/font/otfFont.py", line 14, in _getGlyphOutline
    return makePathFromGlyph(self.shaper.font, self.shaper.glyphMap[name])
  File "/Users/just/code/git/fontgoggles/Lib/fontgoggles/mac/makePathFromOutline.py", line 81, in makePathFromGlyph
    funcs.set_move_to_func(move_to_capsule, path_capsule)
TypeError: DrawFuncs.set_move_to_func() takes exactly one argument (2 given)

[justvanrossum]

(That makes me quite sad, as it was quite some work to get it to work, and to get it to work efficiently.)

[behdad]

(That makes me quite sad, as it was quite some work to get it to work, and to get it to work efficiently.)

It should be fixable. I just removed the userdata from the individual set..._funcs, and you only pass draw_data to draw_glyph. So in your code:

    funcs = DrawFuncs()
    funcs.set_move_to_func(move_to_capsule, path_capsule)
    funcs.set_line_to_func(line_to_capsule, path_capsule)
    funcs.set_cubic_to_func(cubic_to_capsule, path_capsule)
    funcs.set_close_path_func(close_path_capsule, path_capsule)

    funcs.get_glyph_shape(font, gid)

will become:

    funcs = DrawFuncs()
    funcs.set_move_to_func(move_to_capsule)
    funcs.set_line_to_func(line_to_capsule)
    funcs.set_cubic_to_func(cubic_to_capsule)
    funcs.set_close_path_func(close_path_capsule)

    font.draw_glyph(font, gid, funcs, path_capsule)

And in your move_to func etc, instead of using user_data, you use draw_data.

[behdad]

@justvanrossum I just patched FontGoggles to work with this PR: https://github.com/behdad/fontgoggles/commit/644f4f349aac698abdaaab028fbc6701da8f0b15

[behdad]

I can modify it to make FontGoggle continue to work with no modification.

[behdad]

I can modify it to make FontGoggle continue to work with no modification.

But it won't be as clean as it is now.

[behdad]

I can modify it to make FontGoggle continue to work with no modification.

Done. Please test.

[behdad]

I can modify it to make FontGoggle continue to work with no modification.

Done. Please test.

I tested this with FontGoggles and works with no modification. Still, the change I proposed would be a good one if this change lands.

Another optimization you can do there is to create the DrawFuncs only once and reuse it, which would be possible with the change I proposed.

[justvanrossum]

With your FG modification performance is more or less the same as before. With unchanged FG I can measure a slowdown of up to 50%.

[anthrotype]

(without having looked at this PR specific changes)

With your FG modification performance is more or less the same as before...

fontgoggles has control as to the exact version of uharfbuzz it ships with so in theory it could apply behdad's proposed changes at the same time as bumping uharfbuzz requirement, no?

[justvanrossum]

fontgoggles has control as to the exact version of uharfbuzz it ships with so in theory it could apply behdad's proposed changes at the same time as bumping uharfbuzz requirement, no?

Absolutely, and I am totally willing to accept the change. Sorry if I made it sound like I am not!

[behdad]

With your FG modification performance is more or less the same as before. With unchanged FG I can measure a slowdown of up to 50%.

The slowdown is definitely unexpected as I didn't change the PyCapsule codepath at all, and the other codepath I sped up only. And my FG modification does not justify any speedup either.

Can you please tell me how to measure performance there so I can investigate?

[justvanrossum]

This test script shows a small slow-down (without your FG patch): draw_performance.py.zip (Run from FG root dir)

This ad-hoc test funnily enough seems to show more slowdown (again, without your FG patch):

diff --git a/Tests/test_makeOutline.py b/Tests/test_makeOutline.py
index acdf07a..1481e50 100644
--- a/Tests/test_makeOutline.py
+++ b/Tests/test_makeOutline.py
@@ -37,3 +37,19 @@ async def test_getOutlinePath_singleOffCurve():
         (x, y), (w, h) = p.controlPointBounds()
         assert (x, y, w, h) == (0, 50, 0, 0)
         assert p.elementCount() == 4
+
+
+@pytest.mark.asyncio
+async def test_getOutlinePath_performance_hack():
+    font, ttfGlyphSet = _getFonts("IBMPlexSans-Regular.ttf")
+    await font.load(sys.stderr.write)
+    glyphNames = font.ttFont.getGlyphOrder()
+
+    import time
+    t = time.time()
+    outlines = []
+    for i in range(10):
+        for glyphName in glyphNames:
+            outlines.append(font._getGlyphOutline(glyphName))
+    elapsed = time.time() - t
+    assert 0, elapsed

(Run as pytest -k test_getOutlinePath_performance_hack)

With your FG patch applied I'm not sure if the differences I observe are significant.

(To be ultra clear, I mean this FG patch: https://github.com/behdad/fontgoggles/commit/644f4f349aac698abdaaab028fbc6701da8f0b15)

[anthrotype]

i wonder if it's to do with the additional warnings.warn() calls for the deprecated method

[justvanrossum]

i wonder if it's to do with the additional warnings.warn() calls for the deprecated method

That could also explain the difference in my observed timing between the standalone script and the test case.

[behdad]

This ad-hoc test funnily enough seems to show more slowdown (again, without your FG patch):

Humm. I cannot reproduce the difference. In both cases I get around 0.53s.

[behdad]

This ad-hoc test funnily enough seems to show more slowdown (again, without your FG patch):

Humm. I cannot reproduce the difference. In both cases I get around 0.53s.

Nevermind. Pytest is not testing the modified code. I don't know where it's pulling fontgoggles from. Even after pip install it still doesn't see my modifications. Let me figure out.

[behdad]

With your FG modification performance is more or less the same as before. With unchanged FG I can measure a slowdown of up to 50%.

Okay. I managed to reproduce the slowdown. Indeed, it's because of the deprecation warning. If I remove the warning performance is the same.

On the plus side, with my proposed two changes (to create DrawFuncs only once):

https://github.com/behdad/fontgoggles/commits/drawGlyph

There's measurable speedup (~20% or more).

[anthrotype]

If I remove the warning performance is the same.

maybe we can keep the warning but issue it only once instead of for each drawn glyph? We can record the state of whether we have already issued a warning inside the DrawFuncs object.

[behdad]

The API change here is that set_move_to_func etc when passed a Python callable, do not accept a user_data argument anymore. Such user_data now can either be passed to draw_glyph as a draw_state if it's the same for all callbacks. Or the client can use functools.partial to bind per-callable user_data to their callable before passing to us.

[behdad]

One deficiency of the uharfbuzz draw_glyph API, still in this change, is that it doesn't pass the hb_draw_state_t to the callbacks.

[khaledhosny]

Any objection to merge this?

[khaledhosny]

Any objection to merge this?

As the saying goes, السكوت علامة الرضا.