Closed Bug 1464647 Opened 7 years ago Closed 7 years ago

Implement the smarter interpolation for transform list

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

(
URL
)

Details

Attachments

(3 files)

Bug 1464647 - Implement the smarter interporation for transform.
59 bytes, text/x-review-board-request
birtles
: review+
emilio
: review+
Details
Bug 1464647 - Add web platform tests affected by the smarter interpolation of transform list.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1464647 - Add a web platform test that isn't affected by the smarter interpolation of transform list.
59 bytes, text/x-review-board-request
birtles
: review+
Details
The issue has been already fixed [1], we should implement that. https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b244dcd13b3ad79e130345275fd0953dce1614c I did start implementing this to fix bug 1459403, but Emilio has already found the real cause which is really hard to notice. :) [1] https://github.com/w3c/csswg-drafts/issues/927
I think this change will reduce the frequency stack overflow in animate(). e.g. bp-e717e89d-a52a-4824-a25e-ed13a0180527
Assignee: nobody → hikezoe
Priority: -- → P3
As discussed on IRC, I don't think this matches the specced behavior. However, it's not clear to me how we arrived at the spec behavior or if I've understood it correctly. Followed up on the spec: https://github.com/w3c/csswg-drafts/issues/927#issuecomment-392390666
Thanks! I was misunderstanding the spec, I thought the match behavior is more eager than the spec. e.g. 'rotate(90deg) translateX(100px) scale(2)' -> 'rotate(0deg) scale(3)' does match the condition. Anyway here is a try with the revised change and some web-platform-tests. https://treeherder.mozilla.org/#/jobs?repo=try&revision=2915624a6e088e53eb4a6637e28a301dea07471e Another thing I notice is whether we should use this new behavior for additive animation or not. I thought initially we shouldn't but after some more thought we should since it's more intuitive for web developer? Anyway, the change in this try doesn't affect additive animation at all for now.
Comment on attachment 8981046 [details] Bug 1464647 - Add web platform tests affected by the smarter interpolation of transform list. https://reviewboard.mozilla.org/r/247166/#review253230
Attachment #8981046 - Flags: review?(bbirtles) → review+
Comment on attachment 8981047 [details] Bug 1464647 - Add a web platform test that isn't affected by the smarter interpolation of transform list. https://reviewboard.mozilla.org/r/247168/#review253234
Attachment #8981047 - Flags: review?(bbirtles) → review+
Comment on attachment 8981046 [details] Bug 1464647 - Add web platform tests affected by the smarter interpolation of transform list. https://reviewboard.mozilla.org/r/247166/#review253236 ::: testing/web-platform/meta/web-animations/animation-model/animation-types/accumulation-per-property.html.ini:3 (Diff revision 2) > + [transform: rotate() translate() on rotate()] > + expected: FAIL > + > + [transform: rotate() on roate() translate()] These don't seem to match the actual test descriptions below (Presence of (), spelling of "roate" etc.)
None of the added tests here seem to actually test the matrix interpolation of the remainder of the list. Is that because there are other tests that already cover this? It seems like even in that case there would be observable difference in behavior for: rotate(0deg) -> rotate(720deg) translate(100px) than what we currently have? Should we add this test?
I've only skimmed the last patch but so far it looks good. I'll have a proper look tomorrow (that should hopefully give time for Simon and others to respond to the issue on csswg-drafts too).
(In reply to Brian Birtles (:birtles) from comment #14) > None of the added tests here seem to actually test the matrix interpolation > of the remainder of the list. Is that because there are other tests that > already cover this? I didn't intentionally add such test cases since it's not clear to me that we should add *expected* result or current result. But I did look the test cases again, there is an interesting test case[1] which is exactly one of the remainder list mismatches. Its current expected result is discrete animation but once we implement *smoosh* interpolation, the result is interpolatable. [1] https://hg.mozilla.org/mozilla-central/file/db78be8dbc1c/testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js#l1033 > It seems like even in that case there would be observable difference in > behavior for: > > rotate(0deg) -> rotate(720deg) translate(100px) > > than what we currently have? An interesting test case, but in this test case the animated values are the same at the half of its duration on both current interpolation and the new smarter interpolation. Instead rotate(0deg) -> rotate(360deg) translate(100px) In this case the animated values at the harf of the duration are different. I will add this case.
Comment on attachment 8980955 [details] Bug 1464647 - Implement the smarter interporation for transform. https://reviewboard.mozilla.org/r/247108/#review253298 ::: servo/components/style/properties/helpers/animated_properties.mako.rs:2574 (Diff revision 4) > owned_other = other; > } > > + fn extend_shorter_list( > + shorter: &[ComputedTransformOperation], > + longer: &[ComputedTransformOperation], Would it be better to make this take `&mut Vec`, and then you'd only need to mutate the already owned bits? The way I'd structure this would be something like: ``` fn match_operations_if_possible( this: &mut Vec<ComputedTransformOperation>, other: &mut Vec<ComputedTransformOperation>, ) -> Result<(), ()> { if !this.iter().zip(other.iter()).all(is_matched_operation) { return Err(()); } if this.len() == other.len() { return Ok(()); } let (mut shorter, mut longer) = if this.len() < other.len() { (this, other) } else { (other, this) }; shorter.reserve_capacity(other.len()); for op in &longer { shorter.push(op.to_animated_zero()?); } return Ok(()) } ``` This allows you to also handle the empty case in the same piece of code. WDYT?
Attachment #8980955 - Flags: review?(emilio) → review+
(In reply to Emilio Cobos Álvarez [:emilio] from comment #20) > Comment on attachment 8980955 [details] > Bug 1464647 - Implement the smarter interporation for transform. > > https://reviewboard.mozilla.org/r/247108/#review253298 > > ::: > servo/components/style/properties/helpers/animated_properties.mako.rs:2574 > (Diff revision 4) > > owned_other = other; > > } > > > > + fn extend_shorter_list( > > + shorter: &[ComputedTransformOperation], > > + longer: &[ComputedTransformOperation], > > Would it be better to make this take `&mut Vec`, and then you'd only need to > mutate the already owned bits? > > The way I'd structure this would be something like: > > ``` > fn match_operations_if_possible( > this: &mut Vec<ComputedTransformOperation>, > other: &mut Vec<ComputedTransformOperation>, > ) -> Result<(), ()> { > if !this.iter().zip(other.iter()).all(is_matched_operation) { > return Err(()); > } > > if this.len() == other.len() { > return Ok(()); > } > > let (mut shorter, mut longer) = if this.len() < other.len() { (this, > other) } else { (other, this) }; > shorter.reserve_capacity(other.len()); > for op in &longer { > shorter.push(op.to_animated_zero()?); > } > return Ok(()) > } > ``` Pretty neat! I've never thought to modify the lists in the function. Great, thanks!
Comment on attachment 8980955 [details] Bug 1464647 - Implement the smarter interporation for transform. https://reviewboard.mozilla.org/r/247108/#review253404
Attachment #8980955 - Flags: review?(bbirtles) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #16) > An interesting test case, but in this test case the animated values are the > same at the half of its duration on both current interpolation and the new > smarter interpolation. Instead > > rotate(0deg) -> rotate(360deg) translate(100px) > > In this case the animated values at the harf of the duration are different. > I will add this case. I think part of the point of the spec change is that, in particular, rotation angles > 360deg are handled better with this change. If the test framework is testing at 50% then perhaps just replace the second argument with rotate(1080deg) or so.
A final try; https://treeherder.mozilla.org/#/jobs?repo=try&revision=507bc79b37ddc7202531178fbbd1867fee6f40f1 I've changed the values to rotate(1080deg) there.
Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/444fb7b29a2e Add web platform tests affected by the smarter interpolation of transform list. r=birtles https://hg.mozilla.org/integration/autoland/rev/ae2590472b4d Add a web platform test that isn't affected by the smarter interpolation of transform list. r=birtles https://hg.mozilla.org/integration/autoland/rev/01356631164a Implement the smarter interporation for transform. r=birtles,emilio
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11213 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
See Also: → 1472917
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: