522308 - Add support for SMIL animation of the <polygon> and <polyline> element's 'points' attributes

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

We need to add support for SMIL animation of the <polygon> and <polyline> element's 'points' attributes.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Robert Longson [:longsonr]]

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


>+nsresult
>+SVGPointList::SetValueFromString(const nsAString& aValue)
>+{
>+  SVGPointList temp;
>+  
>+  NS_ConvertUTF16toUTF8 value(aValue);
>+  char* start = SkipWhitespace(value.BeginWriting());
>+  
>+  // We can't use strtok with SVG_COMMA_WSP_DELIM because to correctly handle
>+  // invalid input in the form of two commas without a value between them, we
>+  // would need to know if strtok overwrote a comma or not.
>+  

can't we use nsCommaSeparatedTokenizer though? Raise a followup at least for that unless it's a non-starter.

>+  // We return the root of the sum of the squares of the distances between the
>+  // points at each correspanding index.

corresponding

>+  // No need to reset SVGPointList since the default value in always the same
>+  // (an empty list).

s/in/is/

>+
>   // No need to reset SVGPathData since the default value in always the same
>   // (an empty list).

s/in/is/ since this is wrong too


>+                    "Animatinging non-existant path data?");

how many ings do you need?

This spelling mistake is also in nsSVGElement::DidAnimatePathSegList please correct that too while you're at it.

