Closed Bug 540588 Opened 10 years ago Closed 10 years ago

Add support for SMIL animation of <integer> attributes in SVG

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

Attachments

(1 file)

We should add support for SMIL animation of <integer> attributes in SVG.
I have a patch for this, but while writing tests, the behavior seems a bit wrong to me. Forget integers for the moment, and consider enumerations. If you animate from enum value A to B in 1 second, then value B takes affect at 0.5 seconds, presumably because no affect would be observed otherwise, unless fill="freeze" (because the affect would be removed at the same time as it was applied if it took affect at 1 second). Coming back to integers, in our current implementation, if you animate from 1 to 2 in 1 second, the value 2 doesn't seem to take affect until 1 second, and thus you can only observe it with fill="freeze". Is this correct behavior? It seems unexpected to me and inconsistent with animation of enumerations, since going from int 1 to 2 is just as discrete a change as going from enum A to B.
(In reply to comment #1)
> Coming back to integers, in our current
> implementation, if you animate from 1 to 2 in 1 second, the value 2 doesn't
> seem to take affect until 1 second, and thus you can only observe it with
> fill="freeze". Is this correct behavior?

Put another way, the question is: should there be a difference between these animations?
>  <animate attributeName="someIntAttr" from="0" to="1" begin="0" dur="1" calcMode="linear">
>  <animate attributeName="someIntAttr" from="0" to="1" begin="0" dur="1" calcMode="discrete">

Our behavior in the "linear" case above will basically depend on *how* we coerce interpolated intermediate values into integers, within nsISMILType::Interpolate. If we use "floor", then we'll never visit the final value.  If we use "round", then we _will_ visit the final value -- and we'll match the "discrete" behavior exactly.

Right now, for CSS properties "font-weight" and "font-stretch" (which are basically integer-valued, though font-weight uses 100x the units), we have the "floor" behavior -- see bug 524808 comment 1 and 2, where that behavior was added -- apparently this is what the CSS Transitions spec calls for.

I haven't yet found a clear statement either way for SVG or SMIL, but I did find a relevant comment in a different spec, the SMIL Extended Mobile Profile:

> Integration definitions
> The Extended Mobile Profile defines a set of integration definitions as 
> required by the Animation modules. These definitions are:
[SNIP]
> Prior to display, the presentation value should be reduced or increased to
> the nearest value which is within the range of the target attribute.
> For integer attributes the computed value should then be rounded to the
> nearest integer (coerced-integer-value). The mathematical definition of
> rounding is:
> coerced-integer-value = Math.floor( interpolated-value + 0.5 )
http://www.w3.org/TR/SMIL2/smil21-extended-mobile-profile.html#q10

The above is part of an "integration definition" for a particular (unrelated) application of SMIL Animation, in a different spec.  This suggests that SMIL Animation itself leaves this behavior unspecified, and that SVG would need to specify what's correct.  I don't think SVG 1.1 does, though -- at least, http://www.w3.org/TR/SVG11/animate.html makes no mention of the words "coerce", "round", or "floor".

So I think "correct behavior" is unspecified here.  What do Opera & Webkit do, for integer-valued SVG attributes?
Blocks: 436276
Attached patch patchSplinter Review
Webkit is completely broken (as in doesn't even render my test correctly), and Opera appears to use floor(). I believe we should use round() though, both for consistency with enum, and so that you don't get animations that "are ignored" (i.e. only take affect at the end of the simple duration), but mostly because otherwise animation affect would otherwise depend on the "direction" of the animation. Consider:

a) 1 -> 2
b) 2 -> 1

For (a), with floor() the animation wouldn't have any affect at any point. For (b), with floor() the animation would take affect at the very begining of the simple duration and remain in affect until the end of the simple duration.

