Closed
Bug 1464791
Opened 6 years ago
Closed 6 years ago
individual scale addition animation looks fishy
Categories
(Core :: CSS Parsing and Computation, enhancement, P3)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla65
Tracking | Status | |
---|---|---|
firefox65 | --- | fixed |
People
(Reporter: emilio, Assigned: boris)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
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.
Updated•6 years ago
|
Priority: -- → P3
Comment 1•6 years ago
|
||
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'?
Blocks: individual-transform
Flags: needinfo?(boris.chiou)
Assignee | ||
Comment 2•6 years ago
|
||
(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)
Comment 3•6 years ago
|
||
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).
Comment 4•6 years ago
|
||
That is, addition for transforms behaves fairly differently to interpolation/accumulation so we've always treated it specially.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → boris.chiou
Assignee | ||
Comment 5•6 years ago
|
||
(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
Assignee | ||
Comment 6•6 years ago
|
||
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
Comment 8•6 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox65:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Comment 9•6 years ago
|
||
> 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?
Comment 10•6 years ago
|
||
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)
Assignee | ||
Comment 11•6 years ago
|
||
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.
Description
•