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 "SkPreConfig.h"

[egdaniel, 2014/07/21 21:16:03]

In general we like to keep includes in alphabetical order 

[dandov, 2014/07/22 13:55:14]

Done.

+#include "SkTypes.h"
+#include "SkScalar.h"
+#include "SkPoint.h"
+#include "SkColor.h"
+#include "GrColor.h"
+
+#include <stdint.h>
+
+
+class SkPatchMesh;

[jvanverth1, 2014/07/21 22:10:09]

This forward declaration isn't needed.

[dandov, 2014/07/22 13:55:14]

Done.

+
+class SkPatchMesh {

[egdaniel, 2014/07/21 21:16:03]

Comment here describing what an SkPatchMesh is

[dandov, 2014/07/22 13:55:14]

Done.

+
+public:
+
+    /**
+    * Client is responsible for setting up the PatchMesh by providing which variables it is going 

[egdaniel, 2014/07/21 21:16:03]

Nit: all the "*" should be lined up with the first * of /** not the /

[dandov, 2014/07/22 13:55:14]

Done.

+    * to use and by allocating its memory.
+    * Vertices and Indices are always generated.
+    * useColors and useTexCoords define if this patchmesh will handle colors and texture coordinates
+    * intercalated defines if the data is:
+    *        array of objects {{vertex,color,uv}, {vertex,color,uv}, {vertex,color,uv}}
+    *        object of arrays {{vertex, vertex, vertex}, {color, color, color}, {uv, uv, uv}}
+    */
+    SkPatchMesh();
+
+    enum SkColorFormat {
+        kNoColor_ColorFormat,
+        kSkColor_ColorFormat,
+        //kSkPMColor_ColorFormat,
+        kGrColor_ColorFormat
+    };
+
+    void init(int vertCount, int indexCount, SkColorFormat format, bool useTexCoords, 
+        bool intercalate);
+
+    ~SkPatchMesh();
+
+    /**
+    * The client is responsible for setting the values using the functions vertexAt, colorAt, 
+    * texCoordAt. These functions have an assert to check if this patchmesh handles colors or 
+    * texture coordinates and if it is an array of objects or and object of arrays, based on this
+    * and the type of arrays being used they return a reference to the value to be set. 
+    */
+    SkPoint& pointAt(int index);
+
+    uint32_t& colorAt(int index);
+    bool setColorAt(int index,  uint8_t r, uint8_t g, uint8_t b, uint8_t a);
+
+    SkPoint& texCoordAt(int index);
+
+    /**
+    * The client is responsible for setting up the indices. Index refers to which group of indices 
+    * to set, indices refers to the pointer of the data to set and size is the number of elements
+    * that are going to set. 
+    * They need to be separated because (depending on the patch) they might be less number of 
+    * indices than vertices. 
+    */
+    void setIndices(int index, uint16_t* indices, int size);
+
+    int getVertexCount();
+
+    int getIndexCount();
+
+    bool useColors();
+
+    bool useTexCoords();
+
+    bool isIntercalated();
+
+    const SkPoint* getPoints();
+
+    const uint16_t* getIndices();
+
+    const SkColor* getColors();
+
+    const SkPoint* getTexCoords();
+
+    const uint8_t* getData();
+
+private:
+    int                     fVertCount, fIndexCount;
+    uint8_t*        fData;
+    uint16_t*       fIndices;
+    bool            fUseColors, fUseTexCoords, fIntercalate;
+    int             fColorOffset, fTexOffset, fDataSize;
+    SkColorFormat   fColorFormat;
+
+    void reset();
+};
+
+class SK_API SkPatch {

[egdaniel, 2014/07/21 21:16:03]

Comment describing what an SkPatch is

[dandov, 2014/07/22 13:55:14]

Done.

+

[jvanverth1, 2014/07/21 22:10:09]

Unless we're planning on supporting other patch types, I'd just declare SkPatch
to be a Coons patch and get rid of the virtuals.

+public:
+
+    virtual ~SkPatch() { }
+
+    virtual bool genMesh(SkPatchMesh* mesh, SkPatchMesh::SkColorFormat format, bool useTexCoords, 
+        bool intercalate) const = 0;
+    virtual bool usesColors() const = 0;
+    virtual bool usesTexCoords() const = 0;
+};
+
+class CubicEvaluator {

[jvanverth1, 2014/07/21 22:10:09]

SkCubicEvaluator

+
+public:
+    CubicEvaluator(){ }
+
+    /**
+    * Receives the 4 control points for the cubic.
+    */
+    CubicEvaluator(SkPoint a, SkPoint b, SkPoint c, SkPoint d);
+
+    /*
+    * Evaluate cubic bezier given a t parameter.
+    */
+    SkPoint eval(SkScalar t);
+
+    /*
+    * Evaluate cubic bezier using forward differences.
+    * Call hasNext to check if the next value is still within the range t=[0,1].
+    * Call next to obtain the SkPoint sampled at the next t.
+    * Call reset to start sampling from t=0
+    */
+    bool hasNext();
+
+    SkPoint next();
+
+    void reset(int res);

[jvanverth1, 2014/07/21 22:10:09]

Since reset() is the first thing you'd do, I'd put it first. I'm also not sure
about this interface. operator* and operator++ would be more standard.

[dandov, 2014/07/22 13:55:14]

Done.

+
+    const SkPoint* getPoints();
+
+    const SkPoint* getCoefs();
+
+    int getResolution();
+
+private:
+        int     fMax, fCurrent, fRes;

[jvanverth1, 2014/07/21 22:10:09]

Indented too far.

[dandov, 2014/07/22 13:55:14]

I don't know why these are indented like that because on my files they seem to
be correct. 

On 2014/07/21 22:10:09, jvanverth1 wrote:

+        SkPoint fFwDiff[4], fCoefs[4], fPoints[4];
+};
+
+class SK_API SkCoonsPatch : public SkPatch{
+
+public:
+        SkCoonsPatch() { }

[jvanverth1, 2014/07/21 22:10:09]

Indent.

+
+    /*
+    * Points are in the following order:
+    *               (bottom curve)
+    *               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
+    */
+        SkCoonsPatch(SkPoint points[12], SkColor (*colors)[4], int resX);
+
+        virtual bool genMesh(SkPatchMesh* mesh, 
+        SkPatchMesh::SkColorFormat format = SkPatchMesh::kSkColor_ColorFormat, 
+        bool useTexCoords = false, bool intercalate = false) const;

[jvanverth1, 2014/07/21 22:10:09]

With two booleans in a row, it's going to be unclear what genMesh(mesh, format,
false, true); means. Better to define some enums and make it explicit.

[dandov, 2014/07/22 13:55:14]

Added an enum for the array format that also provides better semantics for the
functionality.

+
+    virtual bool usesColors() const {
+        return true;
+    }
+
+    virtual bool usesTexCoords() const SK_OVERRIDE {
+        return false;
+    }
+        
+    CubicEvaluator getTop()    { return fTop; }
+
+    CubicEvaluator getLeft()   { return fLeft; }
+
+    CubicEvaluator getRight()  { return fRight; }
+
+    CubicEvaluator getBottom() { return fBottom; }
+
+    const SkPoint* getTopPoints()    { return fTop.getPoints(); }
+
+    const SkPoint* getLeftPoints()   { return fLeft.getPoints(); }
+
+    const SkPoint* getRightPoints()  { return fRight.getPoints(); }
+
+    const SkPoint* getBottomPoints() { return fBottom.getPoints(); }
+
+private:
+    SkPoint* fPoints;
+        mutable CubicEvaluator fBottom, fTop, fLeft, fRight;

[jvanverth1, 2014/07/21 22:10:09]

Indent.

+    SkPMColor fColors[4];
+    int fResX, fResY;
+
+    typedef SkPatch INHERITED;
+};
+
+#endif