RESOLVED FIXED Bug 74719
TextureMapper: Simplify transform manipulations.
https://bugs.webkit.org/show_bug.cgi?id=74719
Summary TextureMapper: Simplify transform manipulations.
Jocelyn Turcotte
Reported 2011-12-16 08:38:40 PST
TextureMapper: Simplify transform manipulations.
Attachments
Patch (12.53 KB, patch)
2011-12-16 08:39 PST, Jocelyn Turcotte
no flags
Patch (12.17 KB, patch)
2012-01-12 09:06 PST, Jocelyn Turcotte
no flags
Patch (12.64 KB, patch)
2012-01-13 06:19 PST, Jocelyn Turcotte
noam: review+
Jocelyn Turcotte
Comment 1 2011-12-16 08:39:26 PST
Noam Rosenthal
Comment 2 2011-12-16 10:00:10 PST
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
Comment 3 2012-01-12 09:06:50 PST
Noam Rosenthal
Comment 4 2012-01-12 09:21:56 PST
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
Comment 5 2012-01-12 09:24:08 PST
(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 6 2012-01-12 11:54:09 PST
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
Comment 7 2012-01-13 06:19:57 PST
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
Comment 8 2012-01-13 06:40:15 PST
Note You need to log in before you can comment on or make changes to this bug.