Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-transforms] Smarter interpolation of truncated transform lists #927

Closed
smfr opened this issue Jan 14, 2017 · 17 comments · Fixed by #3215
Closed

[css-transforms] Smarter interpolation of truncated transform lists #927

smfr opened this issue Jan 14, 2017 · 17 comments · Fixed by #3215
Assignees
Labels
css-transforms-1 Current Work

Comments

@smfr
Copy link
Contributor

smfr commented Jan 14, 2017

https://www.w3.org/mid/CALXxKfCKZd+P0mH2oFyDkBgps56WqwQnLSMpKk1nrqiFXScqVA@mail.gmail.com
https://www.w3.org/mid/CAAWBYDBPafEbiXNy=Mw4a9w1jmvK6dUJzq28PPjN7APgtjqAEw@mail.gmail.com

Need to decide what goes into Level 1, and what to call the new identify functions. Related to #898

@smfr smfr added the css-transforms-1 Current Work label Jan 14, 2017
@alancutter
Copy link
Contributor

This seems to be the key suggestion from the linked thread.

I wonder if we could take this a bit further - rather than just
removing identical prefixes (where both the functions and arguments
match), we could find the prefix that just has functions matching,
with potentially different arguments. Then we can interpolate that
common prefix normally, and run the rest of the two states back
through the list again as you describe, either matching the remainder
up with identity transforms or mashing the remainder up into a matrix.

My interpretation of this is that translateX(100px) scaleX(2) rotateX(90deg) interpolating with translateY(100px) scaleY(2) rotateY(90deg) would use pairwise function interpolation for translate and scale and use post multiplied matrix interpolation for rotate.

@smfr
Copy link
Contributor Author

smfr commented Apr 19, 2017

Propose moving to level 2.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed Smarter interpolation of truncated lists, and agreed to the following resolutions:

RESOLVED: Accept the issue as written.
The full IRC log of that discussion
<dino> Topic: Smarter interpolation of truncated lists
<dino> Github Topic: https://github.com/w3c/csswg-drafts/issues/927
<dino> smfr: we talked about this in seattle a bit. we suggested adding a special name that can match anything. e.g. identity or none.
<dino> smfr: should we add this to level 1?
<dino> smfr: there are some side-effects of not doing it - e.g. rotations greater than 360
<dino> smfr: i don't like that it is a behaviour change, so suggest deferring
<dino> TabAtkins: I'm ok with deferring any behaviour change
<dino> Rossen: there was a lot of discussion on this. Have you played with it?
<dino> smfr: we haven't implemented.
<dino> Rossen: what is the fear of compatibility risk?
<dino> smfr: they might get different animations
<dino> dbaron: since people have to manually write this, there is no compat risk
<dbaron> (assuming they do, at least)
<dino> smfr: this issue is also asking for magical matching (inserting identity transforms)
<dino> smfr: it's saying that it uses the common prefix for as much as possible, then smoosh together the rest into a matrix
<dino> dbaron: there is more compat risk there
<dino> dbaron: not sure how much interop there is here, since we've changed it a lot
<dino> TabAtkins: better behaviour would be nice, but yes there is a compat risk
<dino> smfr: could we change this behaviour in level 2?
<dino> Rossen: more risky
<dino> dbaron: if we want to change, do it in level 1
<dino> shane: there is a risk. i don't think it is a big issue though. i have no data to support it. we're talking about visual behaviour of an animation
<dino> shane: and this is a fallback behaviour that is now hopefully more closely matching the author intent
<dino> shane: i think this is only stopping messed up animations from looking messed up
<dino> birtles: it's hard to think of a case where it looks worse
<dino> smfr: so change the behaviour for Level 1? As the github issue suggests?
<dino> TabAtkins: that is the most reasonable way to intuit author preference here
<dino> smfr: right
<dino> <no one disagrees>
<dino> <wait.....>
<dino> dbaron: we'd be hesitant to be first implementation, but if everyone else agrees, then we're ok
<dino> Rossen: if we already resolved this, do we need to change anything?
<dino> smfr: i can't find it.
<dino> TabAtkins: i don't think we did
<dino> smfr: maybe we resolved this would be a L2 thing
<dino> Rossen: we can resolve it now
<dino> Rossen: objections?
<dino> <none>
<dino> RESOLVED: Accept the issue as written.

@smfr
Copy link
Contributor Author

smfr commented Apr 9, 2018

That resolution wording is obtuse, but I think means that we resolved to make the behavior change in transforms-1.

@smfr smfr self-assigned this Apr 10, 2018
@smfr smfr closed this as completed in 3281266 Apr 10, 2018
fergald pushed a commit to fergald/csswg-drafts that referenced this issue May 7, 2018
Add section describing the interpolation between transform lists with different lengths where the shorter list has functions that match the prefix of the longer list.
@birtles
Copy link
Contributor

