Bug 52700 – Transition from perspective(500px) to 'none' is probably wrong

• Component: CSS
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

The blendToIdentity clause in PerspectiveTransformOperation::blend() computes p as
p = p + (1. - p) * progress;

which is nonsensical. p is the perspective value (e.g. 1000), so why the 1-p?

[Dean Jackson]

That's weird stuff.

Identity is a zero value in the perspective part of the matrix, which is an infinite perspective(). Is that right? So should this be p + (MAX - p) * progress?

[Chris Marrin]

The formula for blendToIdentity is right:

     if (blendToIdentity)
        return PerspectiveTransformOperation::create(m_p + (1. - m_p) * progress);

Because it's blending down to the identity perspective value, which is 1. 

The other case is:

        return PerspectiveTransformOperation::create(decomp.perspectiveZ ? -1.0 / decomp.perspectiveZ : 0.0);

In the other case we construct from and to matrices and then let TransformationMatrix blend them. TransformationMatrix decomposes both matrices and then linearly interpolates all the decomposed values. Then composes the result into a new matrix and returns the value. PerspectiveTransformOperation::blend then decomposes that matrix and picks out the perspective Z value, which is the one we want.

I did it this crazy way because perspective is not linear, so applying a linear interpolation would give us the wrong results.

So I believe the code is correct, although we could certainly optimize the interpolation!

[Chris Marrin]

Simon, do you see bad behavior? It will interpolate (nonlinearly) from 500 down to the identity value of 1, which is probably wrong. We might have to change the if (blendToIdentity) case. 

Can you post a test case?

It might be better to have the blendToIdentity case go through the same decompose/compose/decompose operation as the normal case, just using an identity matrix for the 'to' matrix. We can look at the values returned. But I think that might give you the right effect.

Thinking back about it, I originally did just linearly interpolate the from and to values. The result was wrong, so I changed it to what we have today. I think I didn't take the identity case into account when I made that change.

[Simon Fraser (smfr)]

(In reply to comment #2)
> The formula for blendToIdentity is right:
> 
>      if (blendToIdentity)
>         return PerspectiveTransformOperation::create(m_p + (1. - m_p) * progress);
> 
> Because it's blending down to the identity perspective value, which is 1. 

But m_p isn't the value in the matrix; that's -1/m_p. So if perspective is 1000, this computes 1000 + (1. - 1000) * progress which makes no sense.

It should really blend with infinity, but that isn't going to give sensible results either.

[Simon Fraser (smfr)]

Created attachment 79464 [details]
Testcase

If you hover the blue box in this testcase it seems OK, but unhover and things go whacko.

[Martin Robinson]

This doesn't reproduce in Safari any longer, but is still an issue in ports that use animations from Nicosia.

[Martin Robinson]

Created attachment 443935 [details]
Patch

[Martin Robinson]

Created attachment 443943 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 443943 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=443943&action=review

> Source/WebCore/platform/graphics/transforms/PerspectiveTransformOperation.cpp:59
> +            const auto* fromOp =
> +                downcast<PerspectiveTransformOperation>(from);

No wrapping.

> Source/WebCore/platform/graphics/transforms/PerspectiveTransformOperation.cpp:60
> +            fromPInverse = !fromOp->isIdentity() ? (1.0 / (*fromOp->floatValue())) : 0.0;

Since this is repeated, put it in a lambda.

> Source/WebCore/platform/graphics/transforms/PerspectiveTransformOperation.h:63
> +        return floatValueForLength(*m_p, 1.0);

Does m_p ever have calc() it in at this point?

[Martin Robinson]

Created attachment 445403 [details]
Patch

[Martin Robinson]

(In reply to Simon Fraser (smfr) from comment #9)
> Comment on attachment 443943 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=443943&action=review

Thanks for the review.
> 
> > Source/WebCore/platform/graphics/transforms/PerspectiveTransformOperation.cpp:59
> > +            const auto* fromOp =
> > +                downcast<PerspectiveTransformOperation>(from);
> 
> No wrapping.

Fixed.


> > Source/WebCore/platform/graphics/transforms/PerspectiveTransformOperation.cpp:60
> > +            fromPInverse = !fromOp->isIdentity() ? (1.0 / (*fromOp->floatValue())) : 0.0;
> 
> Since this is repeated, put it in a lambda.

Great idea. Adding the anonymous function has simplified this code a bit.

> > Source/WebCore/platform/graphics/transforms/PerspectiveTransformOperation.h:63
> > +        return floatValueForLength(*m_p, 1.0);
> 
> Does m_p ever have calc() it in at this point?

Yes, these Lengths are always converted to numerical values in Source/WebCore/css/TransformFunctions.cpp.

[EWS]

Committed r286289 (244648@main): <https://commits.webkit.org/244648@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 445403 [details].

[Radar WebKit Bug Importer]

<rdar://problem/85859222>