Bug 85158 - Accumulation for values-animation is broken
Summary: Accumulation for values-animation is broken
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks: 41761
  Show dependency treegraph
 
Reported: 2012-04-29 07:50 PDT by Nikolas Zimmermann
Modified: 2012-05-03 02:08 PDT (History)
6 users (show)

See Also:


Attachments
Patch (74.37 KB, patch)
2012-04-29 09:14 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Follow-up patch (49.24 KB, patch)
2012-05-02 00:29 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Follow-up patch v2 (56.65 KB, patch)
2012-05-03 00:08 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Follow-up patch v3 (61.26 KB, patch)
2012-05-03 01:12 PDT, Nikolas Zimmermann
zherczeg: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2012-04-29 07:50:20 PDT
Example:
<rect width="999" height="100" fill="green"/>
   <animate begin="0s" values="0; 30; 20" accumulate="sum" repeatCount="5" dur="2s"/>
</rect>

The rect should animate like this:
0.000s -> 0
0.500s -> 15
1.000s -> 30
1.500s -> 25
1.999s -> 20
2.000s -> 20 (first accumulation, starts accumulating from the last set value, here '20').
2.500s -> 45
3.000s -> 50
3.500s -> 45
3.999s -> 40
4.000s -> 40 (second accumulation)
etc.

This is currently broken for values-animation. The accumulation should happen after a full cycle of the values animation ran (aka. at the end of the duration).
Comment 1 Nikolas Zimmermann 2012-04-29 09:14:28 PDT
Created attachment 139402 [details]
Patch
Comment 2 Darin Adler 2012-04-29 10:52:33 PDT
Comment on attachment 139402 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139402&action=review

> Source/WebCore/svg/SVGAnimatedColor.cpp:73
> +    Color& toAtEndOfDurationColor = toAtEndOfDuration->color();

This is a curious idiom. A getter that returns a non-const reference. Do we actually modify the color through this reference?
Comment 3 Nikolas Zimmermann 2012-05-01 07:18:59 PDT
(In reply to comment #2)
> (From update of attachment 139402 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139402&action=review
> 
> > Source/WebCore/svg/SVGAnimatedColor.cpp:73
> > +    Color& toAtEndOfDurationColor = toAtEndOfDuration->color();
> 
> This is a curious idiom. A getter that returns a non-const reference. Do we actually modify the color through this reference?

Thanks Darin for the review! It's a good point - I left it as-is for consistency. A follow-up patch will introduce const getters for SVGAnimatedType to get rid of the design flaw.
Comment 4 Nikolas Zimmermann 2012-05-01 07:20:49 PDT
Comment on attachment 139402 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139402&action=review

> Source/WebCore/svg/SVGAnimateElement.cpp:141
> +    OwnPtr<SVGAnimatedType> toAtEndOfDurationType = m_animator->constructFromString(toAtEndOfDurationString);

Note to self: We should cache the toAtEndOfDurationType to avoid constructing this type for values animations at each point in time.
I'll take care of this when posting a follow-up patch to add the const getters for SVGAnimatedType.
Comment 5 Nikolas Zimmermann 2012-05-01 08:17:40 PDT
Committed r115726: <http://trac.webkit.org/changeset/115726>
Comment 6 Nikolas Zimmermann 2012-05-02 00:28:57 PDT
Reopening to attach new patch.
Comment 7 Nikolas Zimmermann 2012-05-02 00:29:18 PDT
Created attachment 139759 [details]
Follow-up patch
Comment 8 Nikolas Zimmermann 2012-05-02 08:34:09 PDT
Comment on attachment 139759 [details]
Follow-up patch

This actually changes the behavior, we stop mutating from/to values it certain cases, I need to add a new testcase, and do some stylistic fixes.
Comment 9 Nikolas Zimmermann 2012-05-03 00:07:46 PDT
(In reply to comment #8)
> (From update of attachment 139759 [details])
> This actually changes the behavior, we stop mutating from/to values it certain cases, I need to add a new testcase, and do some stylistic fixes.

Ok, I was wrong, it was a bit too late yesterday - in fact I'm changing this to avoid any mutation, but it didn't cause problems in practice before, as we reset the from/to values on every animation step, this is now gone.
Comment 10 Nikolas Zimmermann 2012-05-03 00:08:19 PDT
Created attachment 139967 [details]
Follow-up patch v2
Comment 11 Zoltan Herczeg 2012-05-03 00:48:40 PDT
Comment on attachment 139967 [details]
Follow-up patch v2

Looks good, some comments:

View in context: https://bugs.webkit.org/attachment.cgi?id=139967&action=review

> Source/WebCore/svg/SVGAnimatedBoolean.cpp:84
>      bool& animatedBoolean = animated->boolean();

Could we remove the reference here as well?

> Source/WebCore/svg/SVGAnimatedType.cpp:244
> +const std::pair<SVGAngle, unsigned>& SVGAnimatedType::angleAndEnumeration() const
> +{
> +    ASSERT(m_type == AnimatedAngle);
> +    return *m_data.angleAndEnumeration;
> +}

Ok, these one liners should all be moved to the header, otherwise they would be a pure regressions.
Comment 12 Nikolas Zimmermann 2012-05-03 00:52:33 PDT
(In reply to comment #11)
> (From update of attachment 139967 [details])
> Looks good, some comments:
Thanks!

> > Source/WebCore/svg/SVGAnimatedBoolean.cpp:84
> >      bool& animatedBoolean = animated->boolean();
> Could we remove the reference here as well?
Nope, all "animated*" types are mutable - they will receive the actual animation updates.

> > Source/WebCore/svg/SVGAnimatedType.cpp:244
> > +const std::pair<SVGAngle, unsigned>& SVGAnimatedType::angleAndEnumeration() const
> > +{
> > +    ASSERT(m_type == AnimatedAngle);
> > +    return *m_data.angleAndEnumeration;
> > +}
> 
> Ok, these one liners should all be moved to the header, otherwise they would be a pure regressions.
Ok, you're right, I'll move all of them.
Comment 13 Nikolas Zimmermann 2012-05-03 01:12:42 PDT
Created attachment 139970 [details]
Follow-up patch v3
Comment 14 Zoltan Herczeg 2012-05-03 02:04:44 PDT
Comment on attachment 139970 [details]
Follow-up patch v3

r=me
Comment 15 Nikolas Zimmermann 2012-05-03 02:08:53 PDT
Committed r115950: <http://trac.webkit.org/changeset/115950>