birtles commented May 28, 2018

We started implementing this in Firefox but I'm a little unclear about the intention of this change. In the IRC log I see the comment:

smfr: it's saying that it uses the common prefix for as much as possible, then smoosh together the rest into a matrix

but in the spec change we seem to only handle mismatched lists where the functions of the shorter one match primitives of the longer.

i.e. it seemed like we were initially looking at allowing rotate(0deg) translate(10px)rotate(720deg) scale(2) to interpolate the rotate() component directly and then use matrix interpolation for the remainder but the spec change would make the whole thing use matrix interpolation.

It may well be that the spec change deliberately doesn't allow the "common prefix" behavior due to compatibility concerns but I'd like to confirm we've understood the intention here since it's not clear to me from the IRC log what the outcome was.

@dirkschulze dirkschulze reopened this May 28, 2018
@dirkschulze
Copy link
Contributor

@birtles From the proposal and the discussion, the spec change does not fully support the requested "smoosh". According to the discussion above this could either be by:

  • multiplying all not matching functions to a matrix and interpolate the others.
  • adding neutral transform functions where needed.

Both do not sound ideal to me. The latter mostly because we need to define an algorithm where and when to add neutral transformations. At the first glance this might sound intuitive but really isn't.

I wasn't in the discussion but what @smfr added seems to be logical and matching the behavior of other CSS properties:

  • Fill up the shorter list with matching, neutral transform functions.
  • (Special requirement: only if the transforms are otherwise compatible.)

@birtles
Copy link
Contributor

birtles commented May 28, 2018

Yeah, it seems to be reasonable. I just wanted to check if the spec change was intentionally different from what the discussion seemed to conclude.

@tabatkins
Copy link
Member

My interpretation of the discussion was that both of Dirk's suggestions should be used:

  • If one list is shorter than the other, pad it with identify functions to match lengths.
  • Then do a compatible-prefix match, "smooshing" the incompatible suffix (if any) of each into a matrix() or matrix3d() and transitioning appropriately.

So @birtles' example should work as they expect - rotate(0deg) translate(10px)rotate(720deg) scale(2) will do two full rotations (as rotate() is the compatible prefix) and then do a matrix-interpolation between translate(10px) and scale(2).

@dirkschulze
Copy link
Contributor

dirkschulze commented Jun 1, 2018

@tabatkins Yes, phrased it wrong. I am just suspicious about

  • adding neutral transform functions where needed.

Where should missing/differing functions be added?

Simple example:
rotate(720deg)translate(100px, 100px)
A translate needs to get added to the former and a rotate to the latter transform function list. But in which order? After all the order matters a lot from the visual perspective.

  • We could say that we follow the TRS schema proposed for the translate, rotate and scale properties in Level 2. So translate has precedence over rotate which has precedence over scale. Independent of the lists length we add the pendant function as needed. This potentially doubles the number of transform functions if they all differ. More thoughts are needed for skewX, skewY, skew, matrix and the 3D transform functions.
  • We could provide an algorithm that adds a transform function to the shorter list. Going from left to right: If 2 transform functions between the 2 lists differ, add the neutral pendant at the same spot to the shorter list. We would still need to decide what the precedence is if the transform function lists have the same number of transform functions.

Potentially both algorithms would replace matrix decomposition for the majority of cases.
However, quite often there would be a solution that could have been smarter from a human eye perspective but wasn't used. Sometimes, even the first solution does not give the most pleasant result.

The complex algorithm needs to get implemented in an interoperable way in all UAs as well. Given how long even smaller changes in the spec took, it might be a while and I expect that we will see some additional feedback from UAs after implementing.

@tabatkins
Copy link
Member

Like I just outlined, you fix length mismatches by adding neutral transforms to the end of the shorter list.

In your example, no fixup is possible. They're the same length, and have no common prefix, so you still just go straight to matrix interpolation.

@dirkschulze
Copy link
Contributor

@tabatkins Oh got it. The common transform function primitive is there already and adding neutral transform functions at the end of the list was added recently.

The proposal in #927 (comment) and the question from @birtles #927 (comment) goes adds an additional step:

If there is a mismatch between 2 transform function pairs (no common primitive): Instead of multiplying all transform functions into one matrix before decomposing the suggestion is to do the matrix interpolation part only between not matching transform function pairs.

An additional question I have for that proposal: rotate(180deg) translate(120px) scale(2) -> rotate(180deg) scale(2) skewX(45deg). According to the proposal, rotate uses normal interpolation for rotate:

  • Would the 2nd and 3rd transform functions get multiplied first before the matrix interpolation happens or does it happen per transform function pair? Matrix decomposition and recomposition involves a lot of operations. Doing it for each function pair might be too expensive. The order of multiplication matters.
  • If we collapse multiple not-matching transform function pairs before matrix interpolation, would it happen up to the next matching function pair or do we just do it for the entire rest of the transform function lists?

