Bug 44197 - getPointAtLength returns incorrect values for paths closed with the closepath command
Summary: getPointAtLength returns incorrect values for paths closed with the closepath...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.5
: P2 Normal
Assignee: Dirk Schulze
URL: http://ericwaller.com/getPointAtLengt...
Keywords:
Depends on: 44756
Blocks:
  Show dependency tree / graph
 
Reported: 2010-08-18 13:08 PDT by Eric Waller
Modified: 2010-08-27 06:42 PDT (History)
5 users (show)

See Also:

Attachments
Test case for getPointAtLength bug (30.40 KB, application/zip)
2010-08-18 13:08 PDT, Eric Waller 		no flags Details
Reduced test case (886 bytes, text/html)
2010-08-19 00:27 PDT, Dirk Schulze 		no flags Details
Patch (4.49 KB, patch)
2010-08-26 13:12 PDT, Dirk Schulze 		no flags Details | Formatted Diff | Diff
Patch (4.40 KB, patch)
2010-08-27 04:12 PDT, Dirk Schulze 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Waller 2010-08-18 13:08:24 PDT 
Created attachment 64759 [details]
Test case for getPointAtLength bug

For paths closed with the closepath ("z") command, getPointAtLength returns incorrect values for points along the line segment which closes the path. I.e. for a path like "M x0,y0 x1,y1 x2,y2 x3,y3 z", getPointAtLength will return incorrect values for the implied line from (x3,y3) to (x0,y0). Explicitly closed paths, like "M x0,y0 x1,y1 x2,y2 x3,y3 x0,y0", are handled correctly.
Comment 1 Dirk Schulze 2010-08-19 00:27:27 PDT 
Created attachment 64814 [details]
Reduced test case

I would say, that we set the points in http://trac.webkit.org/browser/trunk/WebCore/platform/graphics/PathTraversalState.cpp#L163 wrong.
The current point should be the start point of the path: m_current = m_control1 = m_control2 = m_start;
Comment 2 Dirk Schulze 2010-08-25 05:31:20 PDT 
This also influences animations on animateTransform for the z-segment. The animated object has a wrong position for this segment.
Comment 3 Dirk Schulze 2010-08-26 13:12:24 PDT 
Created attachment 65606 [details]
Patch
Comment 4 Nikolas Zimmermann 2010-08-27 01:06:47 PDT 
Comment on attachment 65606 [details]
Patch

Great fix, r=me.
Comment 5 Dirk Schulze 2010-08-27 01:20:16 PDT 
Comment on attachment 65606 [details]
Patch

Clearing flags on attachment: 65606

Committed r66188: <http://trac.webkit.org/changeset/66188>
Comment 6 Dirk Schulze 2010-08-27 01:20:25 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 7 WebKit Review Bot 2010-08-27 01:54:47 PDT 
http://trac.webkit.org/changeset/66188 might have broken Qt Linux Release
Comment 8 Dirk Schulze 2010-08-27 03:07:40 PDT 
QtPainterPath makes several optimizations like closing paths, eliminate redundant paths. Thats why DRT results for Paths differ from the other platforms that much. And it's the reason, why this patch fails on Qt. Qt fails anyway, with or without this patch.

I'll upload the patch again, with better results for the other bots, but a long term goal is to do all  the calculation without the graphic libraries but with the SVGPathByteStream. That way we have common results across all platforms.
Comment 9 Dirk Schulze 2010-08-27 04:12:19 PDT 
Created attachment 65697 [details]
Patch
Comment 10 Nikolas Zimmermann 2010-08-27 04:20:35 PDT 
Comment on attachment 65697 [details]
Patch

r=me. Needs updates on most platforms...
Comment 11 Dirk Schulze 2010-08-27 05:09:26 PDT 
Comment on attachment 65697 [details]
Patch

Clearing flags on attachment: 65697

Committed r66208: <http://trac.webkit.org/changeset/66208>
Comment 12 Dirk Schulze 2010-08-27 05:09:36 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 13 Dirk Schulze 2010-08-27 06:09:14 PDT 
Committed r66214: <http://trac.webkit.org/changeset/66214>
Comment 14 WebKit Review Bot 2010-08-27 06:42:53 PDT 
http://trac.webkit.org/changeset/66214 might have broken Leopard Intel Release (Tests)