Adding a reverse function to the TimingFunctions and tests to prove it works.

Adding a reverse function to the TimingFunctions and tests to prove it works.

BUG=

[mithro-old, 2013-10-31 14:30:44]

Hi Steve,

As I was discussing I ended up adding reverse functions to the TimingFunctions.
Make the code in the CompositorAnimations easier to each, specially in the
chained animation case (no trying to backwards iterate over the list).

Can you take a look and see what you think?

Tim

[Steve Block, 2013-11-03 09:52:31]

Can you upload the code that will make use of this? Without seeing it, it's hard
to know whether this is the right approach.

I agree that the current logic to set up keyframes for the compositor is ugly,
but we talked about improving the compositor API in the future to avoid that.
That seems like a better solution that pushing this logic down into
TimingFunction.

https://codereview.chromium.org/54743002/diff/1/Source/core/platform/animatio...
File Source/core/platform/animation/TimingFunction.h (right):

https://codereview.chromium.org/54743002/diff/1/Source/core/platform/animatio...
Source/core/platform/animation/TimingFunction.h:61: virtual
PassRefPtr<TimingFunction> reverse() const = 0;
Can you add a comment describing exactly what you want this reverse() method to
do?

https://codereview.chromium.org/54743002/diff/1/Source/core/platform/animatio...
Source/core/platform/animation/TimingFunction.h:131: 
This introduces a change in behavior for CSS. The operator==() methods are
currently used for comparing render styles to determine when things change.
Currently, 'animation-timing-function: cubic-bezier(0.25, 0.1, 0.25, 1.0)' and
'animation-timing-function: ease'  are considered to be different values, but
with your change, they will be considered equal. Are you sure this is OK?

https://codereview.chromium.org/54743002/diff/1/Source/core/platform/animatio...
Source/core/platform/animation/TimingFunction.h:179: 
Why don't you test for 'this == &other' here, like in the other classes?

https://codereview.chromium.org/54743002/diff/1/Source/core/platform/animatio...
Source/core/platform/animation/TimingFunction.h:198: return
const_cast<CubicBezierTimingFunction*>(this);
This is incorrect. 'Ease' is not symmetric about x=y.

https://codereview.chromium.org/54743002/diff/1/Source/core/platform/animatio...
Source/core/platform/animation/TimingFunction.h:297: return preset(End);
If I'm right about how I think you intend to use reverse(), this isn't correct.
A step-start has output 0 for all inputs in [0, 1]. The 'reversed' version of
this is a timing function that has output 0 for all inputs in [0, 1]. You're
using a step-end, which has output 0 for all inputs in [0, 1), but output 1 for
input 1.

The other cases have similar problems at the points of discontinuity.

https://codereview.chromium.org/54743002/diff/1/Source/core/platform/animatio...
Source/core/platform/animation/TimingFunction.h:349: virtual bool
operator==(const TimingFunction& other) const
What exactly do you need operator==() for? Currently it's used only for CSS.
Keep in mind that things like 'cubic-bezier(0, 0, 1, 1)' and 'linear' are
functionally equivalent, but currently considered unequal.

[mithro-old, 2013-11-04 03:29:00]

On 2013/11/03 09:52:31, Steve Block wrote:
> Can you upload the code that will make use of this? Without seeing it, it's
hard
> to know whether this is the right approach.
> 
> I agree that the current logic to set up keyframes for the compositor is ugly,
> but we talked about improving the compositor API in the future to avoid that.
> That seems like a better solution that pushing this logic down into
> TimingFunction.

I've uploaded two patches now;
 * https://codereview.chromium.org/54743002/ - Adding reverse functionality to
the TimingFunction directly.
 * https://codereview.chromium.org/51943004/ - Reverse functionality to a helper
class inside the compositor.

Which one do you prefer? (Much cleaner now that it doesn't have the operator==
stuff is in a different patch.)

Tim

[mithro-old, 2013-11-04 03:44:53]

>
https://codereview.chromium.org/54743002/diff/1/Source/core/platform/animatio...
> Source/core/platform/animation/TimingFunction.h:61: virtual
> PassRefPtr<TimingFunction> reverse() const = 0;
> Can you add a comment describing exactly what you want this reverse() method
to
> do?