Hence I think we should go with round() behavior.
Assignee: nobody → jwatt
Status: NEW → ASSIGNED
Attachment #423247 - Flags: review?(dholbert)
(In reply to comment #3)
> I believe we should use round() though, both for
> consistency with enum,

Generally, round() doesn't actually make us consistent with enum -- they just happen to match in the two-value case, but in anything more complex (more than 2 values), they won't match.  (Note that enum forces calcMode="discrete", and authors can easily get matching behavior in their |int|-type animations by specifying calcMode="discrete")

e.g. for an enum attribute, with a/b/c being enum values...
   <animate values="a;b;c" dur="2s" ...>  (implicit calcMode="discrete")
we'll spend 0.666s at each value.

For an int attribute, with 1/2/3 being int values...
    <animate values="1;2;3' dur="2s" ...> (implicit calcMode="linear")
or equivalently:
    <animate from="1" to="3" dur="2s" ...> (implicit calcMode="linear")
* if we use floor(): we'll spend 1s at '1', 1s at '2', and 0s at '3'
* if we use round(): we'll spend 0.5s at '1', 1s at '2', and 0.5s at '3'
* if we use calcMode="discrete", we'll spend 0.666 sec at each.

> mostly because
> otherwise animation affect would otherwise depend on the "direction" of the
> animation.

Hmm, good point.  So I guess generally, with an N-second animation through a monotonically increasing or decreasing series of ints...
 - using floor(), we visit each value for 1/Ns, except the highest gets 0s.
 - using round(), we steal half of the time from the lowest value and give it to the highest value.  So the first & last values end up with half the time of every intermediate value.

This gets significantly weirder if we animate through a range that has "peaks" and "valleys".  Then, the directionality of floor() means we don't actually visit any of the peaks (though it *might* give them a single sample, if they get reallyreally lucky), and we'd visit the valleys for extra time.  Details with example below:

e.g. Consider an N-second animation of the form:
   <animate values="0;4;0;4;0;4;0" ...>
 * With floor(), each value gets 1/N seconds, except:
    - Each "4" will get zero seconds per visit. (It might get a single sample, if it's ridiculously lucky and do sample at *exactly* the right instant, but otherwise we never see a 4 value.)
    - Each "3" will essentially get a continuous 2 * 1/N seconds per visit (which has a low probability of being interrupted by the "lucky sample" mentioned above for "4")
    - Each "0" will get 2 * 1/N seconds per visit (besides the first & last 0)

 * With round(), each value gets 1/N seconds, except the first & final values each get half that.

I think I'm swayed to favor round()... The directionality imposed by floor() does seem like it makes things significantly more complex, and it presents a barrier to authors understanding what's going on.   As for round(), the "first and last values get half time" behavior is a little odd, but I think it's acceptable given that the rest of round's behavior is much more predictable & easier to understand.

We should get this specified in the SVG spec (probably using similar language as the chunk quoted from "smil21-extended-mobile-profile" in Comment 2).  Jonathan, could you email www-svg about this?
Comment on attachment 423247 [details] [diff] [review]
patch

>diff --git a/content/smil/SMILIntegerType.cpp b/content/smil/SMILIntegerType.cpp
>+void
>+SMILIntegerType::Destroy(nsSMILValue& aValue) const
>+{
>+  NS_PRECONDITION(aValue.mType == this, "Unexpected SMIL value.");
[...]
>+  NS_PRECONDITION(aDest.mType == aSrc.mType, "Incompatible SMIL types.");
>+  NS_PRECONDITION(aDest.mType == this, "Unexpected SMIL value.");
[...]
>+  NS_PRECONDITION(aStartVal.mType == this,
>+                  "Unexpected types for interpolation.");
>+  NS_PRECONDITION(aResult.mType   == this, "Unexpected result type.");

These warning messages shouldn't end with a period -- if they fail, we already will print ":" after the warning-message, so this would end up as ".:", which looks silly.

>+SMILIntegerType::ComputeDistance(const nsSMILValue& aFrom,
>+                                 const nsSMILValue& aTo,
>+                                 double& aDistance) const
>+{
>+  NS_PRECONDITION(aFrom.mType == aTo.mType,"Trying to compare different types");
>+  NS_PRECONDITION(aFrom.mType == this, "Unexpected source type");
>+
>+  const double from = double(aFrom.mU.mInt);
>+  const double to   = double(aTo.mU.mInt);
>+
>+  aDistance = fabs(to - from);

Rather than doing two separate double-conversions there, it would be simpler to first subtract and *then* convert, like this:

  aDistance = fabs(double(aTo.mU.mInt - aFrom.mU.mInt));

>+# animate some <integer> attributes:
>+== anim-feTurbulence-numOctaves-01.svg anim-feTurbulence-numOctaves-01-ref.svg
>+

We need a few more tests here.  Right now, you test animating from 2-->1 (sampled 40% of the way through) and 1--> 2 (sampled 50% of the way through).  Some other things to test:
 - Animation from 2-->1 at some time >= 50% (to make we actually reach 1)
 - Animation from 1-->2 at some time < 50% (to make sure we sample "1" during the first half)
(Perhaps the above points could both be done in "anim-feTurbulence-numOctaves-02.svg", which would just have the animation-timing reversed w.r.t. -01.svg?  And its reference case would have numOctaves="1")
 - Animation across larger int ranges (i.e. if we go from 1 to 10, we should be at 5 when we're roughly halfway through)
Comment on attachment 423247 [details] [diff] [review]
patch

r=dholbert with previous comment addressed.
Attachment #423247 - Flags: review?(dholbert) → review+
A few more things:
 - Per the first chunk of bug 541884 comment 3, SMILIntegerType::Init should reduce its precondition to only assert that aValue.IsNull().  (We should make that change to all the implementations of nsISMILType::Init -- we already obey this stricter rule, and making that assumption allows more complex types to have much simpler Init() implementations.)
 - We need tests for a few more things, actually -- right now, we're testing SMILIntegerType::Interpolate, but not the other mathematical methods.  We need at least one test with "from/by" animation and one test with calcMode="paced", to assert that our SMILIntegerType::Add() and ::ComputeDistance() implementations are correct.
Pushed http://hg.mozilla.org/mozilla-central/rev/c61d3ed9a015
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
(In reply to comment #4)
> We should get this specified in the SVG spec (probably using similar language
> as the chunk quoted from "smil21-extended-mobile-profile" in Comment 2). 
> Jonathan, could you email www-svg about this?

It seems quite unlikely that anyone will have a problem with lack of interoperability for this case, so rather than try to get the WG to discuss it now, I've just filed a bug against SVG 2.0 in the WG issue tracker:

http://www.w3.org/Graphics/SVG/WG/track/issues/2306
You need to log in before you can comment on or make changes to this bug.