[SVG] animateMotion accumulate doesn't work properly with rotate: auto / auto-reverse

[SVG] animateMotion accumulate doesn't work properly with rotate: auto / auto-reverse

We need not accumulate the angle for animation as we have a method to get normal angle at a given length on the path.
So, only position needs to be accumulated. Also made some changes to simplify the logic for accumulation.

R=pdr@chromium.org, schenney@chromium.org, dschulze@chromium.org
BUG=256656
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154375

[pavane, 2013-07-04 09:07:11]

Hello All,

I don't know who is the specific owner for this. So, i am sending this mail to
all three of you.

Please review this patch.

This is a work in progress patch. Currently i didn't add layout test cases as i
am not sure what kind of layout tests to use for animated content like this. I
will try to add testcases soon.

Thanks in Advance,
Pavan

[Stephen Chennney, 2013-07-08 14:27:01]

We definitely need tests - I'm not confident that this works as you intend.

https://codereview.chromium.org/18436004/diff/1/Source/core/svg/SVGAnimateMot...
File Source/core/svg/SVGAnimateMotionElement.cpp (right):

https://codereview.chromium.org/18436004/diff/1/Source/core/svg/SVGAnimateMot...
Source/core/svg/SVGAnimateMotionElement.cpp:279:
position.move(positionAtEndOfDuration.x() * repeatCount,
positionAtEndOfDuration.y() * repeatCount);
This doesn't seem right for a curved path, or maybe any path. By multiplying by
the repeatCount you are linearly extrapolating.

[pdr., 2013-07-08 14:40:59]

Thanks for the patch!
It may be best to just make the minimal change you're looking to do here,
without the logic simplification. As-is, I don't think this correctly handles
repeatCount.

[pavane, 2013-07-10 01:41:35]

On 2013/07/08 14:27:01, Stephen Chenney wrote:
> We definitely need tests - I'm not confident that this works as you intend.
> 
>
https://codereview.chromium.org/18436004/diff/1/Source/core/svg/SVGAnimateMot...
> File Source/core/svg/SVGAnimateMotionElement.cpp (right):
> 
>
https://codereview.chromium.org/18436004/diff/1/Source/core/svg/SVGAnimateMot...
> Source/core/svg/SVGAnimateMotionElement.cpp:279:
> position.move(positionAtEndOfDuration.x() * repeatCount,
> positionAtEndOfDuration.y() * repeatCount);
> This doesn't seem right for a curved path, or maybe any path. By multiplying
by
> the repeatCount you are linearly extrapolating.

In my opinion it should work even for a curved path as we are concerned only
with the position at the end of each duration for accumulation. I will try to
test and add a test case with curved path also. Even the old logic is doing the
same. It is accumulating the end value but using a loop which i felt is not
required as the basic animation path is same and it will anyways end up adding a
constant value. please let me know if i am wrong.

[pavane, 2013-07-10 01:48:00]

On 2013/07/08 14:40:59, pdr wrote:
> Thanks for the patch!
> It may be best to just make the minimal change you're looking to do here,
> without the logic simplification. As-is, I don't think this correctly handles
> repeatCount.

Sure, i will upload one more patch with minimal changes. I will do it by monday
as I am currently on vacation and don't have facility to submit the patch here. 

Thanks for the reviews.

[pavane, 2013-07-15 09:12:53]

On 2013/07/10 01:41:35, pavane wrote:
> On 2013/07/08 14:27:01, Stephen Chenney wrote:
> > We definitely need tests - I'm not confident that this works as you intend.
> > 
> >
>
https://codereview.chromium.org/18436004/diff/1/Source/core/svg/SVGAnimateMot...
> > File Source/core/svg/SVGAnimateMotionElement.cpp (right):
> > 
> >
>
https://codereview.chromium.org/18436004/diff/1/Source/core/svg/SVGAnimateMot...
> > Source/core/svg/SVGAnimateMotionElement.cpp:279:
> > position.move(positionAtEndOfDuration.x() * repeatCount,
> > positionAtEndOfDuration.y() * repeatCount);
> > This doesn't seem right for a curved path, or maybe any path. By multiplying
> by
> > the repeatCount you are linearly extrapolating.
> 
> In my opinion it should work even for a curved path as we are concerned only
> with the position at the end of each duration for accumulation. I will try to
> test and add a test case with curved path also. Even the old logic is doing
the
> same. It is accumulating the end value but using a loop which i felt is not
> required as the basic animation path is same and it will anyways end up adding
a
> constant value. please let me know if i am wrong.

Added more test cases for behavior with curved paths. Please have a look at
http://crbug.com/256656 for test cases. They are working fine with this patch.

[pavane, 2013-07-15 17:40:17]

On 2013/07/10 01:48:00, pavane wrote:
> On 2013/07/08 14:40:59, pdr wrote:
> > Thanks for the patch!
> > It may be best to just make the minimal change you're looking to do here,
> > without the logic simplification. As-is, I don't think this correctly
handles
> > repeatCount.
> 
> Sure, i will upload one more patch with minimal changes. I will do it by
monday
> as I am currently on vacation and don't have facility to submit the patch
here. 
> 
> Thanks for the reviews.

Please have a look into patch set 2. I made minimal changes to fix this issue.
Both patches work fine even for curved paths. I added test cases at
http://crbug.com/256656. I personally feel that patch set 1 is the better way
but i can even put those refinements as separate patch on top of patch set 2.
Please let me know your views.

[pdr., 2013-07-15 17:43:51]

