Bug 97333 - Implement Canvas Path object
: Implement Canvas Path object
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Canvas
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Dirk Schulze
http://dev.w3.org/html5/2dcontext/#pa...
: WebExposed
Depends on: 107473 108246 108508 108587 109097 109997 131114
Blocks: 107744
  Show dependency treegraph
 
Reported: 2012-09-21 06:33 PDT by Dirk Schulze
Modified: 2014-04-03 14:05 PDT (History)
27 users (show)

See Also:


Attachments
Patch (43.23 KB, patch)
2012-09-21 07:24 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (44.61 KB, patch)
2012-09-21 08:18 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (43.37 KB, patch)
2013-01-22 10:44 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (43.41 KB, patch)
2013-01-22 12:16 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (43.94 KB, patch)
2013-01-22 12:50 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (44.85 KB, patch)
2013-01-22 16:31 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (44.92 KB, patch)
2013-01-22 18:54 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (45.58 KB, patch)
2013-01-23 12:15 PST, Dirk Schulze
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2012-09-21 06:33:28 PDT
The HTML Canvas specification and the WHAT WG HTML specification define the Path object[1][2]. The Path and the CanvasRenderingContext2D objects share some graphics operations like: closePath, lineTo, moveTo, arc, arcTo, ellipse (not implemented yet), quadraticCurveTo, cubicCurveTo, rect [3]. But instead of an Canvas, the operations get applied to a Path object. Furthermore the object allows adding other Paths with transform. A Path object can then be drawn on a Canvas.

[1] http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#path-objects
[2] http://dev.w3.org/html5/2dcontext/#path-objects
[3] http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#2dcontext
Comment 1 Dirk Schulze 2012-09-21 07:24:36 PDT
Created attachment 165130 [details]
Patch

Test if it builds on all systems.
Comment 2 Build Bot 2012-09-21 08:02:22 PDT
Comment on attachment 165130 [details]
Patch

Attachment 165130 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13916636
Comment 3 Build Bot 2012-09-21 08:03:58 PDT
Comment on attachment 165130 [details]
Patch

Attachment 165130 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13951714
Comment 4 Dirk Schulze 2012-09-21 08:18:47 PDT
Created attachment 165134 [details]
Patch

Test if it builds on all systems.
Comment 5 Build Bot 2012-09-21 08:47:45 PDT
Comment on attachment 165134 [details]
Patch

Attachment 165134 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13954638
Comment 6 Build Bot 2012-09-21 09:08:25 PDT
Comment on attachment 165134 [details]
Patch

Attachment 165134 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13954647

