patternTransform in SVG - rotating by 90 degrees not working

VERIFIED FIXED in mozilla17

Status

()

Core
SVG
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: lmrspax, Assigned: Robert Longson)

Tracking

Trunk
mozilla17
x86_64
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.

Updated

5 years ago
Attachment #649621 - Attachment mime type: text/plain → text/html
(Reporter)

Updated

5 years ago
Component: Untriaged → SVG
Product: Firefox → Core

Comment 1

5 years ago
Created attachment 649628 [details]
Simple Animated example

Comment 2

5 years ago
Did a quick animated example and I am not sure what exactly happens since it gets so blurry
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

5 years ago
Version: 14 Branch → Trunk
(Assignee)

Updated

5 years ago
Assignee: nobody → longsonr
(Assignee)

Updated

5 years ago
Blocks: 691646
(Assignee)

Comment 3

5 years ago
Created attachment 649698 [details] [diff] [review]
patch
Attachment #649698 - Flags: review?(dholbert)
(Assignee)

Comment 4

5 years ago
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()?)
(Assignee)

Comment 7

5 years ago
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.
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?)
(Assignee)

Comment 9

5 years ago
Created attachment 650005 [details] [diff] [review]
updated

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+
(Assignee)

Comment 11

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 13

5 years ago
Beautiful
Status: RESOLVED → VERIFIED
(Assignee)

Updated

5 years ago
Blocks: 751198
(Assignee)

Updated

5 years ago
Duplicate of this bug: 786612

Updated

3 years ago
Depends on: 974746
You need to log in before you can comment on or make changes to this bug.