Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
405163003:
SkPatch abstraction (Closed)
DescriptionAdded class SkPatch as a new drawing primitive for the library, it is the abstraction of a coons patch.
SkPatch's method getVertexData generates the points, colors and texture coordinates and indices associated with the patch at a certain level of detail.
Added a slide to sample app to show how to set up this classes and how they interact.
BUG=skia:
Committed: https://skia.googlesource.com/skia/+/50d715476b1d3a00fb43c13e34a80ea0a01d32bf
Patch Set 1 #
Total comments: 28
Patch Set 2 : Fixed most of the issues, still have to modify the API #Patch Set 3 : Fixed indents #Patch Set 4 : New API for SkPatch #Patch Set 5 : Ajusted GM to new API #Patch Set 6 : Get data directly from SkPatch #
Total comments: 7
Patch Set 7 : Adjusted names and comments for more readability #
Total comments: 26
Patch Set 8 : Moved ownership of vertex data out of SkPatch #
Total comments: 12
Patch Set 9 : Enum for control points #
Total comments: 12
Patch Set 10 : Small fixes and recover gyp files #
Total comments: 1
Patch Set 11 : Renamed SkPatch::PatchData to SkPatch::VertexData #Patch Set 12 : Fixed failures #Patch Set 13 : GPU only flag #Patch Set 14 : Skip tiled flag #
Messages
Total messages: 27 (0 generated)
First iteration of the skpatch. Currently the GM just calls drawIndexed for the GPU backed and drawVertices for the other ones.
Just a first quick scan, more later https://codereview.chromium.org/405163003/diff/1/include/core/SkPatch.h File include/core/SkPatch.h (right): https://codereview.chromium.org/405163003/diff/1/include/core/SkPatch.h#newco... include/core/SkPatch.h:11: #include "SkPreConfig.h" In general we like to keep includes in alphabetical order https://codereview.chromium.org/405163003/diff/1/include/core/SkPatch.h#newco... include/core/SkPatch.h:23: class SkPatchMesh { Comment here describing what an SkPatchMesh is https://codereview.chromium.org/405163003/diff/1/include/core/SkPatch.h#newco... include/core/SkPatch.h:28: * Client is responsible for setting up the PatchMesh by providing which variables it is going Nit: all the "*" should be lined up with the first * of /** not the / https://codereview.chromium.org/405163003/diff/1/include/core/SkPatch.h#newco... include/core/SkPatch.h:103: class SK_API SkPatch { Comment describing what an SkPatch is https://codereview.chromium.org/405163003/diff/1/src/core/SkPatch.cpp File src/core/SkPatch.cpp (right): https://codereview.chromium.org/405163003/diff/1/src/core/SkPatch.cpp#newcode14 src/core/SkPatch.cpp:14: CubicEvaluator::CubicEvaluator(SkPoint a, SkPoint b, SkPoint c, SkPoint d){ There are already some of the eval and coeff calculations in SkGeometry (SkGetCubicCoeff, SkEvalCubicAt, etc.). Is it possible for you to use these here so we are not repeating lots of code?
A first pass at comments from me: https://codereview.chromium.org/405163003/diff/1/include/core/SkPatch.h File include/core/SkPatch.h (right): https://codereview.chromium.org/405163003/diff/1/include/core/SkPatch.h#newco... include/core/SkPatch.h:21: class SkPatchMesh; This forward declaration isn't needed. https://codereview.chromium.org/405163003/diff/1/include/core/SkPatch.h#newco... include/core/SkPatch.h:104: 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. https://codereview.chromium.org/405163003/diff/1/include/core/SkPatch.h#newco... include/core/SkPatch.h:115: class CubicEvaluator { SkCubicEvaluator https://codereview.chromium.org/405163003/diff/1/include/core/SkPatch.h#newco... include/core/SkPatch.h:140: void reset(int res); 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. https://codereview.chromium.org/405163003/diff/1/include/core/SkPatch.h#newco... include/core/SkPatch.h:149: int fMax, fCurrent, fRes; Indented too far. https://codereview.chromium.org/405163003/diff/1/include/core/SkPatch.h#newco... include/core/SkPatch.h:156: SkCoonsPatch() { } Indent. https://codereview.chromium.org/405163003/diff/1/include/core/SkPatch.h#newco... include/core/SkPatch.h:172: bool useTexCoords = false, bool intercalate = false) const; 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. https://codereview.chromium.org/405163003/diff/1/include/core/SkPatch.h#newco... include/core/SkPatch.h:200: mutable CubicEvaluator fBottom, fTop, fLeft, fRight; Indent. https://codereview.chromium.org/405163003/diff/1/src/core/SkPatch.cpp File src/core/SkPatch.cpp (right): https://codereview.chromium.org/405163003/diff/1/src/core/SkPatch.cpp#newcode37 src/core/SkPatch.cpp:37: bool CubicEvaluator::hasNext(){ This and next() are simple enough, they could be put in the header so they can be inlined. https://codereview.chromium.org/405163003/diff/1/src/core/SkPatch.cpp#newcode52 src/core/SkPatch.cpp:52: fRes = res; Indent https://codereview.chromium.org/405163003/diff/1/src/core/SkPatch.cpp#newcode98 src/core/SkPatch.cpp:98: fIntercalate = intercalate; I don't know this term. Should this be fInterpolate?
First, the api seems strange to me having public methods for setting all the properties in SkMeshPatch but as your gm shows the actual creator of the SkMeshPatch object doesn't use these directly but instead creates some other object, SkPatch, that will access all these... So looking at the example gm, it seems like the only use of the SkPatch is to hold the points, colors, and then call genMesh. Then all our actual draw calls are all based on the SkMeshPatch. It seems to me it would make more sense to simply pass all the relevant information straight into the SkMeshPatch constructor and let it set itself up. If we go with Jim's suggestion of only caring/implementing coon patches this merging should be pretty straight forward and will only need an SkCoonMeshPatch (or a better name) class. If we want to allow other patches we can just pass in a class or even just a function pointer to a genMesh if that is all that differs.
I fixed most of the small issues from the comments, I still have to see how to fix the API https://codereview.chromium.org/405163003/diff/1/include/core/SkPatch.h File include/core/SkPatch.h (right): https://codereview.chromium.org/405163003/diff/1/include/core/SkPatch.h#newco... include/core/SkPatch.h:11: #include "SkPreConfig.h" On 2014/07/21 21:16:03, egdaniel wrote: > In general we like to keep includes in alphabetical order Done. https://codereview.chromium.org/405163003/diff/1/include/core/SkPatch.h#newco... include/core/SkPatch.h:21: class SkPatchMesh; On 2014/07/21 22:10:09, jvanverth1 wrote: > This forward declaration isn't needed. Done. https://codereview.chromium.org/405163003/diff/1/include/core/SkPatch.h#newco... include/core/SkPatch.h:23: class SkPatchMesh { On 2014/07/21 21:16:03, egdaniel wrote: > Comment here describing what an SkPatchMesh is Done. https://codereview.chromium.org/405163003/diff/1/include/core/SkPatch.h#newco... include/core/SkPatch.h:28: * Client is responsible for setting up the PatchMesh by providing which variables it is going On 2014/07/21 21:16:03, egdaniel wrote: > Nit: all the "*" should be lined up with the first * of /** not the / Done. https://codereview.chromium.org/405163003/diff/1/include/core/SkPatch.h#newco... include/core/SkPatch.h:103: class SK_API SkPatch { On 2014/07/21 21:16:03, egdaniel wrote: > Comment describing what an SkPatch is Done. https://codereview.chromium.org/405163003/diff/1/include/core/SkPatch.h#newco... include/core/SkPatch.h:140: void reset(int res); On 2014/07/21 22:10:09, jvanverth1 wrote: > 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. Done. https://codereview.chromium.org/405163003/diff/1/include/core/SkPatch.h#newco... include/core/SkPatch.h:149: int fMax, fCurrent, fRes; 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: > Indented too far. https://codereview.chromium.org/405163003/diff/1/include/core/SkPatch.h#newco... include/core/SkPatch.h:172: bool useTexCoords = false, bool intercalate = false) const; On 2014/07/21 22:10:09, jvanverth1 wrote: > 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. Added an enum for the array format that also provides better semantics for the functionality. https://codereview.chromium.org/405163003/diff/1/src/core/SkPatch.cpp File src/core/SkPatch.cpp (right): https://codereview.chromium.org/405163003/diff/1/src/core/SkPatch.cpp#newcode14 src/core/SkPatch.cpp:14: CubicEvaluator::CubicEvaluator(SkPoint a, SkPoint b, SkPoint c, SkPoint d){ On 2014/07/21 21:16:03, egdaniel wrote: > There are already some of the eval and coeff calculations in SkGeometry > (SkGetCubicCoeff, SkEvalCubicAt, etc.). Is it possible for you to use these here > so we are not repeating lots of code? I removed the eval function since we already have an eval_cubic function. Also I changed the name of the evaluator CubicFwDEvaluator because it uses foward differences. And I used the function for calculating the coefficients from SkGeometry https://codereview.chromium.org/405163003/diff/1/src/core/SkPatch.cpp#newcode37 src/core/SkPatch.cpp:37: bool CubicEvaluator::hasNext(){ On 2014/07/21 22:10:10, jvanverth1 wrote: > This and next() are simple enough, they could be put in the header so they can > be inlined. I substituted this ones with the operator* and operator++ respectively and made them inline in the header Done. https://codereview.chromium.org/405163003/diff/1/src/core/SkPatch.cpp#newcode52 src/core/SkPatch.cpp:52: fRes = res; On 2014/07/21 22:10:10, jvanverth1 wrote: > Indent Also is ok in my files, I don't know what happened. https://codereview.chromium.org/405163003/diff/1/src/core/SkPatch.cpp#newcode98 src/core/SkPatch.cpp:98: fIntercalate = intercalate; On 2014/07/21 22:10:09, jvanverth1 wrote: > I don't know this term. Should this be fInterpolate? I couldn't find a nice word to say that the data is an array of objects {{point, color, uv},{point, color, uv}...} or parallel arrays {vertex,vertex} {color, color} {uv,uv}
Fixed the API https://codereview.chromium.org/405163003/diff/80002/gm/patch.cpp File gm/patch.cpp (right): https://codereview.chromium.org/405163003/diff/80002/gm/patch.cpp#newcode41 gm/patch.cpp:41: // This is a GPU-specific GM. The GM is not GPU-specific anymore, should I return 0?
https://codereview.chromium.org/405163003/diff/80002/include/core/SkPatch.h File include/core/SkPatch.h (right): https://codereview.chromium.org/405163003/diff/80002/include/core/SkPatch.h#n... include/core/SkPatch.h:27: void reset(int res); what exactly is the meaning behind res? Maybe I just don't understand exactly what the forward difference is doing. Maybe give a description of what this class is actually doing (and forward differencing) and how you derived the various equations used in reset and other functions https://codereview.chromium.org/405163003/diff/80002/include/core/SkPatch.h#n... include/core/SkPatch.h:35: inline bool operator*() { The * operator seems weird to be used as a bool (usually we use it to actually get an object). Maybe call this done() or something? Also is this even actually used anywhere? Seems like the evaluator is only used inside of setData() in SkPatch and here it looks like you are just looping over the x and y resolutions so you know when it is done. Maybe we just remove this? https://codereview.chromium.org/405163003/diff/80002/include/core/SkPatch.h#n... include/core/SkPatch.h:60: const SkPoint* getCoefs(); Do you have a need for getCoefs() or getResolution()? I don't see them getting called anywhere. Are they things you know you will need later as you hook this in? No reason to add code that is not needed.
Added description of forward differences, changed SkColor (*colors)[4] parameter of SkPatch to SkColor color[4] and adjusted some methods on SkFwDCubicEvaluator. https://codereview.chromium.org/405163003/diff/80002/include/core/SkPatch.h File include/core/SkPatch.h (right): https://codereview.chromium.org/405163003/diff/80002/include/core/SkPatch.h#n... include/core/SkPatch.h:27: void reset(int res); On 2014/07/22 19:09:36, egdaniel wrote: > what exactly is the meaning behind res? Maybe I just don't understand exactly > what the forward difference is doing. Maybe give a description of what this > class is actually doing (and forward differencing) and how you derived the > various equations used in reset and other functions I changed the name to divisions meaning the number of times that the cubic is divided into and added a brief explanation of how they work. https://codereview.chromium.org/405163003/diff/80002/include/core/SkPatch.h#n... include/core/SkPatch.h:35: inline bool operator*() { On 2014/07/22 19:09:35, egdaniel wrote: > The * operator seems weird to be used as a bool (usually we use it to actually > get an object). Maybe call this done() or something? > > Also is this even actually used anywhere? Seems like the evaluator is only used > inside of setData() in SkPatch and here it looks like you are just looping over > the x and y resolutions so you know when it is done. Maybe we just remove this? I adjusted the operator* to get the current value and added a method done to have a way of knowing if the evaluator has finished. Not sure if I will use it. https://codereview.chromium.org/405163003/diff/80002/include/core/SkPatch.h#n... include/core/SkPatch.h:60: const SkPoint* getCoefs(); On 2014/07/22 19:09:35, egdaniel wrote: > Do you have a need for getCoefs() or getResolution()? I don't see them getting > called anywhere. Are they things you know you will need later as you hook this > in? No reason to add code that is not needed. Done. https://codereview.chromium.org/405163003/diff/110001/include/core/SkPatch.h File include/core/SkPatch.h (right): https://codereview.chromium.org/405163003/diff/110001/include/core/SkPatch.h#... include/core/SkPatch.h:100: */ 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. https://codereview.chromium.org/405163003/diff/110001/src/core/SkPatch.cpp File src/core/SkPatch.cpp (right): https://codereview.chromium.org/405163003/diff/110001/src/core/SkPatch.cpp#ne... src/core/SkPatch.cpp:69: fCornerColors[0] = SkPreMultiplyColor(colors[0]); will fix this indent in the next set of changes
I think on the surface this meets Mike's request to get just SkPatch and a test landed as a first pass, before adding the interface to SkCanvas. That said, I'm not sure in the end we'll want to store the rendered verts internally -- we may render the SkPatch at different scales, in which case you'd want to change your tessellation to fit the device space. But that can be changed when you add the interface to SkCanvas. Adding bsalomon@ for his thoughts since reed@ is out of town. https://codereview.chromium.org/405163003/diff/110001/include/core/SkPatch.h File include/core/SkPatch.h (right): https://codereview.chromium.org/405163003/diff/110001/include/core/SkPatch.h#... include/core/SkPatch.h:14: 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). https://codereview.chromium.org/405163003/diff/110001/include/core/SkPatch.h#... include/core/SkPatch.h:56: inline SkPoint operator*() { If this isn't used (it looks like you use getPoints()), I'd remove it. https://codereview.chromium.org/405163003/diff/110001/include/core/SkPatch.h#... include/core/SkPatch.h:69: inline SkPoint operator++(int) { I'm not sure what the point of this operator is...
https://codereview.chromium.org/405163003/diff/110001/gm/patch.cpp File gm/patch.cpp (right): https://codereview.chromium.org/405163003/diff/110001/gm/patch.cpp#newcode32 gm/patch.cpp:32: return SkString("skpatch"); maybe just call it patch? I don't think similar gms use the prefix sk. https://codereview.chromium.org/405163003/diff/110001/include/core/SkPatch.h File include/core/SkPatch.h (right): https://codereview.chromium.org/405163003/diff/110001/include/core/SkPatch.h#... include/core/SkPatch.h:14: On 2014/07/24 17:37:05, jvanverth1 wrote: > 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). Yeah it might not even need to be nested in SkPatch, but just be declared in SkPatch.cpp. https://codereview.chromium.org/405163003/diff/110001/include/core/SkPatch.h#... include/core/SkPatch.h:44: void reset(int divisions); Can we call this restart? We usually use reset() to mean that all the object's state is blown away. https://codereview.chromium.org/405163003/diff/110001/include/core/SkPatch.h#... include/core/SkPatch.h:60: inline SkPoint operator++() { 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. https://codereview.chromium.org/405163003/diff/110001/include/core/SkPatch.h#... include/core/SkPatch.h:93: * (bottom curve) It's kind of weird that the diagram has bottom above top. It's strange but in Skia-land the y-axis points down. https://codereview.chromium.org/405163003/diff/110001/include/core/SkPatch.h#... include/core/SkPatch.h:105: void clean(); I think in Skia we'd normally call this reset(). https://codereview.chromium.org/405163003/diff/110001/include/core/SkPatch.h#... include/core/SkPatch.h:115: void setData(); 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. https://codereview.chromium.org/405163003/diff/110001/src/core/SkPatch.cpp File src/core/SkPatch.cpp (right): https://codereview.chromium.org/405163003/diff/110001/src/core/SkPatch.cpp#ne... src/core/SkPatch.cpp:76: SkDELETE_ARRAY (fPoints); remove space between ARRAY and (
https://codereview.chromium.org/405163003/diff/110001/include/core/SkPatch.h File include/core/SkPatch.h (right): https://codereview.chromium.org/405163003/diff/110001/include/core/SkPatch.h#... include/core/SkPatch.h:60: inline SkPoint operator++() { On 2014/07/24 18:10:23, bsalomon wrote: > 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. My bad, I suggested operator++.
https://codereview.chromium.org/405163003/diff/110001/include/core/SkPatch.h File include/core/SkPatch.h (right): https://codereview.chromium.org/405163003/diff/110001/include/core/SkPatch.h#... include/core/SkPatch.h:11: #include "SkColor.h" 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. https://codereview.chromium.org/405163003/diff/110001/include/core/SkPatch.h#... include/core/SkPatch.h:93: * (bottom curve) 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?
Fixed all the minor changes and removed the ownership of the vertex data from SkPatch and passed it to the caller. I think it kind of looks like the previous API but I agree that the patch should not own it. https://codereview.chromium.org/405163003/diff/110001/gm/patch.cpp File gm/patch.cpp (right): https://codereview.chromium.org/405163003/diff/110001/gm/patch.cpp#newcode32 gm/patch.cpp:32: return SkString("skpatch"); On 2014/07/24 18:10:23, bsalomon wrote: > maybe just call it patch? I don't think similar gms use the prefix sk. I changed to patch_primitive because there is already a slide called patch. https://codereview.chromium.org/405163003/diff/110001/include/core/SkPatch.h File include/core/SkPatch.h (right): https://codereview.chromium.org/405163003/diff/110001/include/core/SkPatch.h#... include/core/SkPatch.h:11: #include "SkColor.h" On 2014/07/24 19:07:38, egdaniel wrote: > 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. To make the forward declaration should I declare the typedef or how does it work when they are not classes? https://codereview.chromium.org/405163003/diff/110001/include/core/SkPatch.h#... include/core/SkPatch.h:14: On 2014/07/24 18:10:23, bsalomon wrote: > On 2014/07/24 17:37:05, jvanverth1 wrote: > > 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). > > Yeah it might not even need to be nested in SkPatch, but just be declared in > SkPatch.cpp. 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. https://codereview.chromium.org/405163003/diff/110001/include/core/SkPatch.h#... include/core/SkPatch.h:44: void reset(int divisions); On 2014/07/24 18:10:23, bsalomon wrote: > Can we call this restart? We usually use reset() to mean that all the object's > state is blown away. Done. https://codereview.chromium.org/405163003/diff/110001/include/core/SkPatch.h#... include/core/SkPatch.h:56: inline SkPoint operator*() { On 2014/07/24 17:37:04, jvanverth1 wrote: > If this isn't used (it looks like you use getPoints()), I'd remove it. I removed the operator* and changed the name to getCtrlPoints to make it clearer. https://codereview.chromium.org/405163003/diff/110001/include/core/SkPatch.h#... include/core/SkPatch.h:60: inline SkPoint operator++() { On 2014/07/24 18:25:21, jvanverth1 wrote: > On 2014/07/24 18:10:23, bsalomon wrote: > > 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. > > My bad, I suggested operator++. Done. https://codereview.chromium.org/405163003/diff/110001/include/core/SkPatch.h#... include/core/SkPatch.h:69: inline SkPoint operator++(int) { On 2014/07/24 17:37:05, jvanverth1 wrote: > I'm not sure what the point of this operator is... It goes away with the operator changed to next() function. https://codereview.chromium.org/405163003/diff/110001/include/core/SkPatch.h#... include/core/SkPatch.h:93: * (bottom curve) On 2014/07/24 19:07:38, egdaniel wrote: > 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? 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. https://codereview.chromium.org/405163003/diff/110001/include/core/SkPatch.h#... include/core/SkPatch.h:105: void clean(); On 2014/07/24 18:10:23, bsalomon wrote: > I think in Skia we'd normally call this reset(). Done. https://codereview.chromium.org/405163003/diff/110001/include/core/SkPatch.h#... include/core/SkPatch.h:115: void setData(); On 2014/07/24 18:10:23, bsalomon wrote: > 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. 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?
https://codereview.chromium.org/405163003/diff/130001/include/core/SkPatch.h File include/core/SkPatch.h (right): https://codereview.chromium.org/405163003/diff/130001/include/core/SkPatch.h#... include/core/SkPatch.h:21: struct PatchData { Maybe a comment here about how this is to be used. Can we use "vertex" instead of "vert"? It better matches the drawVertices param naming. https://codereview.chromium.org/405163003/diff/130001/include/core/SkPatch.h#... include/core/SkPatch.h:46: * (bottom curve) I thought from our discussion yesterday that the top curve had ctrl points 0,1,2,3? Either way, I still think its weird for the diagram to have the bottom curve above the top curve. https://codereview.chromium.org/405163003/diff/130001/include/core/SkPatch.h#... include/core/SkPatch.h:65: const void getTopPoints(SkPoint points[4]) { const void? But I wanna change the void! https://codereview.chromium.org/405163003/diff/130001/src/core/SkPatch.cpp File src/core/SkPatch.cpp (right): https://codereview.chromium.org/405163003/diff/130001/src/core/SkPatch.cpp#ne... src/core/SkPatch.cpp:40: FwDCubicEvaluator(SkPoint a, SkPoint b, SkPoint c, SkPoint d){ tiny nit here and a few other places, space between ) and { https://codereview.chromium.org/405163003/diff/130001/src/core/SkPatch.cpp#ne... src/core/SkPatch.cpp:78: inline bool done() { const? https://codereview.chromium.org/405163003/diff/130001/src/core/SkPatch.cpp#ne... src/core/SkPatch.cpp:106: : fDivX(divisions) Maybe SkPatch shouldn't store the divisions but should just take it as a param to getVertexData.
Added the enum for the control points and inverted the top and bottom curves. Passed the divisions parameter to getVertexData call. https://codereview.chromium.org/405163003/diff/130001/include/core/SkPatch.h File include/core/SkPatch.h (right): https://codereview.chromium.org/405163003/diff/130001/include/core/SkPatch.h#... include/core/SkPatch.h:21: struct PatchData { On 2014/07/25 13:14:34, bsalomon wrote: > Maybe a comment here about how this is to be used. Can we use "vertex" instead > of "vert"? It better matches the drawVertices param naming. Done. https://codereview.chromium.org/405163003/diff/130001/include/core/SkPatch.h#... include/core/SkPatch.h:46: * (bottom curve) On 2014/07/25 13:14:34, bsalomon wrote: > I thought from our discussion yesterday that the top curve had ctrl points > 0,1,2,3? Either way, I still think its weird for the diagram to have the bottom > curve above the top curve. Sorry I uploaded this cl before our discussion. It is already fixed. https://codereview.chromium.org/405163003/diff/130001/include/core/SkPatch.h#... include/core/SkPatch.h:65: const void getTopPoints(SkPoint points[4]) { On 2014/07/25 13:14:34, bsalomon wrote: > const void? But I wanna change the void! Missed those ones, thanks. https://codereview.chromium.org/405163003/diff/130001/src/core/SkPatch.cpp File src/core/SkPatch.cpp (right): https://codereview.chromium.org/405163003/diff/130001/src/core/SkPatch.cpp#ne... src/core/SkPatch.cpp:40: FwDCubicEvaluator(SkPoint a, SkPoint b, SkPoint c, SkPoint d){ On 2014/07/25 13:14:35, bsalomon wrote: > tiny nit here and a few other places, space between ) and { Done. https://codereview.chromium.org/405163003/diff/130001/src/core/SkPatch.cpp#ne... src/core/SkPatch.cpp:78: inline bool done() { On 2014/07/25 13:14:35, bsalomon wrote: > const? Done. Also set to const the return value of the next method. https://codereview.chromium.org/405163003/diff/130001/src/core/SkPatch.cpp#ne... src/core/SkPatch.cpp:106: : fDivX(divisions) On 2014/07/25 13:14:34, bsalomon wrote: > Maybe SkPatch shouldn't store the divisions but should just take it as a param > to getVertexData. Yeah I think it makes more sense that way.
https://codereview.chromium.org/405163003/diff/150001/include/core/SkPatch.h File include/core/SkPatch.h (right): https://codereview.chromium.org/405163003/diff/150001/include/core/SkPatch.h#... include/core/SkPatch.h:23: * as a parameter to the function getVertexData. "The generated arrays can be used with SkCanvas::drawVertices to render the patch"? Or is the idea that there will be a future SkCanvas::drawPatch() (and maybe drawPatchMesh())? If so then maybe drawVertices isn't worth mentioning. https://codereview.chromium.org/405163003/diff/150001/include/core/SkPatch.h#... include/core/SkPatch.h:82: bool getVertexData(SkPatch::PatchData* data, int divisions); Comment about what divisions means? https://codereview.chromium.org/405163003/diff/150001/src/core/SkPatch.cpp File src/core/SkPatch.cpp (right): https://codereview.chromium.org/405163003/diff/150001/src/core/SkPatch.cpp#ne... src/core/SkPatch.cpp:78: inline const bool done() { bool done() const { inline is implied by the fact that the function is defined here and not outside the class definition.
Look likes you may have lost your gyp files along the way https://codereview.chromium.org/405163003/diff/150001/include/core/SkPatch.h File include/core/SkPatch.h (right): https://codereview.chromium.org/405163003/diff/150001/include/core/SkPatch.h#... include/core/SkPatch.h:48: enum SkCubicCtrlPts { Remove the Sk since we are already inside of SkPatch https://codereview.chromium.org/405163003/diff/150001/include/core/SkPatch.h#... include/core/SkPatch.h:49: kTopP0_CubicCtrlPts=0, Nit space around = https://codereview.chromium.org/405163003/diff/150001/src/core/SkPatch.cpp File src/core/SkPatch.cpp (right): https://codereview.chromium.org/405163003/diff/150001/src/core/SkPatch.cpp#ne... src/core/SkPatch.cpp:160: int stride = divY+1; annoying nit: add space around + (and all other operators in rest of code)
Recovered gyp files and made small changes based on comments. https://codereview.chromium.org/405163003/diff/150001/include/core/SkPatch.h File include/core/SkPatch.h (right): https://codereview.chromium.org/405163003/diff/150001/include/core/SkPatch.h#... include/core/SkPatch.h:23: * as a parameter to the function getVertexData. On 2014/07/25 14:16:37, bsalomon wrote: > "The generated arrays can be used with SkCanvas::drawVertices to render the > patch"? Or is the idea that there will be a future SkCanvas::drawPatch() (and > maybe drawPatchMesh())? If so then maybe drawVertices isn't worth mentioning. The idea is to add a SkCanvas::drawPatch. https://codereview.chromium.org/405163003/diff/150001/include/core/SkPatch.h#... include/core/SkPatch.h:48: enum SkCubicCtrlPts { On 2014/07/25 14:49:27, egdaniel wrote: > Remove the Sk since we are already inside of SkPatch Done. https://codereview.chromium.org/405163003/diff/150001/include/core/SkPatch.h#... include/core/SkPatch.h:49: kTopP0_CubicCtrlPts=0, On 2014/07/25 14:49:26, egdaniel wrote: > Nit space around = Done. https://codereview.chromium.org/405163003/diff/150001/include/core/SkPatch.h#... include/core/SkPatch.h:82: bool getVertexData(SkPatch::PatchData* data, int divisions); On 2014/07/25 14:16:37, bsalomon wrote: > Comment about what divisions means? Done. https://codereview.chromium.org/405163003/diff/150001/src/core/SkPatch.cpp File src/core/SkPatch.cpp (right): https://codereview.chromium.org/405163003/diff/150001/src/core/SkPatch.cpp#ne... src/core/SkPatch.cpp:78: inline const bool done() { On 2014/07/25 14:16:37, bsalomon wrote: > bool done() const { > > inline is implied by the fact that the function is defined here and not outside > the class definition. Done. Also made getCtrlPoints const. https://codereview.chromium.org/405163003/diff/150001/src/core/SkPatch.cpp#ne... src/core/SkPatch.cpp:160: int stride = divY+1; On 2014/07/25 14:49:27, egdaniel wrote: > annoying nit: add space around + (and all other operators in rest of code) Done.
lgtm. One optional suggestion: rename PatchData to VertexData, , MeshData, or TessData since it captures a transformation of the patch into a vertex/index mesh rather than the patch itself. https://codereview.chromium.org/405163003/diff/170001/include/core/SkPatch.h File include/core/SkPatch.h (right): https://codereview.chromium.org/405163003/diff/170001/include/core/SkPatch.h#... include/core/SkPatch.h:81: SkPatch(SkPoint points[12], SkColor colors[4]); Not a suggestion for this patch, but I wonder whether we should have explicit colors at all. That seems like one of many ways that the interpolation can be used. We could say this always produces texture/local coordinates when it is rendered and an SkShader could be used to do bilerp between 4 colors. Just a thought.
The CQ bit was checked by dandov@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/dandov@google.com/405163003/190001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: Build-Mac10.8-Clang-x86-Release-Trybot on tryserver.skia (http://108.170.220.76:10117/builders/Build-Mac10.8-Clang-x86-Release-Trybot/b...) Build-Ubuntu13.10-Clang-x86_64-Debug-Trybot on tryserver.skia (http://108.170.220.76:10117/builders/Build-Ubuntu13.10-Clang-x86_64-Debug-Try...) Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Android-Trybot on tryserver.skia (http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Andr...) Build-Ubuntu13.10-GCC4.8-x86_64-Release-Trybot on tryserver.skia (http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-x86_64-Release-...) Test-Ubuntu13.10-ShuttleA-NoGPU-x86_64-Debug-Trybot on tryserver.skia (http://108.170.220.120:10117/builders/Test-Ubuntu13.10-ShuttleA-NoGPU-x86_64-...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Test-Ubuntu13.10-ShuttleA-NoGPU-x86_64-Debug-Trybot on tryserver.skia (http://108.170.220.120:10117/builders/Test-Ubuntu13.10-ShuttleA-NoGPU-x86_64-...)
The CQ bit was checked by dandov@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/dandov@google.com/405163003/230001
Message was sent while issue was closed.
Change committed as 50d715476b1d3a00fb43c13e34a80ea0a01d32bf |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
