Issue 6815058: SkRoundRect start (Closed)
|
Created:
12 years, 1 month ago by robertphillips Modified:
10 years, 9 months ago CC:
skia-review_googlegroups.com Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionThis intended as a start for making round rects top level objects to enable future optimizations.
Patch Set 1 #
Total comments: 2
Patch Set 2 : Added more tests & handled negative radii #
Total comments: 2
Patch Set 3 : removed use of memcmp for == check & now use kPoint instead of 2d array #Patch Set 4 : Updated to use SkScalarsEqual #Patch Set 5 : update #Patch Set 6 : Made more like SkMatrix #Patch Set 7 : Now with files #Patch Set 8 : Added "path forward" comment #
Total comments: 7
Patch Set 9 : Addressed code review issues #Patch Set 10 : altered API #
Total comments: 23
Patch Set 11 : Added Validate method & addressed other code review issues #
Total comments: 5
Patch Set 12 : Added comments #
MessagesTotal messages: 23
Do you think we need a separate class for simple or should there be an isSimple() query (maybe with a type mask)? Any reason to have even-simpler (all radii the same) RRs? Do we need to check for neg radii? If we check it in and we're not using it anywhere in core and it's not complete, maybe it should go in experimental? https://codereview.appspot.com/6815058/diff/1/tests/RoundRectTest.cpp File tests/RoundRectTest.cpp (right): https://codereview.appspot.com/6815058/diff/1/tests/RoundRectTest.cpp#newcode60 tests/RoundRectTest.cpp:60: // Test out isOval ?
Sign in to reply to this message.
I think we do want an SkSimpleRoundRect class. Most of the round rects from the SkPictures are simple and I think this is the form of RR we should pipe down to Ganesh. For generic SkRoundRects I was thinking of just reverting to the SkPath code path. SkRoundRect will have an isSimple method to assist switching to the fast path (although I hope to do that in Chrome). I'm not sure if a simpler RR is worth it. It might simplify the contains method but I don't think it would be a big win. I added handling to treat negative radii as zero (which appears to be how CSS handles it). I didn't see anything in experimental appearing in tests but I'm unsure of the conventions. https://codereview.appspot.com/6815058/diff/1/tests/RoundRectTest.cpp File tests/RoundRectTest.cpp (right): https://codereview.appspot.com/6815058/diff/1/tests/RoundRectTest.cpp#newcode60 tests/RoundRectTest.cpp:60: // Test out isOval On 2012/10/31 15:38:55, bsalomon wrote: > ? Done.
Sign in to reply to this message.
LGTM but I defer to Mike https://codereview.appspot.com/6815058/diff/4001/include/core/SkRoundRect.h File include/core/SkRoundRect.h (right): https://codereview.appspot.com/6815058/diff/4001/include/core/SkRoundRect.h#n... include/core/SkRoundRect.h:14: // Negative radii are treated as zeros looks like we assert on negative, not treat as zero... (which is OK by me but different).
Sign in to reply to this message.
- can't use memcmp for equality w/ floats (negative zero for one) - can we store the corners using an array of SkPoint or SkSize, to avoid the 2-d array stuff? - what is the next CL going to be like? new entry-points on canvas/device?
Sign in to reply to this message.
https://codereview.appspot.com/6815058/diff/4001/include/core/SkRoundRect.h File include/core/SkRoundRect.h (right): https://codereview.appspot.com/6815058/diff/4001/include/core/SkRoundRect.h#n... include/core/SkRoundRect.h:14: // Negative radii are treated as zeros We assert in the Make* methods and then deal with possible negative values in the methods. Since the member variables are public I felt this was the only way to make it robust. For example, in isRect I changed the test to treat negative radii as zero.
Sign in to reply to this message.
- can't use memcmp for equality w/ floats (negative zero for one)
Done. Should I also update SkRect?
- can we store the corners using an array of SkPoint or SkSize, to avoid the 2-d
array stuff?
Done.
- what is the next CL going to be like? new entry-points on canvas/device?
I think the overall arc would be:
flesh out these classes more
SkSimpleRoundRect - contains(SkRect),
contains(SkSimpleRR)
SkRoundRect - isRect, isSimple, isOval,
contains(SkPoint), contains(SkRect),
contains(SkSimpleRR)
more testing
add skcanvas entry points (convert to SkPaths)
drawSimpleRR, drawRR
more testing
RR gm
update Chrome to use RRs
Oh the agony
recapture skps
Analyze distribution of RRs
add device plumbing
plumb down into Ganesh
Sign in to reply to this message.
Updated to use SkScalarsEqual
Sign in to reply to this message.
Mike - do you have any more input on this?
Sign in to reply to this message.
PTAL - the SkRRect class has been updated to be more like SkMatrix than SkRect.
Sign in to reply to this message.
1. Do we promis that we always "clean up" the input radii such that they never exceed the width/height of the bounding rect? 2. If #1, do we document and test for that? https://codereview.appspot.com/6815058/diff/18008/include/core/SkRRect.h File include/core/SkRRect.h (right): https://codereview.appspot.com/6815058/diff/18008/include/core/SkRRect.h#newc... include/core/SkRRect.h:20: */ s/struct/class https://codereview.appspot.com/6815058/diff/18008/include/core/SkRRect.h#newc... include/core/SkRRect.h:79: */ s/SkPoint/SkVector seems large enough to move into .cpp https://codereview.appspot.com/6815058/diff/18008/include/core/SkRRect.h#newc... include/core/SkRRect.h:183: Do we need these contains() methods?
Sign in to reply to this message.
We have started to move SkPath::isRect() to just return the boolean, but not the rect as well, since there is already a free getBounds() method. Same for this class?
Sign in to reply to this message.
https://codereview.appspot.com/6815058/diff/18008/include/core/SkRRect.h File include/core/SkRRect.h (right): https://codereview.appspot.com/6815058/diff/18008/include/core/SkRRect.h#newc... include/core/SkRRect.h:183: On 2012/11/26 14:56:28, reed1 wrote: > Do we need these contains() methods? We will definitely want a contains(const SkRect&) to be used with the clipstack
Sign in to reply to this message.
> 1. Do we promise that we always "clean up" the input radii such that they never > exceed the width/height of the bounding rect? In my implementation I do always patch them up to match CSS' expectations but we could rely on the caller to do this. I slightly prefer bullet proofing the code but could be swayed the other way (the browser probably has to compute the actual radii anyway to check overlaps and already has to resize the RRs if they overlap GUI elements (e.g., scroll bars)). > 2. If #1, do we document and test for that? I have added some documentation in SkRRect.h. We do test both overlapping and negative radii in test_round_rect_iffy_parameters. > We have started to move SkPath::isRect() to just return the boolean, but not the > rect as well, since there is already a free getBounds() method. Same for this > class? Done. https://codereview.appspot.com/6815058/diff/18008/include/core/SkRRect.h File include/core/SkRRect.h (right): https://codereview.appspot.com/6815058/diff/18008/include/core/SkRRect.h#newc... include/core/SkRRect.h:20: */ On 2012/11/26 14:56:28, reed1 wrote: > s/struct/class Done. https://codereview.appspot.com/6815058/diff/18008/include/core/SkRRect.h#newc... include/core/SkRRect.h:79: */ On 2012/11/26 14:56:28, reed1 wrote: > s/SkPoint/SkVector > > seems large enough to move into .cpp Done. https://codereview.appspot.com/6815058/diff/18008/include/core/SkRRect.h#newc... include/core/SkRRect.h:183: I would like to keep them for the time being. I was hoping to use them for (at least) an initial version of the contains(SkRect&) method (after some quick reject tests). Additionally they do provide a good method of testing/documenting which pixels are considered inside the RR.
Sign in to reply to this message.
PTAL, this patch: removes non-const accessors adds setRect & setOval entry points converts from bit flags to a straight ahead enum for subtypes
Sign in to reply to this message.
make sense to add a validate() debugging method? see SkPath and SkRegion if you're interested. not required. https://codereview.appspot.com/6815058/diff/28007/include/core/SkRRect.h File include/core/SkRRect.h (right): https://codereview.appspot.com/6815058/diff/28007/include/core/SkRRect.h#newc... include/core/SkRRect.h:55: */ can simplify name to just Type (or other simple word), since this enum is always scoped by SkRRect:: https://codereview.appspot.com/6815058/diff/28007/include/core/SkRRect.h#newc... include/core/SkRRect.h:79: //!< never be seen by caller. Can we remove this from the public enum, and just store -1 or 0xFF internally as a flag to ourselves? https://codereview.appspot.com/6815058/diff/28007/include/core/SkRRect.h#newc... include/core/SkRRect.h:147: // The radii are stored in UL, UR, LR, LL order Corner (no s) ? Seems like a variable of this type just identifies one corner, not all of them. https://codereview.appspot.com/6815058/diff/28007/src/core/SkRRect.cpp File src/core/SkRRect.cpp (right): https://codereview.appspot.com/6815058/diff/28007/src/core/SkRRect.cpp#newcode23 src/core/SkRRect.cpp:23: SkScalar scale = SkMinScalar(..., ...); avoids need for a named temp. https://codereview.appspot.com/6815058/diff/28007/src/core/SkRRect.cpp#newcode24 src/core/SkRRect.cpp:24: SkScalar scale = SkScalarDiv(rect.width(), xRad + xRad); Is it worth it to avoid 2 divides, and just check for > before doing the work? Just asking... https://codereview.appspot.com/6815058/diff/28007/src/core/SkRRect.cpp#newcode41 src/core/SkRRect.cpp:41: if (xRad >= SkScalarHalf(fRect.width()) && yRad >= SkScalarHalf(fRect.height())) { In this case, do we want to re-compute xRad and yRad to be exactly width/2 and height/2? Seems like validating a rrect marked as oval would want to do that. https://codereview.appspot.com/6815058/diff/28007/src/core/SkRRect.cpp#newcode63 src/core/SkRRect.cpp:63: // computation. Hmmm, seems correct to smash both to zero (to me). https://codereview.appspot.com/6815058/diff/28007/src/core/SkRRect.cpp#newcode75 src/core/SkRRect.cpp:75: Can we site some W3 document that explains/justifies this algorithm? https://codereview.appspot.com/6815058/diff/28007/src/core/SkRRect.cpp#newcode80 src/core/SkRRect.cpp:80: Use SkMinScalar(..., ...) for these? https://codereview.appspot.com/6815058/diff/28007/src/core/SkRRect.cpp#newcod... src/core/SkRRect.cpp:107: for (int i = 0; i < 4; ++i) nit: { goes on same line as 'for' https://codereview.appspot.com/6815058/diff/28007/src/core/SkRRect.cpp#newcod... src/core/SkRRect.cpp:113: // At this point we're either simple, oval, or complex (not rect or empty) // but we defer computing that just to be lazy. (i.e. fast) https://codereview.appspot.com/6815058/diff/28007/src/core/SkRRect.cpp#newcod... src/core/SkRRect.cpp:130: Add special case for kOval ?
Sign in to reply to this message.
Added Validate method & addressed other issues https://codereview.appspot.com/6815058/diff/28007/include/core/SkRRect.h File include/core/SkRRect.h (right): https://codereview.appspot.com/6815058/diff/28007/include/core/SkRRect.h#newc... include/core/SkRRect.h:55: */ On 2012/11/28 17:14:06, reed1 wrote: > can simplify name to just Type (or other simple word), since this enum is always > scoped by SkRRect:: Done. https://codereview.appspot.com/6815058/diff/28007/include/core/SkRRect.h#newc... include/core/SkRRect.h:79: //!< never be seen by caller. On 2012/11/28 17:14:06, reed1 wrote: > Can we remove this from the public enum, and just store -1 or 0xFF internally as > a flag to ourselves? Done. https://codereview.appspot.com/6815058/diff/28007/include/core/SkRRect.h#newc... include/core/SkRRect.h:147: // The radii are stored in UL, UR, LR, LL order On 2012/11/28 17:14:06, reed1 wrote: > Corner (no s) ? Seems like a variable of this type just identifies one corner, > not all of them. Done. https://codereview.appspot.com/6815058/diff/28007/src/core/SkRRect.cpp File src/core/SkRRect.cpp (right): https://codereview.appspot.com/6815058/diff/28007/src/core/SkRRect.cpp#newcode23 src/core/SkRRect.cpp:23: On 2012/11/28 17:14:06, reed1 wrote: > SkScalar scale = SkMinScalar(..., ...); > > avoids need for a named temp. Done. https://codereview.appspot.com/6815058/diff/28007/src/core/SkRRect.cpp#newcode24 src/core/SkRRect.cpp:24: SkScalar scale = SkScalarDiv(rect.width(), xRad + xRad); On 2012/11/28 17:14:06, reed1 wrote: > Is it worth it to avoid 2 divides, and just check for > before doing the work? > Just asking... Done. https://codereview.appspot.com/6815058/diff/28007/src/core/SkRRect.cpp#newcode41 src/core/SkRRect.cpp:41: if (xRad >= SkScalarHalf(fRect.width()) && yRad >= SkScalarHalf(fRect.height())) { On 2012/11/28 17:14:06, reed1 wrote: > In this case, do we want to re-compute xRad and yRad to be exactly width/2 and > height/2? Seems like validating a rrect marked as oval would want to do that. Done. https://codereview.appspot.com/6815058/diff/28007/src/core/SkRRect.cpp#newcode75 src/core/SkRRect.cpp:75: On 2012/11/28 17:14:06, reed1 wrote: > Can we site some W3 document that explains/justifies this algorithm? Done. https://codereview.appspot.com/6815058/diff/28007/src/core/SkRRect.cpp#newcode80 src/core/SkRRect.cpp:80: On 2012/11/28 17:14:06, reed1 wrote: > Use SkMinScalar(..., ...) for these? Done. https://codereview.appspot.com/6815058/diff/28007/src/core/SkRRect.cpp#newcod... src/core/SkRRect.cpp:107: for (int i = 0; i < 4; ++i) On 2012/11/28 17:14:06, reed1 wrote: > nit: { goes on same line as 'for' Done. https://codereview.appspot.com/6815058/diff/28007/src/core/SkRRect.cpp#newcod... src/core/SkRRect.cpp:113: On 2012/11/28 17:14:06, reed1 wrote: > // At this point we're either simple, oval, or complex (not rect or empty) > // but we defer computing that just to be lazy. (i.e. fast) Done. https://codereview.appspot.com/6815058/diff/28007/src/core/SkRRect.cpp#newcod... src/core/SkRRect.cpp:130: On 2012/11/28 17:14:06, reed1 wrote: > Add special case for kOval ? Done.
Sign in to reply to this message.
lgtm w/ not important nits. incorporate or not as you please. https://codereview.appspot.com/6815058/diff/20008/include/core/SkRRect.h File include/core/SkRRect.h (right): https://codereview.appspot.com/6815058/diff/20008/include/core/SkRRect.h#newc... include/core/SkRRect.h:205: SkVector fRadii[4]; // Order is UL, UR, LR, LL // use Corner enum to index into fRadii[] https://codereview.appspot.com/6815058/diff/20008/include/core/SkRRect.h#newc... include/core/SkRRect.h:206: mutable Type fType; Possibly we'll want to pad this guy out to int32_t (or whatever is the same size as SkScalar), so we can use memcpy for flattening, etc. and not end up reading/writing uninitialized data. https://codereview.appspot.com/6815058/diff/20008/src/core/SkRRect.cpp File src/core/SkRRect.cpp (right): https://codereview.appspot.com/6815058/diff/20008/src/core/SkRRect.cpp#newcode26 src/core/SkRRect.cpp:26: SkScalarDiv(rect.height(), yRad + yRad)); good luck with this assert :) I bet with very large values, you can end up with scale == 1
Sign in to reply to this message.
may want to add a (possibly private) method addRRectToPath. I know that should live in SkPath.cpp, but perhaps it will want to call something here, so that it need not know about rrect's internals. Just a thought for the future.
Sign in to reply to this message.
committed as r6595 https://codereview.appspot.com/6815058/diff/20008/include/core/SkRRect.h File include/core/SkRRect.h (right): https://codereview.appspot.com/6815058/diff/20008/include/core/SkRRect.h#newc... include/core/SkRRect.h:205: SkVector fRadii[4]; // Order is UL, UR, LR, LL On 2012/11/28 22:09:59, reed1 wrote: > // use Corner enum to index into fRadii[] Done. https://codereview.appspot.com/6815058/diff/20008/include/core/SkRRect.h#newc... include/core/SkRRect.h:206: mutable Type fType; Added as TODO.
Sign in to reply to this message.
Message was sent while issue was closed.
On 2012/11/29 13:25:20, robertphillips wrote: > committed as r6595 > > https://codereview.appspot.com/6815058/diff/20008/include/core/SkRRect.h > File include/core/SkRRect.h (right): > > https://codereview.appspot.com/6815058/diff/20008/include/core/SkRRect.h#newc... > include/core/SkRRect.h:205: SkVector fRadii[4]; // Order is UL, UR, LR, LL > On 2012/11/28 22:09:59, reed1 wrote: > > // use Corner enum to index into fRadii[] > > Done. > > https://codereview.appspot.com/6815058/diff/20008/include/core/SkRRect.h#newc... > include/core/SkRRect.h:206: mutable Type fType; > Added as TODO. Why was it decided to set any empty fRect to a SkRect a XYWH of 0, 0, 0, 0? This can greatly complicate code where it is perfectly valid to a SkRRect represent a point.
Sign in to reply to this message.
Message was sent while issue was closed.
On 2014/02/19 19:36:08, tyler.r.w.abbott wrote: > On 2012/11/29 13:25:20, robertphillips wrote: > > committed as r6595 > > > > https://codereview.appspot.com/6815058/diff/20008/include/core/SkRRect.h > > File include/core/SkRRect.h (right): > > > > > https://codereview.appspot.com/6815058/diff/20008/include/core/SkRRect.h#newc... > > include/core/SkRRect.h:205: SkVector fRadii[4]; // Order is UL, UR, LR, LL > > On 2012/11/28 22:09:59, reed1 wrote: > > > // use Corner enum to index into fRadii[] > > > > Done. > > > > > https://codereview.appspot.com/6815058/diff/20008/include/core/SkRRect.h#newc... > > include/core/SkRRect.h:206: mutable Type fType; > > Added as TODO. > > Why was it decided to set any empty fRect to a SkRect a XYWH of 0, 0, 0, 0? This > can greatly complicate code where it is perfectly valid to a SkRRect represent a > point. depends on how you interpret empty. If empty means zero area (width==0, height==0), but "valid", then 0,0,0,0 is a good (but not unique) representation. This was the thinking at the time.
Sign in to reply to this message.
Message was sent while issue was closed.
On 2014/02/19 19:42:03, reed1 wrote: > On 2014/02/19 19:36:08, tyler.r.w.abbott wrote: > > On 2012/11/29 13:25:20, robertphillips wrote: > > > committed as r6595 > > > > > > https://codereview.appspot.com/6815058/diff/20008/include/core/SkRRect.h > > > File include/core/SkRRect.h (right): > > > > > > > > > https://codereview.appspot.com/6815058/diff/20008/include/core/SkRRect.h#newc... > > > include/core/SkRRect.h:205: SkVector fRadii[4]; // Order is UL, UR, LR, > LL > > > On 2012/11/28 22:09:59, reed1 wrote: > > > > // use Corner enum to index into fRadii[] > > > > > > Done. > > > > > > > > > https://codereview.appspot.com/6815058/diff/20008/include/core/SkRRect.h#newc... > > > include/core/SkRRect.h:206: mutable Type fType; > > > Added as TODO. > > > > Why was it decided to set any empty fRect to a SkRect a XYWH of 0, 0, 0, 0? > This > > can greatly complicate code where it is perfectly valid to a SkRRect represent > a > > point. > > depends on how you interpret empty. If empty means zero area (width==0, > height==0), but "valid", then 0,0,0,0 is a good (but not unique) representation. > This was the thinking at the time. Thanks for the speedy response! My particular use case involves insetting and outsetting a SkRRect and then joining the corners in various ways. If the SkRRect is inset by its width then it "moves" it to XY 0,0 by the `if (isEmpty()) setEmtpy()` convention of SkRRect. This means the code needs to check for an empty SkRRect and do completely different calculations to achieve sensible results. Is there any chance that the behaviour could be changed or at least provide some mechanism to create a SkRRect with zero width and height at a position other than 0,0?
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
