Bug 45250 – Add cubic texture coordinate computation

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Kenneth Russell]

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.

[Kenneth Russell]

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.

[James Robinson]

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.

[Kenneth Russell]

Created attachment 67076 [details]
Revised patch

Addressed review feedback above.

[WebKit Review Bot]

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.

[Kenneth Russell]

(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.

[Kenneth Russell]

Created attachment 67078 [details]
Revised patch

Did the abovementioned reformatting in a way that doesn't make the style bot complain.

[James Robinson]

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.

[Kenneth Russell]

Committed r67104: <http://trac.webkit.org/changeset/67104>