@tabatkins
Copy link
Member

I think the simplest thing is to collapse the rest once you have a mismatch. That's also the minimal change from the current behavior for things that differ early; they can't accidentally get "back on track" and change behavior.

@css-meeting-bot
Copy link
Member

The Working Group just discussed Smarter interpolation of truncated transform lists.

The full IRC log of that discussion <dael> Topic: Smarter interpolation of truncated transform lists
<dael> github: https://github.com//issues/927
<dael> krit: There's one issue on smart interpolation for transform functions. You have to interpolate when there's an animation. Usually they have to have same paremeters to map. Every time those functions do not match tehre is matrix interpolation
<dael> krit: Previous we allowed the transforms to be of differnet size. As long as they map to same size we do the interpolation per transform function. Concern was this wasn't good enough
<dael> krit: In this issue request is interpolation per transform function as long as they match with each other. If they do not match we do matrix intrp but just for the ones that come after the non-mathcing
<dael> krit: Support from TabAtkins , request from smfr to defer to L2. I think we keep what we have b/c it's interop.
<dael> krit: I don't think we can defer because it would change animation in certain situations. Doesn't happen too often that you don't have 2 matching and an animation.
<dael> krit: So do we agree to this proposal? Or do we keep the current spec text where everything multiplied to a matrix.
<dael> krit: Complex, but important for animations
<dael> krit: Questions on what hte request is?
<dael> fantasai: I agree with making the animations do closer to what authors expect and making them more powerful in terms of accurate representation. In favor of the change.
<dael> fantasai: Only question I have is we're using the prefix and padding the end with additional ID transforms. Do we want to allow...if the end of the transform list matches rather then the beginning. You don't interopalte ones you don't have, but if one sequenence is a subset allow padding on either side?
<dael> krit: There are requests to make it smarter, but it's complicated. YOu have to find the pattern and there might be multiple ptterns so you have to decide.
<dael> fantasai: I'm suggesting it has to be exact match. If you have multiple we pick a rule like match against the first one.
<dael> krit: I'm a bit confused. You want them to match on start or end and only if same length?
<dael> fantasai: Different, but only if one is an exact subset of the other.
<dael> krit: List 1 is a subset of list 2
<dael> fantasai: Yes
<dael> krit: And only beginning or end or also middle?
<dael> fantasai: Both. Middle or end.
<dael> dbaron: I worry matching in middle will be harder to understand. We want behavior to be good and understandable and expected. Matching in the middle feels...oh, it'llrandomly search, but only if you have an exact match of seq. I think there's value in doing something simple and explainable.
<dael> fantasai: Okay. That's fine. I wanted to know if it was thought through
<dael> krit: Does the issue in question have support? everything matches at the beginning but not end so we do matrix for the things that do not match at end?
<dael> fantasai: Seems fine
<dael> krit: Looking for browser impl feedback as that's not impl
<dael> krit: If people need time it's fine, but input at some point would be great.
<dael> Rossen_: smfr isn't on the call. We are going to make this change as a part of transforms L1, right?
<dael> krit: Yes, need to for L1 because if we do L2 there will be unexpected repercussions for Animations
<dael> Rossen_: Do people feel they need more time? Can we resolve now and reopen if needed? Preference?
<dael> Rossen_: Objections to...krit?
<dael> krit: I'm not proposing. I'm in favor of not changing text
<dael> Rossen_: Proposal: No change to the current specification
<dael> krit: Only b/c it's impl currently
<dael> Rossen_: Objections to stay with current impl behavior and not change transforms L1?
<dael> fantasai: We've talked about if we make this change it should be L1.
<dael> krit: Correct
<dael> Rossen_: Yes if we make the change it should be L1.
<dael> fantasai: So asking to not change ever?
<dael> Rossen_: No, we can defer this to L2 but for L1 we can stay with current impl solution in the spec.
<dael> fantasai: If you want to eventually make this change, why not put it in L1?
<dael> krit: Depends on impl interest. If there isn't doesn't make sense to put it in.
<dael> fantasai: I don't think it makes sense to decide if we're putting in L1. If we do it it should go in L1. We can put it in L1 and mark as at-risk. This is a WD still.
<dael> fantasai: The longer that we don't impl, content...this is a change in an existing deployed feature. If we want a change we should do it sooner rather than later. Waiting doesn't seem should do
<dael> Rossen_: But I'm not hearing impl step up.
<dael> florian: Then we shouldn't have it at all rather than have it in L2
<dael> Rossen_: And that's the no change resolution. Don't impl it.
<dael> Rossen_: Any interest in working on this feature? If not we can resolve on rejecting it.
<dael> fantasai: Note that birtles and TabAtkins aren't here. Can anyone represent their positions?
<dael> Rossen_: People from Mozilla or Chrome on?
<dael> Rossen_: none to represent this issue
<dael> fantasai: I recommend that given they're not here and critical we resolve on if we do it this is how and then the question of do we do it waits until next week.
<dael> Rossen_: We can try and get to this in F2F
<dael> fantasai: You can resolve on how interpolation happens
<dael> Rossen_: Prefer not to in absense of the people you mentioned. If will discuss at F2F let's do it at that time.
<fantasai> s/absense/absence/

