-
Notifications
You must be signed in to change notification settings - Fork 94
Moved from crop to extent for filling images #34
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
// Load in our dependencies | ||
var assert = require('assert'); | ||
var fs = require('fs'); | ||
var path = require('path'); | ||
var async = require('async'); | ||
|
@@ -8,25 +9,25 @@ var mkdirp = require('mkdirp'); | |
var tmp = require('tmp'); | ||
|
||
// Define custom resize function | ||
// Taken from https://github.com/twolfson/twolfson.com/blob/3.4.0/test/perceptual-tests/twolfson.com_test.js#L88-L107 | ||
// TODO: Make image resizing its own library | ||
// DEV: This does not pollute gm.prototype | ||
gm.prototype.fillFromTo = function (params) { | ||
function transparentExtent(gm, params) { | ||
// Assert we received our parameters | ||
assert.notEqual(params.width, undefined); | ||
assert.notEqual(params.height, undefined); | ||
|
||
// Fill in new space with white background | ||
// TODO: Parameterize background color (should be considered 'transparent' color in this case) | ||
this.borderColor('transparent'); | ||
this.border(Math.max(params.toWidth - params.fromWidth, 0), Math.max(params.toHeight - params.fromHeight, 0)); | ||
gm.background('transparent'); | ||
|
||
// Anchor image to upper-left | ||
// TODO: Parameterize anchor point | ||
this.gravity('SouthEast'); | ||
gm.gravity('NorthWest'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't NorthWest the default value, so we may not have to set it explicitly? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, it's the default but I prefer to be explicit and we could parameterize it easily. For example, in the following images, what if someone wanted the smaller one to move to the lower right when upscaled? https://github.com/uber/image-diff/blob/1.3.0/test/test-files/checkerboard.png https://github.com/uber/image-diff/blob/1.3.0/test/test-files/checkerboard-excess.png Note: Currently for gravity, it will make the 2 images look the same except there will be a transparent edge for the smaller one There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool! |
||
|
||
// Specify new image size | ||
this.crop(params.toWidth, params.toHeight, 0, 0); | ||
gm.extent(params.width, params.height); | ||
|
||
// Return this instance | ||
return this; | ||
}; | ||
// Return gm instance for a fluent interface | ||
return gm; | ||
} | ||
|
||
function ImageDiff() { | ||
} | ||
|
@@ -150,11 +151,9 @@ ImageDiff.prototype = { | |
|
||
// Otherwise, resize the image | ||
actualTmpPath = filepath; | ||
gm(actualPath).fillFromTo({ | ||
fromWidth: actualSize.width, | ||
fromHeight: actualSize.height, | ||
toWidth: maxWidth, | ||
toHeight: maxHeight | ||
transparentExtent(gm(actualPath), { | ||
width: maxWidth, | ||
height: maxHeight | ||
}).write(actualTmpPath, cb); | ||
}); | ||
}, | ||
|
@@ -171,11 +170,9 @@ ImageDiff.prototype = { | |
gm(maxWidth, maxHeight, 'transparent').write(expectedTmpPath, cb); | ||
// Otherwise, resize the image | ||
} else { | ||
gm(expectedPath).fillFromTo({ | ||
fromWidth: expectedSize.width, | ||
fromHeight: expectedSize.height, | ||
toWidth: maxWidth, | ||
toHeight: maxHeight | ||
transparentExtent(gm(expectedPath), { | ||
width: maxWidth, | ||
height: maxHeight | ||
}).write(expectedTmpPath, cb); | ||
} | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not more readable error messages? These parameters are read in using input from the user (eventually) so there's a chance the consuming code could have erroneous input. An assert doesn't really explain the issue all that well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These assertions are for preventing typos from developers. There's not really a case where the error would come from user error:
ENOENT
error during the earlierfs.stat
/fs.exists
call and we won't be upscaling the imageheight
/width
fromgm.size
so if those don't exist, then we have a deeper problem ingm
itselfThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 gotcha