Revert "[skottie] Fix text-on-path tracking"

This reverts commit ca973cbea0.

Reason for revert: g3 image diffs

Original change's description:
> [skottie] Fix text-on-path tracking
>
> Tracking and line spacing computations require knowledge of cumulative
> values for the whole line => we need two passes:
>
>   1) compute cumulative values
>   2) compute per-fragment position adjustments
>
> Currently, #1 is implemented in the main onSync() loop (as we iterate
> to compute fragment props) and #2 is post-applied via adjustLineProps(),
> after the main loop.
>
> The problem is adjustLineProps() is executed after positioning glyphs on
> path, and tracking is not taken into account for path positioning
> (instead it moves glyphs horizontally, unrelated to the path).
>
> To fix this, we need tracking adjustments to be applied before
> positioning on path (which is performed in fragmentMatrix()).
>
>   - move the cumulative tracking computation to a dedicate lambda
>     (compute_linewide_props)
>   - move the fragment position adjustments to the main onSync() loop
>     (that way they participate in path positioning)
>   - to avoid executing the first pass unnecessarily, add flags to detect
>     the presence of tracking and line spacing animators.
>
>
> Change-Id: Ieef2afb53ffe14177eba0ef41dc5c71149cab070
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/518696
> Reviewed-by: Ben Wagner <bungeman@google.com>
> Commit-Queue: Florin Malita <fmalita@chromium.org>
> Commit-Queue: Florin Malita <fmalita@google.com>

Change-Id: Ia99fbb3d7d98eb6a59ff00d796bcc05bc6db63a3
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/519597
Auto-Submit: Florin Malita <fmalita@google.com>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>

modules/skottie/src/text/TextAdapter.cpp