>
https://codereview.chromium.org/54743002/diff/1/Source/core/platform/animatio...
> Source/core/platform/animation/TimingFunction.h:198: return
> const_cast<CubicBezierTimingFunction*>(this);
> This is incorrect. 'Ease' is not symmetric about x=y.

I missed the y1 value being 0.1 - fixed.

>
https://codereview.chromium.org/54743002/diff/1/Source/core/platform/animatio...
> Source/core/platform/animation/TimingFunction.h:297: return preset(End);
> If I'm right about how I think you intend to use reverse(), this isn't
correct.
> A step-start has output 0 for all inputs in [0, 1]. The 'reversed' version of
> this is a timing function that has output 0 for all inputs in [0, 1]. You're
> using a step-end, which has output 0 for all inputs in [0, 1), but output 1
for
> input 1.
> 
> The other cases have similar problems at the points of discontinuity.

If I understand step correctly;
  StepEnd means [-Inf, 1) == 0 + [1, Inf] == 1
  StepStart means [-Inf, 0] == 0 + (0, Inf] == 1

When reversed, x=0 -> x=1, x=0.5 -> x=0.5, x=1 -> x=0
  Reversed(StepEnd) means [-Inf, 0] == 1 + (0, Inf] == 0
  Reversed(StepStart) means [-Inf, 1) == 1 + [1, Inf] == 0

Right? Or am I confused?





> 
>
https://codereview.chromium.org/54743002/diff/1/Source/core/platform/animatio...
> Source/core/platform/animation/TimingFunction.h:349: virtual bool
> operator==(const TimingFunction& other) const
> What exactly do you need operator==() for? Currently it's used only for CSS.
> Keep in mind that things like 'cubic-bezier(0, 0, 1, 1)' and 'linear' are
> functionally equivalent, but currently considered unequal.

[Steve Block, 2013-11-04 04:36:07]

https://codereview.chromium.org/54743002/diff/1/Source/core/platform/animatio...
File Source/core/platform/animation/TimingFunction.h (right):

https://codereview.chromium.org/54743002/diff/1/Source/core/platform/animatio...
Source/core/platform/animation/TimingFunction.h:297: return preset(End);
> A step-start has output 0 for all inputs in [0, 1].
Sorry, should be ...
A step-start has output 1 for all inputs in [0, 1].

[Steve Block, 2013-11-04 04:40:57]

https://codereview.chromium.org/54743002/diff/1/Source/core/platform/animatio...
File Source/core/platform/animation/TimingFunction.h (right):

https://codereview.chromium.org/54743002/diff/1/Source/core/platform/animatio...
Source/core/platform/animation/TimingFunction.h:297: return preset(End);
> If I understand step correctly;
>  StepEnd means [-Inf, 1) == 0 + [1, Inf] == 1
>  StepStart means [-Inf, 0] == 0 + (0, Inf] == 1

step-start has an output of 1 at input 0. See
http://www.w3.org/TR/2012/WD-css3-transitions-20120403/#transition-timing-fun...

> When reversed, x=0 -> x=1, x=0.5 -> x=0.5, x=1 -> x=0
>  Reversed(StepEnd) means [-Inf, 0] == 1 + (0, Inf] == 0
>  Reversed(StepStart) means [-Inf, 1) == 1 + [1, Inf] == 0

No, you need to flip in y as well as in x. A timing function must go through (0,
0) and (1, 1).

step-end:
  [-Inf, 1) -> 0
  [1, Inf] -> 1

So reversed(step-end):
  [-Inf, 0] -> 0
  (0, Inf] -> 1

step-start:
  [-Inf, 0) -> 0
  [0, Inf] -> 1

So reversed(step-start):
  [-Inf, 1] -> 0
  (1, Inf] -> 1

[Steve Block, 2013-11-04 04:44:49]

I definitely prefer putting this reversing logic in a helper class, rather than
in TimingFunction, because I think in the future we should be able to improve
the compositor API so as not to require this reversal at all. I'd still like to
see the patch that makes use of the reverse functionality though.

[mithro-old, 2013-11-04 12:29:48]

On 2013/11/04 04:44:49, Steve Block wrote:
> I definitely prefer putting this reversing logic in a helper class, rather
than
> in TimingFunction, because I think in the future we should be able to improve
> the compositor API so as not to require this reversal at all. I'd still like
to
> see the patch that makes use of the reverse functionality though.

Closing this in favor of https://codereview.chromium.org/51943004/