individual scale addition animation looks fishy

RESOLVED FIXED in Firefox 65

Status

()

enhancement
P3
normal
RESOLVED FIXED
Last year
9 months ago

People

(Reporter: emilio, Assigned: boris)

Tracking

(Blocks 1 bug)

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(1 attachment)

This bit:

  https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/servo/components/style/properties/helpers/animated_properties.mako.rs#2493-2497

Is completely different to what we do for, e.g., scale3d animations.

I don't know why it needs to exist at all.
Priority: -- → P3
That's a really good question. Boris, do you have any idea why we do this or if there are any tests for addition of 'scale'?
Flags: needinfo?(boris.chiou)
(In reply to Brian Birtles (:birtles) from comment #1)
> That's a really good question. Boris, do you have any idea why we do this or
> if there are any tests for addition of 'scale'?

I am still looking at this. It's weird because I cannot remove these lines; otherwise, we got incorrect resulst of these testcases [1] (i.e. got the same result as accumulation.)

[1] https://searchfox.org/mozilla-central/rev/7c848ac7630df5baf1314b0c03e015683599efb9/testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js#1992-2031
Flags: needinfo?(boris.chiou)
It's possible animate_multiplicative_factor just never accounted for addition (since I think everywhere else we use it, it comes after an early return for the addition case).
That is, addition for transforms behaves fairly differently to interpolation/accumulation so we've always treated it specially.
Assignee: nobody → boris.chiou
(In reply to Brian Birtles (:birtles) from comment #4)
> That is, addition for transforms behaves fairly differently to
> interpolation/accumulation so we've always treated it specially.

Yes. we have a special handle for `Procedure::Add`.

1. Let's see transform function lists first:
ex.
      target.style.transform = 'scale(2)';
      const animation = target.animate({ transform: ['scale(-3)', 'scale(5)'] },
                                       { duration: 1000, fill: 'both',
                                         composite: 'add' });
      testAnimationSampleMatrices(animation, transform,
        [{ time: 0,    expected: [ -6, 0, 0, -6, 0, 0 ] }]); // scale(-3) scale(2)

What do we do at 0% in this case?
a) Chain the "from" value and "to" value by `Procedure::Add` [1]:
 i) from: [Scale(2.0, None)] + [Scale(-3.0, None)] => [Scale(2.0, None), Scale(-3.0, None)] // just chain the functions
 ii)  to: [Scale(2.0, None)] + [Scale(5.0, None)]  => [Scale(2.0, None), Scale(5.0, None)]  // just chain the functions
b) Do interpolation on the new "from" and new "to" at `Interpolate { progress: 0.0 }`:
 => result: [Scale(2.0, None), Scale(-3.0, None)]


2. Then, see the individual transform, scale:
ex.
      target.style.scale = '2';
      const animation = target.animate({ scale: ['-3', '5'] },
                                       { duration: 1000, fill: 'both',
                                         composite: 'add' });
      testAnimationSamples(animation, scale,
        [{ time: 0,    expected: '-6' }]);

What do we do at 0% in this case? (If we drop [2], we will get the incorrect value, see below.)
a) We still try to produce the `from` and `to` values by `Procedure::Add`. However, we
   don't have a special handle to chain two transform functions like above. So if we drop [2]:
 i) from: Scale(2.0, 2.0) + Scale(-3.0, -3.0) => Scale(-2.0, -2.0) // by animate(..., Procedure::Add) directly
 ii)  to: Scale(2.0, 2.0) + Scale(5.0, 5.0)   => Scale(6.0, 6.0))  // by animate(..., Procedure::Add) directly
b) Do interpolation on the new "from" and "to" at `Interpolate { progress: 0.0 }`:
 => result: Scale(-2.0, -2.0))


In conclusion, we chain the transform functions for `Procedure::Add` for `transform` property [1]. However, the individual transforms cannot do the same thing because we cannot do concatenation, so we... need this code [2] to do that, just like chain the transform functions. I think we have to add more comment on it. (For now, I don't have a good idea to merge them.)


[1] https://searchfox.org/mozilla-central/rev/c0b26c40769a1e5607a1ae8be37fe64df64fc55e/servo/components/style/properties/helpers/animated_properties.mako.rs#2282-2285
[2] https://searchfox.org/mozilla-central/rev/c0b26c40769a1e5607a1ae8be37fe64df64fc55e/servo/components/style/properties/helpers/animated_properties.mako.rs#2249-2252,2263-2266
Add more comments to let people know the intention of the special case
and move the same code together.
Pushed by boris.chiou@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2c3b27812cbc
Add comments for the calculation of Procedure::Add on Scale and transform list. r=birtles
https://hg.mozilla.org/mozilla-central/rev/2c3b27812cbc
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
> the individual transforms cannot do the same thing because we cannot do concatenation

transform:
translateX(10px) plus translateX(20px) is their concatenation, translateX(10px) translateX(20px), which has the same effect as translateX(30px)
translate:
10px plus 20px results in 30px.

transform:
rotate(10deg) plus rotate(20deg) is their concatenation, rotate(10deg) rotate(20deg), which has the same effect as rotate(30deg)
rotate:
10deg plus 20deg results in 30deg.

This suggests we could have
transform:
scale(2, 3) plus scale(5, 7) is their concatenation, scale(2, 3) scale(5, 7), which has the same effect as scale(10, 21)
scale:
2 3 plus 5 7 could result in 10 21.


Is this what we have?
I'm not sure I understand the question, but for scale I believe we have:

scale(2) ADD scale(2) = scale(2) scale(2) = scale(4)
scale(2) ACCUMULATE scale(2) = scale(3)
Yes. For addition
1. transform list: |transform: scale(2)| + |transform: scale(2)| => |transfrom: scale(2) scale(2)|
2. scale property: |scale: 2| + |scale: 2| => scale: 4

The comment 5 just wants to say, we can concatenate the transform functions into a list if it is addition, and so it's pretty simple to do addition for all the different transform functions by the same way (i.e. just concate the function list)

However, scale property in Gecko is just an enum value (in Rust), and there is no general way to do something like transform list which can concate two scale functions. So for implemention, we have to do a special calculation directly (i.e. a hack?) for scale:
  |scale: 2| + |scale: 2| = |scale: 2 * 2| = |scale: 4|
You need to log in before you can comment on or make changes to this bug.