search

openfl / svg

Provides SVG parsing and rendering for OpenFL and Haxe
MIT License
68 stars 30 forks source link

Add test automation for SVG generation #26

Closed ashes999 closed 8 years ago

ashes999 commented 8 years ago

In a long thread, starting with this post, I began to write unit tests for SVG generation. The tests take two things: an SVG, and an expected-outcome PNG, and generate (and diff) the actual, generated PNG against the expected one.

The diff is pretty simple: it looks at the average diff between pixels. If you have a pixel with RGBA=[255, 128, 64, 255] and you compare it to [0, 128, 192, 0] the diff is:

Average this across the image, and you get the "net difference." A threshold of 2% error seems good enough to account for antialiasing.

The test fails if any of the images exceed the error threshold of 2%. The failure message looks like this:

SVG generation for 2 cases did not match expectations. Check svg-tests.html to see failures.

The test generates svg-tests.html. Here's how that looks now:

results

Overall, I'm very happy with it as a foundational part of the library.

With this PR, can someone please look at the SvgGenerationTest.hx code? I didn't touch any of the library code itself (except to move the code into src, which I need to do for munit to work properly.)

I do have push access, but I want a proper code review :)

If you approve this, I can work on the Travis-CI integration as well. I want the builds to fail if someone breaks SVG generation -- we shouldn't allow the library code to get worse.

ashes999 commented 8 years ago

@jgranick are you available to review the bits that I requsted?

ashes999 commented 8 years ago

@ibilon do you think you could take a quick look at this and tell me what you think?

ibilon commented 8 years ago

Sure, the concept looks good, I'll try it and look at the code later today.

ashes999 commented 8 years ago

Thanks, I really appreciate it. The details are all in the PR description (above). To summarize, please take a look at:

Since I have push permission, I can hook this into the build once the PR is merged so that the tests run with every commit.

ibilon commented 8 years ago

Looked at it, for how the test works and the results all good. For the move to src and code I've made comments on the code here ;)

The detection seems problematic, to try it I changed all_rights_reserved_white.svg line 6 fill color to FF00FF and the output is pink instead of white, but the average pixel diff is only 0.273%.

ashes999 commented 8 years ago

Looks like my diff algorithm is not sound. I'll see what I'm doing wrong. (I thought it was your first comment about using pixel, but that doesn't seem to be enough.)

ibilon commented 8 years ago

Yeah I fixed it locally before testing.

Maybe instead of taking the average diff, using the percentage of pixels that have a diff above the threshold could work.

ashes999 commented 8 years ago

After updating, I now get an 18% diff for the all_rights_reserved image when I change the fill to #FF00FF. The diff image shows most of the image changed, and green.

If I change the algorithm to report a 100% pixel difference if anything changed, I get 49% average for the modified (pink) image instead. That's more conservative. It also reports the Ubuntu image, as-is, as 7% changed, and the correct all_rights_reserved SVG as 5% changed.

What do you think we should do?

ashes999 commented 8 years ago

I think this PR is in good order now. We just need to decide if we want to flag any changed pixel, or use the RGB values to calculate how different a pixel is.

This is mostly based on anti-aliasing differences from what I can tell.

ibilon commented 8 years ago

I feel like when averaging RGB differences there could be pixels that cancel each other, and when you are more different from the original pixel by more than x% the actual value isn't very useful, you just are different.

So I vote for the "Using any changed pixel".

And this and the thresholds are things that can be changed afterward, as long as we catch regressions it'll be good.

Also maybe it'd be better to not compare RBG values but maybe HSL where comparing individual components make more sense, also IIRC RGB isn't linear and you'd need to square the values before comparing/transforming them.

ashes999 commented 8 years ago

Sounds good to me.

I'll rebase and push.

Thanks for taking so much time to give me a sufficiently thorough code-review.

ibilon commented 8 years ago

No problem, that's a very useful pr.