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

Stroke thickness off by sqrt(2)/2 #35

Closed
jcward opened this issue Apr 28, 2016 · 10 comments
Closed

Stroke thickness off by sqrt(2)/2 #35

jcward opened this issue Apr 28, 2016 · 10 comments

Comments

@jcward
Copy link
Contributor

jcward commented Apr 28, 2016

With a simple test SVG, I'm finding that the stroke thickness is off by a factor of sqrt(2)/2 (and, less critically, the default joint type seems to be rounded instead of miter.)

Here's the test SVG:

<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<svg width="256" height="256" version="1.1">
  <rect x="0" y="0" width="256" height="256" fill="#FFFFFF"/>
  <rect x="32" y="32" width="192" height="192" style="fill:#0F0;stroke:#000;stroke-width:32;stroke-opacity:0.5"/>
  <circle cx="128" cy="128" r="32" fill="#0000FF" stroke="#000000" stroke-opacity="0.5" stroke-width="32" />
</svg>

And this is OpenFL on the left vs eog/gimp/etc on the right:

image

I can hack the extra scaling factor into SVGRenderer.hx and it fixes the issue, however, I'm not sure where this extra factor should go in the lib:

image

@ashes999
Copy link
Member

@jgranick can you please comment on this?

@jcward
Copy link
Contributor Author

jcward commented Apr 29, 2016

@jgranick @ashes999 interesting, the scale computation on line 137 is:

var scale = Math.sqrt(m.a*m.a + m.d*m.d);

It's taking the hypotenuse length of the x/y scale. But for an identity matrix, this results in 1.414 (aka, sqrt(2).) so I believe we need to divide out the sqrt(2) like so:

var scale = Math.sqrt(m.a*m.a + m.d*m.d) / 1.41421356237;

I'm going to do a quick scaling test to see how eog/gimp/etc scale strokes.

@ashes999
Copy link
Member

@jcward that bit of code was last changed in this commit, please take a look at the commit (and/or the instigating issue) and double-check that you're not re-breaking something that was fixed.

@jcward
Copy link
Contributor Author

jcward commented Apr 29, 2016

That commit was correct, scaling is the .a and .d components as noted in the Flash matrix docs:

image
image

I'm adding a testcase and creating a PR now...

@ashes999
Copy link
Member

Thanks for spending so much time on this, and for adding test cases to all your PRs. We really need to build that up to get this library more stable.

@jcward
Copy link
Contributor Author

jcward commented Apr 29, 2016

@ashes999 no problem, I want this library to work well, and testcases is how we'll get there without regressions! :)

@ashes999
Copy link
Member

@jcward if you fix one (and only one) SVG issue, I would like to suggest you pick this one. I think this is what's causing all of our existing test images to be rendered with a non-zero difference. (This also prevents us from using smaller test images, because the number of pixels changed becomes a larger percentage of the overall image...)

@jcward
Copy link
Contributor Author

jcward commented May 16, 2016

@ashes999 well, I don't know if you'll get 100% image match, but yes, even small strokes are slightly off. There is a fix in PR #36.

@ashes999
Copy link
Member

Ugh, it's been sitting in PR for a while. I don't know much about the code-base, and I could accept, but I'd rather someone more experienced takes a look at your changes.

Sorry, I know that's not very useful.

@jcward
Copy link
Contributor Author

jcward commented May 16, 2016

Yeah, no worries, probably best to get their feedback. :)

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

No branches or pull requests

2 participants