@css-meeting-bot
Copy link
Member

The Working Group just discussed [css-transforms] Smarter interpolation of truncated transform lists, and agreed to the following:

  • RESOLVED: pad the shorter one with identity functions, find the common prefix, interpolate common prefix pairwise, interpolate the rest matrix-wise.
The full IRC log of that discussion <Rossen> Topic: [css-transforms] Smarter interpolation of truncated transform lists
<Rossen> github: https://github.com//issues/927
<ericwilligers_> Brian: (summarizing) There was a proposal for when we have mismatched transform lists to do something smarter than matrix interpolation
<ericwilligers_> Brian: We resolved on some behavior (April 2017) but when implementing, spec text did not match what was discussed in WG.
<ericwilligers_> Brian: We implemented what is in spec text but we want to do something smarter: interpolate common denominator and use matrix interpolation for the remainder
<ericwilligers_> s/denominator/prefix/
<ericwilligers_> dirk: do other implementers also want to change?
<ericwilligers_> dino: we should do this: go as far as you can, smoosh the rest
<ericwilligers_> ian: will have to check with Waterloo team
<ericwilligers_> rossen: no objection
<ericwilligers_> pad the shorter one with identity functions, find the common prefix.
<ericwilligers_> interpolate common prefix pairwise, interpolate the rest matrix-wise
<ericwilligers_> dirk: proposal from fantasai to do the same from the right if they have a common suffix.
<ericwilligers_> tab: don't like it - common to have ambiguous situations
<ericwilligers_> dirk: agree
<ericwilligers_> tab: prefer prefix over suffix, prefer not to try to do both
<birtles> I agree
<ericwilligers_> RESOLVED: pad the shorter one with identity functions, find the common prefix, interpolate common prefix pairwise, interpolate the rest matrix-wise.

@birtles
Copy link
Contributor

birtles commented Jul 3, 2018

Gecko bug for updating this.

@LeaVerou
Copy link
Member

LeaVerou commented Jul 3, 2018

One different idea, possibly bad, but figured I'd throw it out there just in case: Matrix interpolation looks far worse for angle-based (rotate and skew) transforms than the rest. What if we try to match those up and interpolate the rest via matrix interpolation?

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Oct 19, 2018
…or transform lists; r=hiro

As discussed in:

  w3c/csswg-drafts#927

with tentative spec text:

  w3c/csswg-drafts#3215

Depends on D9184

Differential Revision: https://phabricator.services.mozilla.com/D9185

--HG--
extra : moz-landing-system : lando
emilio pushed a commit to emilio/servo that referenced this issue Oct 28, 2018
birtles added a commit to birtles/csswg-drafts that referenced this issue Nov 7, 2018
birtles added a commit that referenced this issue Nov 7, 2018
@birtles
Copy link
Contributor

birtles commented Jan 7, 2019

It turns out this change breaks the header on Channel News Asia. See webcompat/web-bugs#23687

gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 3, 2019
…or transform lists; r=hiro

As discussed in:

  w3c/csswg-drafts#927

with tentative spec text:

  w3c/csswg-drafts#3215

Depends on D9184

Differential Revision: https://phabricator.services.mozilla.com/D9185

UltraBlame original commit: 660b4ee6c9133fb2fb9fee9d70cf0e8d9b04051a
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 3, 2019
…or transform lists; r=hiro

As discussed in:

  w3c/csswg-drafts#927

with tentative spec text:

  w3c/csswg-drafts#3215

Depends on D9184

Differential Revision: https://phabricator.services.mozilla.com/D9185

UltraBlame original commit: 660b4ee6c9133fb2fb9fee9d70cf0e8d9b04051a
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 3, 2019
…or transform lists; r=hiro

As discussed in:

  w3c/csswg-drafts#927

with tentative spec text:

  w3c/csswg-drafts#3215

Depends on D9184

Differential Revision: https://phabricator.services.mozilla.com/D9185

UltraBlame original commit: 660b4ee6c9133fb2fb9fee9d70cf0e8d9b04051a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
css-transforms-1 Current Work
Projects
None yet
8 participants