Bug 18375 - Make SVG animation work
: Make SVG animation work
Status: RESOLVED FIXED
: WebKit
SVG
: 528+ (Nightly build)
: Macintosh Mac OS X 10.5
: P2 Normal
Assigned To:
: http://www.w3.org/Graphics/SVG/Test/2...
:
:
:
  Show dependency treegraph
 
Reported: 2008-04-08 21:52 PST by
Modified: 2008-04-25 12:40 PST (History)


Attachments
Implement (most of) it (187.90 KB, patch)
2008-04-08 22:06 PST, Antti Koivisto
no flags Review Patch | Details | Formatted Diff | Diff
Updated patch (187.12 KB, patch)
2008-04-09 12:00 PST, Antti Koivisto
no flags Review Patch | Details | Formatted Diff | Diff
another updated patch (187.74 KB, patch)
2008-04-10 11:39 PST, Antti Koivisto
eric: review+
Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2008-04-08 21:52:41 PST
...where "work" is defined as passing reasonable amount of SVG animation test suite so that remaining issues can be tracked as individual bugs.
------- Comment #1 From 2008-04-08 22:06:58 PST -------
Created an attachment (id=20419) [details]
Implement (most of) it

        It does
        - Full SMIL interval timing model including syncbase and event base timing (the hard part).
        - CSS and XML attribute animation.
        - Linear, discrete and spline calcModes.
        - Values animation with keyTimes and keySplines.
        - Link activated animations.
        - Pretty good support for <animate> and <set> animations
        - Basic support for <animateColor>, <animateMotion> and <animateTransform>.

        This passes some 35 of the 56 tests in W3C SVG animation test suite, a bunch more
        with some subtest failures.

        What is still missing
        - Additive animation with multiple animations operating on the same property. This is a
          major architectural feature in animation code. It shouldn't be too hard to add.
        - Only <animate> implements accumulate.
        - <animateMotion> does not do paths, keypoints, rotate.
        - <animateTransform> does not work with text, some transforms are broken.
        - calcMode paced is missing.
        - repeat, beginEvent, endEvent are missing.
        - accesskey() is missing.
        - JS does not see correct values for baseVal/animVal, changing values that are being
          animted for a script produces wrong results. Not maintaining separate presentation 
          values for XML attributes is an architectural problem in our SVG code.
        - Some other stuff I forgot or do not know about.

There are three new classes:

- SMILTime which implements time math including special "indefinite" and "unresolved" values.
- SMILTimeContainer which runs the clock and manages animations ("time container" is a SMIL term)
- SMILTimingElement, an abstract class which inherits SVGElement and is inherited by SVGAnimationElement. It implements SMIL timing model semantics.
------- Comment #2 From 2008-04-08 22:09:37 PST -------
Same text, hopefully more readable:

It does
- Full SMIL interval timing model including syncbase and event base timing (the hard part).
- CSS and XML attribute animation.
- Linear, discrete and spline calcModes.
- Values animation with keyTimes and keySplines.
- Link activated animations.
- Pretty good support for <animate> and <set> animations
- Basic support for <animateColor>, <animateMotion> and <animateTransform>.

This passes some 35 of the 56 tests in W3C SVG animation test suite, a bunch more
with some subtest failures.

What is still missing:
- Additive animation with multiple animations operating on the same property. This is a major architectural feature in animation code. It shouldn't be too hard to add.
- Only <animate> implements accumulate.
- <animateMotion> does not do paths, keypoints, rotate.
- <animateTransform> does not work with text, some transforms are broken.
- calcMode paced is missing.
- repeat, beginEvent, endEvent are missing.
- accesskey() is missing.
- JS does not see correct values for baseVal/animVal, changing values that are being animted for a script produces wrong results. Not maintaining separate presentation values for XML attributes is an architectural problem in our SVG code.
- Some other stuff I forgot or do not know about.

