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

Add support for vips_rotate. #1385

Merged
merged 1 commit into from
Sep 27, 2018
Merged

Add support for vips_rotate. #1385

merged 1 commit into from
Sep 27, 2018

Conversation

freezy
Copy link
Contributor

@freezy freezy commented Sep 24, 2018

This PR adds support for arbitrary rotation angles.

Instead of only being able to rotate multiples of 90 degrees, any angle can now be applied, using libvips' vips_rotate API, introduced in the recently released v8.7.0.

The JavaScript API stays the same, however a new optional background parameter can be provided to set the background color of the rotated image.

The new code is covered in tests, let me know if that's enough or more test cases are needed. For the documentation I'm not sure why it seems to have generated additional documentation for extend, extract and trim, maybe those weren't generated before?

This PR closes #998.

@freezy freezy force-pushed the rotate branch 2 times, most recently from d9ba5cf to 6b49d7d Compare September 24, 2018 09:59
@lovell
Copy link
Owner

lovell commented Sep 24, 2018

Thank you - I should have some time tomorrow to take a proper look at this.

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.

Thanks again for this, I've made a few comments inline. It looks like you'll need to rebase against upstream/teeth then run npm run docs to remove the extraneous changes that have crept into docs/api-operation.md.

lib/operation.js Outdated
} else {
throw new Error('Unsupported angle: angle must be a positive/negative multiple of 90 ' + angle);
throw new Error('Unsupported angle: angle must be a positive or negative number.');
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this now be reduced to angle must be a number?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup!

@@ -108,6 +108,8 @@ const Sharp = function (input, options) {
embed: 0,
useExifOrientation: false,
angle: 0,
rotationAngle: 0,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if it was intentional but using a separate parameter here has the advantage of allowing both EXIF auto-rotation and a custom angle rotation in the same pipeline.

sharp(input)
  .rotate() // auto-rotate based on EXIF, then
  .rotate(42) // rotate to specific angle
  ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually no, that wasn't intentional :)

But sounds good to me. Keep it?

@@ -45,6 +83,17 @@ describe('Rotation', function () {
});
});

[-3750, -510, -150, 30, 390, 3630].forEach(function (angle) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great test scenario, thank you.

lib/operation.js Outdated
if (!is.defined(angle)) {
this.options.useExifOrientation = true;
} else if (is.integer(angle) && !(angle % 90)) {
this.options.angle = angle;
} else if (is.number(angle)) {
this.options.rotationAngle = angle;
if (options && options.background) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an is.object check available that could be used here.

lib/operation.js Outdated
* For example, `-450` will produce a 270deg rotation.
*
* If a non-rectangular angle is provided, the color of the background color
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the mathematical term would be "non-quadrantal angle" but maybe "angles that are not multiple of 90" would be better understood.

@freezy
Copy link
Contributor Author

freezy commented Sep 25, 2018

Thanks for the review, pushed an update with your suggestions.

@lovell lovell merged commit 796738d into lovell:teeth Sep 27, 2018
@lovell
Copy link
Owner

lovell commented Sep 27, 2018

Marvellous, thank you very much, this will be included in sharp v0.21.0.

@lovell lovell added this to the v0.21.0 milestone Sep 27, 2018
@freezy
Copy link
Contributor Author

freezy commented Sep 27, 2018

Wonderful, cheers!

lovell added a commit that referenced this pull request Sep 27, 2018
@benoj
Copy link

benoj commented Sep 30, 2018

Marvellous, thank you very much, this will be included in sharp v0.21.0.

When is v0.21.0 scheduled to be released?

@lovell
Copy link
Owner

lovell commented Sep 30, 2018

@benoj You can track the progress of everything planned for inclusion in v0.21.0 at https://github.com/lovell/sharp/milestone/40

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants