Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
7 years, 8 months ago by dshwang Modified:
7 years, 3 months ago Reviewers:
dglazkov, Stephen White, Justin Novosad, abarth-chromium, alph, Stephen Chennney CC:
blink-reviews, jamesr, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, alph+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, danakj, aandrey+blink_chromium.org, jeez, pdr. Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionAdd support for new canvas ellipse method.
Canvas v5 API adds a new path segment type: ellipse.
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#dom-context-2d-ellipse
Add ellipse API into Path.h because Canvas v5 adds following API also.
path.ellipse(x, y, radiusX, radiusY, rotation, startAngle, endAngle, anticlockwise)
When we support Path primitives, we can reuse Path::ellipse.
BUG=130260
TEST=fast/canvas/canvas-ellipse-360-winding.html,
fast/canvas/canvas-ellipse-circumference.html,
fast/canvas/canvas-ellipse-connecting-line.html,
fast/canvas/canvas-ellipse-zero-lineto.html,
fast/canvas/canvas-ellipse.html
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=156828
Patch Set 1 #
Total comments: 7
Patch Set 2 : WIP: Implementation is completed. I'll add various ellipse tests. #Patch Set 3 : Match addEllipse impl to addArc style. Add 4 more layout tests. #
Total comments: 8
Patch Set 4 : Patch to discuss about degenerate ellipse. #
Total comments: 4
Patch Set 5 : Patch v2 to discuss about degenerateEllipse. Fix rotation problem. #
Total comments: 8
Patch Set 6 : Complete degenerateEllipse. Make canvas-ellipse-zero-lineto.html cover various degenerate edge case… #
Total comments: 5
Patch Set 7 : Rebase to upstream. Make canvas-ellipse-circumference cover more extensive cases. #
Total comments: 6
Patch Set 8 : Add comments for degenerateEllipse(). Remove redundant tests. #Patch Set 9 : Make canvas-ellipse-360-winding.html for virtual/gpu pass. #
Created: 7 years, 3 months ago
Messages
Total messages: 42 (0 generated)
While this is a relatively benign API change, it still needs to go through an "Intent to Implement" phase. See the relevant postings on the blink-dev mailing list.
Link to the relevant spec please. Are there W3C tests for this new functionality? Testing doesn't cover a great range of cases. In particular, I do not see enough tests for degenerate ellipses. https://codereview.chromium.org/14298022/diff/1/Source/core/html/canvas/Canva... File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/1/Source/core/html/canvas/Canva... Source/core/html/canvas/CanvasPathMethods.cpp:137: return sa - 2 * piFloat; This is not correct. If they differ by more than 4 pi you will fail. The right way to do this is with a while loop. And what about cases where you are told to go anticlockwise and the angle is less than -2 pi (or negative at all?). https://codereview.chromium.org/14298022/diff/1/Source/core/html/canvas/Canva... Source/core/html/canvas/CanvasPathMethods.cpp:188: m_path.addEllipse(FloatPoint(x, y), rx, ry, rot, sa, adjustedEndAngle, anticlockwise); You should catch the common case of an entire ellipse, and use the optimized path for that. https://codereview.chromium.org/14298022/diff/1/Source/core/html/canvas/Canva... File Source/core/html/canvas/CanvasRenderingContext2D.idl (right): https://codereview.chromium.org/14298022/diff/1/Source/core/html/canvas/Canva... Source/core/html/canvas/CanvasRenderingContext2D.idl:127: [RaisesException] void ellipse([Default=Undefined] optional float x, What good is an optional default=undefined value? From the code it seems that you must define all of the parameters in order to draw anything.
On 2013/04/26 14:51:02, Stephen Chenney wrote: > Link to the relevant spec please. Sorry, I see you have that.
Corrected myself. https://codereview.chromium.org/14298022/diff/1/Source/core/html/canvas/Canva... File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/1/Source/core/html/canvas/Canva... Source/core/html/canvas/CanvasPathMethods.cpp:137: return sa - 2 * piFloat; I see, it is correct according to the spec. But then you know you have a complete ellipse, so it should be optimizable somehow.
https://codereview.chromium.org/14298022/diff/1/Source/core/html/canvas/Canva... File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/1/Source/core/html/canvas/Canva... Source/core/html/canvas/CanvasPathMethods.cpp:188: m_path.addEllipse(FloatPoint(x, y), rx, ry, rot, sa, adjustedEndAngle, anticlockwise); On 2013/04/26 14:51:03, Stephen Chenney wrote: > You should catch the common case of an entire ellipse, and use the optimized > path for that. Thank you for review, Stephen. Yes, your opinion is great. However, without CanvasPathMethods::ellipse, adjustEndAngle() is just extracted from CanvasPathMethods::arc(). I mean that CanvasPathMethods::arc() behavior is not changed. And your opinion is possible to apply arc() also. How about separating optimization patch for both arc() and ellipse()? If you say ok, I'll file it. https://codereview.chromium.org/14298022/diff/1/Source/core/html/canvas/Canva... File Source/core/html/canvas/CanvasRenderingContext2D.idl (right): https://codereview.chromium.org/14298022/diff/1/Source/core/html/canvas/Canva... Source/core/html/canvas/CanvasRenderingContext2D.idl:127: [RaisesException] void ellipse([Default=Undefined] optional float x, On 2013/04/26 14:51:03, Stephen Chenney wrote: > What good is an optional default=undefined value? From the code it seems that > you must define all of the parameters in order to draw anything. Aha, I see. I'll put [Default=Undefined] carefully after reading spec again. BTW, I'll file another bug to remove [Default=Undefined] from other API (e.g. arc()). I followed arc() without thought.
On 2013/04/26 14:33:24, Stephen Chenney wrote: > While this is a relatively benign API change, it still needs to go through an > "Intent to Implement" phase. See the relevant postings on the blink-dev mailing > list. Thank you for directing. I'll go through "Intent to Implement". And I'll search there are W3C tests for this new functionality.
https://codereview.chromium.org/14298022/diff/1/Source/core/html/canvas/Canva... File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/1/Source/core/html/canvas/Canva... Source/core/html/canvas/CanvasPathMethods.cpp:188: m_path.addEllipse(FloatPoint(x, y), rx, ry, rot, sa, adjustedEndAngle, anticlockwise); On 2013/04/26 15:23:33, dshwang wrote: > On 2013/04/26 14:51:03, Stephen Chenney wrote: > > You should catch the common case of an entire ellipse, and use the optimized > > path for that. > > Thank you for review, Stephen. Yes, your opinion is great. > However, without CanvasPathMethods::ellipse, adjustEndAngle() is just extracted > from CanvasPathMethods::arc(). I mean that CanvasPathMethods::arc() behavior is > not changed. And your opinion is possible to apply arc() also. How about > separating optimization patch for both arc() and ellipse()? > > If you say ok, I'll file it. You can certainly change both ellipse and arc. I'm not certain that it will behave correctly due to possible differences in Skia behavior, but I would certainly like you to try. We are actively attempting to use Skia optimized primitives whenever we can.
Hi, The second patch is WIP because I'll add various tests. I need to feedback for implementation. :) > Are there W3C tests for this new functionality? > Testing doesn't cover a great range of cases. In particular, I do not see enough > tests for degenerate ellipses. 1. Unfortunately, there are not any standard tests for canvas ellipse API. So I'll make various tests based on arc tests. 2. I sent "Intent to Implement and Ship" to blink-dev: https://groups.google.com/a/chromium.org/forum/?fromgroups#!topic/blink-dev/Z... > You should catch the common case of an entire ellipse, and use the optimized > path for that. 3. I add the optimization code of an entire ellipse in Path::addEllipse() instead of CanvasPathMethods::ellipse(), because I need other clients using Path::addEllipse() to have the benefit of the code. > What good is an optional default=undefined value? 4. I removed all [default=undefined] because all parameters are mandatory.
Hi, long time no see. I update this patch. I add 4 tests more. So now there are 6 tests. There are two kinds. 1. 5 tests are ellipse version of 5 arc tests. These tests uses html ref test. I implement ellipse js function: ellipseUsingArc() in js-ellipse-implementation.js. Expected html uses ellipseUsingArc(). It means that let's check ellipse implementation using arc because arc implementation is correct. fast/canvas/canvas-ellipse-360-winding.html, <- ellipse clone of canvas-arc-360-winding.html fast/canvas/canvas-ellipse-connecting-line.html, <- ellipse clone of canvas-arc-connecting-line.html fast/canvas/canvas-ellipse-zero-lineto.html <- ellipse clone of canvas-arc-zero-lineto.html fast/canvas/ellipse-crash.html, <- ellipse clone of arc-crash.html fast/canvas/ellipse360.html <- ellipse clone of arc360.html 2. Common canvas test using getImageData() api. It means that this test does not depend on both arc impl and ellipseUsingArc(). fast/canvas/canvas-ellipse.html <- this test uses ellipse in a bit complex manner.
FYI https://codereview.chromium.org/14298022/diff/15001/Source/core/html/canvas/C... File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/15001/Source/core/html/canvas/C... Source/core/html/canvas/CanvasPathMethods.cpp:178: if (!rx || !ry || sa == endAngle) { sa -> startAngle
https://codereview.chromium.org/14298022/diff/15001/Source/core/platform/grap... File Source/core/platform/graphics/Path.cpp (right): https://codereview.chromium.org/14298022/diff/15001/Source/core/platform/grap... Source/core/platform/graphics/Path.cpp:313: if (!rotation && !startAngle && SkScalarNearlyEqual(twoPiScalar, SkScalarAbs(endAngleScalar))) { If we're doing an approximate comparison for the end angle, shouldn't we also be doing an approximate comparison for the start angle? Or exact for both?
https://codereview.chromium.org/14298022/diff/15001/Source/core/html/canvas/C... File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/15001/Source/core/html/canvas/C... Source/core/html/canvas/CanvasPathMethods.cpp:180: lineTo(x + rx * cosf(sa + rot), y + rx * sinf(sa + rot)); rx used twice here. Also I suppose rotation should also rotate ellipse axes, while here it just affects starting point.
thx you for review :) https://codereview.chromium.org/14298022/diff/15001/Source/core/html/canvas/C... File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/15001/Source/core/html/canvas/C... Source/core/html/canvas/CanvasPathMethods.cpp:178: if (!rx || !ry || sa == endAngle) { On 2013/07/09 13:49:56, aandrey wrote: > sa -> startAngle oops, thx! https://codereview.chromium.org/14298022/diff/15001/Source/core/html/canvas/C... Source/core/html/canvas/CanvasPathMethods.cpp:180: lineTo(x + rx * cosf(sa + rot), y + rx * sinf(sa + rot)); On 2013/07/09 14:16:00, alph wrote: > rx used twice here. > Also I suppose rotation should also rotate ellipse axes, while here it just > affects starting point. thx for review. Yeah, using rx twice is wrong. how about as follows:? lineTo(x + rx * cosf(sa + rot), y + ry * sinf(sa + rot)); The point (x + rx * cosf(sa + rot), y + ry * sinf(sa + rot)) is on the ellipse. It means to draw line to start point. It is because arc() also draw line to start point. line 156: lineTo(x + r * cosf(startAngle), y + r * sinf(startAngle)); The canvas spec does not describe this edge case. So I just match ellipse to arc. https://codereview.chromium.org/14298022/diff/15001/Source/core/platform/grap... File Source/core/platform/graphics/Path.cpp (right): https://codereview.chromium.org/14298022/diff/15001/Source/core/platform/grap... Source/core/platform/graphics/Path.cpp:313: if (!rotation && !startAngle && SkScalarNearlyEqual(twoPiScalar, SkScalarAbs(endAngleScalar))) { On 2013/07/09 13:58:08, Stephen White wrote: > If we're doing an approximate comparison for the end angle, shouldn't we also be > doing an approximate comparison for the start angle? Or exact for both? I made it with intention. Let me explain. First of all, float a = 0 is just zero, not 0.0000000000001. Sencond, I'm not sure that Math.PI * 2 in javascript equals 2 * piFloat in cpp. Could I ensure they are exact in X86, X64 and ARM? In addition, I can not expect how web developer use 2Pi. Smart developers will use Math.PI * 2. However, I found that some code use something like 3.1415 * 2. For example, fast/canvas/canvas-transforms-during-path.html So imho, this code looks good to me. Could you feedback again? :)
https://codereview.chromium.org/14298022/diff/15001/Source/core/html/canvas/C... File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/15001/Source/core/html/canvas/C... Source/core/html/canvas/CanvasPathMethods.cpp:178: if (!rx || !ry || sa == endAngle) { If one of radii is not zero I'd expect the stroked ellipse to degenerate into a line, not to the empty thing.
https://codereview.chromium.org/14298022/diff/15001/Source/core/html/canvas/C... File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/15001/Source/core/html/canvas/C... Source/core/html/canvas/CanvasPathMethods.cpp:180: lineTo(x + rx * cosf(sa + rot), y + rx * sinf(sa + rot)); On 2013/07/09 14:42:05, dshwang wrote: > On 2013/07/09 14:16:00, alph wrote: > > rx used twice here. > > Also I suppose rotation should also rotate ellipse axes, while here it just > > affects starting point. > > thx for review. Yeah, using rx twice is wrong. > > how about as follows:? > lineTo(x + rx * cosf(sa + rot), y + ry * sinf(sa + rot)); > The point (x + rx * cosf(sa + rot), y + ry * sinf(sa + rot)) is on the ellipse. > > It means to draw line to start point. > It is because arc() also draw line to start point. > line 156: lineTo(x + r * cosf(startAngle), y + r * sinf(startAngle)); > > The canvas spec does not describe this edge case. So I just match ellipse to > arc. I don't like it. It works for arc because its radii are the same. If you do it this way for ellipse, then in rotated case with rx!=ry you'll be getting distinct points when startAndle is close to endAngle and when they are equal. So if someone makes an animation where endAngle decreases down to startAngle, suddenly it will jump to a distinct location when endAngle reaches startAngle. That doesn't seem what people would expect. Here's an example: x = 0 y = 0 rx = 100 ry = 5 rot = pi/2 startAngle = 0 endAngle = 1e-6 The point should be (0;100), while for endAngle=0 you'll be getting (0;5)
https://codereview.chromium.org/14298022/diff/27001/Source/core/html/canvas/C... File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/27001/Source/core/html/canvas/C... Source/core/html/canvas/CanvasPathMethods.cpp:215: degenerateEllipse(this, x, y, radiusX, radiusY, rotation, startAngle, endAngle, anticlockwise); @alph yes, you are right. I understand what you say. How about this implementation? Or if we want simpler implementation with losing some precision, below solution can work. Replace below code to degenerateEllipse(...) if (!radiusX) radiusX = 0.00001; if (!radiusY) radiusY = 0.00001; Which do you prefer?
On 2013/07/09 15:48:01, dshwang wrote: You don't have to explain axis or something. I understand now it is wrong. btw, I want your feedback about this approach.
https://codereview.chromium.org/14298022/diff/27001/Source/core/html/canvas/C... File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/27001/Source/core/html/canvas/C... Source/core/html/canvas/CanvasPathMethods.cpp:178: path->lineTo(x + radiusX * cosf(endAngle + rotation), y + radiusY * sinf(endAngle + rotation)); Here's a counter example: rotation = x = y = 0; startAngle = -pi/4; endAngle = pi/4; radiusX = 100; radiusY = 0; The line should go from (sqrt(2)/2*100; 0) to (100; 0). You draw just a single point of (sqrt(2)/2*100; 0). https://codereview.chromium.org/14298022/diff/27001/Source/core/html/canvas/C... Source/core/html/canvas/CanvasPathMethods.cpp:213: lineTo(x + radiusX * cosf(startAngle + rotation), y + radiusY * sinf(startAngle + rotation)); It still does not take into account axes rotation. How about something like this: float cos, sin; sincosf(startAngle, &sin, &cos); float x1 = radiusX * cos; float y1 = radiusY * sin; sincosf(rotation, &sin, &cos); float x2 = x1 * cos - y1 * sin; float y2 = x1 * sin + y1 * cos; lineTo(x + x2, y + y2); https://codereview.chromium.org/14298022/diff/27001/Source/core/html/canvas/C... Source/core/html/canvas/CanvasPathMethods.cpp:215: degenerateEllipse(this, x, y, radiusX, radiusY, rotation, startAngle, endAngle, anticlockwise); On 2013/07/09 15:48:02, dshwang wrote: > @alph yes, you are right. I understand what you say. > > How about this implementation? > > Or if we want simpler implementation with losing some precision, below solution > can work. > Replace below code to degenerateEllipse(...) > if (!radiusX) > radiusX = 0.00001; > if (!radiusY) > radiusY = 0.00001; > > Which do you prefer? degenerateEllipse already looks scary to me despite it still doesn't take into account axes rotation. I don't like scary code as bugs might hide in it. ;-)
Thx for kind explanation about rotation. As I mentioned comment #18, I understand :) And I still have two questions. https://codereview.chromium.org/14298022/diff/32001/Source/core/html/canvas/C... File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/32001/Source/core/html/canvas/C... Source/core/html/canvas/CanvasPathMethods.cpp:244: */ Which do you think right: degenerateEllipse() or approximation(radiusX = 0.0001) ? And do you think degenerateEllipse() still looks scary?
https://codereview.chromium.org/14298022/diff/32001/Source/core/html/canvas/C... File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/32001/Source/core/html/canvas/C... Source/core/html/canvas/CanvasPathMethods.cpp:191: sweep = std::fmod(-sweep, piFloat * 2); I'm not sure how to map negative sweep. Unfortunately the specification is not clear on that. We should probably mimic the behavior of m_path.addEllipse function. https://codereview.chromium.org/14298022/diff/32001/Source/core/html/canvas/C... Source/core/html/canvas/CanvasPathMethods.cpp:195: lineTo(path, endPoint); It now looks much better. But unfortunately this stuff just won't work. See my counter example to the previous patch. How about this approach: 1. canonicalize startAngle and endAngle, so that 0 <= startAngle < 2*pi 0 <= endAngle - startAngle <= 2*pi 2. then run something like: lineTo(<point-for-startAngle>); for (angle = startAngle - fmod(startAngle, pi/2) + pi/2; angle < endAngle; angle += pi/2) lineTo(<point-for-angle>); lineTo(<point-for-endAngle>); https://codereview.chromium.org/14298022/diff/32001/Source/core/html/canvas/C... Source/core/html/canvas/CanvasPathMethods.cpp:244: */ On 2013/07/09 16:37:22, dshwang wrote: > Which do you think right: degenerateEllipse() or approximation(radiusX = 0.0001) > ? > > And do you think degenerateEllipse() still looks scary? This way degenerateEllipse looks much better. I'd go for it. ;-)
Thx for review. I'll make degenerateEllipse more succinct, and make fast/canvas/canvas-ellipse-connecting-line.html cover more extensively. https://codereview.chromium.org/14298022/diff/32001/Source/core/html/canvas/C... File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/32001/Source/core/html/canvas/C... Source/core/html/canvas/CanvasPathMethods.cpp:191: sweep = std::fmod(-sweep, piFloat * 2); On 2013/07/09 17:18:18, alph wrote: > I'm not sure how to map negative sweep. Unfortunately the specification is not > clear on that. We should probably mimic the behavior of m_path.addEllipse > function. The spec said " If the anticlockwise argument is false and endAngle-startAngle is equal to or greater than 2π, or, if the anticlockwise argument is true and startAngle-endAngle is equal to or greater than 2π, then the arc is the whole circumference of this ellipse. Otherwise, the arc is the path along the circumference of this ellipse from the start point to the end point, going anti-clockwise if the anticlockwise argument is true, and clockwise otherwise. Since the points are on the ellipse, as opposed to being simply angles from zero, the arc can never cover an angle greater than 2π radians. " adjustEndAngle() can handle sweep angle in the manner which the spec want. I'll reuse adjustEndAngle(). https://codereview.chromium.org/14298022/diff/32001/Source/core/html/canvas/C... Source/core/html/canvas/CanvasPathMethods.cpp:195: lineTo(path, endPoint); On 2013/07/09 17:18:18, alph wrote: > It now looks much better. But unfortunately this stuff just won't work. See my > counter example to the previous patch. > How about this approach: > 1. canonicalize startAngle and endAngle, so that > 0 <= startAngle < 2*pi > 0 <= endAngle - startAngle <= 2*pi > > 2. then run something like: > lineTo(<point-for-startAngle>); > for (angle = startAngle - fmod(startAngle, pi/2) + pi/2; angle < endAngle; angle > += pi/2) > lineTo(<point-for-angle>); > lineTo(<point-for-endAngle>); Good idea! I buy it. I'll change. On the other hands, when it comes to your previous counter example. This implementation is right :) quote your example. > Here's a counter example: > rotation = x = y = 0; > startAngle = -pi/4; > endAngle = pi/4; > radiusX = 100; > radiusY = 0; > The line should go from (sqrt(2)/2*100; 0) to (100; 0). > You draw just a single point of (sqrt(2)/2*100; 0). Good example, but the premise is inconsistent with the result. you may want endAngle to be 0, not pi/4. The amended example, rotation = x = y = 0; startAngle = -pi/4; endAngle = 0; radiusX = 100; radiusY = 0; The result is the line from (sqrt(2)/2*100; -sqrt(2)/2*100) to (100; 0). This implementation will draw the line from lineTo(path, center + rotationMatrix.mapPoint(ellipsePoint(radiusX, radiusY, startAngle))); to lineTo(path, endPoint); The result of the implementation equals to what we expect. https://codereview.chromium.org/14298022/diff/32001/Source/core/html/canvas/C... Source/core/html/canvas/CanvasPathMethods.cpp:244: */ On 2013/07/09 17:18:18, alph wrote: > On 2013/07/09 16:37:22, dshwang wrote: > > Which do you think right: degenerateEllipse() or approximation(radiusX = > 0.0001) > > ? > > > > And do you think degenerateEllipse() still looks scary? > > This way degenerateEllipse looks much better. I'd go for it. ;-) Good choice :)
https://codereview.chromium.org/14298022/diff/32001/Source/core/html/canvas/C... File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/32001/Source/core/html/canvas/C... Source/core/html/canvas/CanvasPathMethods.cpp:195: lineTo(path, endPoint); On 2013/07/09 17:40:53, dshwang wrote: > On 2013/07/09 17:18:18, alph wrote: > > It now looks much better. But unfortunately this stuff just won't work. See my > > counter example to the previous patch. > > How about this approach: > > 1. canonicalize startAngle and endAngle, so that > > 0 <= startAngle < 2*pi > > 0 <= endAngle - startAngle <= 2*pi > > > > 2. then run something like: > > lineTo(<point-for-startAngle>); > > for (angle = startAngle - fmod(startAngle, pi/2) + pi/2; angle < endAngle; > angle > > += pi/2) > > lineTo(<point-for-angle>); > > lineTo(<point-for-endAngle>); > > Good idea! I buy it. I'll change. Just though that it's probably better to have two loops. One for clockwise and another for anticlockwise case. Otherwise if you exchange the start and end points to make end > start, you'll have to make two additional moveTo's to real start point in the beginning and to real end point at the end to not break the path ordering. > > On the other hands, when it comes to your previous counter example. > This implementation is right :) > > quote your example. > > Here's a counter example: > > rotation = x = y = 0; > > startAngle = -pi/4; > > endAngle = pi/4; > > radiusX = 100; > > radiusY = 0; > > The line should go from (sqrt(2)/2*100; 0) to (100; 0). > > You draw just a single point of (sqrt(2)/2*100; 0). > > Good example, but the premise is inconsistent with the result. > you may want endAngle to be 0, not pi/4. I meant the line should go through points: start is (sqrt(2)/2*100; 0) as it corresponds to -pi/4 then (100; 0) as it corresponds to angle=0 which is inside [-pi/4; pi/4] and end is just the same as start at (sqrt(2)/2*100; 0) corresponding to pi/4 So visually it will be a line from (sqrt(2)/2*100; 0) to (100; 0) But your original code won't go through (100; 0). That's what I meant. > The amended example, > rotation = x = y = 0; > startAngle = -pi/4; > endAngle = 0; > radiusX = 100; > radiusY = 0; > > The result is the line from (sqrt(2)/2*100; -sqrt(2)/2*100) to (100; 0). Hmm, I don't get it. y of the first point cannot be non zero as long as radiusY is 0. I.e. all the arc points have y=0. > > This implementation will draw the line > from lineTo(path, center + rotationMatrix.mapPoint(ellipsePoint(radiusX, > radiusY, startAngle))); > to lineTo(path, endPoint); > The result of the implementation equals to what we expect.
On 2013/07/09 18:16:10, alph wrote: > I meant the line should go through points: > start is (sqrt(2)/2*100; 0) as it corresponds to -pi/4 > then (100; 0) as it corresponds to angle=0 which is inside [-pi/4; pi/4] > and end is just the same as start at (sqrt(2)/2*100; 0) corresponding to pi/4 > > So visually it will be a line from (sqrt(2)/2*100; 0) to (100; 0) > But your original code won't go through (100; 0). That's what I meant. > > Hmm, I don't get it. y of the first point cannot be non zero as long as radiusY > is 0. I.e. all the arc points have y=0. Hi, now I totally understand. Thx for your endurance making me understand :) I found that arc has a bug while I followed your instruction about ellipse. Could you review Issue 18286007 before reviewing this CL?
On 2013/07/09 18:16:10, alph wrote: > I meant the line should go through points: > start is (sqrt(2)/2*100; 0) as it corresponds to -pi/4 > then (100; 0) as it corresponds to angle=0 which is inside [-pi/4; pi/4] > and end is just the same as start at (sqrt(2)/2*100; 0) corresponding to pi/4 > > So visually it will be a line from (sqrt(2)/2*100; 0) to (100; 0) > But your original code won't go through (100; 0). That's what I meant. > Hi, I seem to complete degenerateEllipse implementation. The impl pass your example. And I improve canvas-ellipse-zero-lineto.html to cover various edge cases of degenerateEllipse. Also I add new test: fast/canvas/canvas-ellipse-circumference.html that is similar to canvas-arc-circumference.html that I added in http://crrev.com/18286007/ This CL depends on crrev.com/18286007/
Thanks for addressing my concerns. https://codereview.chromium.org/14298022/diff/43001/LayoutTests/fast/canvas/c... File LayoutTests/fast/canvas/canvas-ellipse-circumference.html (right): https://codereview.chromium.org/14298022/diff/43001/LayoutTests/fast/canvas/c... LayoutTests/fast/canvas/canvas-ellipse-circumference.html:24: ctx.restore(); Again, as for the arc test, I suggest drawing a 2d array of arcs with various start and end angles. https://codereview.chromium.org/14298022/diff/43001/Source/core/html/canvas/C... File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/43001/Source/core/html/canvas/C... Source/core/html/canvas/CanvasPathMethods.cpp:223: canonicalizeAngle(&startAngle, &endAngle); It seems that canonicalizeAngle should depend on anticlockwise, as endAngle should be more than startAngle if !anticlockwise, and opposite otherwise. Also I suggest to just inline it here and get rid of taking address operations on startAngle, endAngle, as they might prohibit some compiler optimizations.
Thx for review, alph :) https://codereview.chromium.org/14298022/diff/43001/LayoutTests/fast/canvas/c... File LayoutTests/fast/canvas/canvas-ellipse-circumference.html (right): https://codereview.chromium.org/14298022/diff/43001/LayoutTests/fast/canvas/c... LayoutTests/fast/canvas/canvas-ellipse-circumference.html:24: ctx.restore(); On 2013/07/10 13:38:04, alph wrote: > Again, as for the arc test, I suggest drawing a 2d array of arcs with various > start and end angles. Yes, I think your idea is great. https://codereview.chromium.org/14298022/diff/43001/Source/core/html/canvas/C... File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/43001/Source/core/html/canvas/C... Source/core/html/canvas/CanvasPathMethods.cpp:223: canonicalizeAngle(&startAngle, &endAngle); On 2013/07/10 13:38:04, alph wrote: > It seems that canonicalizeAngle should depend on anticlockwise, as endAngle > should be more than startAngle if !anticlockwise, and opposite otherwise. > > Also I suggest to just inline it here and get rid of taking address operations > on startAngle, endAngle, as they might prohibit some compiler optimizations. canonicalizeAngle() is independent from whether anticlockwise or not. canonicalizeAngle just translates pair(startAngle, endAngle) so that 0 <= startAngle < 2*PI. adjustEndAngle() guarantees relevant startAngle and endAngle, and adjustEndAngle() works before this routine. I respect your optimization concern, but imho it seems to be premature optimization. degenerateEllipse() would be rarely called, so I prefer a bit more readability.
On 2013/07/10 14:19:56, dshwang wrote: > https://codereview.chromium.org/14298022/diff/43001/LayoutTests/fast/canvas/c... > LayoutTests/fast/canvas/canvas-ellipse-circumference.html:24: ctx.restore(); > On 2013/07/10 13:38:04, alph wrote: > > Again, as for the arc test, I suggest drawing a 2d array of arcs with various > > start and end angles. > > Yes, I think your idea is great. @alph, do you know good reference tests that I can follow. I don't want to reinvent wheel :)
> @alph, do you know good reference tests that I can follow. I don't want to reinvent wheel :) No, I don't know. https://codereview.chromium.org/14298022/diff/43001/Source/core/html/canvas/C... File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/43001/Source/core/html/canvas/C... Source/core/html/canvas/CanvasPathMethods.cpp:223: canonicalizeAngle(&startAngle, &endAngle); On 2013/07/10 14:19:57, dshwang wrote: > On 2013/07/10 13:38:04, alph wrote: > > It seems that canonicalizeAngle should depend on anticlockwise, as endAngle > > should be more than startAngle if !anticlockwise, and opposite otherwise. > > > > Also I suggest to just inline it here and get rid of taking address operations > > on startAngle, endAngle, as they might prohibit some compiler optimizations. > > canonicalizeAngle() is independent from whether anticlockwise or not. > canonicalizeAngle just translates pair(startAngle, endAngle) so that 0 <= > startAngle < 2*PI. Sorry, missed that. > adjustEndAngle() guarantees relevant startAngle and endAngle, and > adjustEndAngle() works before this routine. > > I respect your optimization concern, but imho it seems to be premature > optimization. > degenerateEllipse() would be rarely called, so I prefer a bit more readability.
@senorblanco Could you have a chance to look at this CL?
I appreciate the extensive testing, but I wouldn't bother duplicating arc360.html (ellipse360) or arc-crash.html. The former is probably subsumed by canvas-arc-circumference.html (so canvas-ellipse-circumference in your new patch), and the latter was a regression test for a crash in the old arc code. Both are likely redundant with tests you already have.
https://codereview.chromium.org/14298022/diff/53001/LayoutTests/inspector/pro... File LayoutTests/inspector/profiler/canvas2d/canvas2d-api-changes.html (right): https://codereview.chromium.org/14298022/diff/53001/LayoutTests/inspector/pro... LayoutTests/inspector/profiler/canvas2d/canvas2d-api-changes.html:49: "ellipse", Rather than just adding this, we should mark this test as expected to fail in TestExpectations. I believe this test is here to let the devtools folks know they have to do something on their side; see the text below: "If this test should ever fail, we should re-examine the Canvas 2D state saving/restoring logic in the InjectedScriptModule to include any latest changes to the API." https://codereview.chromium.org/14298022/diff/53001/Source/core/html/canvas/C... File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/53001/Source/core/html/canvas/C... Source/core/html/canvas/CanvasPathMethods.cpp:231: for (float angle = startAngle - fmodf(startAngle, halfPiFloat) + halfPiFloat; angle < endAngle; angle += halfPiFloat) I'm a little confused by this loop (and the one below). Isn't a degenerate ellipse a single line? Perhaps adding a comment (and/or a reference to the spec, although I couldn't find this algorithm there) might help.
https://codereview.chromium.org/14298022/diff/53001/Source/core/html/canvas/C... File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/53001/Source/core/html/canvas/C... Source/core/html/canvas/CanvasPathMethods.cpp:231: for (float angle = startAngle - fmodf(startAngle, halfPiFloat) + halfPiFloat; angle < endAngle; angle += halfPiFloat) On 2013/08/27 14:23:50, Stephen White wrote: > I'm a little confused by this loop (and the one below). Isn't a degenerate > ellipse a single line? Perhaps adding a comment (and/or a reference to the spec, > although I couldn't find this algorithm there) might help. I think the idea to to generate a flat quad to draw said line? Definitely needs more explaining. Also I am not 100% convinced this does the right thing in all cases https://codereview.chromium.org/14298022/diff/53001/Source/core/html/canvas/C... Source/core/html/canvas/CanvasPathMethods.cpp:251: if (!isTransformInvertible()) This early exit is not in the spec. If the implementation can't handle non-invertible transforms, then that should be fixed. Transforming an elliptical arc by a non-invertible transform is perfectly tractable. It gives an area of zero, so ignoring it is fine when the path is filled. When stroked however, the ellipse should be rendered as a line.
Thanks for review! I submitted Patch Set 8 that applies your review! On 2013/08/27 14:07:36, Stephen White wrote: > I appreciate the extensive testing, but I wouldn't bother duplicating > arc360.html (ellipse360) or arc-crash.html. The former is probably subsumed by > canvas-arc-circumference.html (so canvas-ellipse-circumference in your new > patch), and the latter was a regression test for a crash in the old arc code. > Both are likely redundant with tests you already have. Good points! I removes ellipse360 and ellipse-crash. On 2013/08/27 14:23:49, Stephen White wrote: > https://codereview.chromium.org/14298022/diff/53001/LayoutTests/inspector/pro... > File LayoutTests/inspector/profiler/canvas2d/canvas2d-api-changes.html (right): > > https://codereview.chromium.org/14298022/diff/53001/LayoutTests/inspector/pro... > LayoutTests/inspector/profiler/canvas2d/canvas2d-api-changes.html:49: "ellipse", > Rather than just adding this, we should mark this test as expected to fail in > TestExpectations. I believe this test is here to let the devtools folks know > they have to do something on their side; see the text below: "If this test > should ever fail, we should re-examine the Canvas 2D state saving/restoring > logic in the InjectedScriptModule to include any latest changes to the API." Good to know. Done. > https://codereview.chromium.org/14298022/diff/53001/Source/core/html/canvas/C... > File Source/core/html/canvas/CanvasPathMethods.cpp (right): > > https://codereview.chromium.org/14298022/diff/53001/Source/core/html/canvas/C... > Source/core/html/canvas/CanvasPathMethods.cpp:231: for (float angle = startAngle > - fmodf(startAngle, halfPiFloat) + halfPiFloat; angle < endAngle; angle += > halfPiFloat) > I'm a little confused by this loop (and the one below). Isn't a degenerate > ellipse a single line? Perhaps adding a comment (and/or a reference to the spec, > although I couldn't find this algorithm there) might help. I added detail comments for degenerateEllipse().
Thx for your review! https://codereview.chromium.org/14298022/diff/53001/Source/core/html/canvas/C... File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/53001/Source/core/html/canvas/C... Source/core/html/canvas/CanvasPathMethods.cpp:231: for (float angle = startAngle - fmodf(startAngle, halfPiFloat) + halfPiFloat; angle < endAngle; angle += halfPiFloat) On 2013/08/27 17:51:03, junov wrote: > On 2013/08/27 14:23:50, Stephen White wrote: > > I'm a little confused by this loop (and the one below). Isn't a degenerate > > ellipse a single line? Perhaps adding a comment (and/or a reference to the > spec, > > although I couldn't find this algorithm there) might help. > I think the idea to to generate a flat quad to draw said line? > Definitely needs more explaining. Also I am not 100% convinced this does the > right thing in all cases I added detail explanations as comments. To verify this implementation, I made fast/canvas/canvas-ellipse-connecting-line.html that covers extensive cases. And @alph reviewed it very carefully. https://codereview.chromium.org/14298022/diff/53001/Source/core/html/canvas/C... Source/core/html/canvas/CanvasPathMethods.cpp:251: if (!isTransformInvertible()) On 2013/08/27 17:51:03, junov wrote: > This early exit is not in the spec. If the implementation can't handle > non-invertible transforms, then that should be fixed. Transforming an > elliptical arc by a non-invertible transform is perfectly tractable. It gives > an area of zero, so ignoring it is fine when the path is filled. When stroked > however, the ellipse should be rendered as a line. Good concern. However, all primitives early exit as well as ellipse: moveTo, lineTo, quadraticCurveTo, bezierCurveTo, arcTo, arc, rect. I also can not find the spec about invertible current transform. If ellipse has problem, all primitives have also problem. We need to handle them together in the future if you are right.
On 2013/08/27 18:54:16, dshwang wrote: > Good concern. > However, all primitives early exit as well as ellipse: moveTo, lineTo, > quadraticCurveTo, bezierCurveTo, arcTo, arc, rect. > > I also can not find the spec about invertible current transform. > > If ellipse has problem, all primitives have also problem. We need to handle them > together in the future if you are right. Ok, I buy that. I did a search and found that the issue was raised on WhatWG: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2013-July/040185.html Let's see where that goes... Nice ASCII art in the comments by the way! I'm not a Blink owner, but for what it's worth: lgtm for the Source/core changes.
LGTM. Thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/14298022/64001
Thx Stephen, junov and alph for your sincere review! :D
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/14298022/78001
canvas-ellipse-360-winding.html failed in virtual/gpu test because the test is html ref test and some pixels is different. I changed the test to text ref test like canvas-arc-360-winding.html. Patch Set 9 for landing.
Message was sent while issue was closed.
Change committed as 156828 |
