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

Fix stroke width in for lines in rotated transform spaces #36

Merged
merged 4 commits into from
Apr 21, 2022

Conversation

jonmccracken-wf
Copy link
Contributor

Hi! @larrylynn-wf directed me here. I've been debugging some line width fidelity issues in one of our products and have tracked the issue down to applyStroke().

Currently, line widths in canvases with a rotation transform will vary based on the angle of the line. I think this is because applyStroke only takes the transform's X scale into account when calculating line width. This results in rotated lines becoming progressively thinner until they eventually become zero-width lines at 90 / 270 degrees. Here's an SVG illustrating the problem: all of the lines are the same width in the original SVG, but their lineWidth in the exported PDF changes based on the rotation of the canvas.

There's a related issue with dashed lines being drawn in with a rotation transform. This can corrupt the PDF. See this SVG for more info.

To fix it, I'm representing the stroke width / dashes as an imaginary horizontal vector with magnitude == BasicStroke.LineWidth. I'm then using the transform to to transform that vector into the current transform space and calculating the length of the result. This new vector length is the effective line width or dash length under the current transform.

I ran through the output of RenderSVGsTest and compared it to current master and wasn't able to find any fidelity differences. I also ran the fix through our fidelity suite and was able to verify that there weren't any differences.

Thank you for all your work on this project!

-- Jon

// Apply the current transform to the horizontal line.
tf.deltaTransform(lengthVector, lengthVector);
// Calculate the length of the transformed line. This is the new, adjusted length.
return (float)(Math.sqrt(lengthVector.x * lengthVector.x + lengthVector.y * lengthVector.y));
Copy link

Choose a reason for hiding this comment

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

UnnecessaryParentheses: These grouping parentheses are unnecessary; it is unlikely the code will be misinterpreted without them (details)

Suggested change
return (float)(Math.sqrt(lengthVector.x * lengthVector.x + lengthVector.y * lengthVector.y));
return (float) Math.sqrt(lengthVector.x * lengthVector.x + lengthVector.y * lengthVector.y);

(at-me in a reply with help or ignore)


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

rototor added a commit that referenced this pull request Apr 21, 2022
It shows that the line width in fact is not correct if there is a
rotation transform on the Graphics2D.
@rototor
Copy link
Owner

rototor commented Apr 21, 2022

Wow, nice catch. I was able to reproduce this using the Graphics2D API. See ba65fd5.

This is how it looks when drawing to a bitmap

image

And in the PDF it is skewed as you said:

image

Your fix looks sane. I'll merge and retest.

@rototor rototor merged commit bcf8b8f into rototor:master Apr 21, 2022
@rototor
Copy link
Owner

rototor commented Apr 21, 2022

Thanks, you fix works fine. I would like to wait for the PDFBox 2.0.26 release before making a new release here. The vote for the PDFBox release has finished yesterday, so the new version of PDFBox should be released tomorrow.

@rototor
Copy link
Owner

rototor commented Apr 22, 2022

I've just released version 0.38 with your fix.

@jonmccracken-wf
Copy link
Contributor Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants