Bug 37515 - getPathSegAtLength() gives incorrect value
Summary: getPathSegAtLength() gives incorrect value
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Robin Qiu
URL: http://svgtorture.googlecode.com/svn/...
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-13 12:10 PDT by Alexis Deveria
Modified: 2011-03-09 13:21 PST (History)
8 users (show)

See Also:


Attachments
Testcase that alerts the index returned by getPathSegAtLength() (484 bytes, image/svg+xml)
2010-04-13 12:10 PDT, Alexis Deveria
no flags Details
Fixed this bug and added a test case. (4.24 KB, patch)
2010-05-05 01:11 PDT, Robin Qiu
no flags Details | Formatted Diff | Diff
Modified according to Niko's comment. (5.98 KB, patch)
2010-05-07 19:51 PDT, Robin Qiu
no flags Details | Formatted Diff | Diff
Forgot to set "review ?". Modified according to Niko's comment. (5.98 KB, patch)
2010-05-07 19:54 PDT, Robin Qiu
zimmermann: review-
Details | Formatted Diff | Diff
3rd patch (6.46 KB, patch)
2010-05-08 00:44 PDT, Robin Qiu
zimmermann: review-
Details | Formatted Diff | Diff
4th edition (6.46 KB, patch)
2010-05-08 05:51 PDT, Robin Qiu
zimmermann: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
5th patch, fixed that warning. (6.57 KB, patch)
2010-05-09 18:47 PDT, Robin Qiu
zimmermann: review-
Details | Formatted Diff | Diff
7 th patch (6.78 KB, patch)
2010-05-10 22:05 PDT, Robin Qiu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexis Deveria 2010-04-13 12:10:45 PDT
Created attachment 53270 [details]
Testcase that alerts the index returned by getPathSegAtLength()

On a simple path using this data: "M0,0 L0,5 L0,10", using getPathSegAtLength(7) should return the number 2. Instead, it returns the number 1. 

Currently Opera gets this wrong too, while Gecko gets it right.
Comment 1 Robin Qiu 2010-05-04 21:16:32 PDT
According to the spec: "The index of the path segment, where the first path segment is number 0."  I think the return value should be 1.  However, there is indeed a bug in this function, because getPathSegAtLength(2) and getPathSegAtLength(7) both return 1. 

By the way, assuming the index of the path segment starts from 1, Firefox is correct.
Comment 2 Robin Qiu 2010-05-05 00:12:38 PDT
There is a tiny misktake in  SVGPathSegList::getPathSegAtLength(). I'm going to fix it.
Comment 3 Robin Qiu 2010-05-05 01:11:24 PDT
Created attachment 55096 [details]
Fixed this bug and added a test case.

Fixed this bug and added a test case.
Comment 4 Nikolas Zimmermann 2010-05-07 06:47:36 PDT
Comment on attachment 55096 [details]
Fixed this bug and added a test case.

r-, some comments:

> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
> index 6715cab..8bb4169 100644
> --- a/LayoutTests/ChangeLog
> +++ b/LayoutTests/ChangeLog
> @@ -1,3 +1,13 @@
> +2010-05-05  Robin Qiu  <robin.qiu@torchmobile.com.cn>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Fix a bug in SVGPathSegList::getPathSegAtLength().
> +        https://bugs.webkit.org/show_bug.cgi?id=37515

The ChangeLog needs to be much more explicit :-) Please describe in detail what was broken, and what is fixed now.