@@ -234,9 +234,8 @@ sk_sp<TextAdapter> TextAdapter::Make(const skjson::ObjectValue& jlayer,
 
         for (const skjson::ObjectValue* janimator : *janimators) {
             if (auto animator = TextAnimator::Make(janimator, abuilder, adapter.get())) {
-                adapter->fHasBlurAnimator         |= animator->hasBlur();
+                adapter->fHasBlurAnimator     |= animator->hasBlur();
-                adapter->fRequiresAnchorPoint     |= animator->requiresAnchorPoint();
+                adapter->fRequiresAnchorPoint |= animator->requiresAnchorPoint();
-                adapter->fRequiresLineAdjustments |= animator->requiresLineAdjustments();
 
                 adapter->fAnimators.push_back(std::move(animator));
             }
@@ -295,8 +294,7 @@ TextAdapter::TextAdapter(sk_sp<SkFontMgr> fontmgr, sk_sp<Logger> logger, AnchorP
     , fLogger(std::move(logger))
     , fAnchorPointGrouping(apg)
     , fHasBlurAnimator(false)
-    , fRequiresAnchorPoint(false)
+    , fRequiresAnchorPoint(false) {}
-    , fRequiresLineAdjustments(false) {}
 
 TextAdapter::~TextAdapter() = default;
 
@@ -582,43 +580,16 @@ void TextAdapter::onSync() {
     }
 
     size_t grouping_span_index = 0;
-    SkV2   current_line_offset = { 0, 0 }; // cumulative line spacing
+    SkV2           line_offset = { 0, 0 }; // cumulative line spacing
-
-    auto compute_linewide_props = [this](const TextAnimator::ModulatorBuffer& buf,
-                                         const TextAnimator::DomainSpan& line_span) {
-        SkV2  total_spacing  = {0,0};
-        float total_tracking = 0;
-
-        // Only compute these when needed.
-        if (fRequiresLineAdjustments) {
-            for (size_t i = line_span.fOffset; i < line_span.fOffset + line_span.fCount; ++i) {
-                const auto& props = buf[i].props;
-                total_spacing  += props.line_spacing;
-                total_tracking += props.tracking;
-            }
-
-            // The first glyph does not contribute |before| tracking, and the last one does not
-            // contribute |after| tracking.
-            total_tracking -= 0.5f * (buf[line_span.fOffset].props.tracking +
-                                      buf[line_span.fOffset + line_span.fCount - 1].props.tracking);
-        }
-
-        return std::make_tuple(total_spacing, total_tracking);
-    };
 
     // Finally, push all props to their corresponding fragment.
     for (const auto& line_span : fMaps.fLinesMap) {
-        const auto [line_spacing, line_tracking] = compute_linewide_props(buf, line_span);
+        SkV2 line_spacing = { 0, 0 };
-        const auto align_offset = -line_tracking * align_factor(fText->fHAlign);
+        float line_tracking = 0;
+        bool line_has_tracking = false;
 
-        // line spacing of the first line is ignored (nothing to "space" against)
+        // Tracking requires special treatment: unlike other props, its effect is not localized
-        if (&line_span != &fMaps.fLinesMap.front() && line_span.fCount) {
+        // to a single fragment, but requires re-alignment of the whole line.
-            // For each line, the actual spacing is an average of individual fragment spacing
-            // (to preserve the "line").
-            current_line_offset += line_spacing / line_span.fCount;
-        }
-
-        float tracking_acc = 0;
         for (size_t i = line_span.fOffset; i < line_span.fOffset + line_span.fCount; ++i) {
             // Track the grouping domain span in parallel.
             if (grouping_domain && i >= (*grouping_domain)[grouping_span_index].fOffset +
@@ -630,28 +601,27 @@ void TextAdapter::onSync() {
 
             const auto& props = buf[i].props;
             const auto& frag  = fFragments[i];
-
+            this->pushPropsToFragment(props, frag, fGroupingAlignment * .01f, // percentage
-            // AE tracking is defined per glyph, based on two components: |before| and |after|.
-            // BodyMovin only exports "balanced" tracking values, where before = after = tracking/2.
-            //
-            // Tracking is applied as a local glyph offset, and contributes to the line width for
-            // alignment purposes.
-            //
-            // No |before| tracking for the first glyph, nor |after| tracking for the last one.
-            const auto track_before = i > line_span.fOffset
-                                        ? props.tracking * 0.5f : 0.0f,
-                       track_after  = i < line_span.fOffset + line_span.fCount - 1
-                                        ? props.tracking * 0.5f : 0.0f;
-
-            const auto frag_offset = current_line_offset +
-                                     SkV2{align_offset + tracking_acc + track_before, 0};
-
-            tracking_acc += track_before + track_after;
-
-            this->pushPropsToFragment(props, frag, frag_offset, fGroupingAlignment * .01f, // %
                                       grouping_domain ? &(*grouping_domain)[grouping_span_index]
                                                         : nullptr);
+
+            line_tracking += props.tracking;
+            line_has_tracking |= !SkScalarNearlyZero(props.tracking);
+
+            line_spacing += props.line_spacing;
         }
+
+        // line spacing of the first line is ignored (nothing to "space" against)
+        if (&line_span != &fMaps.fLinesMap.front()) {
+            // For each line, the actual spacing is an average of individual fragment spacing
+            // (to preserve the "line").
+            line_offset += line_spacing / line_span.fCount;
+        }
+
+        if (line_offset != SkV2{0, 0} || line_has_tracking) {
+            this->adjustLineProps(buf, line_span, line_offset, line_tracking);
+        }
+
     }
 }
 
@@ -712,10 +682,10 @@ SkV2 TextAdapter::fragmentAnchorPoint(const FragmentRec& rec,
 }
 
 SkM44 TextAdapter::fragmentMatrix(const TextAnimator::ResolvedProps& props,
-                                  const FragmentRec& rec, const SkV2& frag_offset) const {
+                                  const FragmentRec& rec, const SkV2& anchor_point) const {
     const SkV3 pos = {
-        props.position.x + rec.fOrigin.fX + frag_offset.x,
+        props.position.x + rec.fOrigin.fX + anchor_point.x,
-        props.position.y + rec.fOrigin.fY + frag_offset.y,
+        props.position.y + rec.fOrigin.fY + anchor_point.y,
         props.position.z
     };
 
@@ -745,13 +715,12 @@ SkM44 TextAdapter::fragmentMatrix(const TextAnimator::ResolvedProps& props,
 
 void TextAdapter::pushPropsToFragment(const TextAnimator::ResolvedProps& props,
                                       const FragmentRec& rec,
-                                      const SkV2& frag_offset,
                                       const SkV2& grouping_alignment,
                                       const TextAnimator::DomainSpan* grouping_span) const {
     const auto anchor_point = this->fragmentAnchorPoint(rec, grouping_alignment, grouping_span);
 
     rec.fMatrixNode->setMatrix(
-                this->fragmentMatrix(props, rec, anchor_point + frag_offset)
+                this->fragmentMatrix(props, rec, anchor_point)
               * SkM44::Rotate({ 1, 0, 0 }, SkDegreesToRadians(props.rotation.x))
               * SkM44::Rotate({ 0, 1, 0 }, SkDegreesToRadians(props.rotation.y))
               * SkM44::Rotate({ 0, 0, 1 }, SkDegreesToRadians(props.rotation.z))
@@ -774,4 +743,44 @@ void TextAdapter::pushPropsToFragment(const TextAnimator::ResolvedProps& props,
     }
 }
 
+void TextAdapter::adjustLineProps(const TextAnimator::ModulatorBuffer& buf,
+                                  const TextAnimator::DomainSpan& line_span,
+                                  const SkV2& line_offset,
+                                  float total_tracking) const {
+    SkASSERT(line_span.fCount > 0);
+
+    // AE tracking is defined per glyph, based on two components: |before| and |after|.
+    // BodyMovin only exports "balanced" tracking values, where before == after == tracking / 2.
+    //
+    // Tracking is applied as a local glyph offset, and contributes to the line width for alignment
+    // purposes.
+
+    // The first glyph does not contribute |before| tracking, and the last one does not contribute
+    // |after| tracking.  Rather than spill this logic into applyAnimators, post-adjust here.
+    total_tracking -= 0.5f * (buf[line_span.fOffset].props.tracking +
+                              buf[line_span.fOffset + line_span.fCount - 1].props.tracking);
+
+    const auto align_offset = -total_tracking * align_factor(fText->fHAlign);
+
+    float tracking_acc = 0;
+    for (size_t i = line_span.fOffset; i < line_span.fOffset + line_span.fCount; ++i) {
+        const auto& props = buf[i].props;
+
+        // No |before| tracking for the first glyph, nor |after| tracking for the last one.
+        const auto track_before = i > line_span.fOffset
+                                    ? props.tracking * 0.5f : 0.0f,
+                   track_after  = i < line_span.fOffset + line_span.fCount - 1
+                                    ? props.tracking * 0.5f : 0.0f,
+                fragment_offset = align_offset + tracking_acc + track_before;
+
+        const auto& frag = fFragments[i];
+        const auto m = SkM44::Translate(line_offset.x + fragment_offset,
+                                        line_offset.y) *
+                       frag.fMatrixNode->getMatrix();
+        frag.fMatrixNode->setMatrix(m);
+
+        tracking_acc += track_before + track_after;
+    }
+}
+
 } // namespace skottie::internal

modules/skottie/src/text/TextAdapter.h

@@ -69,8 +69,12 @@ private:
     void buildDomainMaps(const Shaper::Result&);
 
     void pushPropsToFragment(const TextAnimator::ResolvedProps&, const FragmentRec&,
-                             const SkV2& frag_offset, const SkV2& grouping_alignment,
+                             const SkV2&, const TextAnimator::DomainSpan*) const;
-                             const TextAnimator::DomainSpan*) const;
+
+    void adjustLineProps(const TextAnimator::ModulatorBuffer&,
+                         const TextAnimator::DomainSpan&,
+                         const SkV2& line_offset,
+                         float line_tracking) const;
 
     SkV2 fragmentAnchorPoint(const FragmentRec&, const SkV2&,
                              const TextAnimator::DomainSpan*) const;
@@ -112,9 +116,8 @@ private:
     struct PathInfo;
     std::unique_ptr<PathInfo> fPathInfo;
 
-    bool                      fHasBlurAnimator         : 1,
+    bool                      fHasBlurAnimator     : 1,
-                              fRequiresAnchorPoint     : 1,
+                              fRequiresAnchorPoint : 1;
-                              fRequiresLineAdjustments : 1;
 };
 
 } // namespace internal

modules/skottie/src/text/TextAnimator.cpp

@@ -182,14 +182,11 @@ TextAnimator::TextAnimator(std::vector<sk_sp<RangeSelector>>&& selectors,
                            const AnimationBuilder* abuilder,
                            AnimatablePropertyContainer* acontainer)
     : fSelectors(std::move(selectors))
-    , fRequiresAnchorPoint(false)
+    , fRequiresAnchorPoint(false) {
-    , fRequiresLineAdjustments(false) {
 
     acontainer->bind(*abuilder, jprops["p" ], fTextProps.position);
-
+    acontainer->bind(*abuilder, jprops["t" ], fTextProps.tracking);
-    // Tracking and line spacing affect all line fragments.
+    acontainer->bind(*abuilder, jprops["ls"], fTextProps.line_spacing);
-    fRequiresLineAdjustments |= acontainer->bind(*abuilder, jprops["t" ], fTextProps.tracking);
-    fRequiresLineAdjustments |= acontainer->bind(*abuilder, jprops["ls"], fTextProps.line_spacing);
 
     // Scale and rotation are anchor-point-dependent.
     fRequiresAnchorPoint |= acontainer->bind(*abuilder, jprops["s"], fTextProps.scale);

modules/skottie/src/text/TextAnimator.h

@@ -87,8 +87,7 @@ public:
 
     bool hasBlur() const { return fHasBlur; }
 
-    bool requiresAnchorPoint()     const { return fRequiresAnchorPoint;     }
+    bool requiresAnchorPoint() const { return fRequiresAnchorPoint; }
-    bool requiresLineAdjustments() const { return fRequiresLineAdjustments; }
 
 private:
     TextAnimator(std::vector<sk_sp<RangeSelector>>&&,
@@ -101,14 +100,13 @@ private:
     const std::vector<sk_sp<RangeSelector>> fSelectors;
 
     AnimatedProps fTextProps;
-    bool          fHasFillColor            : 1,
+    bool          fHasFillColor        : 1,
-                  fHasStrokeColor          : 1,
+                  fHasStrokeColor      : 1,
-                  fHasFillOpacity          : 1,
+                  fHasFillOpacity      : 1,
-                  fHasStrokeOpacity        : 1,
+                  fHasStrokeOpacity    : 1,
-                  fHasOpacity              : 1,
+                  fHasOpacity          : 1,
-                  fHasBlur                 : 1,
+                  fHasBlur             : 1,
-                  fRequiresAnchorPoint     : 1, // animator sensitive to transform origin?
+                  fRequiresAnchorPoint : 1; // animator sensitive to transform origin?
-                  fRequiresLineAdjustments : 1; // animator effects line-wide fragment adjustments
 };
 
 } // namespace internal

resources/skottie/skottie-textpath-tracking.json

@@ -1 +0,0 @@
-{"v":"5.8.3","fr":60,"ip":0,"op":301,"w":500,"h":500,"nm":"tpath tracking","ddd":0,"assets":[],"fonts":{"list":[{"origin":0,"fPath":"","fClass":"","fFamily":"Google Sans","fWeight":"","fStyle":"Bold","fName":"GoogleSans-Bold","ascent":75.6476929411292}]},"layers":[{"ddd":0,"ind":1,"ty":5,"nm":"Foo Bar Baz","sr":1,"ks":{"o":{"a":0,"k":100,"ix":11},"r":{"a":0,"k":0,"ix":10},"p":{"a":0,"k":[250,250,0],"ix":2,"l":2},"a":{"a":0,"k":[0,0,0],"ix":1,"l":2},"s":{"a":0,"k":[100,100,100],"ix":6,"l":2}},"ao":0,"hasMask":true,"masksProperties":[{"inv":false,"mode":"n","pt":{"a":0,"k":{"i":[[66.274,0],[0,-66.274],[-66.274,0],[0,66.274]],"o":[[-66.274,0],[0,66.274],[66.274,0],[0,-66.274]],"v":[[0,-120],[-120,0],[0,120],[120,0]],"c":true},"ix":1},"o":{"a":0,"k":100,"ix":3},"x":{"a":0,"k":0,"ix":4},"nm":"Mask 1"}],"t":{"d":{"k":[{"s":{"s":72,"f":"GoogleSans-Bold","t":"Foo Bar Baz","ca":0,"j":2,"tr":0,"lh":96,"ls":0,"fc":[0,0.522,0]},"t":0}]},"p":{"m":0,"f":{"a":0,"k":377,"ix":5},"l":{"a":0,"k":0,"ix":6},"a":{"a":0,"k":0,"ix":4},"p":{"a":0,"k":1,"ix":3},"r":{"a":0,"k":1,"ix":2}},"m":{"g":1,"a":{"a":0,"k":[0,0],"ix":2}},"a":[{"nm":"Animator 1","s":{"t":0,"xe":{"a":0,"k":0,"ix":7},"ne":{"a":0,"k":0,"ix":8},"a":{"a":0,"k":100,"ix":4},"b":1,"rn":0,"sh":1,"sm":{"a":0,"k":100,"ix":6},"r":1},"a":{"t":{"a":1,"k":[{"i":{"x":[0.336],"y":[0.999]},"o":{"x":[0.669],"y":[0.001]},"t":0,"s":[-35]},{"i":{"x":[0.336],"y":[0.997]},"o":{"x":[0.665],"y":[-0.001]},"t":150,"s":[50]},{"t":300,"s":[-35]}],"ix":89}}}]},"ip":0,"op":301,"st":0,"bm":0},{"ddd":0,"ind":2,"ty":4,"nm":"Shape Layer 1","sr":1,"ks":{"o":{"a":0,"k":100,"ix":11},"r":{"a":0,"k":0,"ix":10},"p":{"a":0,"k":[250,250,0],"ix":2,"l":2},"a":{"a":0,"k":[0,0,0],"ix":1,"l":2},"s":{"a":0,"k":[100,100,100],"ix":6,"l":2}},"ao":0,"shapes":[{"d":1,"ty":"el","s":{"a":0,"k":[240,240],"ix":2},"p":{"a":0,"k":[0,0],"ix":3},"nm":"Ellipse Path 1","mn":"ADBE Vector Shape - Ellipse","hd":false},{"ty":"st","c":{"a":0,"k":[1,0,0,1],"ix":3},"o":{"a":0,"k":100,"ix":4},"w":{"a":0,"k":5,"ix":5},"lc":1,"lj":1,"ml":4,"bm":0,"nm":"Stroke 1","mn":"ADBE Vector Graphic - Stroke","hd":false}],"ip":0,"op":301,"st":0,"bm":0},{"ddd":0,"ind":3,"ty":1,"nm":"Medium Yellow Solid 1","sr":1,"ks":{"o":{"a":0,"k":100,"ix":11},"r":{"a":0,"k":0,"ix":10},"p":{"a":0,"k":[250,250,0],"ix":2,"l":2},"a":{"a":0,"k":[600,600,0],"ix":1,"l":2},"s":{"a":0,"k":[100,100,100],"ix":6,"l":2}},"ao":0,"sw":1200,"sh":1200,"sc":"#ffffbd","ip":0,"op":301,"st":0,"bm":0}],"markers":[]}