Last Comment Bug 522267 - Add support for animating <number> attributes
: Add support for animating <number> attributes
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
:
Mentors:
Depends on:
Blocks: 436276 527396
  Show dependency treegraph
 
Reported: 2009-10-14 09:10 PDT by Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
Modified: 2010-01-15 11:21 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
quick testcase for stop offset (811 bytes, image/svg+xml)
2009-10-14 11:28 PDT, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
no flags Details
WIP (25.12 KB, patch)
2009-10-14 11:30 PDT, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
no flags Details | Diff | Review
patch to make GetAnimVal take its element as an argument (12.53 KB, patch)
2009-11-08 20:13 PST, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
dholbert: review+
Details | Diff | Review
patch to actually make <number> animatable (18.63 KB, patch)
2009-11-08 20:14 PST, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
dholbert: review+
Details | Diff | Review
patch to make GetAnimVal take its element as an argument (12.89 KB, patch)
2010-01-11 08:32 PST, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
dholbert: review+
Details | Diff | Review
patch to actually make <number> animatable (24.92 KB, patch)
2010-01-11 08:34 PST, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
dholbert: review+
Details | Diff | Review

Description Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2009-10-14 09:10:01 PDT
I've been looking at the SMIL code and thought I'd start off by adding support for animating <number> attributes such as the 'offset' attribute on the gradient 'stop' element.
Comment 1 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2009-10-14 11:28:34 PDT
Created attachment 406255 [details]
quick testcase for stop offset
Comment 2 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2009-10-14 11:30:30 PDT
Created attachment 406256 [details] [diff] [review]
WIP

This seems to work, but there are a few issues I need to work out.
Comment 3 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2009-11-08 20:13:47 PST
Created attachment 411125 [details] [diff] [review]
patch to make GetAnimVal take its element as an argument

This is a preliminary patch to make nsSVGNumber::GetAnimVal take it's element as an argument so that we notify the element of animations to it. Split it out to wake the next patch easier to read and use as a reference on how to add an animation type.
Comment 4 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2009-11-08 20:14:32 PST
Created attachment 411126 [details] [diff] [review]
patch to actually make <number> animatable
Comment 5 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2009-11-08 23:45:44 PST
Note to self: need to file a follow-up bug about allowing animation of "offset" on <stop> using percentage values as well as floats.
Comment 6 Daniel Holbert [:dholbert] 2009-11-09 11:32:37 PST
Comment on attachment 411126 [details] [diff] [review]
patch to actually make <number> animatable

Looks good generally! Just a few comments, mostly nits.

>+nsSMILValue
>+nsSVGNumber2::SMILNumber::GetBaseValue() const
>+{
>+  nsSMILValue val(&nsSMILFloatType::sSingleton);
>+  val.mU.mDouble = mVal->GetBaseValue();
>+  return val;
>+}
>+
>+void
>+nsSVGNumber2::SMILNumber::ClearAnimValue()
>+{
>+  if (mVal->mIsAnimated) {
>+    mVal->SetAnimValue(mVal->mBaseVal, mSVGElement);
>+    mVal->mIsAnimated = PR_FALSE;
>+  }
>+}

Here, you use mVal->GetBaseValue() in the first function, and mVal->mBaseVal in the second.  We can just use mVal->mBaseValue in both places, right?

>+    SMILNumber(nsSVGNumber2* aVal, nsSVGElement *aSVGElement)
>+    : mVal(aVal), mSVGElement(aSVGElement) {}
>+
>+    // These will stay alive because a nsISMILAttr only lives as long
>+    // as the Compositing step, and DOM elements don't get a chance to
>+    // die during that.
>+    nsSVGNumber2* mVal;
>+    nsSVGElement* mSVGElement;
>+
>+    // nsISMILAttr methods
>+    virtual nsresult ValueFromString(const nsAString& aStr,
>+                                     const nsISMILAnimationElement* aSrcElement,
>+                                     nsSMILValue &aValue) const;

