Closed Bug 468996 Opened 11 years ago Closed 11 years ago

SVG SMIL: Implement "animateTransform"

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: birtles, Assigned: birtles)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 10 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b3pre) Gecko/20081202 Minefield/3.1b3pre
Build Identifier: 

The current implementation of animateTransform in #216462 is for demonstration purposes only. It lacks support for:
* rotation angles > 180
* rotation around a center
* by-animation (especially for scaling)
* to-animation
* paced animation

As well as that it's very inefficient (using nsIDOMSVGMatrix). A proper implementation is required that addresses these issues.

Reproducible: Always
Depends on: 216462
Attached patch Proposed patch (obsolete) — Splinter Review
This patch addresses this issues outlined in this bug with the exception of paced animation. I'll open a new bug for paced animation as this requires architectural changes.
I forgot to mention, this patch applies on top of patch v4b from #216462
Blocks: 469040
This is an updated patch that is rebased off smil5a.patch from bug 216462.

It also includes the optimisations discussed under brb-22 of that bug. (See bug 216462, comment 142).
Attachment #352444 - Attachment is obsolete: true
Attached patch patch v1c (obsolete) — Splinter Review
Updated patch:
* Re-based off smil5b
* Brought implementation into line with the interpretation of SMIL used by the SVGT1.2 test suite.

Outstanding issues for a subsequent patch:
* Implement paced animation
* Fix naming of reftest files
* Fix naming of some local variables
* Check and update documentation in nsISMILType.h
* Remove aData parameter from nsISMILAttr::GetSMILType() and subclasses
Attachment #353767 - Attachment is obsolete: true
Attached patch patch v1d (obsolete) — Splinter Review
Changes in this version:
* Rebased on patch v5c from bug 216462.
* animateTransform functionality moved entirely to this patch
* changes to nsSMILValue migrated to patch for bug 216462
* implemented paced animation for animateTransform according to the definitions used in SVGT1.2
* other outstanding issues from previous patch addressed (extra param for nsISMILAttr::GetSMILType, naming of reftests and local variables)
Attachment #354333 - Attachment is obsolete: true
Attached patch patch v1e (obsolete) — Splinter Review
* Migrated changes to nsSVGAnimatedTransformList from patch for bug 216462 to this patch
* Rebased off changes to patch for bug 216462 -- now applies on top of patch v5d (attachment 354777 [details] [diff] [review])
Attachment #354749 - Attachment is obsolete: true
Attached patch patch v1g (obsolete) — Splinter Review
Rebasing off updated underlying smil patch v5g -- see bug 216462 attachment 355712 [details] [diff] [review]
(Skipping version 1f so the letters line up with the underlying smil patches)
Assignee: nobody → birtles
Attachment #354778 - Attachment is obsolete: true
Status: NEW → ASSIGNED
That last comment should refer to attachment 355713 [details] [diff] [review] in bug 216462
Attached patch patch v1k (obsolete) — Splinter Review
Rebased off smil patch v5k -- see bug 216462 attachment
357078 [review]
Attachment #355714 - Attachment is obsolete: true
Comment on attachment 357087 [details] [diff] [review]
patch v1k

I've not really looked at all of this but I do have a couple of things to say while I remember...

>+PRBool
>+nsSVGAnimateTransformElement::ParseAttribute(PRInt32 aNamespaceID,
>+                                             nsIAtom* aAttribute,
>+                                             const nsAString& aValue,
>+                                             nsAttrValue& aResult)
>+{
>+  // 'type' is an <animateTransform>-specific attribute, and we'll handle it
>+  // specially.
>+  if (aNamespaceID == kNameSpaceID_None && aAttribute == nsGkAtoms::type) {
>+    aResult.ParseAtom(aValue);
>+    nsIAtom* atom = aResult.GetAtomValue();
>+    if (atom != nsGkAtoms::translate ||
>+        atom != nsGkAtoms::scale ||
>+        atom != nsGkAtoms::rotate ||
>+        atom != nsGkAtoms::skewX ||
>+        atom != nsGkAtoms::skewY) {
>+      ReportAttributeParseFailure(GetOwnerDoc(), aAttribute, aValue);
>+    }
>+    return PR_TRUE;
>+  } else {

} else { is unnecessary

>+    return nsSVGAnimateTransformElementBase::ParseAttribute(aNamespaceID, 
>+                                                            aAttribute, aValue,
>+                                                            aResult);
>+  }
>+}
>+

