Last Comment Bug 780880 - patternTransform in SVG - rotating by 90 degrees not working
: patternTransform in SVG - rotating by 90 degrees not working
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla17
Assigned To: Robert Longson
:
:
Mentors:
: 786612 (view as bug list)
Depends on: 974746
Blocks: 691646 751198
  Show dependency treegraph
 
Reported: 2012-08-07 06:20 PDT by lmrspax
Modified: 2014-02-19 22:20 PST (History)
3 users (show)
longsonr: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
test.html - Three rectangles filled with same pattern rotated by diffrent degress. Last one is broken one. (1.32 KB, text/html)
2012-08-07 06:20 PDT, lmrspax
no flags Details
Simple Animated example (1.95 KB, text/html)
2012-08-07 06:36 PDT, Jesper Hansen
no flags Details
patch (5.76 KB, patch)
2012-08-07 10:50 PDT, Robert Longson
no flags Details | Diff | Splinter Review
updated patch (6.24 KB, patch)
2012-08-07 15:41 PDT, Robert Longson
no flags Details | Diff | Splinter Review
updated (6.56 KB, patch)
2012-08-08 02:21 PDT, Robert Longson
dholbert: review+
Details | Diff | Splinter Review

Description lmrspax 2012-08-07 06:20:35 PDT
Created attachment 649621 [details]
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 2012-08-07 06:36:41 PDT
Created attachment 649628 [details]
Simple Animated example
Comment 2 Jesper Hansen 2012-08-07 06:37:36 PDT
Did a quick animated example and I am not sure what exactly happens since it gets so blurry
Comment 3 Robert Longson 2012-08-07 10:50:13 PDT
Created attachment 649698 [details] [diff] [review]
patch
Comment 4 Robert Longson 2012-08-07 10:54:23 PDT
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] 2012-08-07 11:35:20 PDT
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] 2012-08-07 11:40:08 PDT
(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 2012-08-07 15:41:17 PDT
Created attachment 649842 [details] [diff] [review]
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] 2012-08-07 15:58:58 PDT
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 2012-08-08 02:21:00 PDT
Created attachment 650005 [details] [diff] [review]
updated

We do still need the test tweaks.
Comment 10 Daniel Holbert [:dholbert] 2012-08-08 08:39:20 PDT
Comment on attachment 650005 [details] [diff] [review]
updated

Looks great, thanks! r=me
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-08-09 15:02:02 PDT
https://hg.mozilla.org/mozilla-central/rev/d65e9bb1608c
Comment 13 Jesper Hansen 2012-08-16 10:04:04 PDT
Beautiful
Comment 14 Robert Longson 2012-08-29 04:17:32 PDT
*** Bug 786612 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.