-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Tiling angle support #1121
Tiling angle support #1121
Conversation
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.
Hello Andrea, thank you very much for this PR, it looks very good and will be useful to others. There's one small change this will need plus a couple of other minor comments inline.
src/pipeline.cc
Outdated
@@ -1263,6 +1264,7 @@ NAN_METHOD(pipeline) { | |||
baton->tileSize = AttrTo<uint32_t>(options, "tileSize"); | |||
baton->tileOverlap = AttrTo<uint32_t>(options, "tileOverlap"); | |||
std::string tileContainer = AttrAsStr(options, "tileContainer"); | |||
baton->tileAngle = AttrTo<uint32_t>(options, "tileAngle"); |
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.
The use of uint32_t
here will discard negative values. int32_t
should suffice.
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.
fixed! Sorry for the headless copy-paste
@@ -146,13 +146,38 @@ describe('Tile', function () { | |||
|
|||
it('Prevent larger overlap than default size', function () { | |||
assert.throws(function () { | |||
sharp().tile({overlap: 257}); | |||
sharp().tile({ |
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.
I notice there are code layout changes unrelated to this PR. Is this using prettier
? (Not a problem, more for interest.)
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.
I use atom editor and this is the result of Beautify. I like it, but If you prefer to be consistent with the other convention you were using I can revert these changes.
test/unit/tile.js
Outdated
assert.strictEqual(3, info.channels); | ||
assert.strictEqual('undefined', typeof info.size); | ||
assertDeepZoomTiles(directory, 512, 13, done); | ||
}); |
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.
I wonder if there's a reliable way to test that the tiles have actually been rotated by 90 degrees? Perhaps inspect one tile against a known fixture?
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.
the last commit may work, what do you think?
Brilliant, thanks for the updates Andrea. This feature will be included in the forthcoming v0.19.1. |
following back from issue: #1115
I exposed the angle argument for the .tile() function. It didn't really solve my problem, but it rotates the image. It looks equivalent to do .rotate() before .tile()