643419 - SVG: pathLength attribute don't work

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Бражник Юрий]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/534.16 (KHTML, like Gecko) Chrome/10.0.648.151 Safari/534.16
Build Identifier: 4.0 RC1

Summary say everithing. It's just don't work. It's not working in all other browsers too, except Opera 11.01.
W3 documentation on this: http://www.w3.org/TR/SVG/paths.html#PathLengthAttribute

Reproducible: Always

Steps to Reproduce:
path d="M0,0 L100,0" pathLength="200" stroke-dasharray="100,100"
Actual Results:  
dash appears to be 100% of path istead of only 50%(100 from 200)

Expected Results:  
dash must be only 50% of path length

Comment 1

[Robert Longson [:longsonr]]

Can you add a complete testcase as an attachment using the Add an attachment link above please?

Comment 2

[Бражник Юрий]

Attachment: Test SVG file

This SVG must draw horisontal line from left to center(100 is 50% of 200). But in firefox 4.0 RC1 it draws line from left to right because pathLength don't work.

Comment 3

[Бражник Юрий]

Attachment: Example of use case

This SVG demonstrates that with pathLength attribute one can make smooth curve drawing animation without javascript(using only SMIL). To see the proper animation use Opera 11.

This functionality wuld be very helpfull in drawing, map and navigation software.

Comment 4

[Daniel Holbert [:dholbert]]

FWIW, if you're designing an app that depends on this behavior, you can get the same effect using an animation on "stroke-dashoffset". See e.g. http://hoffmann.bplaced.net/svgtest/stroke16a.svg

Comment 5

[Бражник Юрий]

That approach relies on precomputed path length. Using pathLength attribute we dont nead to know actual path length.

Another approach is to use javascript getTotalLength() to determine the path length and change dash length accordingly. But this approach neads javascript and all animated objects shuld have "id". Not so preaty.

Comment 6

[Dr. Olaf Hoffmann]

A simpler method is to animate stroke-dasharray directly, not stroke-dashoffset
and you need no pathLength, just the second list element has to be large enough:
http://hoffmann.bplaced.net/svgtest/lorenz2tiny.php
Unfortunatly several viewers have problems with animation of stroke-dasharray
and one should not use a path with more than one M/m command together with
stroke-dasharray at all, because this is somehow between undefined and typically
wrong implemented.

On the other hand pathLength is basically intended to provide information about
own numerical results for the calculation of the path length to allow the viewer
to adjust several features to the intended effect, therefore the provided 
pathLength will typically only deviate slightly from the result of the viewer.

But obviously because numerics is not simple for every author, pathLength provides an important functionality to get predictable results in a simple way.

Comment 7

[Бражник Юрий]