New failing tests:
fast/dom/constructed-objects-prototypes.html
Comment 7 Simon Fraser (smfr) 2012-09-21 09:45:42 PDT
I think we might want to wait for this API to settle down before implementing it. I don't really like the new API.
Comment 8 Dirk Schulze 2012-09-21 11:53:43 PDT
(In reply to comment #7)
> I think we might want to wait for this API to settle down before implementing it. I don't really like the new API.

As a compromise, can we put the new API changes behind a flag? SVG WG decided to reuse the concept of Path for their own use. It would be extremely helpful to experiment with it. The flag could be called CANVAS_NEW or CANVAS_NEXT :) (Hope that someone has better suggestions).
Comment 9 Ian 'Hixie' Hickson 2012-09-21 12:29:26 PDT
smfr: The API is entirely "settled" (there's no outstanding feedback on it, last I checked). If you would like to unsettle it, please send e-mail to the WHATWG list with your concerns! (In particular, knowing that you don't want to implement it is very valuable feedback, but knowing why not would make it much easier for us to fix it.)
Comment 10 Dirk Schulze 2012-09-22 00:24:44 PDT
To the build problems: I tried a build from scratch on Mac and it works for me. Maybe the bots need to do the same.
Comment 11 Mike Reed 2012-09-24 06:34:13 PDT
Can you add a comment above transformIsInvertible()? What is it here? Who would override it?
Comment 12 Dirk Schulze 2013-01-08 14:28:29 PST
*** Bug 82790 has been marked as a duplicate of this bug. ***
Comment 13 Dirk Schulze 2013-01-08 15:01:37 PST
(In reply to comment #11)
> Can you add a comment above transformIsInvertible()? What is it here? Who would override it?

Not sure what you mean with "Who would override it?". The problem that some libraries are focused with is the following:

ctx.scale(0)
ctx.lineTo(100,100);

freezes the library. This was in particular the case for Cairo Graphics. For this reason it is necessary to check before applying the function and stop there. This problem might be fixed in Cairo Graphics already and can finally be removed. This is in the scope of a different patch and a different investigation.
Comment 14 Dirk Schulze 2013-01-15 08:45:47 PST
*** Bug 96826 has been marked as a duplicate of this bug. ***
Comment 15 Mike Reed 2013-01-15 10:23:45 PST
What is the status of this. Are you waiting on more reviews?
Comment 16 Dirk Schulze 2013-01-15 11:14:13 PST
(In reply to comment #15)
> What is the status of this. Are you waiting on more reviews?

We don't have a consensus on how we proceed with the proposal, see comment 7.
Comment 17 Mike Reed 2013-01-15 11:31:12 PST
#7 smfr

Do you think there will be changes to the API, given the statement in #9? Should we continue to wait?
Comment 18 Dean Jackson 2013-01-15 12:36:07 PST
(In reply to comment #17)

> Do you think there will be changes to the API, given the statement in #9? Should we continue to wait?

Might as well go ahead, but please put this behind an ENABLE flag. It's a Web-facing feature so implementations might want to hold off on releasing it if they plan to request significant changes to the specification. I'm not aware of any, but there are other reasons such as content compatibility (e.g. the new API has a name clash with an important site) that means people might want to live on this before shipping.
Comment 19 Rik Cabanier 2013-01-15 12:50:48 PST
(In reply to comment #18)
> (In reply to comment #17)
> 
> > Do you think there will be changes to the API, given the statement in #9? Should we continue to wait?
> 
> Might as well go ahead, but please put this behind an ENABLE flag. It's a Web-facing feature so implementations might want to hold off on releasing it if they plan to request significant changes to the specification. I'm not aware of any, but there are other reasons such as content compatibility (e.g. the new API has a name clash with an important site) that means people might want to live on this before shipping.

I think that the API should change. The current API is not all that useful and will not accomplish what people want (for instance, you can't use it to aggregate paths)
Comment 20 Ian 'Hixie' Hickson 2013-01-15 12:56:38 PST
(In reply to comment #7 and comment #19)

As always, if you want the API to change, send use cases and design constraints to the WHATWG list. I'm happy to update the API to address new use cases. (Please focus on problems that authors try to address that the current spec doesn't handle, not alternative proposed solutions, in initial e-mails on any topic.)
Comment 21 Justin Novosad 2013-01-16 11:52:49 PST
How about landing an implementation that reflects the current state of the draft specification as "webkitPath"?
Comment 22 Dirk Schulze 2013-01-16 21:36:38 PST
(In reply to comment #21)
> How about landing an implementation that reflects the current state of the draft specification as "webkitPath"?

I'll upload a new patch with the flag for canvas path first. Actually, Rik's request is not that far from the WHAT WG version, it just goes beyond it (with very good use cases). It should not be seen as counter proposal.

I am not so sure about prefixing this new object. I would rather not compile it by default on the bots until we are more certain about the API. That gives us a lot more freedom to change our mind.
Comment 23 Ian 'Hixie' Hickson 2013-01-18 12:53:31 PST
No need to prefix. I will make sure the spec doesn't conflict with shipped implementations. If changes are desired to the spec, make sure the use cases are mailed to the WHATWG list or a bug is filed.
Comment 24 Dirk Schulze 2013-01-21 13:04:57 PST
Making this to a master bug report.
Comment 25 Dirk Schulze 2013-01-22 10:44:36 PST
Created attachment 184013 [details]
Patch
Comment 26 WebKit Review Bot 2013-01-22 10:56:03 PST
Comment on attachment 184013 [details]
Patch

Attachment 184013 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16031730
Comment 27 Early Warning System Bot 2013-01-22 10:58:34 PST
Comment on attachment 184013 [details]
Patch

Attachment 184013 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16040699
Comment 28 Early Warning System Bot 2013-01-22 11:05:00 PST
Comment on attachment 184013 [details]
Patch

Attachment 184013 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16041740
Comment 29 Build Bot 2013-01-22 11:24:00 PST
Comment on attachment 184013 [details]
Patch

Attachment 184013 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16033762
Comment 30 Peter Beverloo (cr-android ews) 2013-01-22 11:24:42 PST
Comment on attachment 184013 [details]
Patch

Attachment 184013 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16065018
Comment 31 Build Bot 2013-01-22 11:32:28 PST
Comment on attachment 184013 [details]
Patch

Attachment 184013 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16037735
Comment 32 EFL EWS Bot 2013-01-22 11:53:01 PST
Comment on attachment 184013 [details]
Patch

Attachment 184013 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16045725
Comment 33 Dirk Schulze 2013-01-22 12:16:33 PST
Created attachment 184032 [details]
Patch

Patch. Should build on Mac now.
Comment 34 Early Warning System Bot 2013-01-22 12:28:52 PST
Comment on attachment 184032 [details]
Patch

Attachment 184032 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16033805
Comment 35 Early Warning System Bot 2013-01-22 12:31:36 PST
Comment on attachment 184032 [details]
Patch

Attachment 184032 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16063158
Comment 36 Dirk Schulze 2013-01-22 12:50:40 PST
Created attachment 184036 [details]
Patch

Patch. Should fix more builds.
Comment 37 WebKit Review Bot 2013-01-22 14:01:07 PST
Comment on attachment 184036 [details]
Patch

Attachment 184036 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16031825

New failing tests:
platform/chromium/virtual/gpu/fast/canvas/canvas-path-object.html
fast/canvas/canvas-path-object.html
Comment 38 WebKit Review Bot 2013-01-22 14:43:00 PST
Comment on attachment 184036 [details]
Patch

Attachment 184036 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16041824

New failing tests:
platform/chromium/virtual/gpu/fast/canvas/canvas-path-object.html
fast/canvas/canvas-path-object.html
Comment 39 Dirk Schulze 2013-01-22 16:31:26 PST
Created attachment 184070 [details]
Patch

Patch. Update build files after changes from other patches.
Comment 40 WebKit Review Bot 2013-01-22 17:22:09 PST
Comment on attachment 184070 [details]
Patch

Attachment 184070 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16065192

New failing tests:
platform/chromium/virtual/gpu/fast/canvas/canvas-path-object.html
Comment 41 Dirk Schulze 2013-01-22 18:54:38 PST
Created attachment 184104 [details]
Patch

Patch.
Comment 42 Dean Jackson 2013-01-23 10:58:31 PST
Comment on attachment 184104 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184104&action=review

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:17
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY

APPLE -> ADOBE ?

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:39
> +void CanvasPathMethods::closePath()
> +{
> +    if (m_path.isEmpty())
> +        return;
> +

Should you check the transform here as well?

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:47
> +    if (!isfinite(x) | !isfinite(y))

||

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:56
> +    if (!isfinite(x) | !isfinite(y))

||

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:65
> +    FloatPoint p1 = FloatPoint(x, y);
> +    if (!m_path.hasCurrentPoint())
> +        m_path.moveTo(p1);
> +    else if (p1 != m_path.currentPoint())
> +        m_path.addLineTo(FloatPoint(x, y));

Use p1 in addLineTo since you have it?

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:70
> +    if (!isfinite(cpx) | !isfinite(cpy) | !isfinite(x) | !isfinite(y))

||

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:79
> +    if (!m_path.hasCurrentPoint())
> +        m_path.moveTo(FloatPoint(cpx, cpy));
> +
> +    FloatPoint p1 = FloatPoint(x, y);
> +    if (p1 != m_path.currentPoint())
> +        m_path.addQuadCurveTo(FloatPoint(cpx, cpy), p1);

Is it defined behaviour that if (cpx, cpy) == (x, y) and you didn't have a control point then you still do the moveTo? In fact, it seems a little weird to move to the control point at all.

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:84
> +    if (!isfinite(cp1x) | !isfinite(cp1y) | !isfinite(cp2x) | !isfinite(cp2y) | !isfinite(x) | !isfinite(y))

||

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:93
> +    if (!m_path.hasCurrentPoint())
> +        m_path.moveTo(FloatPoint(cp1x, cp1y));
> +
> +    FloatPoint p1 = FloatPoint(x, y);
> +    if (p1 != m_path.currentPoint())
> +        m_path.addBezierCurveTo(FloatPoint(cp1x, cp1y), FloatPoint(cp2x, cp2y), p1);

Same question as for quad above.

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:99
> +    if (!isfinite(x1) | !isfinite(y1) | !isfinite(x2) | !isfinite(y2) | !isfinite(r))

||

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:114
> +    if (!m_path.hasCurrentPoint())
> +        m_path.moveTo(p1);

Again, is this defined? In this case you're moving to the start of the arc, then not adding the arc, so subsequent calls would not draw from the end. I have no idea what is correct, but that seems weird.

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:124
> +    if (!isfinite(x) | !isfinite(y) | !isfinite(r) | !isfinite(sa) | !isfinite(ea))

||

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:133
> +        // The arc is empty but we still need to draw the connecting line

Nit: trailing .

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:141
> +    // If 'sa' and 'ea' differ by more than 2Pi, just add a circle starting/ending at 'sa'

Nit: trailing .

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:151
> +    if (anticlockwise && sa - ea >= 2 * piFloat) {
> +        m_path.addArc(FloatPoint(x, y), r, sa, sa - 2 * piFloat, anticlockwise);
> +        return;
> +    }
> +    if (!anticlockwise && ea - sa >= 2 * piFloat) {
> +        m_path.addArc(FloatPoint(x, y), r, sa, sa + 2 * piFloat, anticlockwise);
> +        return;
> +    }
> +
> +    m_path.addArc(FloatPoint(x, y), r, sa, ea, anticlockwise);

Might as well create a variable FloatPoint(x, y)

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:159
> +    if (!isfinite(x) || !isfinite(y) || !isfinite(width) || !isfinite(height))

This one is ok!! :)

> Source/WebCore/html/canvas/CanvasPathMethods.h:17
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY

APPLE!

> Source/WebCore/html/canvas/CanvasPathMethods.h:49
> +    void arc(float x, float y, float r, float sa, float ea, bool clockwise, ExceptionCode&);

Watch out - you name this clockwise here and anticlockwise in the implementation!

> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:133
>               in [Optional=DefaultIsUndefined] boolean anticlockwise)
>          raises (DOMException);
> +
>      void fill(in [Optional] DOMString winding);

Intentional?

> Source/WebCore/html/canvas/DOMPath.h:17
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY

APPLE!

> Source/WebCore/html/canvas/DOMPath.idl:17
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY

APPLE!

> Source/WebCore/html/canvas/DOMPath.idl:63
> +             in [Optional=DefaultIsUndefined] boolean anticlockwise)

And anticlockwise here.
Comment 43 Dirk Schulze 2013-01-23 12:07:55 PST
Comment on attachment 184104 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184104&action=review

>> Source/WebCore/html/canvas/CanvasPathMethods.cpp:39
>> +
> 
> Should you check the transform here as well?

Very good question. I checked the spec, but couldn't find any wording what happens on a non-invertible CTM. Implementations are fairly different.

Here are two examples:
http://jsfiddle.net/Dghuh/1/
http://jsfiddle.net/Dghuh/2/

The first one does scale(0), which should make the CTM non-invertibe, we still apply lineTo and closePath for some reason. IE and FF refuse to draw anything.

The second does setTransform(0,0,0,0,0), which should reset the CTM to a zero matrix (again, not invertible). IE, Opera and FF draw a line to 0,0 and close the path afterwards (which kind of makes sense, since the universe is convoluted to one point). We refuse the lineTo command and close the path as expected.

There is clearly a need to specify this case properly. For now, I preserve our behavior.

>> Source/WebCore/html/canvas/CanvasPathMethods.cpp:79
>> +        m_path.addQuadCurveTo(FloatPoint(cpx, cpy), p1);
> 
> Is it defined behaviour that if (cpx, cpy) == (x, y) and you didn't have a control point then you still do the moveTo? In fact, it seems a little weird to move to the control point at all.

"The quadraticCurveTo(cpx, cpy, x, y) method must ensure there is a subpath for (cpx, cpy)"

and later:

"When the user agent is to ensure there is a subpath for a coordinate (x, y) on a path, the user agent must check to see if the path has any subpaths, and if it does not, then the user agent must create a new subpath with the point (x, y) as its first (and only) point, as if the moveTo() method had been called."

We are following the spec, or better, the spec is following us ;)

>> Source/WebCore/html/canvas/CanvasPathMethods.cpp:93
>> +        m_path.addBezierCurveTo(FloatPoint(cp1x, cp1y), FloatPoint(cp2x, cp2y), p1);
> 
> Same question as for quad above.

Ditto: "method must ensure there is a subpath for (cp1x, cp1y)".

>> Source/WebCore/html/canvas/CanvasPathMethods.cpp:114
>> +        m_path.moveTo(p1);
> 
> Again, is this defined? In this case you're moving to the start of the arc, then not adding the arc, so subsequent calls would not draw from the end. I have no idea what is correct, but that seems weird.

Yes: " method must first ensure there is a subpath for (x1, y1)"

Definition of "ensure there is a subpath for" is above.

>> Source/WebCore/html/canvas/CanvasPathMethods.cpp:151
>> +    m_path.addArc(FloatPoint(x, y), r, sa, ea, anticlockwise);
> 
> Might as well create a variable FloatPoint(x, y)

The point is never used twice. I wouldn't do it right now.

>> Source/WebCore/html/canvas/CanvasPathMethods.cpp:159
>> +    if (!isfinite(x) || !isfinite(y) || !isfinite(width) || !isfinite(height))
> 
> This one is ok!! :)

Hehe, true.

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:133
>>      void fill(in [Optional] DOMString winding);
> 
> Intentional?

Yes, to separate the functions that can be moved from the functions that can't.
Comment 44 Dirk Schulze 2013-01-23 12:15:10 PST
Created attachment 184279 [details]
Patch

Fixed requested changes.
Comment 45 Dirk Schulze 2013-01-23 15:05:08 PST
Committed r140604: <http://trac.webkit.org/changeset/140604>
Comment 46 Dirk Schulze 2013-01-29 16:15:58 PST
Reopen this bug report as meta bug.
Comment 47 Dirk Schulze 2014-04-03 14:05:14 PDT
All pending patches landed.