545042 - Add support for SMIL animation of <angle> attributes in SVG

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

We should add support for SMIL animation of <integer> attributes in SVG.

Comment 1

[Jonathan Watt [:jwatt]]

Darn, C&P bug! I meant <angle> attributes in comment 0 of course.

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: patch

I've still to write the tests for this, but it's otherwise ready for review.

Comment 3

[Jonathan Watt [:jwatt]]

-  if (mOrientType.GetAnimValue() != SVG_MARKER_ORIENT_AUTO) {
-    aAngle = mAngleAttributes[ORIENT].GetAnimValue();
-  }
+  float angle = mOrientType.GetAnimValue() == SVG_MARKER_ORIENT_AUTO ?
+                aAutoAngle :
+                mAngleAttributes[ORIENT].GetAnimValue() * M_PI / 180.0;

The extra | * M_PI / 180.0 | is to fix a regression described in bug 540726 comment 4.

Comment 4

[Robert Longson [:longsonr]]

Lengths only have a single unit type shared between the animated and base values. Do angles really need separate unit types and if so why don't lengths? Perhaps that change should be separated out?

Comment 5

[Jonathan Watt [:jwatt]]

Lengths need it too, because you can legitimately have:

  <rect x="10px" ...>
    <animate from="10cm" to="20cm" .../>
  </rect>

I fix this in my patch for animating length-lists.

I don't really want to take time to separate that change out. I don't think that's important, and I just want to get on and get animation done (which is important). :-)

Comment 6

[Robert Longson [:longsonr]]

OK as long as it's on your to do list that they be consistent eventually.

Comment 7

[Daniel Holbert [:dholbert]]

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

Patch looks good! Just some nits.

