Fix case where alpha value is an exponent #29
Conversation
From error,
```
! Exception ocurred when performing image diff for tiedChords-ipad-pro-portrait@1-p1
[Error: Expected `image-diff's stderr` to contain 'all' but received "/tmp/tmp-76818mswa0g.png PNG 1024x1366 1024x1366+0+0 8-bit DirectClass 14.2KB 0.010u 0:00.019
/tmp/tmp-76818m6035c.png PNG 1024x1366 1024x1366+0+0 8-bit DirectClass 14.2KB 0.010u 0:00.019
Image: /tmp/tmp-76818mswa0g.png
Channel distortion: RMSE
gray: 0.532272 (8.12195e-06)
alpha: 0 (0)
all: 0.460961 (7.03381e-06)
/tmp/tmp-76818mswa0g.png=>artifacts/diff/features/tie/[email protected] PNG 1024x1366 1024x1366+0+0 8-bit PseudoClass 3c 0.150u 0:00.149
"]
```
|
Can we add a test case for this comment? Maybe we should break out the extraction method so we can test it more independently (i.e. without needing an image) |
| @@ -79,7 +79,7 @@ ImageDiff.createDiff = function (options, cb) { | |||
| // DEV: According to http://www.imagemagick.org/discourse-server/viewtopic.php?f=1&t=17284 | |||
There was a problem hiding this comment.
Can we update the comment in the previous line to include the E/e format?
// Attempt to find variant between 'all: 0 (0)' or 'all: 40131.8 (0.612372)'
|
Do you want a test with two images that are so similar to cause this scenario? |
|
Nah, I imagine that would require very large images to be stored in the repo. We should relocate the |
|
It’s certainly possible to add these tests. Although Imagemagick is pretty bad for maintaining compatibility, so it might not be the best use of time. |
|
Yea, but this will also put us in a more flexible state for the future. I am going to open a secondary PR to add that functionality. This PR looks good for now. |
| @@ -79,7 +79,7 @@ ImageDiff.createDiff = function (options, cb) { | |||
| // DEV: According to http://www.imagemagick.org/discourse-server/viewtopic.php?f=1&t=17284 | |||
| // DEV: These values are the total square root mean square (RMSE) pixel difference across all pixels and its percentage | |||
| // TODO: This is not very multi-lengual =( | |||
| var resultInfo = stderr.match(/all: (\d+\.?\d*) \((\d+\.?\d*)\)/); | |||
| var resultInfo = stderr.match(/all: (\d+(?:\.\d+)?) \((\d+(?:\.\d+)?(?:[Ee]-?\d+)?)\)/); | |||
There was a problem hiding this comment.
This regexp grew a lot in complexity. Why are we moving to ?: for some of the groups?
There was a problem hiding this comment.
Ah, I see. We are trying to be more accurate with the matches.
There was a problem hiding this comment.
For extra safety, I am going to copy over the Ee- logic to the first half as well.
|
I have opened up #30 which brings this PR over the finish line and ready for landing. |
|
Sorry for the delay. Thanks! |
|
Woot, thanks @mlmorg =) |
From error,