The "*" and "&" type-or-variable-hugging could use a bit of consistency-cleanup here.  e.g. the SMILNumber constructor uses (Foo* val, Foo *val) and ValueFromString() uses (Foo& val, ..., Foo &val).

I think we prefer type-hugging (rather than variable-name-hugging), right?

>diff --git a/layout/svg/base/src/nsSVGUtils.h
[...]
>+  static nsresult NumberFromString(const nsAString &aString, float *aValue,
>+                                   PRBool aAllowPercentages = PR_FALSE);

Does this new function really need to return a nsresult?  It seems like it'd be simpler to just have it return PRBool.

Also, it looks like local style in this .h file (at least, the two functions immediately above this one) use type-hugging for *'s and &'s -- you should match that in this function signature (and in the corresponding chunk of the .cpp file)

>+== anim-offset-01.svg lime.svg
>+fails == anim-pathLength-01.svg anim-pathLength-01-ref.svg

Why does anim-pathLength-01.svg fail?  That could probably use a comment -- ideally with a bug #, or at least an explanation of what's missing.

Also -- it looks like both of the added reftests only test the end condition (when we've actually reached the "to" value). We should probably check an intermediate value somewhere, too, to make sure we're actually interpolating.

(If you want to use a 9-value grid like the ones in reftests/svg/smil/style/, I've got a Gnumeric spreadsheet that will auto-generate the expected intermediate values for use in the reference case.)

>diff --git a/content/svg/content/src/nsSVGStopElement.cpp
[snip]
>+#include "nsSVGUtils.h"
> #include "nsGenericHTMLElement.h"
> #include "prdtoa.h"

Now that you've moved the parsing code out of this file, I think you can remove the |#include "prdtoa.h"|  here.

r=dholbert with the above addressed.
Comment 7 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2010-01-11 08:32:52 PST
Created attachment 421060 [details] [diff] [review]
patch to make GetAnimVal take its element as an argument
Comment 8 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2010-01-11 08:34:35 PST
Created attachment 421061 [details] [diff] [review]
patch to actually make <number> animatable

I changed a few things, maybe not enough to re-request review, but I'll err on the side of caution. I've added reftests, but no mochitests yet. They can come later.
Comment 9 Daniel Holbert [:dholbert] 2010-01-13 12:55:26 PST
Comment on attachment 421061 [details] [diff] [review]
patch to actually make <number> animatable

>diff --git a/content/svg/content/src/nsSVGNumber2.cpp b/content/svg/content/src/nsSVGNumber2.cpp
>+  PRBool ok = nsSVGUtils::NumberFromString(aStr, &value);
>+  if (!ok) {
>+    return NS_ERROR_FAILURE;
>+  }

Perhaps ditch the "ok" variable, and move the NumberFromString call inside the "if()"? The same applies to the other NumberFromString call, in nsSVGStopElement::ParseAttribute.

Just a thought - the current way is fine, too.

>diff --git a/layout/reftests/svg/smil/anim-feOffset-01.svg b/layout/reftests/svg/smil/anim-feOffset-01.svg
>+      <animate attributeName="dx"
>+               calcMode="linear"

The calcMode="linear" setting is unnecessary -- that's the default value of that attribute, and it's the intuitively-expected <animate> behavior.  So, unless there's a reason for that, I'd suggest removing the |calcMode="linear"| settings.  (It's in all 3 added reftests, I think.)

>diff --git a/layout/reftests/svg/smil/lime.svg b/layout/reftests/svg/smil/lime.svg
>+<svg xmlns="http://www.w3.org/2000/svg" version="1.1">
>+  <title>Testcase reference file for generic pass condition</title>
>+  <rect width="100%" height="100%" fill="lime"/>
>+</svg>

We actually already have a file "anim-standard-ref.svg" in that folder, to play the role of "generic reference case".  I'd rather not add a second generic reference case to the same folder when we could just use the same one.  Could you use anim-standard-ref.svg for these instead?  (requires some minor tweaks to your reftests, but not much I think.)

r=dholbert with that.
Comment 10 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2010-01-15 05:54:27 PST
Pushed:

http://hg.mozilla.org/mozilla-central/rev/8d44dc537f43
http://hg.mozilla.org/mozilla-central/rev/eedaca875fa1

(In reply to comment #9)
> Perhaps ditch the "ok" variable, and move the NumberFromString call inside the
> "if()"? The same applies to the other NumberFromString call, in
> nsSVGStopElement::ParseAttribute.

I just left this as it is.

> The calcMode="linear" setting is unnecessary

I agree that it's technically unnecessary, but I think it's useful for clarity for anyone looking at tests that isn't so familiar with SMIL animation, so I also left this unchanged. (I'm not sure I agree that "linear" is necessarily the intuitive default - ease-in-out would seem just as intuitive to me.)

> We actually already have a file "anim-standard-ref.svg" in that folder, to play
> the role of "generic reference case".  I'd rather not add a second generic
> reference case to the same folder when we could just use the same one.  Could
> you use anim-standard-ref.svg for these instead?  (requires some minor tweaks
> to your reftests, but not much I think.)

The convention I'm familiar with (and strongly prefer) is that a pass should result in the entire viewport being filled with green, if it's possible to write the test that way. This makes it very easy to tell immediately from visual inspection without any ambiguity whether a test has passed or not. Blue on the other hand is used in Hixie's/the CSS WG's conventions to mean "further inspection to determine pass/fail required". The edges of the rect in anim-standard-ref.svg also makes it difficult to tell from visual inspection whether they are in the correct place or not.

Given that animation of attributes is dragging on (totally on me!) and that it's really hard to find landing slots in the tree nowadays, I went ahead and pushed in the slot that just opened. We can always discuss things and change them. Hope you don't mind. :-)
Comment 11 Daniel Holbert [:dholbert] 2010-01-15 11:20:19 PST
(In reply to comment #10)
> > The calcMode="linear" setting is unnecessary
> 
> I agree that it's technically unnecessary, but I think it's useful for clarity
> for anyone looking at tests that isn't so familiar with SMIL animation, so I
> also left this unchanged. (I'm not sure I agree that "linear" is necessarily
> the intuitive default - ease-in-out would seem just as intuitive to me.)

Ok, sounds good. I only mentioned this since "linear" seemed intuitive to me (and there are plenty of other default attributes whose values we could provide but don't) -- but you make a good point about ease-in-out being another reasonable default behavior that a reader of the test might be expecting.

> > We actually already have a file "anim-standard-ref.svg" 
> The convention I'm familiar with (and strongly prefer) is that a pass should
> result in the entire viewport being filled with green

Fair enough.  I'm happy to use lime.svg as the reference case for basic SMIL tests that I write in the future.  Maybe I'll port the other tests to use it instead of anim-standard-ref.svg at some point.

(FWIW, just to explain my original rationale behind anim-standard-ref's  characteristics: it's non-full-screen so that it can more intuitively be used as a reference for x/y/width/height animations. It's blue because I wanted to be able to use it for "!=" reftests, and it seemed like it'd be backwards to have to say "this testcase passes if it's _not_ green" in that situation. And I wasn't aware that blue had a semantic meaning -- thanks for the heads up on that.)

> Given that animation of attributes is dragging on (totally on me!) and that
> it's really hard to find landing slots in the tree nowadays, I went ahead and
> pushed in the slot that just opened. We can always discuss things and change
> them. Hope you don't mind. :-)

That's fine. :) All the review suggestions were basically nits, and given your justifications, I'm not particularly concerned about them.
Comment 12 Daniel Holbert [:dholbert] 2010-01-15 11:21:48 PST
(In reply to comment #11)
> Ok, sounds good. I only mentioned this since "linear" seemed intuitive to me
> (and there are plenty of other default attributes whose values we could provide
> but don't) -- but you make a good point about ease-in-out being another
> reasonable default behavior that a reader of the test might be expecting.

oops, s/default attributes whose values/attributes whose default values/

Note You need to log in before you can comment on or make changes to this bug.