Closed Bug 780880 Opened 8 years ago Closed 8 years ago

patternTransform in SVG - rotating by 90 degrees not working

Categories

(Core :: SVG, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla17

People

(Reporter: lmrspax, Assigned: longsonr)

References

Details

Attachments

(3 files, 2 obsolete files)

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.
Attachment #649621 - Attachment mime type: text/plain → text/html
Component: Untriaged → SVG
Product: Firefox → Core
Did a quick animated example and I am not sure what exactly happens since it gets so blurry
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: 14 Branch → Trunk
Assignee: nobody → longsonr
Blocks: 691646
Attached patch patch (obsolete) — Splinter Review
Attachment #649698 - Flags: review?(dholbert)
The double transform introduces slightly greater rounding errors so I need to increase the stroke-width of the covering circle slightly.
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)
(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()?)
Attached patch updated patch (obsolete) — Splinter Review
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.
Attachment #649698 - Attachment is obsolete: true
Attachment #649698 - Flags: review?(dholbert)
Attachment #649842 - Flags: review?(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?)
Attached patch updatedSplinter Review
We do still need the test tweaks.
Attachment #649842 - Attachment is obsolete: true
Attachment #649842 - Flags: review?(dholbert)
Attachment #650005 - Flags: review?(dholbert)
Comment on attachment 650005 [details] [diff] [review]
updated

Looks great, thanks! r=me
Attachment #650005 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/d65e9bb1608c
Flags: in-testsuite+
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/d65e9bb1608c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Beautiful
Status: RESOLVED → VERIFIED
Blocks: 751198
Duplicate of this bug: 786612
Depends on: 974746
You need to log in before you can comment on or make changes to this bug.