(In reply to comment #6)
> A simpler method is to animate stroke-dasharray directly, not stroke-dashoffset
> and you need no pathLength, just the second list element has to be large
> enough:
> http://hoffmann.bplaced.net/svgtest/lorenz2tiny.php

The example you provide use pathLength! And it's not simpler to use stroke-dasharray...

Comment 8

[Dr. Olaf Hoffmann]

But my calculated pathLength is only of minor importance. It is not used within 
the animation of stroke-dasharray.
You can see, that with Gecko the circle is always on the beginning of the
stroked part of the stroke-dasharray pattern, but Gecko currently ignores 
pathLength. This attribute becomes only important, if the pathLength calculation
of the viewer deviates significantly from my calculation. Because fortunately this is not the case for Gecko, you still get the intended result.

Animation of stroke-dasharray for this purpose is simpler than that of stroke-dashoffset, because you don't have to calculate the pathLength at all, if the
timing within the painting of stroke pattern is not so important. In this case
the timing is important, because it has to be the same as for the circle in motion. For this reason, the pathLength gets relevant, but you need it anyway
for each path segment to get the timing synchronised - this will be always the
task for the author. The value of the pathLength attribute is only an additional
help to improve the accuracy of the presentation of the viewer and the calculated values. If this is good enough, pathLength has no influence at all.
It matters only, if the accuracy is bad.

Comment 9

[Бражник Юрий]

You precalculate path length in your php script and then generate svg accordingly?

PS: my skype: brazhnyk_yuriy

Comment 10

[Dr. Olaf Hoffmann]

Yes, for this application this is required. Else, if 1% of the active duration of
the animation does not matter, you can simply use stroke-dashoffset itself to get
an approximation of the pathLength for the last value in the values list of
the animation, if you do not use a PHP-script. For static files I typically use
this approach to get the aproximate pathLength for the last value from a reliable
viewer...

Comment 11

[Бражник Юрий]

You can read my 5 comment about precomputed path length and another aproach based on JavaScript...
Your approach is working in Gekko(unlike approach proposed in 4 comment) but still this is not an ultimate answer... And your approach generates huge output...
One good reason to use your method is to support devices that dont have JS or have JS turned off...

I will attach example of JS method soon...

Comment 12

[Robert Longson [:longsonr]]

Attachment: patch

Comment 13

[Robert Longson [:longsonr]]

Attachment: test adapted from patch

Comment 14

[Robert Longson [:longsonr]]

Olaf, are you happy that the testcase in comment 13 should display as an entirely lime page?

Comment 15

[Dr. Olaf Hoffmann]

Not sure, if I understand the test at all (and the behaviour might depend on
the script).
At least, because animateMotion uses calcMode paced by default, it is pretty
useless to provide keyPoints and keyTimes. 
Anyway, even for calcMode discrete, linear or spline, keyPoints, keyTimes,
keySplines etc do not depend on the absolute length of the path, therefore you
should get the same result with and without pathLength (there are for example
bugs in Operas pathLength implementation related to this and resulting in nonsense-effects ;o) - therefore to compare animateMotion with a path with
pathLength and one without could be a useful test to ensure, that bugs from
Opera are not duplicated ;o)
Up to now, concerning animateMotion I have no example, where pathLength could
influence the behaviour. Of course it has for stroke-dasharray, text on a path
etc, if you use something not relative to the pathLength. 
If the notation is relative to the pathLength, obviously this notation covers
the effect of pathLength itself. Or at least to see some deviation, one might
have to use path segments, that cause accuracy problems for the viewer in 
pathLength calculation for such segments. But because one can only provide
pathLength for the complete path, not for each segment, one can in the worst
case only see the accuracy problem of the viewer, but pathLength provided by the author cannot help to fix this.

If you write stroke-dasharray="20,20", pathLength has clearly an effect, if
there is a deviation between the authors calculation and the viewers
calculation. Authors can never note values for stroke-dasharray relative to
the pathLength. For animateMotion I think they cannot note something not
relative to the pathLength.

Comment 16

[Robert Longson [:longsonr]]

What I'm trying to do is freeze an animation along a path half way along the path. If the pathLength is set to a different value to the actual length of the path then it should freeze at a different point shouldn't it?

Comment 17

[Robert Longson [:longsonr]]

In http://www.w3.org/TR/SVG/animate.html#AnimateMotionElement the keyPoints text says...

Distance calculations use the user agent's distance along the path algorithm. Each progress value in the list corresponds to a value in the ‘keyTimes’ attribute list.

Comment 18

[Dr. Olaf Hoffmann]

Attachment: extreme animateMotion sample, where pathLength, accuracy and timing model problems might matter. The frozen position of the rect and circle will depend on this.

Comment 19

[Dr. Olaf Hoffmann]

With my sample, you can see for example differences between Opera and 
Gecko, maybe others as well - you can see it with and without pathLength.

Because for calcMode paced (the default for animateMotion) is defined, that
keyTimes are ignored, nothing can depend on it. You have to set calcMode
linear or discrete to set keyPoints and keyTimes, obviously then it is better to
have a more exiting structure like
<animateMotion dur="5s" "calcMode="discrete" keyPoints="0.7;0.1;0;1;0.5" keyTimes="0;0.3;0.7;1" fill="freeze"/> ...

Comment 20

[Robert Longson [:longsonr]]

I'll attach a new patch.

Comment 21

[Robert Longson [:longsonr]]

Attachment: updated patch

Updated reftest in this one with both calcMode=linear and calcMode=paced based on recent comments

Comment 22

[Daniel Holbert [:dholbert]]

IIUC, this patch will make pathLength affect the behavior of a "keyPoints" animateMotion animation.  That doesn't make sense to me.

I interpret "keyPoints" values to be fractional progress-values from range [0,1], with 0 matching the beginning of the rendered path and 1 matching the end of the rendered path.