> +++ b/LayoutTests/svg/dom/svgpath-getPathSegAtLength.html
> @@ -0,0 +1,28 @@
> +<!DOCTYPE html>
> +<html>
> +<head>
> +<link rel="stylesheet" href="../../fast/js/resources/js-test-style.css">
> +<script src="../../fast/js/resources/js-test-pre.js"></script>
> +</head>
> +<body>
> +<p id="description"></p>
> +<div id="console"></div>
> +<script>
> +    description("Test SVG path.getPathSegAtLength().");
> +    path = document.createElementNS("http://www.w3.org/2000/svg", "path");
> +    path.setAttributeNS(null, "d", "M0 0 L0 5 L5 5 L 5 0");
> +    var segIndex = [ path.getPathSegAtLength(0),
> +                        path.getPathSegAtLength(1),
> +                        path.getPathSegAtLength(5),
> +                        path.getPathSegAtLength(6),
> +                        path.getPathSegAtLength(10),
> +                        path.getPathSegAtLength(11),
> +                        path.getPathSegAtLength(16)];
> +    shouldBe("segIndex.toString()", "'0,0,1,1,2,2,0'");
> +    successfullyParsed = true;
> +</script>
> +<script src="../../fast/js/resources/js-test-post.js"></script>
> +</body>
> +</html>
In general this looks fine, though the prefered style is to add a new svgpath-getPathSegAtLength.js file in svg/dom/script-tests.
See svg/dom/id-reflect.html & svg/dom/script-tests/id-reflect.js as example template. Should be trivial to change.
A comment within the test would also be nice, to why the segIndex should look like this.
I'd advice to use "shouldBe" though, as it's much easier to read in the expected output.
IIRC: shouldBe("path.getPathSegAtLength(0)", "0"); etc..
This will include the actual command that you're executing in the expected.txt file.


> +2010-05-05  Robin Qiu  <robin.qiu@torchmobile.com.cn>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Fix a bug in SVGPathSegList::getPathSegAtLength().
> +        https://bugs.webkit.org/show_bug.cgi?id=37515
> +
> +        Test: svg/dom/svgpath-getPathSegAtLength.html

Same as above, needs better explaination.