>+inline void
>+nsSVGTransformSMILAttr::SkipCommaWsp(nsACString::const_iterator& aIter,
>+                               const nsACString::const_iterator& aIterEnd) const
>+{
>+  while (aIter != aIterEnd && (IsSpace(*aIter) || *aIter == ','))
>+    ++aIter;
>+}

This will just skip over ,,,,,, It probably shouldn't should it? Instead that should likely be a parse error.

Perhaps you could consider redoing the parsing using nsCommaSeparatedTokenizer and/or nsWhitespaceTokenizer as appropriate.
Attached patch patch v1l (obsolete) — Splinter Review
I've reworked most of the patch to provide more sensible return values to getAnimVal. There's a new reftest to check this behaviour. You can also see it in action at: http://brian.sol1.net/svg/tests/transformAnimVal.svg

I've also fixed a number of other bugs and addressed some of the Robert Longson's comments (comment 10).
Attachment #357087 - Attachment is obsolete: true
(In reply to comment #10)
> This will just skip over ,,,,,, It probably shouldn't should it? Instead that
> should likely be a parse error.

Thanks for that, it should be fixed by the latest patch. However, a trailing comma is still accepted.

> Perhaps you could consider redoing the parsing using nsCommaSeparatedTokenizer
> and/or nsWhitespaceTokenizer as appropriate.

I had a look at those and I can't see how to use them here as items can either be separated by whitespace or a comma and nsCommaSeparatedTokenizer seems to require a comma. (Actually, SVG's a little contradictory here, the spec for animateTransform shows commas for translate and scale but not for rotate. I'm following the BNF in http://www.w3.org/TR/SVG/coords.html#TransformAttribute instead which allows either whitespace or a comma or both.)
(In reply to comment #12)
> (Actually, SVG's a little contradictory here, the spec for
> animateTransform shows commas for translate and scale but not for rotate. I'm
> following the BNF in http://www.w3.org/TR/SVG/coords.html#TransformAttribute
> instead which allows either whitespace or a comma or both.)

I have an action to investigate lists in attribute values in SVG, to check what implementations allow and write an erratum if necessary.  I've noticed at least that the grammar for transform lists includes:

transforms:
    transform
    | transform comma-wsp+ transforms
  
which doesn't seem correct.  I think it should be 'comma-wsp?' in that second production, i.e., nothing or whitespace with an optional comma in it.  (Gecko seems to use 'comma-wsp*' when parsing the @transform attribute.)
Attached patch patch v1m (obsolete) — Splinter Review
Addressed an issue with correctly getting the rotation centre of an underlying value.
Attachment #357298 - Attachment is obsolete: true
Attached patch patch v1n (obsolete) — Splinter Review
Minor fixes. Prepare for review.
Attachment #357305 - Attachment is obsolete: true
Attachment #357576 - Flags: superreview?(roc)
Attachment #357576 - Flags: review?(roc)
+    NS_WARNING("Trying to add incompatible types.");

This (and the equivalent in Add above) should be an NS_ERROR, assuming there's no markup which can get us into this situation.

+  if (aName == nsGkAtoms::transform) {
+    nsCOMPtr<nsIDOMSVGTransformable> transformable(
+            do_QueryInterface(static_cast<nsIContent*>(this)));
+    NS_ENSURE_TRUE(transformable, nsnull);

I think this should just be 
  if (!transformable)
    return nsnull;
We don't want to produce any warnings or errors in this case (unless they go to the error console), it's quite possible for people to write markup that triggers this.

+class nsSVGSMILTransform
+{

Add a comment explaining what this class is for, and especially what the parameters mean for each transform type.

+    for (int i = 0; i < 6; ++i)
+      mParams[i] = 0;

{} around the one-line statements in this file.

+class nsSVGTransformSMILType : public nsISMILType
+{

Here we need some comments explaining how many transforms are allowed in the transform array under different conditions, and what the various operations require in terms of the number of transforms, and why those requirements actually hold.

Otherwise looks great!
Attached patch patch v1oSplinter Review
Address comments from review.
Attachment #357576 - Attachment is obsolete: true
Attachment #357597 - Flags: superreview?(roc)
Attachment #357597 - Flags: review?(roc)
Attachment #357576 - Flags: superreview?(roc)
Attachment #357576 - Flags: review?(roc)
Attachment #357597 - Flags: superreview?(roc)
Attachment #357597 - Flags: superreview+
Attachment #357597 - Flags: review?(roc)
Attachment #357597 - Flags: review+
Pushed c8da73bbcb1b
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Depends on: 539236
You need to log in before you can comment on or make changes to this bug.