Even if the user provides us with their own calculation of the path's length, that shouldn't visually affect our calculation of what "50% of the way along the path" means, IMHO.  (Perhaps under-the-hood it could affect our calculations, but it should end up canceling out, I'd imagine.)

Comment 23

[Daniel Holbert [:dholbert]]

(In reply to comment #22)
> I interpret "keyPoints" values to be fractional progress-values from range
> [0,1], with 0 matching the beginning of the rendered path and 1 matching the
> end of the rendered path.

Contrast this to e.g. GetPointAtLength, which takes an actual length value.  In that case, it makes sense to me that we should interpret that (and normalize) with respect to the author's specified pathLength.

So e.g. supposing we have a 400px-long straight line for our path, with an author-specified pathLength="100".  In that case, I'd expect getPointAtLength(50) to return the path's actual center point, and your patch seems to do that, which is great.  But regardless of the pathLength attr, I'd also expect keyPoints="0.5" to put us at the path's actual center point, which I think your patch would break.

This all (comment 22 & this comment) is to say: I'm not sure I agree that we need the SVGMotionSMILAnimationFunction.* changes.

Comment 24

[Daniel Holbert [:dholbert]]

Comment on attachment 527987 [details] [diff] [review]
updated patch

>+++ b/layout/reftests/svg/pathLength-01.svg
>@@ -0,0 +1,4 @@
>+<svg viewBox="0 0 100 2" preserveAspectRatio="none" xmlns="http://www.w3.org/2000/svg">
>+  <rect width="100%" height="100%" fill="red"/>
>+	<path d="M-100,1 L200,1" pathLength="50" stroke-dasharray="100,100" fill="none" stroke="lime" stroke-width="2"/>
>+</svg>

This test is a bit coarse - both the path itself and its stroked dash extend far to the right of the viewPort, which allows our code to be off by quite a lot without the test noticing.

To address that, could you:

 (a) Tweak this test so that it just *barely* has enough lime to pass, so we can be sure that we're scaling stroke-dasharray enough -- e.g., replace your <path> element with the following (or something like it):
>    <!-- This path is really 400 units long (and its halfway point is at the
>         right edge of our viewBox). We use pathLength to normalize its length
>         to 20, though, so the first 10-unit-long dash in stroke-dasharray ends
>         up covering 10/20 = 1/2 of the path. This covers the whole viewBox. -->
>    <path d="M-100,1 h400" pathLength="20" stroke-dasharray="10"
>          fill="none" stroke="lime" stroke-width="2"/>

 (b) Add a second testcase that ensures we're not scaling *too much* -- e.g. make a copy of your existing test, after the above tweak (or something like it), and then flip the lime vs. red colors, and change stroke-dasharray to "5".  (That will make a red stroke go *just* up to our viewport, and then be transparent across the viewport, and then start the red stroke again *just* to the right of the viewport).

>+++ b/layout/reftests/svg/smil/motion/animateMotion-mpath-pathLength-1.svg

So in this test, IIUC, you're you're using pathLength to make an animateMotion-targeted element hit the end of its path when the animation's duration is only 50% completed (and when it otherwise would only be 50% of the way along the path).

Per my previous 2 posts, I'm not sure I agree that's correct.  (Whatever we end up doing w.r.t. that, though, I agree that we need an animateMotion reftest to check the settled-on behavior.)

> nsSVGTextPathFrame::GetStartOffset()
> {
[...]
>+  val *= GetPathScale();
>+
>   if (length->IsPercentage()) {
>     nsRefPtr<gfxFlattenedPath> data = GetFlattenedPath();
>     return data ? (val * data->GetLength() / 100.0) : 0.0;
>-  } else {
>-    return val * GetPathScale();
>   }
>+  return val;

My reasoning regarding animateMotion/keyPoints applies here, too -- I'd imagine that e.g. startOffset="50%" should always place us 50% of the way along the path, regardless of the user's pathLength normalization value.

So I'd think we'd want to preserve the existing logic here -- that is, I don't think we should use GetPathScale() in the length->IsPercentage() case.  What do you think?

Also, could you add a reftest with a pathLength and a percent-valued startOffset?  Since your change in this function doesn't break any existing reftests, that leads me to believe that this is un-tested right now, which we should fix. :)

Comment 25

[Dr. Olaf Hoffmann]

About comment 22:
This is, what I already tried to explain, typically there is no visible effect
from pathLength for animateMotion. Only if the pathLength calculation of the
viewer has low accuracy for some path segments, the pathLength may have some 
influence. For example if one uses a simple integration method together with
the parametrisation of a cubic path, the numerical result will be typically
slightly shorter than the correct length. If something like the De Casteljau's
algorithm is used to approximate the cubic curve with lines, the result will be
slightly larger than the correct length. If as in attachment 527954 [details] the path
consists of a segment of known length and one with a numerical result and a
keyPoint or the frozen end of the animation or something similar is set close
to M between those two path segments, there can be some visible difference.
Of course, if the calculation of the author is low and that of viewers is good,
the difference between expected result and rendered result will be always the
same, only if the accuracies of the viewers are different, their rendering might
get different and may depend on pathLength as well.
But the accuracy of pathLength calculation seems to be good for Gecko,
therefore it should be difficult to see an effect at all. This does not mean,
that it will fit to the expectation of an author with a low accuracy calculation ;o)
pathLength calculation of Opera is good as well, the reason why it has a wrong frozen position for the given sample is maybe an accuracy problem for timing, a pessimisation introduced already for Opera 9.5alpha.

Comment 26

[Robert Longson [:longsonr]]

I'll remove all the animateMotion stuff and the percentage text change. I must admit I found the text in http://www.w3.org/TR/SVG/animate.html#AnimateMotionElement about the keyPoints using the user agent's distance along the path algorithm confusing as well as this text further on...

At any time t within a motion path animation of duration dur, the computed coordinate (x,y) along the motion path is determined by finding the point (x,y) which is t/dur distance along the motion path using the user agent's distance along the path algorithm.

Comment 27

[Daniel Holbert [:dholbert]]

(One other thing I just realized - we have no tests at all for getPointAtLength right now -- could you either add a mochitest for that (with/without pathLength) or file a quick followup bug on that?)

Comment 28

[Robert Longson [:longsonr]]

Attachment: address more review comments

Comment 29

[Robert Longson [:longsonr]]

(In reply to comment #27)
> (One other thing I just realized - we have no tests at all for getPointAtLength
> right now -- could you either add a mochitest for that (with/without
> pathLength) or file a quick followup bug on that?)

I'll file a followup.

Comment 30

[Daniel Holbert [:dholbert]]

Comment on attachment 529296 [details] [diff] [review]
address more review comments

>+++ b/layout/reftests/svg/textPath-01-ref.svg
[...]
>+  <text>
>+    <textPath xlink:href="#x" font-size="20" fill="black" startOffset="50%">Should see this</textPath>
>+  </text>
>diff --git a/layout/reftests/svg/textPath-01.svg b/layout/reftests/svg/textPath-01.svg
[...]
>+++ b/layout/reftests/svg/textPath-01.svg
[...]
>+  <text><textPath xlink:href="#x" font-size="20" fill="black" startOffset="50%">Should see this</textPath></text>
[...]

Nit: Where possible, it's nice to make the testcase & reference case easily diff-able (with the only difference being the thing that you're testing), so that people investigating test-failures can easily figure out what's actually being tested.

Could you make the above chunks match, so that the presence of |pathLength| is really the only difference between these two files?

Also: per the end of my note on animateMotion-mpath-pathLength-1.svg above, I think we need a reftest for <mpath> with keyPoints & pathLength (like you had in your previous patch, except now asserting that the pathLength *doesn't* affect us).  Something like the above textPath testcase would be nice (just two copies of the same file, with pathLength added to one).

r=dholbert with the above.

Comment 31

[Daniel Holbert [:dholbert]]

(In reply to comment #30)
> Also: per the end of my note on animateMotion-mpath-pathLength-1.svg above, I
> think we need a reftest for <mpath> with keyPoints & pathLength

(This test isn't super-crucial to have now, since we're not expecting its behavior to change -- so if you don't have time for it right now, feel free to bundle it in with the followup getPointAtLength bug, and somebody can address it there.)

Comment 32

[Robert Longson [:longsonr]]

Attachment: hg changeset patch with extra tests

Comment 33

[Robert Longson [:longsonr]]

push to try http://hg.mozilla.org/try/pushloghtml?changeset=eddc7bf8d690

Comment 34

[Robert Longson [:longsonr]]

try was fine modulo known intermittent oranges

Comment 35

[Robert Longson [:longsonr]]

Followups won't be necessary as I added additional tests

Comment 36

[Jacek Caban]

http://hg.mozilla.org/mozilla-central/rev/133040714411

Comment 37

[Eric Shepherd [:sheppy]]

Documented the pathLength attribute:

https://developer.mozilla.org/en/SVG/Attribute/pathLength

And mentioned on Firefox 6 for developers.