> 
>-    nsCOMPtr<nsIDOMSVGPoint> point;
>-    mPoints->GetItem(i, getter_AddRefs(point));
>-
>-    float x, y;
>-    point->GetX(&x);
>-    point->GetY(&y);
>+  for (PRUint32 i = 0; i < points.Length(); ++i) {
>     if (i == 0)
>-      aCtx->MoveTo(gfxPoint(x, y));
>+      aCtx->MoveTo(points[i]);
>     else
>-      aCtx->LineTo(gfxPoint(x, y));
>+      aCtx->LineTo(points[i]);

It would be more efficient to pull out the i == 0 action and then run the loop from i = 1.

r=longsonr assuming review comments addressed or follow up bug raised as appropriate

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to comment #2)
> >+SVGPointList::SetValueFromString(const nsAString& aValue)
> can't we use nsCommaSeparatedTokenizer though? Raise a followup at least for
> that unless it's a non-starter.

(or rather, nsCharSeparatedTokenizerTemplate<IsSVGWhitespace>, as used in the final patch for bug 589439)

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: patch with r=longsonr

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 495938 [details] [diff] [review]
patch with r=longsonr

>+class SVGPointList
[...]
>+protected:
>+
>+  /* See SVGLengthList for the rationale for using nsTArray<float> instead
>+   * of nsTArray<SVGPoint, 1>.
>+   */
>+  nsTArray<SVGPoint> mItems;

s/float/SVGPoint/

>+nsresult
>+SVGPointList::SetValueFromString(const nsAString& aValue)
>+{
...
>+    const char *token1 = str1.get();
>+    if (token1 == '\0' || !tokenizer.hasMoreTokens()) {

Need a '*' here -- should be:
      if (*token1 == '\0'

>+    const char *token2 = str2.get();
>+    if (token2 == '\0') {

Same here.

>+SVGPointListSMILType::ComputeDistance(const nsSMILValue& aFrom,
[...]
>+  if (from.Length() != to.Length()) {
>+    // Lists in the 'values' attribute must have the same length.
>+    // nsSVGUtils::ReportToConsole
>+    aDistance = 0.0;
>+    return NS_ERROR_FAILURE;
>+  }
[...]
>+  if (!NS_FloatIsFinite(distance)) {
>+    return NS_ERROR_FAILURE;
>+  }

This is inconsistent -- in one failure case, we set the outparam, and in the other we don't.  These cases should match -- we should either set or not set the outparam, in both places.  I personally lean towards not setting the outparam, because the caller will see our failure rv and ignore the outparam anyway.  But I don't have a strong preference. (It looks like our existing classes vary a bit on this point - e.g. nsSVGTransformSMILType sets its outparam on failure, whereas nsSMILCSSValueType does not.)

Whichever route you choose, could you also tweak SVGNumberListSMILType::ComputeDistance with the same (one-liner) fix, for consistency, since it's very similar to this method & we might as well keep them in sync?

>+void
>+nsSVGElement::DidAnimatePointList()
>+{
>+  NS_ABORT_IF_FALSE(GetPointListAttrName(),
>+                    "Animating non-existant path data?");

s/path data/point list/

Also:
 s/existant/existent/ (apply throughout -- this is in ~4 other places in the patch)

>+#animate points list:
>+== anim-polyline-points-01.svg anim-polyline-points-01-ref.svg

Since there are only two affected elements here (polygon & polyline), we should probably have a test for each one, since it's easy enough -- could you add a test for <polygon>?

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+   * Unlike the other list classes, we hide our ctor (because no one should be
+   * creating instances this class directly). This factory method in exposed

instances *of*

+  // The following member is only used when we're not in a list:
+  SVGPoint mPt;

Can we stick this in a union with mList and use a special value for mListIndex to indicate whether we're in a list? That would save space. You'd need to manually addref/release mList. Maybe it's not worth it ... depends on how hard that change is. I suppose this applies to DOMSVGNumber and DOMSVGLength as well, though. Maybe we should consider it as a separate patch. Hmm, maybe we had this discussion before.

+  /**
+   * In future, if this class is used for non-list points, this will be
+   * different to IsInList().
+   */

Hmm ... it is used for non-list points!

+    if (aPt) {
+      mPt = *aPt;

How can you set an SVGPoint to a DOMSVGPoint? Oh, via the magic operator SVGPoint(). How about we get rid of that operator and just use a normal method, it feels too magical to me.

+    if (mItems[i]) {
+      mItems[i]->UpdateListIndex(i);
+    }

At some point we might want to lazify this code. We could allow an item's list index to be just a hint, possibly incorrect. Then before we use the list index in an item, we'd have to verify that the list's item at the given index is actually the item we expect; if not, then we should iterate through every item in the list setting the indices correctly. Future work I guess if we ever hit a benchmark where someone's inserting a lot of elements into the front of a list.

+inline mozilla::SVGPoint operator+(const mozilla::SVGPoint& aP1,
+                                   const mozilla::SVGPoint& aP2)
+{
+  return mozilla::SVGPoint(aP1.mX + aP2.mX, aP1.mY + aP2.mY);
+}
+
+inline mozilla::SVGPoint operator-(const mozilla::SVGPoint& aP1,
+                                   const mozilla::SVGPoint& aP2)
+{
+  return mozilla::SVGPoint(aP1.mX - aP2.mX, aP1.mY - aP2.mY);
+}
+
+inline mozilla::SVGPoint operator*(float aFactor,
+                                   const mozilla::SVGPoint& aPoint)
+{
+  return mozilla::SVGPoint(aFactor * aPoint.mX, aFactor * aPoint.mY);
+}
+
+inline mozilla::SVGPoint operator*(const mozilla::SVGPoint& aPoint,
+                                   float aFactor)

Why not move these inside the namespace and lose the mozilla:: prefixes?

+  // XXX scary amount of multiplication and sqrt'ing going on here
+  for (PRUint32 i = 0; i < to.Length(); ++i) {
+    double dist = NS_hypot(to[i].mX - from[i].mX, to[i].mY - from[i].mY);
+    total += dist * dist;
+  }
+  double distance = sqrt(total);

Is this right? Are you sure we don't want to just sum the distances between the points?

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: patch r=longsonr,dholbert

Currently going through Try.

Comment 8

[Jonathan Watt [:jwatt]]

(In reply to comment #6)
> +  // The following member is only used when we're not in a list:
> +  SVGPoint mPt;
> 
> Can we stick this in a union with mList and use a special value for mListIndex
> to indicate whether we're in a list? That would save space. You'd need to
> manually addref/release mList. Maybe it's not worth it ... depends on how hard
> that change is. I suppose this applies to DOMSVGNumber and DOMSVGLength as
> well, though. Maybe we should consider it as a separate patch. Hmm, maybe we
> had this discussion before.

I don't think we discussed this before. Is it okay if I file a followup to address this across all list classes?

> Hmm ... it is used for non-list points!

The meaning for HasOwner for the DOM wrappers is really more "HasWrappee" - in other works, whether it has an internal object that it is wrapping and from which its values come. In the case of nsDOMSVGZoomEvent which "owns" instances of DOMSVGPoint in some sense of the word "own" there is no internal SVGPoint.

> +    if (aPt) {
> +      mPt = *aPt;
> 
> How can you set an SVGPoint to a DOMSVGPoint? Oh, via the magic operator
> SVGPoint(). How about we get rid of that operator and just use a normal method,
> it feels too magical to me.

Yeah? It seems like it shouldn't be much different to assigning a float to a double. Less so in some sense since there's no precision change. I guess I can add an explicit method instead though.

> +    if (mItems[i]) {
> +      mItems[i]->UpdateListIndex(i);
> +    }
> 
> At some point we might want to lazify this code. We could allow an item's list
> index to be just a hint, possibly incorrect. Then before we use the list index
> in an item, we'd have to verify that the list's item at the given index is
> actually the item we expect; if not, then we should iterate through every item
> in the list setting the indices correctly. Future work I guess if we ever hit a
> benchmark where someone's inserting a lot of elements into the front of a list.

nsTArray means we end up traversing the list moving items in the prepend case anyway, no the growth rate is O(n) even without the second traversal to update the indexes. But yes, if it turns up in profiles we'll need to do something.

> +inline mozilla::SVGPoint operator*(const mozilla::SVGPoint& aPoint,
> +                                   float aFactor)
> 
> Why not move these inside the namespace and lose the mozilla:: prefixes?

I was thinking the operator couldn't then be used by non mozilla namespace code without |using namespace|...must have been late. :)

> +  // XXX scary amount of multiplication and sqrt'ing going on here
> 
> Is this right? Are you sure we don't want to just sum the distances between
> the points?

It seems right to me, since it's the distances between points that count. Then again it does make us inconsistent with <path d="L x,y x2,y2 ..."/>. I'll change it.

Comment 9

[Daniel Holbert [:dholbert]]

> Is this right? Are you sure we don't want to just sum the distances between the
> points?

Hmm... Yeah -- a simple sum-of-pairwise-point-distances does strike me as a more intuitive & useful distance metric.

I think that metric corresponds to all points (collectively) moving through a fixed amount of 2D distance, for a given unit time.  Or, put another way: every second of paced-mode animation, the sum of the distances traveled by all of our polygon's points would be constant.

That seems like a reasonable interpretation of paced animation for polygon / polyline.
(oops, I midair'd with jwatt -- looks like we all agree on this -- good!)

Comment 10

[Jonathan Watt [:jwatt]]

Attachment: patch r=longsonr,dholbert

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #8)
> I don't think we discussed this before. Is it okay if I file a followup to
> address this across all list classes?

Yes.

> > Hmm ... it is used for non-list points!
> 
> The meaning for HasOwner for the DOM wrappers is really more "HasWrappee" - in
> other works, whether it has an internal object that it is wrapping and from
> which its values come. In the case of nsDOMSVGZoomEvent which "owns" instances
> of DOMSVGPoint in some sense of the word "own" there is no internal SVGPoint.

I see.

> > +    if (aPt) {
> > +      mPt = *aPt;
> > 
> > How can you set an SVGPoint to a DOMSVGPoint? Oh, via the magic operator
> > SVGPoint(). How about we get rid of that operator and just use a normal method,
> > it feels too magical to me.
> 
> Yeah? It seems like it shouldn't be much different to assigning a float to a
> double. Less so in some sense since there's no precision change. I guess I can
> add an explicit method instead though.

Please do. It's not the same because a DOMSVGPoint is a very different beast to an SVGPoint. They're not interconvertible at all.

> nsTArray means we end up traversing the list moving items in the prepend case
> anyway, no the growth rate is O(n) even without the second traversal to update
> the indexes.

Good point.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  for (PRUint32 i = 0; i < to.Length(); ++i) {
+    double dx = to[i].mX - from[i].mX;
+    double dy = to[i].mY - from[i].mY;
+    total += dx * dx + dy * dy;
+  }
+  double distance = sqrt(total);

This isn't what I had in mind (nor dholbert I think). I suggested summing the distances between corresponding points, and this doesn't do that.

Comment 13

[Jonathan Watt [:jwatt]]

Attachment: patch r=longsonr,dholbert

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 496015 [details] [diff] [review]
patch r=longsonr,dholbert

I changed my mind. I think this is quite reasonable. The other approach I suggested would work too.

The spec needs to say what to do here, though.

Comment 15

[Jonathan Watt [:jwatt]]

Attachment: patch for checkin r=longsonr,dholbert,roc a=roc

Comment 16

[Daniel Holbert [:dholbert]]

So just for posterity's sake: so the impl quoted in comment 12 (which we're going with, I think) is basically just treating an N-length point list like a giant vector with 2N values, and we just use the 2N-space euclidean distance as our distance metric.

Comment 17

[Daniel Holbert [:dholbert]]

Put another way, comment 12 is
   sqrt(sum(square of each per-point distance))
whereas comment 9 was:
   sum(per-point distances)

Comment 18

[Jonathan Watt [:jwatt]]

Pushed http://hg.mozilla.org/mozilla-central/rev/10fc5a720ed0

Comment 19

[Jonathan Watt [:jwatt]]

(In reply to comment #6)
> Can we stick this in a union with mList and use a special value for mListIndex
> to indicate whether we're in a list? That would save space. You'd need to
> manually addref/release mList. Maybe it's not worth it ... depends on how hard
> that change is.

Filed followup bug 617980.