780880 - patternTransform in SVG - rotating by 90 degrees not working

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[lmrspax]

Attachment: test.html - Three rectangles filled with same pattern rotated by diffrent degress. Last one is broken one.

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20100101 Firefox/14.0.1
Build ID: 20120713134347

Steps to reproduce:

Make a pattern rotated with patterTransform by 90 degrees.
Make a shape filled with this pattern.


Actual results:

The shape is filled with one uniform color somehow similiar to the color of the pattern.


Expected results:

The shape should be filled with the pattern rotated by 90 degrees.

Comment 1

[Jesper Hansen]

Attachment: Simple Animated example

Comment 2

[Jesper Hansen]

Did a quick animated example and I am not sure what exactly happens since it gets so blurry

Comment 3

[Robert Longson [:longsonr]]

Attachment: patch

Comment 4

[Robert Longson [:longsonr]]

The double transform introduces slightly greater rounding errors so I need to increase the stroke-width of the covering circle slightly.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 649698 [details] [diff] [review]
patch

>+++ b/layout/svg/base/src/nsSVGPatternFrame.cpp
>   gfxIntSize surfaceSize =
>     nsSVGUtils::ConvertToSurfaceSize(
>-      gfxSize(patternWidth * fabs(patternTransform.xx),
>-              patternHeight * fabs(patternTransform.yy)),
>+      gfxSize(patternWidth * fabs(patternTransform.xx) + patternHeight * fabs(patternTransform.xy),
>+              patternHeight * fabs(patternTransform.yy) + patternWidth * fabs(patternTransform.yx)),
>       &resultOverflows);

At first glance, I don't grok what all the math inside of the gfxSize() parens is trying to do -- what's the goal there?  (perhaps a comment would be helpful)

Comment 6

[Daniel Holbert [:dholbert]]

(If we're trying to get the bounding-box of a rect of size [patternWidth, patternHeight] after it's been transformed. maybe we should use gfxMatrix.TransformBounds()?)

Comment 7

[Robert Longson [:longsonr]]

Attachment: updated patch

TransformBounds amounts to the same thing but the calculates the transformed x and y too. Since TransformBounds is tested code it does seem better to use it.

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 649842 [details] [diff] [review]
updated patch

>--- a/layout/svg/base/src/nsSVGPatternFrame.cpp
>+++ b/layout/svg/base/src/nsSVGPatternFrame.cpp
>@@ -305,27 +305,31 @@ nsSVGPatternFrame::PaintPattern(gfxASurf
>   // Now that we have all of the necessary geometries, we can
>   // create our surface.
>   gfxFloat patternWidth = bbox.Width();
>   gfxFloat patternHeight = bbox.Height();
> 
>+  gfxRect transformedBBox = patternTransform.TransformBounds(bbox);
>+
>   bool resultOverflows;
>   gfxIntSize surfaceSize =
>     nsSVGUtils::ConvertToSurfaceSize(
>-      gfxSize(patternWidth * fabs(patternTransform.xx),
>-              patternHeight * fabs(patternTransform.yy)),
>+      gfxSize(transformedBBox.Width(), transformedBBox.Height()),
>       &resultOverflows);

Should be able to replace
  gfxSize(transformedBBox.Width(), transformedBBox.Height())
with
  transformedBBox.Size()

Also, nit: can you bump the patternWidth and patternHeight decls down lower, since we don't use them until later now?

Also, it'd be worth checking if TransformBounds() still produces the rounding-errors from comment 4 (we might not need the test-tweaks in this patch after all?)

Comment 9

[Robert Longson [:longsonr]]

Attachment: updated

We do still need the test tweaks.

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 650005 [details] [diff] [review]
updated

Looks great, thanks! r=me

Comment 11

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d65e9bb1608c

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d65e9bb1608c

Comment 13

[Jesper Hansen]

Beautiful