Bug 74719 – TextureMapper: Simplify transform manipulations.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jocelyn Turcotte]

TextureMapper: Simplify transform manipulations.

[Jocelyn Turcotte]

Created attachment 119613 [details]
Patch

[Noam Rosenthal]

Comment on attachment 119613 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        - Make sure that the replica node has a complete transform and
> +          use it directly instead of keeping a copy in the source.

I can't see how this code would work; Since the replica's parent layer is always null, and you don't check for replicatedLayer.
Someone needs to combine the "regular" layer's transform and the replica's local transform. 
That code was pretty tricky and is tested by many layout tests (compositing/reflection). I see no mention in this changelog that those were tested before+after the changes, which makes me think that these changes would break those reflection tests :)

> Source/WebCore/ChangeLog:18
> +        - Fix the fillsForward transform adjustment in syncAnimations and
> +          make the intention clearer by using setTransform and setOpacity.

Different patch.

[Jocelyn Turcotte]

Created attachment 122255 [details]
Patch

[Noam Rosenthal]

Comment on attachment 122255 [details]
Patch

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

Good direction, needs some improvements.

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:69
> +void TextureMapperNode::computeTransformsSelf(const TransformationMatrix& parentTransform)

I don't see why we need to functions. We should just have one function, and use (parent || effectTarget).

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:76
> +    m_transforms.target = parentTransform;
> +    m_transforms.target
>          .translate3d(originX + m_state.pos.x(), originY + m_state.pos.y(), m_state.anchorPoint.z() )
> -        .multiply(m_transforms.base)
> -        .translate3d(-originX, -originY, -m_state.anchorPoint.z());
> +        .multiply(m_transforms.base);

You can do
m_transforms.target = TransformationMatrix(parentTransform).multiply(m_transforms.base);
<nitpick/>

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:78
> +    // In case a parent had preserves3D and this layer has not, flatten our children.

We should early-return if we don't have children, after we've translated the target transform back.

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:95
> +    // The replica layer isn't parented and needs its transform to be stacked atop ours.
> +    if (m_state.replicaLayer)
> +        m_state.replicaLayer->computeTransformsSelf(m_transforms.target);

Instead, you can call effectTarget from within the replica layer if there's no parent.

[Noam Rosenthal]

(In reply to comment #4)
> (From update of attachment 122255 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122255&action=review
> 
> Good direction, needs some improvements.
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:69
> > +void TextureMapperNode::computeTransformsSelf(const TransformationMatrix& parentTransform)
> 
> I don't see why we need to functions. We should just have one function, and use (parent || effectTarget).
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:76
> > +    m_transforms.target = parentTransform;
> > +    m_transforms.target
> >          .translate3d(originX + m_state.pos.x(), originY + m_state.pos.y(), m_state.anchorPoint.z() )
> > -        .multiply(m_transforms.base)
> > -        .translate3d(-originX, -originY, -m_state.anchorPoint.z());
> > +        .multiply(m_transforms.base);
> 
> You can do
> m_transforms.target = TransformationMatrix(parentTransform).multiply(m_transforms.base);
> <nitpick/>
I mean
m_transforms.target = 
    TransformationMatrix(parentTransform)
        .translate3d(originX + m_state.pos.x(), originY + m_state.pos.y(), m_state.anchorPoint.z() )
        .multiply(m_transforms.base);

[Noam Rosenthal]

Comment on attachment 122255 [details]
Patch

After discussing this on IRC, I believe that my comments actually don't make this patch any better.

[Jocelyn Turcotte]

Created attachment 122421 [details]
Patch

Here, it should be on the right bug. Tried to use webkit-patch upload -g SHA^..SHA, but I guess I'll stick with -g HEAD..

[Jocelyn Turcotte]

Committed r104931: <http://trac.webkit.org/changeset/104931>