> -unsigned SVGPathSegList::getPathSegAtLength(double, ExceptionCode& ec)
> +unsigned SVGPathSegList::getPathSegAtLength(double length, ExceptionCode& ec)
>  {
>      // FIXME : to be useful this will need to support non-normalized SVGPathSegLists
>      int len = numberOfItems();
>      // FIXME: Eventually this will likely move to a "path applier"-like model, until then PathTraversalState is less useful as we could just use locals
>      PathTraversalState traversalState(PathTraversalState::TraversalSegmentAtLength);
> +    traversalState.m_desiredLength = length;
>      for (int i = 0; i < len; ++i) {
>          SVGPathSeg* segment = getItem(i, ec).get();
>          if (ec)
> @@ -90,6 +91,8 @@ unsigned SVGPathSegList::getPathSegAtLength(double, ExceptionCode& ec)
>              ASSERT(false); // FIXME: This only works with normalized/processed path data.
>              break;
>          }
> +        if (!segmentLength)
> +            continue;
>          traversalState.m_totalLength += segmentLength;
>          if ((traversalState.m_action == PathTraversalState::TraversalSegmentAtLength)
>              && (traversalState.m_totalLength > traversalState.m_desiredLength)) {

Wow, I wouldn't have expected it's so easy to implement. I wonder that this doesn't have any side effects :-)
Can you elaborate a bit more here to why this is correct? Maybe my question is resolved with a better ChangeLog!

Thanks a lot for tackling this,
Niko
Comment 5 Robin Qiu 2010-05-07 19:51:23 PDT
Created attachment 55450 [details]
Modified according to Niko's comment.

This bug is just a misktake: almost all of the code is OK, but the parameter is not used at all, therefore, this function always returns "1".

Another tiny problem is that the SVG spec says the index of the first path segment is number 0, but we start from number 1.
(By the way, Firefox starts from 1 as well), So I added this code to let it start from 0. 

+        if (!segmentLength)
+            continue

Here is a question, does "M0 0 L5 5 L5 5 L10 10" equal to "M0 0 L5 5 L10 10" ? 
If not, those two line code I added is incorrect, I will put it right.
Comment 6 Robin Qiu 2010-05-07 19:54:22 PDT
Created attachment 55452 [details]
Forgot to set "review ?".  Modified according to Niko's comment.
Comment 7 Nikolas Zimmermann 2010-05-07 22:15:40 PDT
Comment on attachment 55452 [details]
Forgot to set "review ?".  Modified according to Niko's comment.

WebCore/svg/SVGPathSegList.cpp:96
 +          if (!segmentLength)
Almost r+, the test is perfect. Just this line bugs me, from a SVG DOM perspective, "M 5 5" and "M 5 5 M 5 5" are two different path segments, no matter how it's rendered.
Also how can you be sure this is just a "move"? And why is the first move the one ignored? I guess this is not correct.
Comment 8 Nikolas Zimmermann 2010-05-07 23:32:02 PDT
Okay, I've tried the testcase across the SVG implementations:

Safari 4 (latest release)
FAIL path.getPathSegAtLength(0) should be 0. Was 1.
FAIL path.getPathSegAtLength(1) should be 0. Was 1.
PASS path.getPathSegAtLength(5) is 1
PASS path.getPathSegAtLength(6) is 1
FAIL path.getPathSegAtLength(10) should be 2. Was 1.
FAIL path.getPathSegAtLength(11) should be 2. Was 1.
FAIL path.getPathSegAtLength(16) should be 0. Was 1.

Opera 10.10

FAIL path.getPathSegAtLength(0) should be 0. Was 2.
FAIL path.getPathSegAtLength(1) should be 0. Was 2.
FAIL path.getPathSegAtLength(5) should be 1. Was 2.
FAIL path.getPathSegAtLength(6) should be 1. Was 2.
PASS path.getPathSegAtLength(10) is 2
PASS path.getPathSegAtLength(11) is 2
FAIL path.getPathSegAtLength(16) should be 0. Was 3.

FireFox trunk (Minefield)

PASS path.getPathSegAtLength(0) is 0
FAIL path.getPathSegAtLength(1) should be 0. Was 1.
PASS path.getPathSegAtLength(5) is 1
FAIL path.getPathSegAtLength(6) should be 1. Was 2.
PASS path.getPathSegAtLength(10) is 2
FAIL path.getPathSegAtLength(11) should be 2. Was 3.
FAIL path.getPathSegAtLength(16) should be 0. Was 3.

I think FireFox does it right.
The path is auto-closed because of the last segment, so the distance=16 should actually say 3 IMHO.
Comment 9 Nikolas Zimmermann 2010-05-07 23:59:10 PDT
Some more results on distances that exceed the actual path length, Opera/FF always return the last segment, we should mimic that.

PASS path.getPathSegAtLength(20) is 3
PASS path.getPathSegAtLength(24) is 3
PASS path.getPathSegAtLength(25) is 3
PASS path.getPathSegAtLength(100) is 3
Comment 10 Robin Qiu 2010-05-08 00:44:27 PDT
Created attachment 55468 [details]
3rd patch

Thanks for Niko's comment!

According to his comment, I changed some code, now, WebKit/Opera/FF all return the last path segment if the distance exceeds the actual path length, and also added some code in test case to verify this.
Comment 11 Nikolas Zimmermann 2010-05-08 00:53:20 PDT
Comment on attachment 55468 [details]
3rd patch

WebCore/svg/SVGPathSegList.cpp:103
 +      return --traversalState.m_segmentIndex;
Hm, why is that? Just return "traversalState.m_segmentIndex - 1". That saves one assignment operation :-)
Sorry for that, but r- :( Other than that it looks very good, If you change that last thing, I'm going to set r+/cq+
Comment 12 Robin Qiu 2010-05-08 05:51:19 PDT
Created attachment 55473 [details]
4th edition

Changed the last line. 
I admit that " return --traversalState.m_segmentIndex; " really looks a little strange. :P
Comment 13 Nikolas Zimmermann 2010-05-09 01:32:31 PDT
Comment on attachment 55473 [details]
4th edition

Good patch, r=me! Should land soon...
Comment 14 WebKit Commit Bot 2010-05-09 01:57:45 PDT
Comment on attachment 55473 [details]
4th edition

Rejecting patch 55473 from commit-queue.

Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1
Last 500 characters of output:
 setenv YACC /Developer/usr/bin/yacc
    /bin/sh -c /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Script-5DF50887116F3077005202AB.sh
** BUILD FAILED **

The following build commands failed:
WebCore:
	Distributed-CompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/i386/SVGPathSegList.o /Users/eseidel/Projects/CommitQueue/WebCore/svg/SVGPathSegList.cpp normal i386 c++ com.apple.compilers.gcc.4_2
(1 failure)


Full output: http://webkit-commit-queue.appspot.com/results/2199083
Comment 15 Eric Seidel (no email) 2010-05-09 14:05:59 PDT
Comment on attachment 55473 [details]
4th edition

I kinda dislike the behavior of returning the last path segment when you go past the path end, but I guess that makes the API more user friendly.  Glad we're finally adding testing!  That's why the method didn't work before was there was no testing. :)

Beautiful patch, btw.  Very well done!
Comment 16 Eric Seidel (no email) 2010-05-09 14:06:56 PDT
/Users/eseidel/Projects/CommitQueue/WebCore/svg/SVGPathSegList.cpp: In member function ‘unsigned int WebCore::SVGPathSegList::getPathSegAtLength(double, WebCore::ExceptionCode&)’:
/Users/eseidel/Projects/CommitQueue/WebCore/svg/SVGPathSegList.cpp:60: warning: implicit conversion shortens 64-bit value into a 32-bit value

was the error on the commit queue, btw.
Comment 17 Robin Qiu 2010-05-09 18:47:32 PDT
Created attachment 55515 [details]
5th patch, fixed that warning.
Comment 18 Eric Seidel (no email) 2010-05-09 18:55:19 PDT
Comment on attachment 55515 [details]
5th patch, fixed that warning.

Should we just have changed m_desiredLength to be a double?  I'm not sure what type Path exposes for its length,
Comment 19 Nikolas Zimmermann 2010-05-10 01:28:43 PDT
Comment on attachment 55515 [details]
5th patch, fixed that warning.

Hey Eric,

I just saw that you have already set r+, I'm taking it back to r-, because of two issues:
WebCore/svg/SVGPathSegList.cpp:60
 +      traversalState.m_desiredLength = static_cast<float>length;
How can this compile? Missing paranthesis, it should be: static_cast<float>(length).
Second issue: We should not just cast double to float, but instead use "narrowPrecisionToFloat".

Please upload a revised version.
Comment 20 Robin Qiu 2010-05-10 03:04:51 PDT
(In reply to comment #19)
> (From update of attachment 55515 [details])

I'm really sorry for this fault. I should always check the code in any case. 
I'm re-compiling all code and will do all layout tests. After that, I will upload a new one.
Comment 21 Robin Qiu 2010-05-10 22:05:08 PDT
Created attachment 55663 [details]
7 th patch

Changes in this patch:
1.  use narrowPrecisionToFloat() to converst double to float.
2. add a check before return "traversalState.m_segmentIndex - 1", otherwise, it maybe return 4294967295(-1) in some occasions.
Comment 22 Dirk Schulze 2010-05-10 22:56:59 PDT
Comment on attachment 55663 [details]
7 th patch

Great! Hopefully it lands this time.
Comment 23 WebKit Commit Bot 2010-05-12 01:19:31 PDT
Comment on attachment 55663 [details]
7 th patch

Clearing flags on attachment: 55663

Committed r59213: <http://trac.webkit.org/changeset/59213>
Comment 24 WebKit Commit Bot 2010-05-12 01:19:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Alexis Deveria 2010-05-18 08:06:43 PDT
Just wanted to say thanks for fixing this so fast! Great work! :)
Comment 26 Gavin Kistner 2011-03-09 13:21:51 PST
This bug appears to be correctly fixed in WebKit nightly builds, but is still present in the just-released Safari v5.0.4. How can I a) find out why it has not been accepted there, and/or b) encourage it to be applied to Safari?