- SMILTime which implements time math including special "indefinite" and "unresolved" values.
- SMILTimeContainer which runs the clock and manages animations ("time container" is a SMIL term)
- SMILTimingElement, an abstract class which inherits SVGElement and is inherited by SVGAnimationElement. It implements SMIL timing model semantics.
------- Comment #3 From 2008-04-08 22:56:43 PST -------
(From update of attachment 20419 [details])
First run through it (feel to tired for a full review atm :( ), it looks mostly fine, a few comments though:

In +void SVGAnimateElement::applyAnimatedValueToElement(unsigned repeat)

"valueToApple" seems like it may be a tyop :D

I suspect parseNumberValueAndUnit will parse "100 px/%" etc which seems bogus

Adding commented out code in SVGAnimateMotionElement::parseMappedAttributes seems like an accident, and isn't commented itself so i have no idea why :(

Tyop in m_baseIndexInTranformList -- missing 's'

Are you sure about the comment in SVGElement::updateStyleAttributeIfNeeded? i've seen plenty of examples of people using "style" attributes in svg, it seems odd for it to not be accessible form the dom...

I'm not happy with doubleSort/sortTimeList.  doubleSort makes an assumption that each m_beginTime will always be the first member of SMILTime which seems really icky...
------- Comment #4 From 2008-04-09 03:48:39 PST -------
Hey Antti,

had a quick look, unfortunately not much time atm, but I'm impressed! Excellent work!

I just want to comment on one issue, as it's related to the previous SVG AnimProperty Liveness work:

> - JS does not see correct values for baseVal/animVal, changing values that are
> being animted for a script produces wrong results. Not maintaining separate
> presentation values for XML attributes is an architectural problem in our SVG
> code.

The architecture for handling these updates is basically existant. That's part of the animated_property_* mystery (start/stop##Property functions). The idea is that <animate> has to mark a certain property (if it animates an XML prop) as start()ed. The underlying value is cached in a HashMap in the SVGDocumentExtensions, and all calls to 'set##Property' only influence the value residing in the HashMap, but not the actual property itself. This assures that you can still call 'setAttribute(...)' from JS, while an animation is running. You may want to have a look at the concept. It's old and Eric & me came up with this more than a year ago, while planning the future animation framework.

Greetings,
Niko
------- Comment #5 From 2008-04-09 10:46:59 PST -------
(In reply to comment #4)

> The architecture for handling these updates is basically existant. That's part
> of the animated_property_* mystery (start/stop##Property functions). The idea
> is that <animate> has to mark a certain property (if it animates an XML prop)
> as start()ed. The underlying value is cached in a HashMap in the
> SVGDocumentExtensions, and all calls to 'set##Property' only influence the
> value residing in the HashMap, but not the actual property itself. This assures
> that you can still call 'setAttribute(...)' from JS, while an animation is
> running. You may want to have a look at the concept. It's old and Eric & me
> came up with this more than a year ago, while planning the future animation
> framework.

Yeah, I know there is code there but I can't figure out if it does the right thing and indeed how it is supposed to be used. I think someone with better understanding of that code will have to wire it together.

Note that there are two separate problems here with different solution. One for animated CSS properties and another for XML attributes. 

For CSS properties the solution is simple and the spec tells exactly what to do. You write your animated values to the override stylesheet and the base value is what you get with getComputedStyle() (without override style sheet). Since WebCore does not support override stylesheets, for now the values are written to inline stylesheet instead. Since many SVG attributes are mapped to CSS this applies to them as well. Pure XML attributes are the problematic ones.

> 
> Greetings,
> Niko
> 
------- Comment #6 From 2008-04-09 12:00:22 PST -------
Created an attachment (id=20438) [details]
Updated patch

Olliej's comments and some other tweaks
------- Comment #7 From 2008-04-09 13:41:02 PST -------
(In reply to comment #3)
> In +void SVGAnimateElement::applyAnimatedValueToElement(unsigned repeat)
> 
> "valueToApple" seems like it may be a tyop :D

Lol, fixed.

> I suspect parseNumberValueAndUnit will parse "100 px/%" etc which seems bogus

Added check for that.

> Adding commented out code in SVGAnimateMotionElement::parseMappedAttributes
> seems like an accident, and isn't commented itself so i have no idea why :(

Replaced with comment.

> Tyop in m_baseIndexInTranformList -- missing 's'

Fixed.

> Are you sure about the comment in SVGElement::updateStyleAttributeIfNeeded?
> i've seen plenty of examples of people using "style" attributes in svg, it
> seems odd for it to not be accessible form the dom...

Yeah, that was a bad idea Reverted that part.

> I'm not happy with doubleSort/sortTimeList.  doubleSort makes an assumption
> that each m_beginTime will always be the first member of SMILTime which seems
> really icky...

Yeah, that was cheesy. A leftover from before I switched to SMILTime, now fixed.
------- Comment #8 From 2008-04-10 11:39:40 PST -------
Created an attachment (id=20459) [details]
another updated patch

Code cleanups and some more comments. One bug fix (base value string is compared to null instead of empty in SVGAnimationElement::unapplyAnimation() so that empty base value can still be restored) to make test 40 pass completely.
------- Comment #9 From 2008-04-10 18:29:47 PST -------
(From update of attachment 20459 [details])
So I'm not sure it even makes sense to do any kind of real hard review of this patch.  Obviously this work is incomplete.

I did a cursory review anyway:


parseNumberValueAndUnit
the .endsWith stuff is about as inefficient as possible. :)  Normally we use hash tables when you have more than 3-4 values.  In this case, we're re-walking the string each time. :)

  if (isAdditive()) {
 97             valueToApply = ColorDistance::addColorsAndClamp(base, m_animatedColor).name();
 98         } else

no { } needed.

I'm in general not a big fan of shadowing:
AnimationMode animationMode = this->animationMode();


m_cachedMax = -1.;

-1 should have some name here.  The comparisons with 0 should also use a variable of this same name.  

if (m_cachedMax.isValid())
    return m_cachedMax;


You might want to add
using SVGNames; at the top of SMILTimingElement.cpp


I don't like your use of copying parsers.  We've spent a lot of time in the last 3 years removing copying parsers and replacing them with scaning parsers.  I agree with you that the rest of this work is much more important than that small issue however.


All in all, I'm very very very very glad to see this patch land.  I really hope that you plan to push this through to completion.  This is the 3rd animation system we've seen, and they've all been partial. :)  Hopefully this is the last one we'll see.

We talked a lot over IRC.  You said you'd land the UnitBezier stuff separately.  I don't need to see the updated patch.

Thanks again.
------- Comment #10 From 2008-04-11 00:12:01 PST -------
Thanks for the review!

Sending        WebCore/ChangeLog
Sending        WebCore/GNUmakefile.am
Sending        WebCore/WebCore.pro
Sending        WebCore/WebCore.vcproj/WebCore.vcproj
Sending        WebCore/WebCore.xcodeproj/project.pbxproj
Sending        WebCore/dom/Document.cpp
Sending        WebCore/history/CachedPage.cpp
Sending        WebCore/svg/SVGAElement.cpp
Sending        WebCore/svg/SVGAnimateColorElement.cpp
Sending        WebCore/svg/SVGAnimateColorElement.h
Sending        WebCore/svg/SVGAnimateElement.cpp
Sending        WebCore/svg/SVGAnimateElement.h
Sending        WebCore/svg/SVGAnimateMotionElement.cpp
Sending        WebCore/svg/SVGAnimateMotionElement.h
Sending        WebCore/svg/SVGAnimateTransformElement.cpp
Sending        WebCore/svg/SVGAnimateTransformElement.h
Sending        WebCore/svg/SVGAnimationElement.cpp
Sending        WebCore/svg/SVGAnimationElement.h
Sending        WebCore/svg/SVGDocumentExtensions.cpp
Sending        WebCore/svg/SVGElement.cpp
Sending        WebCore/svg/SVGSVGElement.cpp
Sending        WebCore/svg/SVGSVGElement.h
Sending        WebCore/svg/SVGSetElement.cpp
Sending        WebCore/svg/SVGSetElement.h
Deleting       WebCore/svg/SVGTimer.cpp
Deleting       WebCore/svg/SVGTimer.h
Sending        WebCore/svg/SVGUseElement.cpp
Deleting       WebCore/svg/TimeScheduler.cpp
Deleting       WebCore/svg/TimeScheduler.h
Adding         WebCore/svg/animation
Adding         WebCore/svg/animation/SMILTime.cpp
Adding         WebCore/svg/animation/SMILTime.h
Adding         WebCore/svg/animation/SMILTimeContainer.cpp
Adding         WebCore/svg/animation/SMILTimeContainer.h
Adding         WebCore/svg/animation/SVGSMILElement.cpp
Adding         WebCore/svg/animation/SVGSMILElement.h
Transmitting file data ...............................
Committed revision 31801.
------- Comment #11 From 2008-04-25 12:40:12 PST -------
*** Bug 18187 has been marked as a duplicate of this bug. ***
------- Comment #12 From 2008-04-25 12:40:42 PST -------
*** Bug 18188 has been marked as a duplicate of this bug. ***