Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply transformation matrix to RadialGradient radiuses #6640

Merged
merged 1 commit into from
Nov 17, 2015
Merged

Apply transformation matrix to RadialGradient radiuses #6640

merged 1 commit into from
Nov 17, 2015

Conversation

dsprenkels
Copy link
Contributor

It looked like #6006 was caused by the fact that the transformation matrix of some pattern was only applied to the origin points of the two circles describing the gradient. This change makes it so that also the radii of the circles will be scaled.

Note that this commit does not fix #6296.

I have not really tested the commit very thoroughly, using the unit tests. I ran into some problems.

Fixes #6006.

@dsprenkels dsprenkels changed the title apply transformation matrix to RadialGradient radiuses Apply transformation matrix to RadialGradient radiuses Nov 16, 2015
@@ -205,6 +205,8 @@ Shadings.RadialAxial = (function RadialAxialClosure() {
if (matrix) {
p0 = Util.applyTransform(p0, matrix);
p1 = Util.applyTransform(p1, matrix);
r0 *= matrix[0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use Util.singularValueDecompose2dScale https://github.com/mozilla/pdf.js/blob/master/src/shared/util.js#L701 instead of the matrix[0]

@yurydelendik
Copy link
Contributor

Please include "eq" test file.

@Snuffleupagus
Copy link
Collaborator

The PDF file included in this PR seems to be slightly corrupt, since the xref table is missing.
(Note that PDF.js warns about this in the console, and that e.g. Adobe Reader prompts you to re-save the file on closing.)

@dsprenkels I hope you don't mind that I took the liberty of linking to a "cleaned-up" version (obtained by running the PDF file through PDFtk) of the PDF file: issue6006.pdf

@dsprenkels
Copy link
Contributor Author

@Snuffleupagus So I know for the next time: is it this or this pdftk?

@Snuffleupagus
Copy link
Collaborator

So I know for the next time: is it this or this pdftk?

I'm using the command-line version, called PDFtk Server (on Windows), since I didn't personally feel the need for a GUI.

@@ -205,6 +205,9 @@ Shadings.RadialAxial = (function RadialAxialClosure() {
if (matrix) {
p0 = Util.applyTransform(p0, matrix);
p1 = Util.applyTransform(p1, matrix);
var scale = Util.singularValueDecompose2dScale(matrix);
r0 *= scale[0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

notice that r0 can be null (see above).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perform the multiplication only for shadingType === ShadingType.RADIAL case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, that's a nasty mistake.

not only to circle origin points
fix for #6006
@timvandermeij
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/449aed77d8383fb/output.txt

@timvandermeij
Copy link
Contributor

/botio test

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/0429acb79bc1aaa/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://107.22.172.223:8877/5c498b9ef545dab/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/5c498b9ef545dab/output.txt

Total script time: 18.67 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Linux)


Success

Full output at http://107.21.233.14:8877/0429acb79bc1aaa/output.txt

Total script time: 20.05 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

@timvandermeij
Copy link
Contributor

Looks good to me. I'll leave the final review to @yurydelendik.

@yurydelendik
Copy link
Contributor

Thank you for the patch.

/botio makeref

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_makeref from @yurydelendik received. Current queue size: 0

Live output at: http://107.22.172.223:8877/0e986fa62717e8c/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

Command cmd_makeref from @yurydelendik received. Current queue size: 0

Live output at: http://107.21.233.14:8877/d7aef6f4e51de0d/output.txt

yurydelendik added a commit that referenced this pull request Nov 17, 2015
Apply transformation matrix to RadialGradient radiuses
@yurydelendik yurydelendik merged commit 2f1a626 into mozilla:master Nov 17, 2015
@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/0e986fa62717e8c/output.txt

Total script time: 18.94 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

@pdfjsbot
Copy link

From: Bot.io (Linux)


Success

Full output at http://107.21.233.14:8877/d7aef6f4e51de0d/output.txt

Total script time: 19.54 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

@dsprenkels dsprenkels deleted the issue-6006-radial-gradient-size branch November 19, 2015 08:52
@dsprenkels dsprenkels restored the issue-6006-radial-gradient-size branch November 20, 2015 14:17
@dsprenkels dsprenkels deleted the issue-6006-radial-gradient-size branch November 20, 2015 14:17
@dsprenkels dsprenkels restored the issue-6006-radial-gradient-size branch November 20, 2015 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Radial shading (type 3) too small Radial gradient
5 participants