[webkit-reviews] review denied: [Bug 45251] Add math utilities for cubic curve processing : [Attachment 66610] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Kenneth Russell
<kbr at google.com>'s request for review:
Bug 45251: Add math utilities for cubic curve processing
https://bugs.webkit.org/show_bug.cgi?id=45251

Attachment 66610: Patch
https://bugs.webkit.org/attachment.cgi?id=66610&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context:
https://bugs.webkit.org/attachment.cgi?id=66610&action=prettypatch

> WebCore/platform/graphics/gpu/LoopBlinnMathUtils.cpp:129
> +    float x0 = c.x() - a.x();
> +    float y0 = c.y() - a.y();
> +    float x1 = b.x() - a.x();
> +    float y1 = b.y() - a.y();
> +    float x2 = point.x() - a.x();
> +    float y2 = point.y() - a.y();
> +
> +    float dot00 = x0 * x0 + y0 * y0;
> +    float dot01 = x0 * x1 + y0 * y1;
> +    float dot02 = x0 * x2 + y0 * y2;
> +    float dot11 = x1 * x1 + y1 * y1;
> +    float dot12 = x1 * x2 + y1 * y2;
Could this use FloatPoint's dot product?

> WebCore/platform/graphics/gpu/LoopBlinnMathUtils.cpp:177
> +// Helper routines for public XRay queries below.
Could you add a link to the original source of these routines (presumably
skia)?

> WebCore/platform/graphics/gpu/LoopBlinnMathUtils.cpp:181
> +bool nearlyZero(float x, float tolerance = NearlyZeroConstant)
Seems redundant with approxEqual()

> WebCore/platform/graphics/gpu/LoopBlinnMathUtils.cpp:190
> +inline float interpolate(float a, float b, float t)
nit: drop the inline, I believe it'll be ignored.

> WebCore/platform/graphics/gpu/LoopBlinnMathUtils.cpp:196
> +float evaluateCubic(float s0, float s2, float s4, float s6, float t)
nit: rename s0, s2, s4, s6 to something else.

> WebCore/platform/graphics/gpu/LoopBlinnMathUtils.cpp:217
> +bool evaluateCubicAt(const FloatPoint cubic[4], float t, FloatPoint& pt)
> +{
> +    pt.set(evaluateCubic(cubic[0].x(), cubic[1].x(), cubic[2].x(),
cubic[3].x(), t),
> +	      evaluateCubic(cubic[0].y(), cubic[1].y(), cubic[2].y(),
cubic[3].y(), t));
> +}
This should return a FloatPoint.

> WebCore/platform/graphics/gpu/LoopBlinnMathUtils.cpp:262
> +    FloatPoint evaluate;
evaluatedPoint? interpolatedPoint?

> WebCore/platform/graphics/gpu/LoopBlinnMathUtils.cpp:284
> +    if (xRay.x() <= evaluate.x()) {
Add a FIXME: here to check if this test should be fuzzy once we have more
regression tests for this code.

> WebCore/platform/graphics/gpu/LoopBlinnMathUtils.cpp:291
> +int validUnitDivide(float numer, float denom, float* ratio)
numerator, denominator

> WebCore/platform/graphics/gpu/LoopBlinnMathUtils.cpp:337
> +	   else if (roots[0] == roots[1]) // nearly-equal?
comment isn't a sentence.  Should this be a FIXME?

> WebCore/platform/graphics/gpu/LoopBlinnMathUtils.cpp:346
> +    Solve for t, keeping only those that fit betwee 0 < t < 1
typo: between

> WebCore/platform/graphics/gpu/LoopBlinnMathUtils.cpp:421
> +	   memcpy(dst, src, 4*sizeof(FloatPoint));
should loop through here and do dst[i] = src[i] to invoke FloatPoint's copy
constructor.

> WebCore/platform/graphics/gpu/LoopBlinnMathUtils.cpp:435
> +	   memcpy(tmp, dst, 4 * sizeof(FloatPoint));
same as above

> WebCore/platform/graphics/gpu/LoopBlinnMathUtils.cpp:438
> +	   // watch out in case the renormalized t isn't in range
Needs to be a sentence

> WebCore/platform/graphics/gpu/LoopBlinnMathUtils.cpp:440
> +				1 - tValues[i], &t)) {
1.0f

> WebCore/platform/graphics/gpu/LoopBlinnMathUtils.cpp:442
> +	       dst[4] = dst[5] = dst[6] = src[3];
check the values here, the dst indices look off.

> WebCore/platform/graphics/gpu/LoopBlinnMathUtils.cpp:460
> +    if (dst && roots > 0) {
the null check for dst isn't needed here.  Also, just check for 'roots' rather
than 'roots > 0'

> WebCore/platform/graphics/gpu/LoopBlinnMathUtils.cpp:492
> +    // intersect, for symmetry with SkXRayCrossesMonotonicCubic.
SkXRayCross... -> xRayCross..

> WebCore/platform/graphics/gpu/LoopBlinnMathUtils.cpp:530
> +    return xRay.x() <= x;
add a FIXME here to check if this should be a fuzzy check

> WebCore/platform/graphics/gpu/LoopBlinnMathUtils.cpp:533
> +int numXRayCrossingsForCubic(const XRay& pt, const FloatPoint cubic[4],
bool& ambiguous)
pt -> xRay

> WebCore/platform/graphics/gpu/LoopBlinnMathUtils.cpp:550
> +    if (xRayCrossesMonotonicCubic(pt, &monotonicCubics[0],
locallyAmbiguous))
> +	   ++numCrossings;
> +    ambiguous |= locallyAmbiguous;
> +    if (numMonotonicCubics > 0)
> +	   if (xRayCrossesMonotonicCubic(pt, &monotonicCubics[3],
locallyAmbiguous))
> +	       ++numCrossings;
> +    ambiguous |= locallyAmbiguous;
> +    if (numMonotonicCubics > 1)
> +	   if (xRayCrossesMonotonicCubic(pt, &monotonicCubics[6],
locallyAmbiguous))
> +	       ++numCrossings;
> +    ambiguous |= locallyAmbiguous;
Make this a loop over [0,numMonotonicCubics) and early-out if it detects
ambiguity.

> WebCore/platform/graphics/gpu/LoopBlinnMathUtils.h:34
> +class FloatPoint;
This has to be pulled in via in #include explicitly rather than forward
declared since we call functions on FloatPoints (like diagonalLengthSquared()).


> WebCore/platform/graphics/gpu/LoopBlinnMathUtils.h:43
> +const float Epsilon = 5.0e-4f;
I think this needs a storage declaration in the .cpp.