[webkit-reviews] review requested: [Bug 94860] [chromium] Should accelerate rotations of >= 180 degrees : [Attachment 160761] Patch

[bugzilla-daemon at webkit.org]

vollick at chromium.org has asked	for review:
Bug 94860: [chromium] Should accelerate rotations of >= 180 degrees
https://bugs.webkit.org/show_bug.cgi?id=94860

Attachment 160761: Patch
https://bugs.webkit.org/attachment.cgi?id=160761&action=review

------- Additional Comments from vollick at chromium.org
(In reply to comment #4)
> (From update of attachment 160485 [details])
>
> Seems like a nice cleanup to me!
>
> I'm a bit worried about the additional identity stuff.  It seems to be a bit
precarious in places where if we want to modify the code later, we might need
to remember to check isIdentity() which could be easy to overlook.  Otherwise
LGTM =)
>
>
> View in context:
https://bugs.webkit.org/attachment.cgi?id=160485&action=review
>
> > Source/WebCore/platform/chromium/support/WebTransformOperations.cpp:65
> >	 WebTransformationMatrix matrix;
> > +	 double x;
> > +	 double y;
> > +	 double z;
> > +	 double angle;
>
> do we use the matrix anymore, for any operation other than
WebTransformOperationMatrix?
>
> maybe we could use unions here.  Not only could that save space, but would
allow us to have more intuitive names for the parameters of each transform.
I have used a union, but the matrix couldn't be included as it has a
non-trivial copy constructor.
>
> > Source/WebCore/platform/chromium/support/WebTransformOperations.cpp:154
> > +	     double fromX = isIdentity(from) ? 0 : from->x;
> > +	     double fromY = isIdentity(from) ? 0 : from->y;
> > +	     double fromZ = isIdentity(from) ? 0 : from->z;
> > +	     double toX = isIdentity(to) ? 0 : to->x;
> > +	     double toY = isIdentity(to) ? 0 : to->y;
> > +	     double toZ = isIdentity(to) ? 0 : to->z;
>
> this pattern shows up a lot in this patch - is it really necessary?  or could
we just initialize to and from x, y, z appropriately in all cases?   Seems like
this isIdentity() ? 0 : value  pattern may be error-prone down the road...
I do think it's necessary. It can be the case that to or from is NULL. It's
also possible that 'to' is a translation and 'from' is a rotation, but it just
so happens that the 'from' rotation is 0 degrees (and is, therefore, an
identity transform).
So if I tried to access from.translate.x, it would not be initialized correctly
(since we set the rotate part of the union, not the translate).
>
> > Source/WebCore/platform/chromium/support/WebTransformOperations.cpp:177
> > +	     else {
> > +		 WebTransformationMatrix toMatrix;
> > +		 if (!isIdentity(to))
> > +		     toMatrix = to->matrix;
> > +		 WebTransformationMatrix fromMatrix;
> > +		 if (!isIdentity(from))
> > +		     fromMatrix = from->matrix;
> > +		 toReturn = toMatrix;
> > +		 toReturn.blend(fromMatrix, progress);
> > +	     }
>
> Will this else-statement also work for angles greater than 180?  I thought we
would have to interpolate the axis and angle separately to get this part
correct?
I experimented with interpolating the axes, but the results were unattractive
and do not match the unaccelerated path. It appears that the correct behavior
is to
rotate3d(x, y, z, blend(fromAngle, toAngle, progress)) if the axes line up, but
revert to blending the rotation matrices otherwise (which decomposes to
quaternions
and does a quaternion slerp under the covers).
>
> > Source/WebCore/platform/graphics/chromium/AnimationTranslationUtil.cpp:117
> > +		 webTransformOperations.appendIdentity();
>
> I'm willing to believe that this is correct, but I'm just wondering why this
change is necessary?   Especially for the NONE case?
This was a bug -- we should do nothing in the NONE case. Fixed.