From: epoger@google.com Date: Wed, 11 Apr 2012 17:51:01 +0000 (+0000) Subject: Fix SkPathStroker::lineTo() for line with length SK_ScalarNearlyZero X-Git-Tag: accepted/tizen/5.0/unified/20181102.025319~16431 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=94fa43c6255906660c2ff001fb462b6492cbdc07;p=platform%2Fupstream%2FlibSkiaSharp.git Fix SkPathStroker::lineTo() for line with length SK_ScalarNearlyZero Review URL: https://codereview.appspot.com/5992077 git-svn-id: http://skia.googlecode.com/svn/trunk@3650 2bbb7eff-a529-9590-31e7-b0007b416f81 --- diff --git a/include/core/SkPath.h b/include/core/SkPath.h index 957d50e..01b5488 100644 --- a/include/core/SkPath.h +++ b/include/core/SkPath.h @@ -185,7 +185,7 @@ public: @return true if the line is of zero length; otherwise false. */ static bool IsLineDegenerate(const SkPoint& p1, const SkPoint& p2) { - return p1.equalsWithinTolerance(p2, SK_ScalarNearlyZero); + return p1.equalsWithinTolerance(p2); } /** Test a quad for zero length @@ -194,8 +194,8 @@ public: */ static bool IsQuadDegenerate(const SkPoint& p1, const SkPoint& p2, const SkPoint& p3) { - return p1.equalsWithinTolerance(p2, SK_ScalarNearlyZero) && - p2.equalsWithinTolerance(p3, SK_ScalarNearlyZero); + return p1.equalsWithinTolerance(p2) && + p2.equalsWithinTolerance(p3); } /** Test a cubic curve for zero length @@ -204,9 +204,9 @@ public: */ static bool IsCubicDegenerate(const SkPoint& p1, const SkPoint& p2, const SkPoint& p3, const SkPoint& p4) { - return p1.equalsWithinTolerance(p2, SK_ScalarNearlyZero) && - p2.equalsWithinTolerance(p3, SK_ScalarNearlyZero) && - p3.equalsWithinTolerance(p4, SK_ScalarNearlyZero); + return p1.equalsWithinTolerance(p2) && + p2.equalsWithinTolerance(p3) && + p3.equalsWithinTolerance(p4); } /** Returns true if the path specifies a rectangle. If so, and if rect is diff --git a/include/core/SkPoint.h b/include/core/SkPoint.h index d371e64..4570e25 100644 --- a/include/core/SkPoint.h +++ b/include/core/SkPoint.h @@ -315,11 +315,29 @@ struct SK_API SkPoint { return a.fX != b.fX || a.fY != b.fY; } - /** Return true if this and the given point are componentwise within tol. + /** Return true if this point and the given point are far enough apart + such that a vector between them would be non-degenerate. + + WARNING: Unlike the deprecated version of equalsWithinTolerance(), + this method does not use componentwise comparison. Instead, it + uses a comparison designed to match judgments elsewhere regarding + degeneracy ("points A and B are so close that the vector between them + is essentially zero"). + */ + bool equalsWithinTolerance(const SkPoint& p) const { + return !CanNormalize(fX - p.fX, fY - p.fY); + } + + /** DEPRECATED: Return true if this and the given point are componentwise + within tolerance "tol". + + WARNING: There is no guarantee that the result will reflect judgments + elsewhere regarding degeneracy ("points A and B are so close that the + vector between them is essentially zero"). */ - bool equalsWithinTolerance(const SkPoint& v, SkScalar tol) const { - return SkScalarNearlyZero(fX - v.fX, tol) - && SkScalarNearlyZero(fY - v.fY, tol); + bool equalsWithinTolerance(const SkPoint& p, SkScalar tol) const { + return SkScalarNearlyZero(fX - p.fX, tol) + && SkScalarNearlyZero(fY - p.fY, tol); } /** Returns a new point whose coordinates are the difference between diff --git a/src/core/SkPoint.cpp b/src/core/SkPoint.cpp index 5747504..a347b86 100644 --- a/src/core/SkPoint.cpp +++ b/src/core/SkPoint.cpp @@ -87,31 +87,49 @@ bool SkPoint::setLength(SkScalar length) { return this->setLength(fX, fY, length); } +#ifdef SK_SCALAR_IS_FLOAT + +// Returns the square of the Euclidian distance to (dx,dy). +static inline float getLengthSquared(float dx, float dy) { + return dx * dx + dy * dy; +} + +// Calculates the square of the Euclidian distance to (dx,dy) and stores it in +// *lengthSquared. Returns true if the distance is judged to be "nearly zero". +// +// This logic is encapsulated in a helper method to make it explicit that we +// always perform this check in the same manner, to avoid inconsistencies +// (see http://code.google.com/p/skia/issues/detail?id=560 ). +static inline bool isLengthNearlyZero(float dx, float dy, + float *lengthSquared) { + *lengthSquared = getLengthSquared(dx, dy); + return *lengthSquared <= (SK_ScalarNearlyZero * SK_ScalarNearlyZero); +} + SkScalar SkPoint::Normalize(SkPoint* pt) { - SkScalar mag = SkPoint::Length(pt->fX, pt->fY); - if (mag > SK_ScalarNearlyZero) { - SkScalar scale = SkScalarInvert(mag); - pt->fX = SkScalarMul(pt->fX, scale); - pt->fY = SkScalarMul(pt->fY, scale); + float mag2; + if (!isLengthNearlyZero(pt->fX, pt->fY, &mag2)) { + float mag = sk_float_sqrt(mag2); + float scale = 1.0 / mag; + pt->fX = pt->fX * scale; + pt->fY = pt->fY * scale; return mag; } return 0; } -#ifdef SK_SCALAR_IS_FLOAT - bool SkPoint::CanNormalize(SkScalar dx, SkScalar dy) { - float mag2 = dx * dx + dy * dy; - return mag2 > SK_ScalarNearlyZero * SK_ScalarNearlyZero; + float mag2_unused; + return !isLengthNearlyZero(dx, dy, &mag2_unused); } SkScalar SkPoint::Length(SkScalar dx, SkScalar dy) { - return sk_float_sqrt(dx * dx + dy * dy); + return sk_float_sqrt(getLengthSquared(dx, dy)); } bool SkPoint::setLength(float x, float y, float length) { - float mag2 = x * x + y * y; - if (mag2 > SK_ScalarNearlyZero * SK_ScalarNearlyZero) { + float mag2; + if (!isLengthNearlyZero(x, y, &mag2)) { float scale = length / sk_float_sqrt(mag2); fX = x * scale; fY = y * scale; @@ -124,12 +142,25 @@ bool SkPoint::setLength(float x, float y, float length) { #include "Sk64.h" -bool SkPoint::CanNormalize(SkScalar dx, SkScalar dy) { - Sk64 tmp1, tmp2, tolSqr; - - tmp1.setMul(dx, dx); - tmp2.setMul(dy, dy); - tmp1.add(tmp2); +// Returns the square of the Euclidian distance to (dx,dy) in *result. +static inline void getLengthSquared(SkScalar dx, SkScalar dy, Sk64 *result) { + Sk64 dySqr; + + result->setMul(dx, dx); + dySqr.setMul(dy, dy); + result->add(dySqr); +} + +// Calculates the square of the Euclidian distance to (dx,dy) and stores it in +// *lengthSquared. Returns true if the distance is judged to be "nearly zero". +// +// This logic is encapsulated in a helper method to make it explicit that we +// always perform this check in the same manner, to avoid inconsistencies +// (see http://code.google.com/p/skia/issues/detail?id=560 ). +static inline bool isLengthNearlyZero(SkScalar dx, SkScalar dy, + Sk64 *lengthSquared) { + Sk64 tolSqr; + getLengthSquared(dx, dy, lengthSquared); // we want nearlyzero^2, but to compute it fast we want to just do a // 32bit multiply, so we require that it not exceed 31bits. That is true @@ -138,17 +169,30 @@ bool SkPoint::CanNormalize(SkScalar dx, SkScalar dy) { SkASSERT(SK_ScalarNearlyZero <= 0xB504); tolSqr.set(0, SK_ScalarNearlyZero * SK_ScalarNearlyZero); - return tmp1 > tolSqr; + return *lengthSquared <= tolSqr; } -SkScalar SkPoint::Length(SkScalar dx, SkScalar dy) { - Sk64 tmp1, tmp2; +SkScalar SkPoint::Normalize(SkPoint* pt) { + Sk64 mag2; + if (!isLengthNearlyZero(pt->fX, pt->fY, &mag2)) { + SkScalar mag = mag2.getSqrt(); + SkScalar scale = SkScalarInvert(mag); + pt->fX = SkScalarMul(pt->fX, scale); + pt->fY = SkScalarMul(pt->fY, scale); + return mag; + } + return 0; +} - tmp1.setMul(dx, dx); - tmp2.setMul(dy, dy); - tmp1.add(tmp2); +bool SkPoint::CanNormalize(SkScalar dx, SkScalar dy) { + Sk64 mag2_unused; + return !isLengthNearlyZero(dx, dy, &mag2_unused); +} - return tmp1.getSqrt(); +SkScalar SkPoint::Length(SkScalar dx, SkScalar dy) { + Sk64 tmp; + getLengthSquared(dx, dy, &tmp); + return tmp.getSqrt(); } #ifdef SK_DEBUGx diff --git a/tests/CanvasTest.cpp b/tests/CanvasTest.cpp index 7cafda2..65a3494 100644 --- a/tests/CanvasTest.cpp +++ b/tests/CanvasTest.cpp @@ -286,6 +286,29 @@ SIMPLE_TEST_STEP(DrawData, drawData(kTestText.c_str(), kTestText.size())); /////////////////////////////////////////////////////////////////////////////// // Complex test steps +// exercise fix for http://code.google.com/p/skia/issues/detail?id=560 +// ('SkPathStroker::lineTo() fails for line with length SK_ScalarNearlyZero') +static void DrawNearlyZeroLengthPathTestStep(SkCanvas* canvas, + skiatest::Reporter* reporter, + CanvasTestStep* testStep) { + SkPaint paint; + paint.setStrokeWidth(SkIntToScalar(1)); + paint.setStyle(SkPaint::kStroke_Style); + + SkPath path; + SkPoint pt1 = { 0, 0 }; + SkPoint pt2 = { 0, SK_ScalarNearlyZero }; + SkPoint pt3 = { SkIntToScalar(1), 0 }; + SkPoint pt4 = { SkIntToScalar(1), SK_ScalarNearlyZero/2 }; + path.moveTo(pt1); + path.lineTo(pt2); + path.lineTo(pt3); + path.lineTo(pt4); + + canvas->drawPath(path, paint); +} +TEST_STEP(DrawNearlyZeroLengthPath, DrawNearlyZeroLengthPathTestStep); + static void DrawVerticesShaderTestStep(SkCanvas* canvas, skiatest::Reporter* reporter, CanvasTestStep* testStep) {