Skparagraph binding - Githubissues

HinTak commented 3 months ago

Binding skparagraph and adding text decoration. Fixes #225 and #224

@kyamagu this is operational in running https://github.com/HinTak/skia-python-examples/blob/main/shape_text.py

which is a python port of upstream's

https://github.com/google/skia/blob/main/example/external_client/src/shape_text.cpp

Transplanted to build standalone against libskia as a shared library in

https://github.com/HinTak/skia-c-examples/blob/main/shape_text.cpp

Important note: https://issues.skia.org/358587937 and its windows equivalent https://issues.skia.org/307357528

basically NONE of skparagraph is meant to be exposed outside of google's family of software, let alone stable API!

And other issues in https://github.com/HinTak/skia-building-fun/blob/main/ReportedIssues.md

HinTak commented 3 months ago

Missing a lot of documentations and tests, but operational - in running that particular example. Since this is google's own example, we can at least hope that if they break it, they'll update the example and points towards a modification/update of the example code too.

HinTak commented 3 months ago

The skia-python example gives output which are pixels identical to the c++ example, if you take into account of how they load the same font differently. I haven't figured out how to use skparagraph with an exact font file, but that's a limitation of my python knowledge about sub-class'ing, rather than this pull. We might be able to get around it by adding a python FontMgr.OneFontMgr(filename) class static method to do the equivalent of the c++ subclassing on this line : https://github.com/HinTak/skia-c-examples/blob/15b3b2b9381df682fb84b1605cc447af88d46c3f/shape_text.cpp#L63 to give python code the ability of the c++ example of using a specific font file.

HinTak commented 3 months ago

@kyamagu how do you feel about indentation, regarding the "class OneFontStyleSet and OneFontMgr" code copied from upstream? If you feel we should re-indent it to ours, I'll do that. I did a look around, trying to make a python class deriving from a c++ class and being able to pass it back to c++ which takes the derived class is hard, and needs some helper c++ code anyway. So I thought it is probably simpler just to offer that as a static method which returns the derived class "OneFontMgr". Usage as in the newly added tests.

HinTak commented 3 months ago

The example is called shaped_text, and is able to do multi-line paragraphs in Arabic correctly.

kyamagu commented 3 months ago

Looks good to me. No need to care too much about indentation if that complicates code migration from the upstream.

MeetWq commented 2 months ago

Hi, I'm trying to implement a text to image function using skia-python.

Here is a demo I wrote with rust-skia: https://gist.github.com/MeetWq/c56635fbf886c2c511c2ac1ec8e0aa48 test

It seems to be some features missing in the current skparagraph binding. The main ones are ParagraphBuilder's pushStyle and pop functions to realize multiple TextStyle in a Paragraph; and Paragraph's properties like width, height, longestLine, etc. Hope these can be added in the skparagraph binding.

HinTak commented 2 months ago

@MeetWq thanks for the interesting demo. Yes, the current skparagraph binding in skia-python is mainly geared towards it being sufficient to run google's own example - this is somewhat intentional, as none of skparagraph is declared API stable nor documented ( https://issues.skia.org/358587937 , https://issues.skia.org/307357528 ). That said, I'll add those bits you mentioned in the next round of updates. I am concerned about using skparagraph bits which are still subjected to change though, so I think the rust-skia people are perhaps somewhat reckless in providing too much of skparagraph, without asking upstream for clarification (I.e basically filing or commenting on https://issues.skia.org/358587937 and https://issues.skia.org/307357528 and equivalents.

HinTak commented 2 months ago

What I mean is that, while I'll add those, I would also update those issues upstream to push upstream to doing something about formally declaring those API as public symbols and etc.

MeetWq commented 2 months ago

Thanks. Got it.

HinTak commented 2 months ago

@MeetWq I added all the bits, and also ported your code to skia-python: https://github.com/HinTak/skia-python-examples/blob/main/skparagraph-example.py

Took me some effort to match your output (mainly unsetting my FC_LANG="zh-TW" and set it to FC_LANG=en temporarily):

test-en

Just saving you the trouble of eye-balling mine against yours, this is from ImageMagick's compare - You can see mine is pixel-level identical to yours, even the Arabic section, until it gets to the Devanagari section and the Color Emoji:

FWIW, if I keep my default FC_LANG="zh-TW", this is the result - you can see all the non-English sections are broken, because it overwhelmingly prefer a chinese font: test-zh-TW

So this code is hightly sensitive to LANG/FC_LANG. I did try a pure Arabic example earlier, and it did the correct thing (see my comment above; it was able to do Arabic run-directions, shaping and line breaks), so it is probably a case it get confused if there are more than one language, too.

HinTak commented 2 months ago

@MeetWq A few items from the rust->python porting:

You may want to report to the rust-skia people - the 2-arg constructor ParagraphBuilder.make is marked SK_DISABLE_LEGACY_PARAGRAPH_UNICODE in the upstream header (i.e. indicating it is scheduled for removal later). Hence I am not adding it to skia-python, and just change the code to use the 3-arg constructor.
cloneForPlaceholder/clone, style_underline.setDecoration*, paragraph.paint(canvas, x, y) / paragraph.paint(canvas, Point()), I choose to stay close to upstream or the equivalent c++ code, the rust-people seems to want to think different. I kind of see their point, so I might put some rust-like alias in in the future.
I checked upstream, TextDecorationMode.kThrough is the default; that rust seems to default to kGaps. although it is nice (and I agree it is an improvement), you might want to ask the rust-skia people to properly document it as a "different default behavior from upstream", because it is surprising to people who are familiar to skia via a different binding, or actually use skia directly in c/c++.
I learned something from your code style.setForegroundPaint is preferred over style.setForegroundColor (which I added earlier, but marked DEPRECATED; this is used by the google example!).

I think from the first item and the last item, you can see why I am cautious about binding too much of skparagraph, which is itself a changing target.

HinTak commented 2 months ago

@kyamagu m130 is out, and it is quite small. I intend to pull #265 (and therefore also #260) and #256 into this, and do the README.m129 / m130 here (and mention this as a m130 change although it was post m128 against m128). Do you want to review this pre-pull of those, or post? Or doesn't matter? The pulls are all updating separate area of code, so it is fairly obvious which is which, even if I join all of them together before review.

Still missing are some tests from this. Upstream hasn't changed much documentation wise.

kyamagu commented 2 months ago

@HinTak It's good m130 update is small. At a glance, the code change looks good to me. It doesn't matter whether to review before or after the pull. Thanks!