651036 - SMIL: Change discrete to-animations visit the underlying and the 'to' value

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Cameron McCormack (:heycam)]

Following up from bug 544855, Brian recently brought up the counter-intuitiveness of discrete to-animations and requested that the behaviour be changed so that they are consistent with from-to animations.

This would change

  <circle r="10">
    <animate attributeName="r" to="20" calcMode="discrete" dur="2s"/>
  </circle>

from the current behaviour of setting the value to 20 for the entire duration of the animation, to setting 10 from 0s to 1s followed by 20 from 1s to 2s.

The SVG WG has agreed to make this change if we have two implementations passing a new test for this, http://dev.w3.org/SVG/profiles/1.1F2/test/svg/animate-elem-92-t.svg.  This patch will make us the second passing implementation (the first being Batik).

Comment 1

[Cameron McCormack (:heycam)]

Attachment: patch

Green test run here: http://tbpl.mozilla.org/?tree=MozillaTry&rev=65f15fbae3d6

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 526930 [details] [diff] [review]
patch

>+      if (aBaseValue.IsNull()) {
>+        rv = NS_ERROR_FAILURE;

Two things on this:
 (1) We could equivalently just return early here. (but read on for what I'd prefer)
 (2) It looks like this matches a similar case higher up, in the non-discrete section, highlighted here:
http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILAnimationFunction.cpp?mark=431-432#422
... and *that* existing code would end up dropping us into the (CALC_DISCRETE || NS_FAILED(rv)) case, and ultimately we'll hit the chunk of your patch that I've quoted above. So in that case, we just end up setting "rv = NS_ERROR_FAILURE" twice.

I think it'd be simpler to snip out both of those cases, and add a chunk up higher (before we even declare |rv|) like this:
>  if (IsToAnimation() && aBaseValue.IsNull()) {
>    return NS_ERROR_FAILURE;
>  }

Unless I'm missing something, I think this would produce the same behavior as your existing patch, and it'd simplify the logic a bit (and remove duplicated code).

>-  // For to-animation the number of values is considered to be 2 unless it's
>-  // discrete to-animation in which case either 1 or 2 is acceptable.
>+  // For to-animation the number of values is considered to be 2.
>   PRBool matchingNumOfValues = IsToAnimation() ?
>-      calcMode == CALC_DISCRETE ? numKeyTimes <= 2 : numKeyTimes == 2 :
>+      numKeyTimes == 2 :
>       numKeyTimes == aNumValues;

Why this change? Isn't it still sensible to have one keyTime for discrete to-animation?  This wasn't something that we changed in bug 544855.

>+++ b/content/smil/test/test_smilKeyTimes.xhtml
>@@ -148,35 +148,43 @@
>   testCases.push({
>     'attr' : { 'to': '100',
>                'calcMode': 'discrete',
>                'keyTimes': '0.0; 1.0' },
>-    'times': [ [ 0, 100 ],
>-               [ 7, 100 ] ]
>+    'times': [ [ 0, -100 ],
>+               [ 7, -100 ] ]
>   });

The "times" (expected output) for this test-bundle are identical to the first few times in the next test-bundle that you add in your patch.  Perhaps you could add a few more times here (as you do in your next test-bundle) to differentiate them?  Without that, the above test seems superfluous.

Comment 3

[Cameron McCormack (:heycam)]

(In reply to comment #2)
> I think it'd be simpler to snip out both of those cases, and add a chunk up
> higher (before we even declare |rv|) like this:
> >  if (IsToAnimation() && aBaseValue.IsNull()) {
> >    return NS_ERROR_FAILURE;
> >  }
> 
> Unless I'm missing something, I think this would produce the same behavior as
> your existing patch, and it'd simplify the logic a bit (and remove duplicated
> code).

Yes, that's better.

> >-      calcMode == CALC_DISCRETE ? numKeyTimes <= 2 : numKeyTimes == 2 :
...
> Why this change? Isn't it still sensible to have one keyTime for discrete
> to-animation?  This wasn't something that we changed in bug 544855.

Given that discrete to-animations are now always like from-to animations (where the from is the underlying value), does it actually make sense to allow keyTimes to have only a single value (which would be required to be keyTimes="0" anyway)?  Allowing it would be inconsistent with, say

  <animate attributeName="r" calcMode="discrete"
           values="10; 20; 30; 40" keyTimes="0; 0.2; 0.5"/>

being disallowed.

> The "times" (expected output) for this test-bundle are identical to the first
> few times in the next test-bundle that you add in your patch.  Perhaps you
> could add a few more times here (as you do in your next test-bundle) to
> differentiate them?  Without that, the above test seems superfluous.

True, I'll make the above test have times

 [[ 0, -100 ],
  [ 7, -100 ],
  [ 10, -100 ],
  [ 12, -100 ]]

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 526930 [details] [diff] [review]
patch

(In reply to comment #3)
> Given that discrete to-animations are now always like from-to animations (where
> the from is the underlying value), does it actually make sense to allow
> keyTimes to have only a single value (which would be required to be
> keyTimes="0" anyway)?  Allowing it would be inconsistent with, say
> 
>   <animate attributeName="r" calcMode="discrete"
>            values="10; 20; 30; 40" keyTimes="0; 0.2; 0.5"/>
> 
> being disallowed.

Ah, sorry - objection withdrawn.  I was thinking of the 1-keyTime as a shorthand syntax of a sort, but it wasn't really -- it made sense previously because there would be just 1 value, whereas now there'll be 2, so we need 2 keyTimes.

I was confused because that chunk of code changed *after* bug 544855, in bug 557885.

So: r=dholbert, with the first bit and last bit of Comment 3.

Comment 5

[Daniel Holbert [:dholbert]]

(outside the scope of this bug's patch, but semi-on-topic nonetheless):
This got me wondering, though -- from re-reading the SMIL spec on keyTimes, it sounds like they are only supposed to have an effect for |values| animations.

In particular...
> Each time in the [keyTimes] list corresponds to a value in the values attribute list
That's pretty definitive -- specifically mentioning the "values attribute".
And the first mention of from/to/by after that is in a subsequent chunk of text, which says:
[...]
> For the shorthand forms from-to animation and from-by animation, there
> are only 2 values.  A discrete from-to animation will set the "from" value
> for the first half of the simple duration and the "to" value for the second
> half of the simple duration
That chunk appears to specify the timing of from/to/by animations exactly, without any mention of keyTimes.
http://www.w3.org/TR/SVG11/animate.html#KeyTimesAttribute

Cameron / Brian, do you know if it's mentioned anywhere that keyTimes are supposed to apply to animations with from/to/by?  Or, perhaps that's just a place where we (and others) intentionally deviate from the spec?

Comment 6

[Cameron McCormack (:heycam)]

(In reply to comment #4)
> So: r=dholbert, with the first bit and last bit of Comment 3.

Thanks!

(In reply to comment #5)
> Cameron / Brian, do you know if it's mentioned anywhere that keyTimes are
> supposed to apply to animations with from/to/by?  Or, perhaps that's just a
> place where we (and others) intentionally deviate from the spec?

Right at the end of this section of this section http://www.w3.org/TR/SMIL/smil-animation.html#animationNS-InterpolationKeysplines of SMIL Animation it says

  The keyTimes and keySplines attributes can also be used with the from/to/by
  shorthand forms for specifying values, as in the following example:

  <animate attributeName="foo" from="10" to="20" 
     dur="10s" keyTimes="0.0; 1.0"
     calcMode="spline" keySplines=".5 0 .5 1" />

Comment 7

[Daniel Holbert [:dholbert]]

Aha! Thanks for clearing that up.

Comment 8

[Cameron McCormack (:heycam)]

http://hg.mozilla.org/mozilla-central/rev/8b9d7089124f