On 2013/07/15 17:40:17, pavane wrote:
> On 2013/07/10 01:48:00, pavane wrote:
> > On 2013/07/08 14:40:59, pdr wrote:
> > > Thanks for the patch!
> > > It may be best to just make the minimal change you're looking to do here,
> > > without the logic simplification. As-is, I don't think this correctly
> handles
> > > repeatCount.
> > 
> > Sure, i will upload one more patch with minimal changes. I will do it by
> monday
> > as I am currently on vacation and don't have facility to submit the patch
> here. 
> > 
> > Thanks for the reviews.
> 
> Please have a look into patch set 2. I made minimal changes to fix this issue.
> Both patches work fine even for curved paths. I added test cases at
> http://crbug.com/256656. I personally feel that patch set 1 is the better way
> but i can even put those refinements as separate patch on top of patch set 2.
> Please let me know your views.

Can you please include the tests in your patch? You can see an example of this
in https://codereview.chromium.org/18421006, and feel free to ping me on irc
(pdr) if you need help.

[pavane, 2013-07-16 14:15:55]

On 2013/07/15 17:43:51, pdr wrote:
> On 2013/07/15 17:40:17, pavane wrote:
> > On 2013/07/10 01:48:00, pavane wrote:
> > > On 2013/07/08 14:40:59, pdr wrote:
> > > > Thanks for the patch!
> > > > It may be best to just make the minimal change you're looking to do
here,
> > > > without the logic simplification. As-is, I don't think this correctly
> > handles
> > > > repeatCount.
> > > 
> > > Sure, i will upload one more patch with minimal changes. I will do it by
> > monday
> > > as I am currently on vacation and don't have facility to submit the patch
> > here. 
> > > 
> > > Thanks for the reviews.
> > 
> > Please have a look into patch set 2. I made minimal changes to fix this
issue.
> > Both patches work fine even for curved paths. I added test cases at
> > http://crbug.com/256656. I personally feel that patch set 1 is the better
way
> > but i can even put those refinements as separate patch on top of patch set
2.
> > Please let me know your views.
> 
> Can you please include the tests in your patch? You can see an example of this
> in https://codereview.chromium.org/18421006, and feel free to ping me on irc
> (pdr) if you need help.

Please have a look at patch set 3

[pdr., 2013-07-17 02:10:29]

Pavane,

For some reason some of the files are showing errors ("Upload in progress") in
Rietveld, even though your patch is fine. This isn't your fault, I've filed
http://crbug.com/260988 for the infrastructure team to look into this.

Overall, these tests look great! I think the flakiness is due to numeric
precision issues. Can you add extra padding around your blue circles so that
they completely overlap eachother?

I noticed some very minor indentation issues, for example
animateMotion-accumulate-2b.svg has </path> indented too far.

[pavane, 2013-07-17 03:10:19]

On 2013/07/17 02:10:29, pdr wrote:
> Pavane,
> 
> For some reason some of the files are showing errors ("Upload in progress") in
> Rietveld, even though your patch is fine. This isn't your fault, I've filed
> http://crbug.com/260988 for the infrastructure team to look into this.
> 
> Overall, these tests look great! I think the flakiness is due to numeric
> precision issues. Can you add extra padding around your blue circles so that
> they completely overlap eachother?
> 
> I noticed some very minor indentation issues, for example
> animateMotion-accumulate-2b.svg has </path> indented too far.

pdr, please have a look at patch set 4. I made changes based on your review.

[pdr., 2013-07-17 04:11:30]

LGTM.

Please fix the style issues and re-upload, and then press the commit button :)

https://codereview.chromium.org/18436004/diff/27001/LayoutTests/svg/animation...
File LayoutTests/svg/animations/animateMotion-accumulate-2a.svg (right):

https://codereview.chromium.org/18436004/diff/27001/LayoutTests/svg/animation...
LayoutTests/svg/animations/animateMotion-accumulate-2a.svg:59: </path>
Minor nit: the indentation is wrong here

https://codereview.chromium.org/18436004/diff/27001/LayoutTests/svg/animation...
File LayoutTests/svg/animations/animateMotion-accumulate-2b.svg (right):

https://codereview.chromium.org/18436004/diff/27001/LayoutTests/svg/animation...
LayoutTests/svg/animations/animateMotion-accumulate-2b.svg:45: dur="4s"
Minor nit: the indentation is wrong here

https://codereview.chromium.org/18436004/diff/27001/LayoutTests/svg/animation...
LayoutTests/svg/animations/animateMotion-accumulate-2b.svg:58: </path>
Minor nit: the indentation is wrong here

https://codereview.chromium.org/18436004/diff/27001/LayoutTests/svg/animation...
LayoutTests/svg/animations/animateMotion-accumulate-2b.svg:71:
document.documentElement.setCurrentTime(12);
Just a question: In this test and the others, the setCurrentTime parameter is
exactly equal to the repeatDur value of the animateMotion. Is there a reason for
that? Do the tests still pass if you use 24 instead of 12?

[pavane, 2013-07-17 05:30:16]

Made changes based on review comments

https://codereview.chromium.org/18436004/diff/27001/LayoutTests/svg/animation...
File LayoutTests/svg/animations/animateMotion-accumulate-2b.svg (right):

https://codereview.chromium.org/18436004/diff/27001/LayoutTests/svg/animation...
LayoutTests/svg/animations/animateMotion-accumulate-2b.svg:71:
document.documentElement.setCurrentTime(12);
On 2013/07/17 04:11:30, pdr wrote:
> Just a question: In this test and the others, the setCurrentTime parameter is
> exactly equal to the repeatDur value of the animateMotion. Is there a reason
for
> that? Do the tests still pass if you use 24 instead of 12?

anything above repeatDur value is fine as i am freezing the animation at the
end.

[commit-bot: I haz the power, 2013-07-17 05:31:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pavan.e@samsung.com/18436004/35001

[commit-bot: I haz the power, 2013-07-17 06:30:52]

Change committed as 154375