Bug 67601 - Return to transform multiplication: motion transform * other transforms
Summary: Return to transform multiplication: motion transform * other transforms
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 52843 (view as bug list)
Depends on:
Blocks: 41761
  Show dependency treegraph
 
Reported: 2011-09-05 06:58 PDT by Dirk Schulze
Modified: 2019-10-22 07:17 PDT (History)
3 users (show)

See Also:


Attachments
Patch (8.90 KB, patch)
2011-09-05 14:00 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2011-09-05 06:58:35 PDT
Right now we take the current transform of a transformable element, post multiply the animation transform, post multiply the motion transform. While the spec demands us to do so, no other SVG viewer is doing it that way. Writing a patch to return to our old behavior (motion transform * transform * animation transform) right now. This was realized by other viewers as well. (Opera, FF, Batik ... all use other transform mulitplications. But none of these viewers match the behavior of the spec).

We will pass two more tests on SVG W3C test suite.
Comment 1 Dirk Schulze 2011-09-05 14:00:05 PDT
Created attachment 106357 [details]
Patch
Comment 2 Nikolas Zimmermann 2011-09-06 00:29:33 PDT
Comment on attachment 106357 [details]
Patch

Looks reasonable, r=me. Is Shane CC'ed, btw? I'd like to let him know that this has changed again.
Comment 3 Dirk Schulze 2011-09-06 01:17:22 PDT
Comment on attachment 106357 [details]
Patch

Clearing flags on attachment: 106357

Committed r94558: <http://trac.webkit.org/changeset/94558>
Comment 4 Dirk Schulze 2011-09-06 01:17:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Shane Stephens 2011-09-06 01:20:21 PDT
This is one of those funny situations where neither the specified behaviour nor the current behaviour of Opera, FF, Batik, etc. makes sense - there are solid examples of unexpected things happening in each case.

