[skottie] More snug kTop Shaper alignment
The current implementation relies on SkShaper ascent values for top
text box alignment, but the results are not as visually accurate as AE
(or Lottie).
Use the computed tight bounds instead.
Change-Id: I4447a834fe3cae398fc887766daa68802e7f50a5
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/206684
Reviewed-by: Ben Wagner <bungeman@google.com>
Commit-Queue: Florin Malita <fmalita@chromium.org>
diff --git a/modules/skottie/src/SkottieAdapter.cpp b/modules/skottie/src/SkottieAdapter.cpp
index 6c5804e..097d22e 100644
--- a/modules/skottie/src/SkottieAdapter.cpp
+++ b/modules/skottie/src/SkottieAdapter.cpp
@@ -470,6 +470,24 @@
};
const auto shape_result = Shaper::Shape(fText.fText, text_desc, fText.fBox);
+#if (0)
+ // Enable for text box debugging/visualization.
+ auto box_color = sksg::Color::Make(0xffff0000);
+ box_color->setStyle(SkPaint::kStroke_Style);
+ box_color->setStrokeWidth(1);
+ box_color->setAntiAlias(true);
+
+ auto bounds_color = sksg::Color::Make(0xff00ff00);
+ bounds_color->setStyle(SkPaint::kStroke_Style);
+ bounds_color->setStrokeWidth(1);
+ bounds_color->setAntiAlias(true);
+
+ fRoot->addChild(sksg::Draw::Make(sksg::Rect::Make(fText.fBox),
+ std::move(box_color)));
+ fRoot->addChild(sksg::Draw::Make(sksg::Rect::Make(shape_result.computeBounds()),
+ std::move(bounds_color)));
+#endif
+
fTextNode->setBlob(shape_result.fBlob);
fTextNode->setPosition(shape_result.fPos);
diff --git a/modules/skottie/src/SkottieShaper.cpp b/modules/skottie/src/SkottieShaper.cpp
index 782197f..d801455 100644
--- a/modules/skottie/src/SkottieShaper.cpp
+++ b/modules/skottie/src/SkottieShaper.cpp
@@ -73,9 +73,7 @@
fMaxRunLeading = SkTMax(fMaxRunLeading, metrics.fLeading);
}
- void commitRunInfo() override {
- fCurrentPosition.fY -= fMaxRunAscent;
- }
+ void commitRunInfo() override {}
Buffer runBuffer(const RunInfo& info) override {
int glyphCount = SkTFitsIn<int>(info.glyphCount) ? info.glyphCount : INT_MAX;
@@ -102,11 +100,6 @@
void commitLine() override {
fOffset += { 0, fMaxRunDescent + fMaxRunLeading - fMaxRunAscent };
-
- // Grab the first line ascent for vertical alignment.
- if (SkScalarIsNaN(fFirstLineAscent)) {
- fFirstLineAscent = fMaxRunAscent;
- }
}
Shaper::Result makeBlob() {
@@ -114,12 +107,14 @@
SkPoint pos {fBox.x(), fBox.y()};
+ // By default, first line is vertical-aligned on a baseline of 0.
+ // Perform additional adjustments based on VAlign.
switch (fDesc.fVAlign) {
- case Shaper::VAlign::kTop:
- // Nothing to do (SkShaper default behavior).
- break;
+ case Shaper::VAlign::kTop: {
+ pos.offset(0, -ComputeBlobBounds(blob).fTop);
+ } break;
case Shaper::VAlign::kTopBaseline:
- pos.offset(0, fFirstLineAscent);
+ // Default behavior.
break;
case Shaper::VAlign::kCenter: {
const auto bounds = ComputeBlobBounds(blob).makeOffset(pos.x(), pos.y());
@@ -185,7 +180,6 @@
SkTextBlobBuilder fBuilder;
std::unique_ptr<SkShaper> fShaper;
- SkScalar fFirstLineAscent = SK_ScalarNaN;
SkScalar fMaxRunAscent;
SkScalar fMaxRunDescent;
SkScalar fMaxRunLeading;
diff --git a/modules/skottie/src/SkottieShaper.h b/modules/skottie/src/SkottieShaper.h
index 7005fe9..816141d 100644
--- a/modules/skottie/src/SkottieShaper.h
+++ b/modules/skottie/src/SkottieShaper.h
@@ -30,7 +30,7 @@
};
enum class VAlign : uint8_t {
- // Align the first line ascent with the text box top.
+ // Align the first line visual top with the text box top.
kTop,
// Align the first line baseline with the text box top.
kTopBaseline,
diff --git a/modules/skottie/src/SkottieTest.cpp b/modules/skottie/src/SkottieTest.cpp
index 4b550c7..34a4073 100644
--- a/modules/skottie/src/SkottieTest.cpp
+++ b/modules/skottie/src/SkottieTest.cpp
@@ -222,7 +222,7 @@
REPORTER_ASSERT(reporter, std::get<2>(observer->fMarkers[1]) == 0.75f);
}
-DEF_TEST(Skottie_Shaper, reporter) {
+DEF_TEST(Skottie_Shaper_HAlign, reporter) {
auto typeface = SkTypeface::MakeDefault();
REPORTER_ASSERT(reporter, typeface);
@@ -281,3 +281,64 @@
}
}
}
+
+DEF_TEST(Skottie_Shaper_VAlign, reporter) {
+ auto typeface = SkTypeface::MakeDefault();
+ REPORTER_ASSERT(reporter, typeface);
+
+ static constexpr struct {
+ SkScalar text_size,
+ tolerance;
+ } kTestSizes[] = {
+ // These gross tolerances are required for the test to pass on NativeFonts bots.
+ // Might be worth investigating why we need so much slack.
+ { 5, 2.0f },
+ { 10, 2.0f },
+ { 15, 2.4f },
+ { 25, 4.4f },
+ };
+
+ struct {
+ skottie::Shaper::VAlign align;
+ SkScalar topFactor;
+ } kTestAligns[] = {
+ { skottie::Shaper::VAlign::kTop , 0.0f },
+ { skottie::Shaper::VAlign::kCenter, 0.5f },
+ // TODO: any way to test kTopBaseline?
+ };
+
+ const SkString text("Foo, bar.\rBaz.");
+ const auto text_box = SkRect::MakeXYWH(100, 100, 1000, 1000); // large-enough to avoid breaks.
+
+
+ for (const auto& tsize : kTestSizes) {
+ for (const auto& talign : kTestAligns) {
+ const skottie::Shaper::TextDesc desc = {
+ typeface,
+ tsize.text_size,
+ SkTextUtils::Align::kCenter_Align,
+ talign.align,
+ };
+
+ const auto shape_result = skottie::Shaper::Shape(text, desc, text_box);
+ REPORTER_ASSERT(reporter, shape_result.fBlob);
+
+ const auto shape_bounds = shape_result.computeBounds();
+ REPORTER_ASSERT(reporter, !shape_bounds.isEmpty());
+
+ const auto v_diff = text_box.height() - shape_bounds.height();
+
+ const auto expected_t = text_box.top() + v_diff * talign.topFactor;
+ REPORTER_ASSERT(reporter,
+ std::fabs(shape_bounds.top() - expected_t) < tsize.tolerance,
+ "%f %f %f %f %d", shape_bounds.top(), expected_t, tsize.tolerance,
+ tsize.text_size, SkToU32(talign.align));
+
+ const auto expected_b = text_box.bottom() - v_diff * (1 - talign.topFactor);
+ REPORTER_ASSERT(reporter,
+ std::fabs(shape_bounds.bottom() - expected_b) < tsize.tolerance,
+ "%f %f %f %f %d", shape_bounds.bottom(), expected_b, tsize.tolerance,
+ tsize.text_size, SkToU32(talign.align));
+ }
+ }
+}