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

Tiling angle support #1121

Merged
merged 4 commits into from
Feb 18, 2018
Merged

Tiling angle support #1121

merged 4 commits into from
Feb 18, 2018

Conversation

BiancoA
Copy link
Contributor

@BiancoA BiancoA commented Feb 9, 2018

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()

@coveralls
Copy link

coveralls commented Feb 9, 2018

Coverage Status

Coverage increased (+0.005%) to 99.244% when pulling 6857fd6 on BiancoA:tiling-angle-support into 1a4e680 on lovell:master.

Copy link
Owner

@lovell lovell left a 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");
Copy link
Owner

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.

Copy link
Contributor Author

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({
Copy link
Owner

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.)

Copy link
Contributor Author

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.

assert.strictEqual(3, info.channels);
assert.strictEqual('undefined', typeof info.size);
assertDeepZoomTiles(directory, 512, 13, done);
});
Copy link
Owner

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?

Copy link
Contributor Author

@BiancoA BiancoA Feb 15, 2018

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?

@lovell lovell added this to the v0.19.1 milestone Feb 18, 2018
@lovell lovell merged commit f86ae79 into lovell:master Feb 18, 2018
@lovell
Copy link
Owner

lovell commented Feb 18, 2018

Brilliant, thanks for the updates Andrea. This feature will be included in the forthcoming v0.19.1.

@lovell lovell mentioned this pull request Feb 18, 2018
lovell added a commit that referenced this pull request Feb 18, 2018
@BiancoA BiancoA deleted the tiling-angle-support branch March 3, 2018 14:10
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

Successfully merging this pull request may close these issues.

3 participants