Don't canonicalize empty SkRRects. They stroke differently.

Make insetting greater than width or height collapse to a point/line.

SkPath::addRRect() doesn't ignore an empty SkRRect.


Change-Id: I933a3419a6d75be534f1d8328faa715772045f67
Reviewed-on: https://skia-review.googlesource.com/85680
Reviewed-by: Mike Reed <reed@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>

8 files changed, 101 insertions, 73 deletions

diff --git a/include/core/SkRRect.h b/include/core/SkRRect.h
index 06edab9245..798dd47f13 100644
--- a/include/core/SkRRect.h
+++ b/include/core/SkRRect.h
@@ -48,7 +48,9 @@ class SkWBuffer;
 */
 class SK_API SkRRect {
 public:
-    SkRRect() { this->setEmpty(); }
+    /** Default initialized to a rrect at the origin with zero width and height. */
+    SkRRect() = default;
+
     SkRRect(const SkRRect&) = default;
     SkRRect& operator=(const SkRRect&) = default;
 
@@ -57,7 +59,7 @@ public:
      * by type(). The subtypes become progressively less restrictive.
      */
     enum Type {
-        // !< The RR is empty
+        // !< The RR has zero width and/or zero height. All radii are zero.
         kEmpty_Type,
 
         //!< The RR is actually a (non-empty) rect (i.e., at least one radius
@@ -120,25 +122,15 @@ public:
     SkScalar height() const { return fRect.height(); }
 
     /**
-     * Set this RR to the empty rectangle (0,0,0,0) with 0 x & y radii.
+     * Same as default initialized - zero width and height at the origin.
      */
-    void setEmpty() {
-        fRect.setEmpty();
-        memset(fRadii, 0, sizeof(fRadii));
-        fType = kEmpty_Type;
-
-        SkASSERT(this->isValid());
-    }
+    void setEmpty() { *this = SkRRect(); }
 
     /**
      * Set this RR to match the supplied rect. All radii will be 0.
      */
     void setRect(const SkRect& rect) {
-        fRect = rect;
-        fRect.sort();
-
-        if (fRect.isEmpty() || !fRect.isFinite()) {
-            this->setEmpty();
+        if (!this->initializeRect(rect)) {
             return;
         }
 
@@ -148,11 +140,8 @@ public:
         SkASSERT(this->isValid());
     }
 
-    static SkRRect MakeEmpty() {
-        SkRRect rr;
-        rr.setEmpty();
-        return rr;
-    }
+    /** Makes an empty rrect at the origin with zero width and height. */
+    static SkRRect MakeEmpty() { return SkRRect(); }
 
     static SkRRect MakeRect(const SkRect& r) {
         SkRRect rr;
@@ -177,11 +166,7 @@ public:
      * width and all y radii will equal half the height.
      */
     void setOval(const SkRect& oval) {
-        fRect = oval;
-        fRect.sort();
-
-        if (fRect.isEmpty() || !fRect.isFinite()) {
-            this->setEmpty();
+        if (!this->initializeRect(oval)) {
             return;
         }
 
@@ -247,6 +232,12 @@ public:
      *  otherwise we grow/shrink the radii by the amount of the inset. If a
      *  given radius becomes negative, it is pinned to 0.
      *
+     *  If the inset amount is larger than the width/height then the rrect collapses to
+     *  a degenerate line or point.
+     *
+     *  If the inset is sufficiently negative to cause the bounds to become infinite then
+     *  the result is a default initialized rrect.
+     *
      *  It is valid for dst == this.
      */
     void inset(SkScalar dx, SkScalar dy, SkRRect* dst) const;
@@ -337,18 +328,23 @@ private:
         , fRadii{radii[0], radii[1], radii[2], radii[3]}
         , fType(type) {}
 
-    SkRect fRect;
-    // Radii order is UL, UR, LR, LL. Use Corner enum to index into fRadii[]
-    SkVector fRadii[4];
-    // use an explicitly sized type so we're sure the class is dense (no uninitialized bytes)
-    int32_t fType;
-    // TODO: add padding so we can use memcpy for flattening and not copy
-    // uninitialized data
+    /**
+     * Initializes fRect. If the passed in rect is not finite or empty the rrect will be fully
+     * initialized and false is returned. Otherwise, just fRect is initialized and true is returned.
+     */
+    bool initializeRect(const SkRect&);
 
     void computeType();
     bool checkCornerContainment(SkScalar x, SkScalar y) const;
     void scaleRadii();
 
+    SkRect fRect = SkRect::MakeEmpty();
+    // Radii order is UL, UR, LR, LL. Use Corner enum to index into fRadii[]
+    SkVector fRadii[4] = {{0, 0}, {0, 0}, {0,0}, {0,0}};
+    // use an explicitly sized type so we're sure the class is dense (no uninitialized bytes)
+    int32_t fType = kEmpty_Type;
+    // TODO: add padding so we can use memcpy for flattening and not copy uninitialized data
+
     // to access fRadii directly
     friend class SkPath;
 };
diff --git a/src/core/SkPath.cpp b/src/core/SkPath.cpp
index 422c2b911d..ff0152bb4a 100644
--- a/src/core/SkPath.cpp
+++ b/src/core/SkPath.cpp
@@ -1127,14 +1127,10 @@ void SkPath::addRRect(const SkRRect& rrect, Direction dir) {
 void SkPath::addRRect(const SkRRect &rrect, Direction dir, unsigned startIndex) {
         assert_known_direction(dir);
 
-        if (rrect.isEmpty()) {
-            return;
-        }
-
         bool isRRect = hasOnlyMoveTos();
         const SkRect& bounds = rrect.getBounds();
 
-        if (rrect.isRect()) {
+        if (rrect.isRect() || rrect.isEmpty()) {
             // degenerate(rect) => radii points are collapsing
             this->addRect(bounds, dir, (startIndex + 1) / 2);
         } else if (rrect.isOval()) {
diff --git a/src/core/SkRRect.cpp b/src/core/SkRRect.cpp
index 63fd8ccfcd..7cdac3b512 100644
--- a/src/core/SkRRect.cpp
+++ b/src/core/SkRRect.cpp
@@ -7,6 +7,7 @@
 
 #include <cmath>
 #include "SkRRect.h"
+#include "SkScopeExit.h"
 #include "SkBuffer.h"
 #include "SkMatrix.h"
 #include "SkScaleToSides.h"
@@ -14,11 +15,7 @@
 ///////////////////////////////////////////////////////////////////////////////
 
 void SkRRect::setRectXY(const SkRect& rect, SkScalar xRad, SkScalar yRad) {
-    fRect = rect;
-    fRect.sort();
-
-    if (fRect.isEmpty() || !fRect.isFinite()) {
-        this->setEmpty();
+    if (!this->initializeRect(rect)) {
         return;
     }
 
@@ -52,11 +49,7 @@ void SkRRect::setRectXY(const SkRect& rect, SkScalar xRad, SkScalar yRad) {
 
 void SkRRect::setNinePatch(const SkRect& rect, SkScalar leftRad, SkScalar topRad,
                            SkScalar rightRad, SkScalar bottomRad) {
-    fRect = rect;
-    fRect.sort();
-
-    if (fRect.isEmpty() || !fRect.isFinite()) {
-        this->setEmpty();
+    if (!this->initializeRect(rect)) {
         return;
     }
 
@@ -123,11 +116,7 @@ static double compute_min_scale(double rad1, double rad2, double limit, double c
 }
 
 void SkRRect::setRectRadii(const SkRect& rect, const SkVector radii[4]) {
-    fRect = rect;
-    fRect.sort();
-
-    if (fRect.isEmpty() || !fRect.isFinite()) {
-        this->setEmpty();
+    if (!this->initializeRect(rect)) {
         return;
     }
 
@@ -162,6 +151,21 @@ void SkRRect::setRectRadii(const SkRect& rect, const SkVector radii[4]) {
     this->scaleRadii();
 }
 
+bool SkRRect::initializeRect(const SkRect& rect) {
+    // Check this before sorting because sorting can hide nans.
+    if (!rect.isFinite()) {
+        *this = SkRRect();
+        return false;
+    }
+    fRect = rect.makeSorted();
+    if (fRect.isEmpty()) {
+        memset(fRadii, 0, sizeof(fRadii));
+        fType = kEmpty_Type;
+        return false;
+    }
+    return true;
+}
+
 void SkRRect::scaleRadii() {
 
     // Proportionally scale down all radii to fit. Find the minimum ratio
@@ -289,13 +293,13 @@ static bool radii_are_nine_patch(const SkVector radii[4]) {
 
 // There is a simplified version of this method in setRectXY
 void SkRRect::computeType() {
-    struct Validator {
-        Validator(const SkRRect* r) : fR(r) {}
-        ~Validator() { SkASSERT(fR->isValid()); }
-        const SkRRect* fR;
-    } autoValidate(this);
+    SK_AT_SCOPE_EXIT(SkASSERT(this->isValid()));
 
     if (fRect.isEmpty()) {
+        SkASSERT(fRect.isSorted());
+        for (size_t i = 0; i < SK_ARRAY_COUNT(fRadii); ++i) {
+            SkASSERT((fRadii[i] == SkVector{0, 0}));
+        }
         fType = kEmpty_Type;
         return;
     }
@@ -369,10 +373,11 @@ bool SkRRect::transform(const SkMatrix& matrix, SkRRect* dst) const {
     }
 
     // The matrix may have scaled us to zero (or due to float madness, we now have collapsed
-    // some dimension of the rect, so we need to check for that.
-    if (newRect.isEmpty()) {
-        dst->setEmpty();
-        return true;
+    // some dimension of the rect, so we need to check for that. Note that matrix must be
+    // scale and translate and mapRect() produces a sorted rect. So an empty rect indicates
+    // loss of precision.
+    if (!newRect.isFinite() || newRect.isEmpty()) {
+        return false;
     }
 
     // At this point, this is guaranteed to succeed, so we can modify dst.
@@ -431,6 +436,7 @@ bool SkRRect::transform(const SkMatrix& matrix, SkRRect* dst) const {
     }
 
     dst->scaleRadii();
+    dst->isValid();
 
     return true;
 }
@@ -438,10 +444,24 @@ bool SkRRect::transform(const SkMatrix& matrix, SkRRect* dst) const {
 ///////////////////////////////////////////////////////////////////////////////
 
 void SkRRect::inset(SkScalar dx, SkScalar dy, SkRRect* dst) const {
-    const SkRect r = fRect.makeInset(dx, dy);
-
-    if (r.isEmpty()) {
-        dst->setEmpty();
+    SkRect r = fRect.makeInset(dx, dy);
+    bool degenerate = false;
+    if (r.fRight <= r.fLeft) {
+        degenerate = true;
+        r.fLeft = r.fRight = SkScalarAve(r.fLeft, r.fRight);
+    }
+    if (r.fBottom <= r.fTop) {
+        degenerate = true;
+        r.fTop = r.fBottom = SkScalarAve(r.fTop, r.fBottom);
+    }
+    if (degenerate) {
+        dst->fRect = r;
+        memset(dst->fRadii, 0, sizeof(dst->fRadii));
+        dst->fType = kEmpty_Type;
+        return;
+    }
+    if (!r.isFinite()) {
+        *dst = SkRRect();
         return;
     }
 
@@ -608,7 +628,7 @@ bool SkRRect::isValid() const {
 }
 
 bool SkRRect::AreRectAndRadiiValid(const SkRect& rect, const SkVector radii[4]) {
-    if (!rect.isFinite()) {
+    if (!rect.isFinite() || !rect.isSorted()) {
         return false;
     }
     for (int i = 0; i < 4; ++i) {
diff --git a/src/core/SkScopeExit.h b/src/core/SkScopeExit.h
index 95804e637a..bdec7b34f0 100644
--- a/src/core/SkScopeExit.h
+++ b/src/core/SkScopeExit.h
@@ -9,6 +9,7 @@
 #define SkScopeExit_DEFINED
 
 #include "SkTypes.h"
+#include <functional>
 
 /** SkScopeExit calls a std:::function<void()> in its destructor. */
 class SkScopeExit {
diff --git a/src/gpu/GrShape.cpp b/src/gpu/GrShape.cpp
index 91cd3c8eb1..8c56054135 100644
--- a/src/gpu/GrShape.cpp
+++ b/src/gpu/GrShape.cpp
@@ -539,6 +539,7 @@ void GrShape::attemptToSimplifyPath() {
 void GrShape::attemptToSimplifyRRect() {
     SkASSERT(Type::kRRect == fType);
     SkASSERT(!fInheritedKey.count());
+    // TODO: This isn't valid for strokes.
     if (fRRectData.fRRect.isEmpty()) {
         // Dashing ignores the inverseness currently. skbug.com/5421
         fType = fRRectData.fInverted && !fStyle.isDashed() ? Type::kInvertedEmpty : Type::kEmpty;
diff --git a/src/gpu/effects/GrRRectEffect.cpp b/src/gpu/effects/GrRRectEffect.cpp
index 871578f924..c6430abd15 100644
--- a/src/gpu/effects/GrRRectEffect.cpp
+++ b/src/gpu/effects/GrRRectEffect.cpp
@@ -132,9 +132,7 @@ std::unique_ptr<GrFragmentProcessor> CircularRRectEffect::TestCreate(GrProcessor
 
 class GLCircularRRectEffect : public GrGLSLFragmentProcessor {
 public:
-    GLCircularRRectEffect() {
-        fPrevRRect.setEmpty();
-    }
+    GLCircularRRectEffect() = default;
 
     virtual void emitCode(EmitArgs&) override;
 
@@ -483,9 +481,7 @@ std::unique_ptr<GrFragmentProcessor> EllipticalRRectEffect::TestCreate(GrProcess
 
 class GLEllipticalRRectEffect : public GrGLSLFragmentProcessor {
 public:
-    GLEllipticalRRectEffect() {
-        fPrevRRect.setEmpty();
-    }
+    GLEllipticalRRectEffect() = default;
 
     void emitCode(EmitArgs&) override;
 
diff --git a/tests/PathTest.cpp b/tests/PathTest.cpp
index 88ca3f8f05..4bb8c88ae1 100644
--- a/tests/PathTest.cpp
+++ b/tests/PathTest.cpp
@@ -3662,7 +3662,10 @@ static void test_rrect(skiatest::Reporter* reporter) {
     SkRect emptyR = {10, 20, 10, 30};
     rr.setRectRadii(emptyR, radii);
     p.addRRect(rr);
-    REPORTER_ASSERT(reporter, p.isEmpty());
+    // The round rect is "empty" in that it has no fill area. However,
+    // the path isn't "empty" in that it should have verbs and points.
+    REPORTER_ASSERT(reporter, !p.isEmpty());
+    p.reset();
     SkRect largeR = {0, 0, SK_ScalarMax, SK_ScalarMax};
     rr.setRectRadii(largeR, radii);
     p.addRRect(rr);
diff --git a/tests/RoundRectTest.cpp b/tests/RoundRectTest.cpp
index 43a51430ab..a32e347b6f 100644
--- a/tests/RoundRectTest.cpp
+++ b/tests/RoundRectTest.cpp
@@ -71,36 +71,51 @@ static void test_empty(skiatest::Reporter* reporter) {
     for (size_t i = 0; i < SK_ARRAY_COUNT(oooRects); ++i) {
         r.setRect(oooRects[i]);
         REPORTER_ASSERT(reporter, !r.isEmpty());
+        REPORTER_ASSERT(reporter, r.rect() == oooRects[i].makeSorted());
 
         r.setOval(oooRects[i]);
         REPORTER_ASSERT(reporter, !r.isEmpty());
+        REPORTER_ASSERT(reporter, r.rect() == oooRects[i].makeSorted());
 
         r.setRectXY(oooRects[i], 1, 2);
         REPORTER_ASSERT(reporter, !r.isEmpty());
+        REPORTER_ASSERT(reporter, r.rect() == oooRects[i].makeSorted());
 
         r.setNinePatch(oooRects[i], 0, 1, 2, 3);
         REPORTER_ASSERT(reporter, !r.isEmpty());
+        REPORTER_ASSERT(reporter, r.rect() == oooRects[i].makeSorted());
 
         r.setRectRadii(oooRects[i], radii);
         REPORTER_ASSERT(reporter, !r.isEmpty());
+        REPORTER_ASSERT(reporter, r.rect() == oooRects[i].makeSorted());
     }
 
     for (size_t i = 0; i < SK_ARRAY_COUNT(emptyRects); ++i) {
         r.setRect(emptyRects[i]);
         REPORTER_ASSERT(reporter, r.isEmpty());
+        REPORTER_ASSERT(reporter, r.rect() == emptyRects[i]);
 
         r.setOval(emptyRects[i]);
         REPORTER_ASSERT(reporter, r.isEmpty());
+        REPORTER_ASSERT(reporter, r.rect() == emptyRects[i]);
 
         r.setRectXY(emptyRects[i], 1, 2);
         REPORTER_ASSERT(reporter, r.isEmpty());
+        REPORTER_ASSERT(reporter, r.rect() == emptyRects[i]);
 
         r.setNinePatch(emptyRects[i], 0, 1, 2, 3);
         REPORTER_ASSERT(reporter, r.isEmpty());
+        REPORTER_ASSERT(reporter, r.rect() == emptyRects[i]);
 
         r.setRectRadii(emptyRects[i], radii);
         REPORTER_ASSERT(reporter, r.isEmpty());
+        REPORTER_ASSERT(reporter, r.rect() == emptyRects[i]);
     }
+
+    r.setRect({SK_ScalarNaN, 10, 10, 20});
+    REPORTER_ASSERT(reporter, r == SkRRect::MakeEmpty());
+    r.setRect({0, 10, 10, SK_ScalarInfinity});
+    REPORTER_ASSERT(reporter, r == SkRRect::MakeEmpty());
 }
 
 static const SkScalar kWidth = 100.0f;