Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(121)

Issue 2237503003: Revert of Refactor SkCurveMeasure to use existing eval code (Closed)

Created:
4 years, 4 months ago by hal.canary
Modified:
4 years, 4 months ago
CC:
reviews_skia.org
Base URL:
https://skia.googlesource.com/skia.git@curvemeasure_rework
Target Ref:
refs/heads/master
Project:
skia
Visibility:
Public.

Description

Revert of Refactor SkCurveMeasure to use existing eval code (patchset #4 id:60001 of https://codereview.chromium.org/2226973004/ ) Reason for revert: perf debug assert. Original issue's description: > Refactor SkCurveMeasure to use existing eval code > > - Use quad, cubic, conic eval code from SkGeometry.h > - Implement evaluateDerivativeLength, evaluateDerivative and evaluate switch cases for lines along with the refactor > > BUG=skia: > GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2226973004 > > Committed: https://skia.googlesource.com/skia/+/4ab47e087ecfc82f070cbbaef4d9eb562d3fd163 TBR=reed@google.com,caryclark@google.com,hstern@google.com # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=skia:

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+62 lines, -94 lines) Patch
M src/utils/SkCurveMeasure.h View 2 chunks +7 lines, -9 lines 0 comments Download
M src/utils/SkCurveMeasure.cpp View 11 chunks +55 lines, -85 lines 0 comments Download

Messages

Total messages: 5 (2 generated)
hal.canary
Created Revert of Refactor SkCurveMeasure to use existing eval code
4 years, 4 months ago (2016-08-10 18:35:45 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2237503003/1
4 years, 4 months ago (2016-08-10 18:35:57 UTC) #3
commit-bot: I haz the power
4 years, 4 months ago (2016-08-10 18:36:01 UTC) #5
Failed to apply patch for src/utils/SkCurveMeasure.cpp:
While running git apply --index -3 -p1;
  error: patch failed: src/utils/SkCurveMeasure.cpp:78
  Falling back to three-way merge...
  Applied patch to 'src/utils/SkCurveMeasure.cpp' with conflicts.
  U src/utils/SkCurveMeasure.cpp

Patch:       src/utils/SkCurveMeasure.cpp
Index: src/utils/SkCurveMeasure.cpp
diff --git a/src/utils/SkCurveMeasure.cpp b/src/utils/SkCurveMeasure.cpp
index
823f56adcff9388f8e7f82980afee91bfa1c2e5d..fc2aa84faa119558d5dce1c30f6bdd826c3eb890
100644
--- a/src/utils/SkCurveMeasure.cpp
+++ b/src/utils/SkCurveMeasure.cpp
@@ -6,66 +6,10 @@
  */
 
 #include "SkCurveMeasure.h"
-#include "SkGeometry.h"
 
 // for abs
 #include <cmath>
 
-#define UNIMPLEMENTED SkDEBUGF(("%s:%d unimplemented\n", __FILE__, __LINE__))
-
-/// Used inside SkCurveMeasure::getTime's Newton's iteration
-static inline SkPoint evaluate(const SkPoint pts[4], SkSegType segType,
-                               SkScalar t) {
-    SkPoint pos;
-    switch (segType) {
-        case kQuad_SegType:
-            pos = SkEvalQuadAt(pts, t);
-            break;
-        case kLine_SegType:
-            pos = SkPoint::Make(SkScalarInterp(pts[0].x(), pts[1].x(), t),
-                                SkScalarInterp(pts[0].y(), pts[1].y(), t));
-            break;
-        case kCubic_SegType:
-            SkEvalCubicAt(pts, t, &pos, nullptr, nullptr);
-            break;
-        case kConic_SegType: {
-            SkConic conic(pts, pts[3].x());
-            conic.evalAt(t, &pos);
-        }
-            break;
-        default:
-            UNIMPLEMENTED;
-    }
-
-    return pos;
-}
-
-/// Used inside SkCurveMeasure::getTime's Newton's iteration
-static inline SkVector evaluateDerivative(const SkPoint pts[4],
-                                          SkSegType segType, SkScalar t) {
-    SkVector tan;
-    switch (segType) {
-        case kQuad_SegType:
-            tan = SkEvalQuadTangentAt(pts, t);
-            break;
-        case kLine_SegType:
-            tan = pts[1] - pts[0];
-            break;
-        case kCubic_SegType:
-            SkEvalCubicAt(pts, t, nullptr, &tan, nullptr);
-            break;
-        case kConic_SegType: {
-            SkConic conic(pts, pts[3].x());
-            conic.evalAt(t, nullptr, &tan);
-        }
-            break;
-        default:
-            UNIMPLEMENTED;
-    }
-
-    return tan;
-}
-/// Used in ArcLengthIntegrator::computeLength
 static inline Sk8f evaluateDerivativeLength(const Sk8f& ts,
                                             const Sk8f (&xCoeff)[3],
                                             const Sk8f (&yCoeff)[3],
@@ -78,18 +22,17 @@
             y = yCoeff[0]*ts + yCoeff[1];
             break;
         case kLine_SegType:
-            // length of line derivative is constant
-            // and we precompute it in the constructor
-            return xCoeff[0];
+            SkDebugf("Unimplemented");
+            break;
         case kCubic_SegType:
             x = (xCoeff[0]*ts + xCoeff[1])*ts + xCoeff[2];
             y = (yCoeff[0]*ts + yCoeff[1])*ts + yCoeff[2];
             break;
         case kConic_SegType:
-            UNIMPLEMENTED;
+            SkDebugf("Unimplemented");
             break;
         default:
-            UNIMPLEMENTED;
+            SkDebugf("Unimplemented");
     }
 
     x = x * x;
@@ -97,7 +40,6 @@
 
     return (x + y).sqrt();
 }
