w3c / csswg-drafts

CSS Working Group Editor Drafts
https://drafts.csswg.org/
Other
4.48k stars 659 forks source link

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

Closed smfr closed 5 years ago

smfr commented 7 years ago

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 https://github.com/w3c/csswg-drafts/issues/898

alancutter commented 7 years ago

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 commented 7 years ago

Propose moving to level 2.

css-meeting-bot commented 7 years ago

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

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

birtles commented 6 years ago

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 commented 6 years ago

@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:

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:

birtles commented 6 years ago

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 commented 6 years ago

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

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 commented 6 years ago

@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.

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 commented 6 years ago

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 commented 6 years ago

@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 https://github.com/w3c/csswg-drafts/issues/927#issue-200763204 and the question from @birtles https://github.com/w3c/csswg-drafts/issues/927#issuecomment-392390666 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:

tabatkins commented 6 years ago

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 commented 6 years ago

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/w3c/csswg-drafts/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 commented 6 years ago

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

The full IRC log of that discussion <Rossen> Topic: [css-transforms] Smarter interpolation of truncated transform lists
<Rossen> github: https://github.com/w3c/csswg-drafts/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 commented 6 years ago

Gecko bug for updating this.

LeaVerou commented 6 years ago

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?

birtles commented 5 years ago

It turns out this change breaks the header on Channel News Asia. See https://github.com/webcompat/web-bugs/issues/23687