As part of the GPU accelerated path rendering algorithm in https://bugs.webkit.org/show_bug.cgi?id=44729 , code which computes the three-dimensional texture coordinates per cubic curve control point needs to be added.
Created attachment 66609 [details] Patch From the ChangeLog: Adding the texture coordinate computation for cubic curves per the GPU Gems 3 chapter. No tests yet; will be tested in conjunction with later code.
Comment on attachment 66609 [details] Patch r- for nits again, looks good overall. View in context: https://bugs.webkit.org/attachment.cgi?id=66609&action=prettypatch > WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.cpp:36 > +using namespace LoopBlinnMathUtils; Check if this is being used any more and remove if not. > WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.cpp:38 > +void LoopBlinnTextureCoords::Compute(const LoopBlinnClassifier::Result& c, Should be 'compute'. Use a more descriptive name than 'c' > WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.cpp:51 > + const float OneThird = 1.0f / 3.0f; > + const float TwoThirds = 2.0f / 3.0f; These should be static > WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.cpp:76 > + result->coords[1] = FloatPoint3D(OneThird * (3.0f * ls * ms - > + ls * mt - > + lt * ms), > + ls * ls * (ls - lt), > + ms * ms * (ms - mt)); > + result->coords[2] = FloatPoint3D(OneThird * (lt * (mt - 2.0f * ms) + > + ls * (3.0f * ms - 2.0f * mt)), Try to avoid line wrapping in the middle of an expression. > WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.cpp:101 > + // TODO(kbr): may require more work? I think this TODO is no longer needed. Looks like we should return immediately here, or have a more descriptive FIXME. > WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.cpp:108 > + // TODO(kbr): may require more work? Same as above. > WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.cpp:134 > + reverseOrientation = > + ((c.d1() > 0.0f && result->coords[0].x() < 0.0f) > + || (c.d1() < 0.0f && result->coords[0].x() > 0.0f)); nit: this wraps oddly > WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.cpp:183 > + result->coords[i].setX(-result->coords[i].x()); > + result->coords[i].setY(-result->coords[i].y()); Add a comment or rename the type so it's clearer that the X and Y here are actually the K and L coordinates from the paper. > WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.h:71 > + static void Compute(const LoopBlinnClassifier::Result& classification, Should be compute (lowercase). > WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.h:72 > + bool fillRightSide, Can this be an enum to make callsites clearer? > WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.h:73 > + Result* result); This should return a Result.
Created attachment 67076 [details] Revised patch Addressed review feedback above.
Attachment 67076 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.cpp:114: Extra space after ( [whitespace/parens] [2] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #2) > (From update of attachment 66609 [details]) > r- for nits again, looks good overall. > > View in context: https://bugs.webkit.org/attachment.cgi?id=66609&action=prettypatch > > > WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.cpp:36 > > +using namespace LoopBlinnMathUtils; > Check if this is being used any more and remove if not. Not being used any more. Removed. > > WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.cpp:38 > > +void LoopBlinnTextureCoords::Compute(const LoopBlinnClassifier::Result& c, > Should be 'compute'. Use a more descriptive name than 'c' Done. > > WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.cpp:51 > > + const float OneThird = 1.0f / 3.0f; > > + const float TwoThirds = 2.0f / 3.0f; > These should be static Done. > > WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.cpp:76 > > + result->coords[1] = FloatPoint3D(OneThird * (3.0f * ls * ms - > > + ls * mt - > > + lt * ms), > > + ls * ls * (ls - lt), > > + ms * ms * (ms - mt)); > > + result->coords[2] = FloatPoint3D(OneThird * (lt * (mt - 2.0f * ms) + > > + ls * (3.0f * ms - 2.0f * mt)), > Try to avoid line wrapping in the middle of an expression. Done. > > WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.cpp:101 > > + // TODO(kbr): may require more work? > I think this TODO is no longer needed. Looks like we should return immediately here, or have a more descriptive FIXME. Agreed; removed and re-tested. > > WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.cpp:108 > > + // TODO(kbr): may require more work? > Same as above. Done. > > WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.cpp:134 > > + reverseOrientation = > > + ((c.d1() > 0.0f && result->coords[0].x() < 0.0f) > > + || (c.d1() < 0.0f && result->coords[0].x() > 0.0f)); > nit: this wraps oddly Changed. Emacs doesn't auto-format it correctly but it does line up nicer. > > WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.cpp:183 > > + result->coords[i].setX(-result->coords[i].x()); > > + result->coords[i].setY(-result->coords[i].y()); > Add a comment or rename the type so it's clearer that the X and Y here are actually the K and L coordinates from the paper. Renamed coords to klmCoordinates. > > WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.h:71 > > + static void Compute(const LoopBlinnClassifier::Result& classification, > Should be compute (lowercase). Done. > > WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.h:72 > > + bool fillRightSide, > Can this be an enum to make callsites clearer? Made into an enum in LoopBlinnConstants.h (shared between this code and other coming code). > > WebCore/platform/graphics/gpu/LoopBlinnTextureCoords.h:73 > > + Result* result); > This should return a Result. Done.
Created attachment 67078 [details] Revised patch Did the abovementioned reformatting in a way that doesn't make the style bot complain.
Comment on attachment 67078 [details] Revised patch R=me. I was about to suggest that exact same change to get the style bot to shut up.
Committed r67104: <http://trac.webkit.org/changeset/67104>