add two rect detector to path

add two nested rect detector to path

Tease apart existing one rect path detector so
that a new variant can detect two nested rects as well.

Add tests to verify that both one and two rect
detectors both work and return the correct results.

Suppress other warnings in PathTest.
Committed: https://code.google.com/p/skia/source/detail?r=6475

[reed1, 2012-11-15 17:31:36]

The need that came up was for concentric rects, where one contains the other.
Seems more conservative for a public API if we restrict it to that case (?)

https://codereview.appspot.com/6850059/diff/2001/include/core/SkPath.h
File include/core/SkPath.h (right):

https://codereview.appspot.com/6850059/diff/2001/include/core/SkPath.h#newcod...
include/core/SkPath.h:254: */
What if we rename this to isNestedRects(...) and make the comment clear that
true means rect[0] contains rect[1].

nested was brian's idea. I was working isRectannulus, but that got voted down :)

https://codereview.appspot.com/6850059/diff/2001/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

https://codereview.appspot.com/6850059/diff/2001/src/core/SkPath.cpp#newcode595
src/core/SkPath.cpp:595: rects[1].set(last, pts - last);
// if one rect does not contain the other, we should return false.

[caryclark1, 2012-11-15 18:28:39]

Done.
On 2012/11/15 17:31:36, reed1 wrote:
> The need that came up was for concentric rects, where one contains the other.
> Seems more conservative for a public API if we restrict it to that case (?)
> 
> https://codereview.appspot.com/6850059/diff/2001/include/core/SkPath.h
> File include/core/SkPath.h (right):
> 
>
https://codereview.appspot.com/6850059/diff/2001/include/core/SkPath.h#newcod...
> include/core/SkPath.h:254: */
> What if we rename this to isNestedRects(...) and make the comment clear that
> true means rect[0] contains rect[1].
> 
> nested was brian's idea. I was working isRectannulus, but that got voted down
:)
> 
> https://codereview.appspot.com/6850059/diff/2001/src/core/SkPath.cpp
> File src/core/SkPath.cpp (right):
> 
>
https://codereview.appspot.com/6850059/diff/2001/src/core/SkPath.cpp#newcode595
> src/core/SkPath.cpp:595: rects[1].set(last, pts - last);
> // if one rect does not contain the other, we should return false.

[reed1, 2012-11-15 20:10:54]

lgtm

what happens if both or the inner rect is empty? I wonder what we *want* to
happen in that case...?

1. nothing, let the caller check
2. return an int-count of the number of nested rects found [0..2], so this guy
sort of subsumes isRect as well.

Not meant to hold up checking this in, just thinking.

[caryclark1, 2012-11-15 21:13:51]

I'm confused. SkRect::contains() returns true iff both are not empty. What does
an empty rectangle in a path look like? The absence of another contour? A single
moveTo point? A line? Would an empty path return true, and 0 rects?

If the purpose of your thought-experiment is to avoid calling both isRect() and
isNestedRects(), then perhaps it would make sense to include something like

int SkPath::isRects(SkTDArray<SkRect>& rectArray);

that could return [0..n] for the number of rects in the path, and return -1 if
the SkPath contains a contour which is not a rect.

On 2012/11/15 20:10:54, reed1 wrote:
> lgtm
> 
> what happens if both or the inner rect is empty? I wonder what we *want* to
> happen in that case...?
> 
> 1. nothing, let the caller check
> 2. return an int-count of the number of nested rects found [0..2], so this guy
> sort of subsumes isRect as well.
> 
> Not meant to hold up checking this in, just thinking.

[reed1, 2012-11-15 21:27:56]

Good point about contains returning false for empty.

Yes, I was trying to avoid two diff calls when classifying a path.

SkRect storage[2];
int n = path.isNestedRects(storage);
switch (n) {
   case 0:
       // not anything I can specialize on
   case 1:
       // same as isRect(&storage[0]), treat as single rect
   case 2:
       // new donut case
}

I don't have any interesting use in mind for more than two. I guess we
could take a max parameter if we wanted to keep that possibility open
(isNested(SkRect [], int maxRects)) but I'd be fine to not do that now.


On Thu, Nov 15, 2012 at 4:13 PM, <caryclark@google.com> wrote:

> I'm confused. SkRect::contains() returns true iff both are not empty.
> What does an empty rectangle in a path look like? The absence of another
> contour? A single moveTo point? A line? Would an empty path return true,
> and 0 rects?
>
> If the purpose of your thought-experiment is to avoid calling both
> isRect() and isNestedRects(), then perhaps it would make sense to
> include something like
>
> int SkPath::isRects(SkTDArray<**SkRect>& rectArray);
>
> that could return [0..n] for the number of rects in the path, and return
> -1 if the SkPath contains a contour which is not a rect.
>
>
> On 2012/11/15 20:10:54, reed1 wrote:
>
>> lgtm
>>
>
>  what happens if both or the inner rect is empty? I wonder what we
>>
> *want* to
>
>> happen in that case...?
>>
>
>  1. nothing, let the caller check
>> 2. return an int-count of the number of nested rects found [0..2], so
>>
> this guy
>
>> sort of subsumes isRect as well.
>>
>
>  Not meant to hold up checking this in, just thinking.
>>
>
>
>
>
https://codereview.appspot.**com/6850059/<https://codereview.appspot.com/6850...
>