With this in mind I elected to follow the specification rather than current practice of other browsers (I also did this because it appeared that the code was inadvertently doing the wrong thing due to the fact that WebKit's matrix multiply operator was using operands in the wrong order). I also emailed www-svg with a query about what was appropriate: http://lists.w3.org/Archives/Public/www-svg/2010Dec/0033.html. This email generated a pretty long discussion, which as far as I can tell is ongoing - this is a genuinely difficult situation to resolve.

I don't oppose returning WebKit to the behaviour exemplified by the current patch, although I would prefer we await final resolution from the SVG WG.

If you're interested in specific examples of what breaks each way, let me know and I'll spend some time putting something together for this bug.
Comment 6 Shane Stephens 2011-09-06 01:21:03 PDT
Not sure if I should be reopening this or not, feel free to close if this was inappropriate.
Comment 7 Dirk Schulze 2011-09-06 01:51:20 PDT
(In reply to comment #5)
> This is one of those funny situations where neither the specified behaviour nor the current behaviour of Opera, FF, Batik, etc. makes sense - there are solid examples of unexpected things happening in each case.
> 
> With this in mind I elected to follow the specification rather than current practice of other browsers (I also did this because it appeared that the code was inadvertently doing the wrong thing due to the fact that WebKit's matrix multiply operator was using operands in the wrong order). I also emailed www-svg with a query about what was appropriate: http://lists.w3.org/Archives/Public/www-svg/2010Dec/0033.html. This email generated a pretty long discussion, which as far as I can tell is ongoing - this is a genuinely difficult situation to resolve.
The discussion is still open. But their is no progress on it right now. But their seems to be at least one consensus: The specified behavior shouldn't be used. Other SVG viewers already followed the old behavior of WebKit. In a short testing it seems FireFox follows WebKit as well. The same for Abbra. Given the fact that Firefox is the SVG viewer with the most users on the desktop market, it makes sense to follow them here. We would get a lot of negative feedback if we do something different than Opera or Firefox for the simple test cases like the two tests on the official test suite -- even if we follow the specification. And I hope that we can get back to the discussion on www-svg with this change. Once all major SVG viewers reach a consensus, we can switch to any other behavior. And that is more important to follow the spec. All implementers are members of the SVG WG. Shouldn't be hard to change the spec at this point :)

> 
> I don't oppose returning WebKit to the behaviour exemplified by the current patch, although I would prefer we await final resolution from the SVG WG.
Like I said, I hope that we get progress on this topic again now.

> 
> If you're interested in specific examples of what breaks each way, let me know and I'll spend some time putting something together for this bug.
Oh of course. I am writing a mail to www-svg at the moment. I'll describe our current behavior in detail. If you see more differences across viewers, just post them on www-svg!
Comment 8 Shane Stephens 2011-09-06 03:42:07 PDT
It's not really my fight, but my sense was that neither the specified behavior nor the tested behavior was optimal. It's a while since I've looked at this, but here goes:

If I do:

<g transform="translate(300, 30)">
  <rect width="40" height="40" fill="green"></rect>
  <animateMotion dur="1s" repeatCount="1" rotate="auto" path="M
100,250 C 100,50 400,50 400,250"></animateMotion>
</g>

Then what firefox does is premultiply the supplemental transform from animateMotion. At the beginning of the path, this equates to a translate of (100, 250) and a rotation of approximately -90, so we get:

translate(100, 250) rotate(-90) translate(300, 30)
=
translate(-50, 270) rotate(-90)

Which is weird. On the other hand, if rotate="auto" is taken away, we get:

translate(100, 250) translate(300, 30)
=
translate(400, 280)

So the essential weirdness of premultiplication is that the container transform is used in the wrong place, which results in strange outcomes. I felt at the time that wildly different locations depending on whether rotate="auto" was set or not was worse than the alternatives.

On the other hand, if you postmultiply the supplemental transform as currently specified, then something like:

<g>
  <rect width="40" height="40" fill="green"></rect>
  <animateMotion dur="1s" repeatCount="1" path="M
100,250 C 100,50 400,50 400,250"></animateMotion>
  <animateTransform attributeName="transform" type="rotate" from="-30" to="0" begin="0s" dur="1s" fill="freeze"/>
</g>

behaves oddly because the animateTransform rotation affects the animateMotion translation even though it's specified after it. Neither the specified behavior nor the current favorite behavior (which is apparently an artifact of Adobe's that has spread to the other implementations) really does the right thing.

Now what I'd like to see is for animateMotion to work off the CTM in the same way that animateTransform does, which would mean that there wouldn't be a supplemental transform to pre- or post- multiply, and this problem would go away. Apparently this is hard for other reasons but I've never seen a clear explanation of why.
Comment 9 Dirk Schulze 2011-09-06 04:19:23 PDT
(In reply to comment #8)
> It's not really my fight, but my sense was that neither the specified behavior nor the tested behavior was optimal. It's a while since I've looked at this, but here goes:
> 
> If I do:
> 
> <g transform="translate(300, 30)">
>   <rect width="40" height="40" fill="green"></rect>
>   <animateMotion dur="1s" repeatCount="1" rotate="auto" path="M
> 100,250 C 100,50 400,50 400,250"></animateMotion>
> </g>
> 
> Then what firefox does is premultiply the supplemental transform from animateMotion. At the beginning of the path, this equates to a translate of (100, 250) and a rotation of approximately -90, so we get:
> 
> translate(100, 250) rotate(-90) translate(300, 30)
> =
> translate(-50, 270) rotate(-90)

Yes that is right: animate motion * animate rotation * transform. Even if I agree the premultiplication of animate motion is not ideally, Firefox and WebKit trunk animations look identical now. So we have at least three viewers (with Abbas) that perform identically. 

> 
> Which is weird. On the other hand, if rotate="auto" is taken away, we get:
> 
> translate(100, 250) translate(300, 30)
> =
> translate(400, 280)

Thats correct as well.

> 
> So the essential weirdness of premultiplication is that the container transform is used in the wrong place, which results in strange outcomes. I felt at the time that wildly different locations depending on whether rotate="auto" was set or not was worse than the alternatives.
> 
> On the other hand, if you postmultiply the supplemental transform as currently specified, then something like:
> 
> <g>
>   <rect width="40" height="40" fill="green"></rect>
>   <animateMotion dur="1s" repeatCount="1" path="M
> 100,250 C 100,50 400,50 400,250"></animateMotion>
>   <animateTransform attributeName="transform" type="rotate" from="-30" to="0" begin="0s" dur="1s" fill="freeze"/>
> </g>
> 
> behaves oddly because the animateTransform rotation affects the animateMotion translation even though it's specified after it. Neither the specified behavior nor the current favorite behavior (which is apparently an artifact of Adobe's that has spread to the other implementations) really does the right thing.
> 
> Now what I'd like to see is for animateMotion to work off the CTM in the same way that animateTransform does, which would mean that there wouldn't be a supplemental transform to pre- or post- multiply, and this problem would go away. Apparently this is hard for other reasons but I've never seen a clear explanation of why.
I totally agree! But right now a common solution is more important than different solutions across all viewers (where just one viewer has the current specified behavior). This would just lead into resigned web developers and they don't use even one of the solutions. We can just try to get this problem on the agenda of one of the next SVG telcons.
Comment 10 Shane Stephens 2011-09-06 04:34:00 PDT
Sounds good :)

