SkPatch abstraction

include/core/SkPatch.h

+/*
+ * Copyright 2014 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#ifndef SkPatch_DEFINED
+#define SkPatch_DEFINED
+
+#include "SkColor.h"

[egdaniel, 2014/07/24 19:07:38]

Is it possible to not include skcolor or point in the header? Can they be
forward declared and then include in cpp? SkPoint may require the Evaluator to
moved into the cpp file first as Brian had suggested.

[dandov, 2014/07/24 19:56:46]

To make the forward declaration should I declare the typedef or how does it work
when they are not classes?

+#include "SkPoint.h"
+
+

[jvanverth1, 2014/07/24 17:37:05]

If this class is internal to the SkPatch implementation, I'd make it a subclass
of SkPatch and not expose it to the interface (it'd be called FwDCubicEvaluator
then).

[bsalomon, 2014/07/24 18:10:23]

Yeah it might not even need to be nested in SkPatch, but just be declared in
SkPatch.cpp.

[dandov, 2014/07/24 19:56:46]

I moved it to SkPatch.cpp and removed the FwDCubicEvaluators from the attributes
of SkPatch, I think it makes more sense to create them in the method where the
vertex data is created. 

+/**
+ * Evaluator to sample the values of a cubic bezier using forward differences. 
+ * Forward differences is a method for evaluating a nth degree polynomial at a uniform step by only
+ * adding precalculated values. 
+ * For a linear example we have the function f(t) = m*t+b, then the value of that function at t+h 
+ * would be f(t+h) = m*(t+h)+b. If we want to know the uniform step that we must add to the first 
+ * evaluation f(t) then we need to substract f(t+h) - f(t) = m*t + m*h + b - m*t + b = mh. After 
+ * obtaining this value (mh) we could just add this constant step to our first sampled point
+ * to compute the next one. 
+ * 
+ * For the cubic case the first difference gives as a result a quadratic polynomial to which we can 
+ * apply again forward differences and get linear function to which we can apply again forward 
+ * differences to get a constant difference. This is why we keep an array of size 4, the 0th 
+ * position keeps the sampled value while the next ones keep the quadratic, linear and constant 
+ * difference values. 
+ */
+class SkFwDCubicEvaluator {
+
+public:
+    SkFwDCubicEvaluator(){ }
+
+    /**
+     * Receives the 4 control points for the cubic.
+     */
+    SkFwDCubicEvaluator(SkPoint a, SkPoint b, SkPoint c, SkPoint d);
+    
+    /**
+     * Resets the forward differences evaluator to the first value of t = 0.
+     */
+    void reset(int divisions);

[bsalomon, 2014/07/24 18:10:23]

Can we call this restart? We usually use reset() to mean that all the object's
state is blown away.

[dandov, 2014/07/24 19:56:46]

Done.

+
+    /*
+     * Evaluate cubic bezier using forward differences.
+     * Call operator* to check the current sampled value.
+     * Call operator++ to obtain the SkPoint sampled and move to the next one.
+     * Call reset to start sampling from t=0
+     */
+    inline bool done() {
+        return fCurrent <= fMax;
+    }
+
+    inline SkPoint operator*() {

[jvanverth1, 2014/07/24 17:37:04]

If this isn't used (it looks like you use getPoints()), I'd remove it.

[dandov, 2014/07/24 19:56:46]

I removed the operator* and changed the name to getCtrlPoints to make it
clearer. 

+        return fFwDiff[0];
+    }
+    
+    inline SkPoint operator++() {

[bsalomon, 2014/07/24 18:10:23]

We're pretty conservative about using operators. Using ++ here instead something
like next() or advance() seems dubious to me. I don't think it helps
readability.

[jvanverth1, 2014/07/24 18:25:21]

My bad, I suggested operator++.

[dandov, 2014/07/24 19:56:46]

Done.

+        SkPoint point = fFwDiff[0];
+        fFwDiff[0]    += fFwDiff[1];
+        fFwDiff[1]    += fFwDiff[2];
+        fFwDiff[2]    += fFwDiff[3];
+        fCurrent++;
+        return point;
+    }
+    
+    inline SkPoint operator++(int) {

[jvanverth1, 2014/07/24 17:37:05]

I'm not sure what the point of this operator is...

[dandov, 2014/07/24 19:56:46]

It goes away with the operator changed to next() function.  

+        SkPoint point = fFwDiff[0];
+        fFwDiff[0]    += fFwDiff[1];
+        fFwDiff[1]    += fFwDiff[2];
+        fFwDiff[2]    += fFwDiff[3];
+        fCurrent++;
+        return point;
+    }
+    
+    const SkPoint* getPoints();
+
+private:
+    int fMax, fCurrent, fDivisions;
+    SkPoint fFwDiff[4], fCoefs[4], fPoints[4];
+};
+
+/**
+ * Class that represents a coons patch.
+ */
+class SK_API SkPatch {
+
+public:
+    /**
+     * Points are in the following order:
+     *              (bottom curve)

[bsalomon, 2014/07/24 18:10:23]

It's kind of weird that the diagram has bottom above top. It's strange but in
Skia-land the y-axis points down.

[egdaniel, 2014/07/24 19:07:38]

Also I feel like it would make more sense to pass the points in circularly so
that the 4 points of the bezier curves are always contiguous (expect for last
curve and first point).

Using your diagram numbers as "pnt locations" I personally would pass them in
0,1,2,3,10,11,7,6,5,4,9,8. Is there a reason you chose bottom, top, then middle
side control points?

[dandov, 2014/07/24 19:56:46]

I had them like that before but I changed them because I kept getting confused
with the direction of parametric evaluation which is left to right and bottom to
top.

Also, I made the diagram like that because I kept getting confused on how the
vertices were arranged because of the inverted y axis. Should I change it so
that it makes more sense or maybe invert them inside the class? I think the
latter one would be more confusing. 

+     *               0 1 2 3
+     * (left curve)  8     10   (right curve)
+     *               9     11
+     *               4 5 6 7  
+     *              (top curve)
+     * Used pointer to an array to guarantee that this method receives an array of 4 SkColors
+     */

[dandov, 2014/07/22 20:23:28]

Changed colors parameter to be just pointer to SkColor instead of a pointer to
an array of 4 SkColor. It is more flexible for cases in which the client may
pass a pointer that is at a certain offset of an array for example. 

+    SkPatch(SkPoint points[12], SkColor colors[4], int res);
+
+    ~SkPatch();
+
+    void clean();

[bsalomon, 2014/07/24 18:10:23]

I think in Skia we'd normally call this reset().

[dandov, 2014/07/24 19:56:46]

Done.

+
+    const SkPoint* getTopPoints()    { return fTop.getPoints(); }
+
+    const SkPoint* getLeftPoints()   { return fLeft.getPoints(); }
+
+    const SkPoint* getRightPoints()  { return fRight.getPoints(); }
+
+    const SkPoint* getBottomPoints() { return fBottom.getPoints(); }
+
+    void setData();

[bsalomon, 2014/07/24 18:10:23]

It's non-obvious what this does.

Also, does it make sense for the patch to own the generated data? Maybe it
should construct the mesh and give ownership to the caller of this function.

[dandov, 2014/07/24 19:56:46]

Changed the method signature to void setVertexData(SkPatch::PatchData* data),
that way it gives the ownership to the caller. This struct has separate arrays
for the points, colors and texture coordinates, which are passed to
drawVertices. Would it be better to interleave the data and directly call
drawIndexed and let the CPU side rearrange it into separate arrays?

+
+    int getVertexCount() { return fVertCount; }
+
+    int getIndexCount() { return fIndexCount; }
+
+    SkPoint* getPoints()    { return fPoints; }
+
+    SkPoint* getTexCoords() { return fTexCoords; }
+
+    uint32_t* getColors()   { return fColors; }
+
+    uint16_t* getIndices()  { return fIndices; }
+
+private:
+    SkPoint* fCtrlPoints;
+    SkFwDCubicEvaluator fBottom, fTop, fLeft, fRight;
+    SkPMColor fCornerColors[4];
+    int fDivX, fDivY;
+
+    int fVertCount, fIndexCount;
+    SkPoint* fPoints;
+    SkPoint* fTexCoords;
+    uint32_t* fColors;
+    uint16_t* fIndices;
+    bool fIsSet;
+
+    typedef SkPatch INHERITED;
+};
+
+#endif