GNOME Bugzilla – Bug 649513
St: clamp border-radius properly
Last modified: 2011-07-10 00:14:49 UTC
We didn't clamp border-radius to halfwidth or halfheight before, so having a big border-radius combined with a small-sized actor gave extremely incorrect rendering.
Created attachment 187323 [details] [review] St: clamp border-radius to an actor's halfsizes
Created attachment 187332 [details] [review] tests: Add tests for border-radius clamping
Review of attachment 187323 [details] [review]: Code looks good except for some style nitpicks. The approach is pretty simple and looks OK, but nevertheless I wonder if it's worth sticking to the standard[0] here instead. [0]http://www.w3.org/TR/css3-background/#corner-overlap ::: src/st/st-theme-node-drawing.c @@ +308,3 @@ cache = st_texture_cache_get_default (); + int max_radius = MIN (node->alloc_width, node->alloc_height)/2; Variables must be declared at the start of a block. Missing spaces around '/'. @@ +946,3 @@ get_arbitrary_border_color (node, &border_color); + int max_radius = MIN (node->alloc_width, node->alloc_height)/2; Dto. @@ +1483,3 @@ &border_width_1, &border_width_2); + border_radius[corner_id] = MIN (node->border_radius[corner_id], MIN (width, height)/2); Missing spaces around '/'.
Review of attachment 187332 [details] [review]: Looks like you didn't test your test case ... ::: tests/interactive/border-radius.js @@ +53,3 @@ +// radius clamping +box.add(new St.Label({ text: "20px", text should be "999px;" - that's the border-radius used (and the actual point of the test case) @@ +54,3 @@ +// radius clamping +box.add(new St.Label({ text: "20px", + style: 'width: 20px; height: 20px;' That's too small for the text - you don't need to set any width/height (but padding would do good). In fact, you should probably just use addTestCase("999px", false); @@ +57,3 @@ + + ' border-radius: 999px;' + + ' background-color: red; ', + x_fill: false })); That is wrong - St.Label does not have a x_fill property. The intention is probably to pass it as 2nd parameter to box.add(), but you messed up the braces/parentheses (again: just use addTestCase ;-)
Created attachment 187674 [details] [review] St: clamp border-radius to an actor's halfsizes We didn't clamp border-radius to halfwidth or halfheight before, so having a big border-radius combined with a small-sized actor gave extremely incorrect rendering.
Created attachment 187675 [details] [review] tests: Add tests for border-radius clamping
Review of attachment 187674 [details] [review]: this is a WIP... run the tests and you'll see why.
Created attachment 187676 [details] [review] debugging
Review of attachment 187676 [details] [review]: This is just some simple debugging to help me diagnose the problem... not meant to go into the tree
Created attachment 187939 [details] [review] St: clamp border-radius properly Welp, OK. Answer for why the rendering errors happened? When the corners went beyond the halfsizes, the corners overlapped, so the assumption that we could lay down each corner, in no order, individually was wrong... So, we can either take the cairo path and implement CSS3 properly, or just clamp to the halfsizes. Additionally, I made an HTML testcase to help me out for what correct renderings should look like. It might help you guys as well: http://magcius.mecheye.net/shell/border-radius.html
Created attachment 187940 [details] [review] St: Take the cairo fallback for overlapping corners. The cogl path makes the assumption that we don't have overlapping corners. It'd be extremely complicated to fix this assumption, so just take the slow path.
Created attachment 187941 [details] [review] tests: Add tests for border-radius clamping
Created attachment 187998 [details] [review] St: Take the cairo fallback for overlapping corners. The cogl path makes the assumption that we don't have overlapping corners. It'd be extremely complicated to fix this assumption, so just take the slow path. Florian mentioned on IRC that I wasn't taking the reduced borders into account when determining whether to take the slow path. Fixed.
*** Bug 651310 has been marked as a duplicate of this bug. ***
Review of attachment 187939 [details] [review]: The code looks good except for some nitpicks, but the commit message is lying - "clamping" is what you did in a previous patch, but now the standard's behavior of scaling the radii proportionally to fit the allocation is implemented. ::: src/st/st-theme-node-drawing.c @@ +259,3 @@ +/** + * st_theme_node_reduce_border_radius: g-ir-scanner can probably deal with it, but I'd still use normal /* */ comments for non-public methods. @@ +270,3 @@ + guint *corners) +{ + gfloat min; "min" is a bit of a misnomer for a scaling factor - I don't think it needs to be overly specific, just calling it "scale" should be clearer. @@ +1003,2 @@ for (i = 0; i < 4; i++) { Style nit - the brackets can be removed now (not a biggie, so feel free to ignore)
Review of attachment 187941 [details] [review]: I'd go with much simpler test cases and merge the patch with the main one. ::: tests/interactive/border-radius.js @@ +24,3 @@ scroll.add_actor(box); +function addTestCase(radii, useGradient, size) { I don't think the additional parameter is helpful. In theory, it can be used to confirm that border-radius: 200px; width: 400px; height: 400px; does not trigger scaling of the radius, but as our test cases are purely visual, there's no way to tell that the radius does not end up at 199px anyway. @@ +60,3 @@ addTestCase("15px 0px 5px 10px", true); +// radius clamping "clamping" is wrong (see previous review) @@ +64,3 @@ +addTestCase("200px 200px 0px 200px", false, "400px"); +addTestCase("100px 200px 400px 800px", false, "400px"); +addTestCase("800px 400px 200px 100px", false, "400px"); It's not really obvious why those test cases are separate - I'd just add 2-3 cases to each the filled and gradient cases above, e.g. addTestCase("999px", false); addTestCase("100px 200px 400px 800px", false); @@ +73,3 @@ +// no borders +addTestCase("0px 0px 0px 0px", false, "500px"); +addTestCase("0px 0px 0px 0px", true, "500px"); What is this testing?
Review of attachment 187998 [details] [review]: Looks good, except for a variable misnomer. ::: src/st/st-theme-node-drawing.c @@ +1333,3 @@ gboolean has_border_radius; gboolean has_inset_box_shadow; + gboolean has_complex_border; Confusing variable name - how about "has_overlapping_corners"?
Created attachment 191425 [details] [review] St: Implement CSS3 specification for reducing border-radius Currently, any cases of overlapping corners were just ignored and rendered incorrectly. Implement the corner overlap algorithm as specified by the W3C to fix this. OK
Created attachment 191426 [details] [review] St: Take the cairo fallback for complex corners The cogl path makes the assumption that we don't have complex corners, which is when a corner texture is bigger than the actor's half-size. It'd be extremely complicated to fix this assumption, so just take the slow path for now. "overlapping corners" is bad too. When we reduce, we lose all overlapping corners. This is for when we have corners that are past halfsizes: the cogl code is just incorrect in this regard. So I'm calling it "complex corners", and put in a comment explaining it.
(In reply to comment #16) > Review of attachment 187941 [details] [review]: > > I'd go with much simpler test cases and merge the patch with the main one. I guess so. This was my main tester when working on the patches, so it contains tests for a bunch of edge cases I was seeing. > ::: tests/interactive/border-radius.js > @@ +24,3 @@ > scroll.add_actor(box); > > +function addTestCase(radii, useGradient, size) { > > I don't think the additional parameter is helpful. In theory, it can be used to > confirm that > border-radius: 200px; > width: 400px; > height: 400px; > does not trigger scaling of the radius, but as our test cases are purely > visual, there's no way to tell that the radius does not end up at 199px anyway. I simply wanted to test "what happens if this is larger/smaller than the halfsizes" > @@ +60,3 @@ > addTestCase("15px 0px 5px 10px", true); > > +// radius clamping > > "clamping" is wrong (see previous review) > > @@ +64,3 @@ > +addTestCase("200px 200px 0px 200px", false, "400px"); > +addTestCase("100px 200px 400px 800px", false, "400px"); > +addTestCase("800px 400px 200px 100px", false, "400px"); > It's not really obvious why those test cases are separate - I'd just add 2-3 > cases to each the filled and gradient cases above, e.g. Rendering errors: http://magcius.mecheye.net/shell/border-radius.png http://magcius.mecheye.net/shell/border-radius-2.png http://magcius.mecheye.net/shell/border-radius-3.png > addTestCase("999px", false); > addTestCase("100px 200px 400px 800px", false); > > @@ +73,3 @@ > +// no borders > +addTestCase("0px 0px 0px 0px", false, "500px"); > +addTestCase("0px 0px 0px 0px", true, "500px"); > > What is this testing? I had an accident where this happened: http://magcius.mecheye.net/shell/shell-round-corners.png
(In reply to comment #19) > "overlapping corners" is bad too. When we reduce, we lose all overlapping > corners. > > This is for when we have corners that are past halfsizes: the cogl code is just > incorrect in this regard. So I'm calling it "complex corners", and put in a > comment > explaining it. I'm not particularly fond of "overlapping", but "complex" is really confusing - it implies that the corners are somehow constructed differently than "non-complex" corners, which simply isn't true. The only thing special about them is that they break the assumption of the padding code (resulting in overlapping corner areas, which is where my suggestion comes from). Maybe "large corner" is better?
Review of attachment 191425 [details] [review]: Looks good.
Review of attachment 191425 [details] [review]: Looks good. (edit: Didn't you plan to merge the test cases?)
Review of attachment 191426 [details] [review]: Not quite. ::: src/st/st-theme-node-drawing.c @@ +1372,3 @@ + * halfsize of the actor. The cogl code doesn't support these + * cases, so for now, set a flag to fall us back to cairo. */ + has_complex_corners = FALSE; Sorry, but I don't think the comment is very clear without the context of this bug. As mentioned in a previous comment, I'd suggest "large corners" if you don't like "overlap". Then a comment along the lines of /* The cogl code pads each corner to the maximum border radius, which results in overlapping corner areas if the radius exceeds the actor's halfsize. Set a flag to fall back to cairo in these cases. */ should be clear enough.
(In reply to comment #20) > I simply wanted to test "what happens if this is larger/smaller than the > halfsizes" Sure, but this doesn't make much sense without debug printfs in the code (which I assume you had when using those test cases). > Rendering errors: > > http://magcius.mecheye.net/shell/border-radius.png > http://magcius.mecheye.net/shell/border-radius-2.png > http://magcius.mecheye.net/shell/border-radius-3.png Those are actually good examples why the explicit size is bad - as the labels are cut off, it is impossible to see what's tested without reading the code ;-) > > What is this testing? > > I had an accident where this happened: > > http://magcius.mecheye.net/shell/shell-round-corners.png I still don't see how this relates to a "0px border radius" border-radius test ...
Created attachment 191570 [details] [review] St: Implement CSS3 specification for reducing border-radius Currently, any cases of overlapping corners were just ignored and rendered incorrectly. Implement the corner overlap algorithm as specified by the W3C to fix this. + if (sum > 0) + scale = MIN (Node->alloc_height/sum, scale); oops. I need to find that emacs keybinding that does that silly capitalization and disable it.
Created attachment 191571 [details] [review] St: Take the cairo fallback for large corners The cogl path pads the corners out to the maximum corner radius to make the math and painting logic easier. Unfortunately, when the radius exceeds the actor's halfsize, the padding ends up interfering with other corners, creating a big mess of rendering errors. It'd be extremely complicated to fix this properly in the Cogl code, so take the Cairo fallback. Is this comment+commit message any better? Overlapping is really the wrong word, because they're not overlapping at all -- that's what the corner reduction is for! I didn't see your suggestion of "large corners" before, and yep, it fits much better.
Created attachment 191572 [details] [review] tests: Add tests for border-radius clamping OK, I'm trying to jog my memory here, but I got nothing. Simplified tests a lot.
Review of attachment 191572 [details] [review]: I still don't get why you insist on the "size" parameter - the additional test cases basically end up as "border-radius: 2..." which means that it is not clear what is tested only from running the test. Removing it the label is informative, and assuming that the labels' heights don't end up exceeding 400px (font-size: 390px?) covers the cases just fine. Other than that, the commit message is wrong :-)
Review of attachment 191570 [details] [review]: OK to commit with the style fixes pointed out below: ::: src/st/st-theme-node-drawing.c @@ +280,3 @@ + + if (sum > 0) + scale = MIN (node->alloc_width/sum, scale); Missing spaces around operator (likewise the other MIN()s)
Review of attachment 191571 [details] [review]: "creating a big mess" is a bit colloquial, but well ... Other than that, OK to commit with the style fixes pointed out below: ::: src/st/st-theme-node-drawing.c @@ +1383,3 @@ + for (corner = 0; corner < 4; corner ++) { + if (border_radius[corner]*2 > height || + border_radius[corner]*2 > width) { Missing spaces around operator
(In reply to comment #29) > Review of attachment 191572 [details] [review]: > > I still don't get why you insist on the "size" parameter - the additional test > cases basically end up as > "border-radius: 2..." > which means that it is not clear what is tested only from running the test. > > Removing it the label is informative, and assuming that the labels' heights > don't end up exceeding 400px (font-size: 390px?) covers the cases just fine. > > Other than that, the commit message is wrong :-) Because otherwise the container is sized based upon the label. All I want to do is test if there's any rendering errors. How do you suggest I do that without a constant width/height?
Should I just land these without tests for now?
(In reply to comment #32) > Because otherwise the container is sized based upon the label. All I want to do > is test if there's any rendering errors. How do you suggest I do that without a > constant width/height? As long as either width or height of the label is smaller than twice the radius, the reducing code path is triggered. I don't see why rendering errors in that path wouldn't show up then.
(In reply to comment #33) > Should I just land these without tests for now? Sure.
Attachment 191570 [details] pushed as ee4ae62 - St: Implement CSS3 specification for reducing border-radius Attachment 191571 [details] pushed as 325462d - St: Take the cairo fallback for large corners
Created attachment 191604 [details] [review] tests: Add tests for large rounded corners I was mostly afraid of depending on the size of the font, given that all we do is: stage { font: 16pt serif; } With a special font configuriation, the tests may not be testing large corners at all!
Review of attachment 191604 [details] [review]: (In reply to comment #37) > I was mostly afraid of depending on the size of the font, given that all we do > is: > > stage { > font: 16pt serif; > } > > With a special font configuriation, the tests may not be testing large corners > at all! Sure, but that configuration is probably special enough to render the entire system unusable :-) (It's probably far more likely to hit a font configuration where the "normal" test cases hit the large-corner path anyway)
Attachment 191604 [details] pushed as 5bc1ded - tests: Add tests for large rounded corners OK.