canvas: arcTo after closePath does not create a tangent line to the arc
Categories
(Core :: Graphics: Canvas2D, defect, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox69 | --- | fixed |
People
(Reporter: qdirks, Assigned: nical)
Details
Attachments
(4 files)
User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:66.0) Gecko/20100101 Firefox/66.0
Steps to reproduce:
Make one ctx.moveTo(x,y) call and two ctx.lineTo(x,y) calls to form the first two sides of a triangle. Complete the triangle by calling ctx.closePath(). Then call ctx.arcTo(x1,y1,x2,y2,radius). Then call ctx.stroke().
Actual results:
The line drawn to the arc is not tangent to the arc. The last point of the previous subpath, as specified by the second lineTo call, is incorrectly used as the (x0, y0) control point of the arc. To clarify, it would be called the last point of the 'previous' subpath because of the supposed functioning of the closePath method preceding the call to arcTo.
Expected results:
The last point of the current subpath, as specified by the moveTo call because of the closePath call, should be used as the (x0, y0) control point of the arc.
Following is the relevant sections from https://www.w3.org/TR/2dcontext/#building-paths:
closePath: """
The closePath() method must do nothing if the object's path has no subpaths. Otherwise, it must mark the last subpath as closed, create a new subpath whose first point is the same as the previous subpath's first point, and finally add this new subpath to the path.
"""
arcTo: """
The arcTo(x1, y1, x2, y2, radius) method must first ensure there is a subpath for (x1, y1). Then, the behavior depends on the arguments and the last point in the subpath, as described below.
Negative values for radius must cause the implementation to throw an IndexSizeError exception.
Let the point (x0, y0) be the last point in the subpath.
If the point (x0, y0) is equal to the point (x1, y1), or if the point (x1, y1) is equal to the point (x2, y2), or if the radius radius is zero, then the method must add the point (x1, y1) to the subpath, and connect that point to the previous point (x0, y0) by a straight line.
Otherwise, if the points (x0, y0), (x1, y1), and (x2, y2) all lie on a single straight line, then the method must add the point (x1, y1) to the subpath, and connect that point to the previous point (x0, y0) by a straight line.
Otherwise, let The Arc be the shortest arc given by circumference of the circle that has radius radius, and that has one point tangent to the half-infinite line that crosses the point (x0, y0) and ends at the point (x1, y1), and that has a different point tangent to the half-infinite line that ends at the point (x1, y1), and crosses the point (x2, y2). The points at which this circle touches these two lines are called the start and end tangent points respectively. The method must connect the point (x0, y0) to the start tangent point by a straight line, adding the start tangent point to the subpath, and then must connect the start tangent point to the end tangent point by The Arc, adding the end tangent point to the subpath.
"""
|
||
Comment 2•5 years ago
|
||
Moving this over to the component. Unfortunately, I don't understand what is the actual problem but developers will know for sure.
|
Assignee | |
Comment 3•5 years ago
|
||
Thanks for the bug report and analysis. It appears that the way we track the current point in the skia backend doesn't properly take close events into account.
|
||
Updated•5 years ago
|
|
|
Assignee | |
Comment 4•5 years ago
|
||
|
|
Assignee | |
Comment 5•5 years ago
|
||
Pushed by nsilva@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5bc7f1dbafe5 Fix PathBuilderCapture::CurrentPoint when closing a path. r=lsalzman https://hg.mozilla.org/integration/autoland/rev/b3176ad6f3a7 Fix PathBuilderSkia::CurrentPoint. r=lsalzman
|
||
Comment 7•5 years ago
|
||
Backed out 2 changesets for causing mochitest failures at test_canvas.html
Backout link: https://hg.mozilla.org/integration/autoland/rev/a0d1ac18b8c7a6dfec03404efd944980b10d043b
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=b3176ad6f3a71b98d7511c1c8ffbecf18f4fadf9&selectedJob=248526903
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=248526903&repo=autoland&lineNumber=4403
|
||
Comment 8•5 years ago
|
||
There are also wpt failures: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=248546932&repo=autoland&lineNumber=766
|
|
Assignee | |
Comment 9•5 years ago
|
||
The canvas implementation maintains user-space and a device-space path builder, and switch from user-space to the device-space one when a a transform is set in the middle of building a path. When that happens we "finish" the original builder and create the new one out of the resulting path, but as a result we lose the information about the current point and the previous MoveTo location.
All backends appear to get this wrong to some varying extent. For example the Cairo backend stores the the current point in the path just to avoid this problem but doesn't keep the previous MoveTo position which means we don't do the right thing if the we get a transform followed by a Close() and an edge without a MoveTo.
What would make sense here is to use the PathBuilder rather than the Path in TransformedCopyToBuilder. I didn't know why we do don't this in the first place (all of this dates from the original moz2d implementation).
|
|
Assignee | |
Comment 10•5 years ago
|
||
Because of the way the canvas 2D implementation juggles between path builders and paths, we have to keep track of current and first points in the path, in case a builder is created out of it.
|
||
Comment 11•5 years ago
|
||
Pushed by nsilva@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c246c374ce11 Fix PathBuilderSkia::CurrentPoint. r=lsalzman https://hg.mozilla.org/integration/mozilla-inbound/rev/cea22203c716 Fix PathBuilderCapture::CurrentPoint when closing a path. r=lsalzman https://hg.mozilla.org/integration/mozilla-inbound/rev/45f8d9f8f333 Keep track of current and begin points in path objects in case we'll create a builder from them. r=lsalzman
|
||
Comment 12•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/c246c374ce11
https://hg.mozilla.org/mozilla-central/rev/cea22203c716
https://hg.mozilla.org/mozilla-central/rev/45f8d9f8f333
Description
•