Improve skia text shaping & bounds calculation #103
Conversation
ckaiser
force-pushed
the
fix/skia-shaper-crashes
branch
from
1e97f55
to
6d7b22f
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
| auto bidiRun = SkShaper::MakeBiDiRunIterator(text.c_str(), text.size(), 0xfe); | ||
| constexpr SkFourByteTag tag = SkSetFourByteTag('Z', 'y', 'y', 'y'); | ||
| auto scriptRun = SkShaper::MakeScriptRunIterator(text.c_str(), text.size(), tag); | ||
| auto languageRun = SkShaper::MakeStdLanguageRunIterator(text.c_str(), text.size()); | ||
| auto fontRun = SkShaper::MakeFontMgrRunIterator(text.c_str(), | ||
| text.size(), | ||
| skFont, | ||
| skFontMgr, | ||
| "Arial", // Fallback | ||
| SkFontStyle::Normal(), | ||
| &*languageRun); | ||
|
|
||
| shaper->shape(text.c_str(), | ||
| text.size(), | ||
| *fontRun, | ||
| *bidiRun, | ||
| *scriptRun, | ||
| *languageRun, | ||
| std::numeric_limits<float>::max(), | ||
| &shaperHandler); | ||
|
|
There was a problem hiding this comment.
It looks like MakeBiDiRunIterator and MakeScriptRunIterator functions are legacy too.
Probably we could do something similar to SkShaperPrimitive::shape() (using values instead of smart pointers for each run iterator):
void SkShaperPrimitive::shape(const char* utf8,
size_t utf8Bytes,
const SkFont& font,
bool leftToRight,
SkScalar width,
RunHandler* handler) const {
std::unique_ptr<FontRunIterator> fontRuns(
MakeFontMgrRunIterator(utf8, utf8Bytes, font, nullptr));
if (!fontRuns) {
return;
}
// bidi, script, and lang are all unused so we can construct them with empty data.
TrivialBiDiRunIterator bidi{0, 0};
TrivialScriptRunIterator script{0, 0};
TrivialLanguageRunIterator lang{nullptr, 0};
return this->shape(utf8, utf8Bytes, *fontRuns, bidi, script, lang, nullptr, 0, width, handler);
}Anyway I tried to replace with these iterators and didn't work (I got a crash), so there is something I'm missing here.
There was a problem hiding this comment.
I'll look into it, it's very strange that we're getting different behaviors per-platform
There was a problem hiding this comment.
Oh in this case your code worked on Linux 👍 I was just commenting this because it might be better if we avoid calling those legacy Make*RunIterator functions and create those iterators in the stack directly.
There was a problem hiding this comment.
The thing is that we need to use these functions because they have different implementations depending on what's available, so on a platform without harfbuzz or on a compile without it, it'll use a trivial one - etc. If we don't use them we'd have to include that logic ourselves.
The legacy stuff is kind of interesting too because the only place where that SK_DISABLE_LEGACY_SKSHAPER_FUNCTIONS is declared is in the Bazel compile scripts, not in the GN ones, so it's not even possible to disable these functions for anyone building aseprite atm. I think it's probably ok to keep using them for now.
text/text_blob.cpp
Outdated
| // avgCharWidth can be 0, so we grab the next most useful thing, the height | ||
| bounds.w = metrics.avgCharWidth > 0 ? metrics.avgCharWidth : metrics.xHeight; | ||
| bounds.h = 1.0; |
There was a problem hiding this comment.
I'm not sure about this one, xHeight is the height of the lowercase x character. It feels wrong to use this value as the whitespace width (and hard coding 1.0 as the height).
To test this we can use the text_shape which is a mini text editor, put the cursor at a whitespace to test the whitespace bounds.
We should be able to set bounds.w = the advance in X axis for whitespaces, but I'm not sure where to find this value on Skia.
There was a problem hiding this comment.
Yeah this one's the most contentious change, the hardcoded 1 was to try and fix a specific bug of things being too tall in some circumstances, it should not stay like this. I need to look into more of the documentation
There was a problem hiding this comment.
I'm trying a new approach with just the normal " " character glyph bounds and it looks to behave correctly - at least more consistently than attempting to use FontMetrics. I think the ideal solution would be to shape the entire set of glyphs in one go with skia using getWidthsBounds but that'd mean a full refactor of how the runs are working currently.
|
clang-tidy review says "All clean, LGTM! 👍" |
ckaiser
force-pushed
the
fix/skia-shaper-crashes
branch
from
b94d27a
to
d9beb70
Compare
ckaiser
force-pushed
the
fix/skia-shaper-crashes
branch
from
d9beb70
to
d1dc97d
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
Fixes Skia crashes on Windows by using different bounds calculations and a more complete shape() function call.