-
 ArcLengthIntegrator::ArcLengthIntegrator(const SkPoint* pts, SkSegType segType)
     : fSegType(segType) {
     switch (fSegType) {
@@ -117,13 +59,8 @@
             yCoeff[1] = Sk8f(2.0f*(By - Ay));
         }
             break;
-        case kLine_SegType: {
-            // the length of the derivative of a line is constant
-            // we put in in both coeff arrays for consistency's sake
-            SkScalar length = (pts[1] - pts[0]).length();
-            xCoeff[0] = Sk8f(length);
-            yCoeff[0] = Sk8f(length);
-        }
+        case kLine_SegType:
+            SkDEBUGF(("Unimplemented"));
             break;
         case kCubic_SegType:
         {
@@ -136,7 +73,6 @@
             float Cy = pts[2].y();
             float Dy = pts[3].y();
 
-            // precompute coefficients for derivative
             xCoeff[0] = Sk8f(3.0f*(-Ax + 3.0f*(Bx - Cx) + Dx));
             xCoeff[1] = Sk8f(3.0f*(2.0f*(Ax - 2.0f*Bx + Cx)));
             xCoeff[2] = Sk8f(3.0f*(-Ax + Bx));
@@ -147,10 +83,10 @@
         }
             break;
         case kConic_SegType:
-            UNIMPLEMENTED;
+            SkDEBUGF(("Unimplemented"));
             break;
         default:
-            UNIMPLEMENTED;
+            SkDEBUGF(("Unimplemented"));
     }
 }
 
@@ -181,8 +117,7 @@
             }
             break;
         case SkSegType::kLine_SegType:
-            fPts[0] = pts[0];
-            fPts[1] = pts[1];
+            SkDebugf("Unimplemented");
             break;
         case SkSegType::kCubic_SegType:
             for (size_t i = 0; i < 4; i++) {
@@ -190,12 +125,10 @@
             }
             break;
         case SkSegType::kConic_SegType:
-            for (size_t i = 0; i < 4; i++) {
-                fPts[i] = pts[i];
-            }
+            SkDebugf("Unimplemented");
             break;
         default:
-            UNIMPLEMENTED;
+            SkDEBUGF(("Unimplemented"));
             break;
     }
     fIntegrator = ArcLengthIntegrator(fPts, fSegType);
@@ -266,8 +199,9 @@
 
         prevT = currentT;
         if (iterations < kNewtonIters) {
+            // TODO(hstern) switch here on curve type.
             // This is just newton's formula.
-            SkScalar dt = evaluateDerivative(fPts, fSegType,
currentT).length();
+            SkScalar dt = evaluateQuadDerivative(currentT).length();
             newT = currentT - (lengthDiff / dt);
 
             // If newT is out of bounds, bisect inside newton.
@@ -284,7 +218,7 @@
             newT = (minT + maxT) * 0.5f;
         } else {
             SkDEBUGF(("%.7f %.7f didn't get close enough after bisection.\n",
-                      currentT, currentLength));
+                     currentT, currentLength));
             break;
         }
         currentT = newT;
@@ -301,16 +235,52 @@
 }
 
 void SkCurveMeasure::getPosTanTime(SkScalar targetLength, SkPoint* pos,
-                                   SkVector* tan, SkScalar* time) {
+                               SkVector* tan, SkScalar* time) {
     SkScalar t = getTime(targetLength);
 
     if (time) {
         *time = t;
     }
     if (pos) {
-        *pos = evaluate(fPts, fSegType, t);
+        // TODO(hstern) switch here on curve type.
+        *pos = evaluateQuad(t);
     }
     if (tan) {
-        *tan = evaluateDerivative(fPts, fSegType, t);
-    }
-}
+        // TODO(hstern) switch here on curve type.
+        *tan = evaluateQuadDerivative(t);
+    }
+}
+
+// this is why I feel that the ArcLengthIntegrator should be combined
+// with some sort of evaluator that caches the constants computed from the
+// control points. this is basically the same code in ArcLengthIntegrator
+SkPoint SkCurveMeasure::evaluateQuad(SkScalar t) {
+    SkScalar ti = 1.0f - t;
+
+    SkScalar Ax = fPts[0].x();
+    SkScalar Bx = fPts[1].x();
+    SkScalar Cx = fPts[2].x();
+    SkScalar Ay = fPts[0].y();
+    SkScalar By = fPts[1].y();
+    SkScalar Cy = fPts[2].y();
+
+    SkScalar x = Ax*ti*ti + 2.0f*Bx*t*ti + Cx*t*t;
+    SkScalar y = Ay*ti*ti + 2.0f*By*t*ti + Cy*t*t;
+    return SkPoint::Make(x, y);
+}
+
+SkVector SkCurveMeasure::evaluateQuadDerivative(SkScalar t) {
+    SkScalar Ax = fPts[0].x();
+    SkScalar Bx = fPts[1].x();
+    SkScalar Cx = fPts[2].x();
+    SkScalar Ay = fPts[0].y();
+    SkScalar By = fPts[1].y();
+    SkScalar Cy = fPts[2].y();
+
+    SkScalar A2BCx = 2.0f*(Ax - 2*Bx + Cx);
+    SkScalar A2BCy = 2.0f*(Ay - 2*By + Cy);
+    SkScalar ABx = 2.0f*(Bx - Ax);
+    SkScalar ABy = 2.0f*(By - Ay);
+
+    return SkPoint::Make(A2BCx*t + ABx, A2BCy*t + ABy);
+}

Powered by Google App Engine
This is Rietveld 408576698