BTW I think Opera's behavior is different to everyone else's - from memory they actually implement animateMotion in the CTM (i.e. our hypothetical desired solution), but I could be wrong.
Comment 11 Dirk Schulze 2011-09-07 01:19:07 PDT
*** Bug 52843 has been marked as a duplicate of this bug. ***
Comment 12 Robert Longson 2011-09-17 01:58:22 PDT
(In reply to comment #9)
> 
> Yes that is right: animate motion * animate rotation * transform. Even if I agree the premultiplication of animate motion is not ideally, Firefox and WebKit trunk animations look identical now. So we have at least three viewers (with Abbas) that perform identically. 

Not since firefox changed to match you last week https://bugzilla.mozilla.org/show_bug.cgi?id=665392 I expect we'll move back to the status quo again though.

> I totally agree! But right now a common solution is more important than different solutions across all viewers (where just one viewer has the current specified behavior). This would just lead into resigned web developers and they don't use even one of the solutions. We can just try to get this problem on the agenda of one of the next SVG telcons.

Indeed.
Comment 13 Dirk Schulze 2011-09-17 21:09:59 PDT
(In reply to comment #12)
> (In reply to comment #9)
> > 
> > Yes that is right: animate motion * animate rotation * transform. Even if I agree the premultiplication of animate motion is not ideally, Firefox and WebKit trunk animations look identical now. So we have at least three viewers (with Abbas) that perform identically. 
> 
> Not since firefox changed to match you last week https://bugzilla.mozilla.org/show_bug.cgi?id=665392 I expect we'll move back to the status quo again though.

Oh, bad timing :). I checked the behavior of a Firefox nightly right before switching back to our previous behavior. As I noted on the www-svg mailing list, I do not prefer to premultiply animate motion + rotation. Our intermediate solution (the specified solution) is much easier to understand for web developers IMHO (see comment #8).

The solution that I prefer most is following the sandwich model (like we do for every other animated type) and not to define a special order for multiplication.

> 
> > I totally agree! But right now a common solution is more important than different solutions across all viewers (where just one viewer has the current specified behavior). This would just lead into resigned web developers and they don't use even one of the solutions. We can just try to get this problem on the agenda of one of the next SVG telcons.
> 
> Indeed.
I fear that the SVG WG is not willing to change the specification text on SVG animations and replace it by CSS animations completely instead.
Comment 14 Robert Longson 2011-09-17 23:53:20 PDT
(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #9)
> > > 
> > > Yes that is right: animate motion * animate rotation * transform. Even if I agree the premultiplication of animate motion is not ideally, Firefox and WebKit trunk animations look identical now. So we have at least three viewers (with Abbas) that perform identically. 
> > 
> > Not since firefox changed to match you last week https://bugzilla.mozilla.org/show_bug.cgi?id=665392 I expect we'll move back to the status quo again though.
> 
> Oh, bad timing :). I checked the behavior of a Firefox nightly right before switching back to our previous behavior. As I noted on the www-svg mailing list, I do not prefer to premultiply animate motion + rotation. Our intermediate solution (the specified solution) is much easier to understand for web developers IMHO (see comment #8).
> 

The patch in https://bugzilla.mozilla.org/show_bug.cgi?id=665392 has just come back out of the firefox codebase. It only landed a few days ago. I still think firefox needs to change though so I don't think that's the end of it on our end.