>+void
>+SVGOrientSMILType::Destroy(nsSMILValue& aValue) const
>+{
>+  NS_PRECONDITION(aValue.mType == this, "Unexpected SMIL value.");
>+  aValue.mU.mPtr = NULL;

s/NULL/nsnull/

>+SVGOrientSMILType::ComputeDistance(const nsSMILValue& aFrom,
[snip]
>+  if (aFrom.mU.mOrient.mOrientType != nsIDOMSVGMarkerElement::SVG_MARKER_ORIENT_ANGLE ||
>+      aTo.mU.mOrient.mOrientType != nsIDOMSVGMarkerElement::SVG_MARKER_ORIENT_ANGLE) {
>+    // TODO: it would be nice to be able to add to auto angles

Looks like a copy-paste error in that coment -- "add" --> "compute distance with", or "handle", or something.

>+SVGOrientSMILType::Interpolate(const nsSMILValue& aStartVal,
[snip]
>+  if (aStartVal.mU.mOrient.mOrientType != nsIDOMSVGMarkerElement::SVG_MARKER_ORIENT_ANGLE ||
>+      aStartVal.mU.mOrient.mOrientType != nsIDOMSVGMarkerElement::SVG_MARKER_ORIENT_ANGLE) {
>+    // TODO: it would be nice to be able to include auto angles in
>+    // interpolation, or at least do a discrete animation.
>+    return NS_ERROR_FAILURE;
>+  }

Remove the "or at least do a discrete animation" there -- your patch already accomplishes that.  If Interpolate fails (as it does in this clause), we'll already "fail over" into discrete-calcMode animation.  (Interpolate() itself should never need to explicitly do discrete animation -- the code that calls it in nsSMILAnimationFunction handles that.)

>@@ -306,45 +326,54 @@ nsSVGAngle::SetBaseValueString(const nsA
>   mBaseVal = mAnimVal = value;
>-  mSpecifiedUnitType = PRUint8(unitType);
>+  mBaseValUnit = mAnimValUnit = PRUint8(unitType);

I think SetBaseValueString needs...
  #ifdef MOZ_SMIL
    if (mIsAnimated) {
      aSVGElement->AnimationNeedsResample();
    }
  #endif
...right? It should match nsSVGLength2::SetBaseValueString in that respect, I think.
http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGLength2.cpp#330

>+nsISMILAttr*
>+nsSVGAngle::ToSMILAttr(nsSVGElement *aSVGElement)
>+{
>+  if (aSVGElement->NodeInfo()->Equals(nsGkAtoms::marker, kNameSpaceID_SVG)) {
>+    nsSVGMarkerElement *marker = static_cast<nsSVGMarkerElement*>(aSVGElement);
>+    return new SMILOrient(marker->GetOrientType(), this, aSVGElement);
>+  }
>+  // SMILOrient would not be useful for general angle attributes
>+  NS_NOTREACHED("Trying to animate unknown angle attribute.");

Probably worth clarifying this comment to indicate that other angle attributes aren't supposed to be animatable in the first place.
e.g. extend the existing comment with something like this:
    // (also, "orient" is the only animatable <angle>-valued attribute in SVG 1.1)
(You already mention this fact elsewhere in the patch, but it's worth stating it in this method as well.)

>+void
>+nsSVGAngle::SMILOrient::ClearAnimValue()
>+{
>+  if (mAngle->mIsAnimated) {
>+    mOrientType->SetBaseValue(mOrientType->GetBaseValue(), mSVGElement);
>+    mAngle->SetAnimValue(mAngle->mBaseVal, mAngle->mBaseValUnit, mSVGElement);

I think that first line is supposed to be mOrientType->SetAnimValue, not SetBaseValue, right?

Speaking of which, do we actually have a nsSVGOrientType::SetAnimValue method implemented?  It looks like you're calling it elsewhere, inside of SMILOrient::SetAnimValue:
>+    mOrientType->SetAnimValue(aValue.mU.mOrient.mOrientType);
but I don't see this method defined in the patch or in the existing nsSVGOrientType class definition here:
http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGMarkerElement.h#52

Maybe I just missed it, but if not, we need to add that method. :)

>diff --git a/content/svg/content/src/nsSVGAngle.h b/content/svg/content/src/nsSVGAngle.h
[snip]
>   void SetBaseValue(float aValue, nsSVGElement *aSVGElement);
>+  void SetAnimValue(float aValue, PRUint8 aUnit, nsSVGElement *aSVGElement);
> 
>-  PRUint8 GetSpecifiedUnitType() const { return mSpecifiedUnitType; }
>+  PRUint8 GetBaseValueUnit() const { return mBaseValUnit; }
>+  PRUint8 GetAnimValueUnit() const { return mAnimValUnit; }
>   float GetAnimValInSpecifiedUnits() const { return mAnimVal; }
>   float GetBaseValInSpecifiedUnits() const { return mBaseVal; }

Now that we're adding a few more AnimVal/BaseVal method-pairs, you should probably reverse the order of those last two (preexisting) lines to match the ordering of the added methods.
i.e. put GetBaseValInSpecifiedUnits before GetAnimValInSpecifiedUnits.

>+#ifdef MOZ_SMIL
>+  // struct SMILAngle ... not currently implemented because in SVG 1.1 the only
>+  // *animatable* attribute that takes an <angle> is 'orient' on the 'marker'
>+  // element, and 'orient' must be special cased since it can also take the
>+  // value 'auto', making it a more complex type.
>+
>+  struct SMILOrient : public nsISMILAttr
>+  {

The "struct SMILAngle ..." at the start of this comment is initially confusing, given that the struct immediately after it is called SMILOrient.  To a casual reader, this looks like a comment that got left behind in a mass-renaming.

To clear this up, begin this comment with something like:
 // struct SMILOrient: specialization of nsISMILAttr for animating the
 // 'orient' attribute on <marker> elements.  Note that there is no
 // "SMILAngle" struct for generic angle-valued attributes, because
 // [etc etc etc]

r=dholbert with the above addressed. (plus tests, as you said in comment 2)

Comment 8

[Daniel Holbert [:dholbert]]

Can you also add fixes along the lines of bug 545550 to this patch?

i.e. for all lines in angle-related code with "mBaseVal = mAnimVal = [...]" or "mAnimVal = mBaseVal = [...]" where mIsAnimated is true, we really shouldn't be setting mAnimVal.

Comment 9

[Jonathan Watt [:jwatt]]

Pushed http://hg.mozilla.org/mozilla-central/rev/43f1520ffd3e