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 rotate operation for Willow - Images #52

Merged
merged 4 commits into from
Mar 8, 2019

Conversation

mrchrisadams
Copy link
Contributor

Hi there,

I'm opening a PR here, partly to start some work being able to work on issue #2901 on the main wagtail repo.

#wagtail/wagtail#2901

As I understand it at the mo, I'd need to implement rotate() transformations along the lines of:

@Image.operation
    def rotate(self, size):
        image = self.image
        return PillowImage(image.rotate(90, expand=True))

…for Pillow, and Wand, before I can call these higher up the stack from within Wagtail, in the image editing view,

Would you let me know if this is the case? If so, I'll implement the other rotate method, and start on making the necessary changes for a PR on Wagtail too.

@kaedroho
Copy link
Contributor

Hi @mrchrisadams. Yep, you just need to implement the rotate method on each image class that supports rotation.

I think we should allow the rotation and expand parameters to be specified, and these would also need to be implemented for the Wand version.

Thanks for working on this!

@mrchrisadams
Copy link
Contributor Author

Hi @kaedroho, I know it's been almost two years since I touched this PR, but I've finally found the time to implement the image rotation methods to allow for server-side rotation, in 90 degree increments.

Why just 90 degree increments

I've just chosen the 90 degree increments as this is the precursor to some work I'm doing with @simo97 on implementing a image rotation in the Wagtail UI itself, and we're aiming to just support the most common use cases for image rotation to begin with.

It turns out that Wand and Pillow handle image rotation differently, which is why I've kept the API simple - just accepting a multiple of 90 degrees for a rotation.

As far as I can tell, PIL uses a transpose to rotate an image from landscape to portrait format, and rotate to move the contents of the image.

This is different to wand, which allows for arbitrary rotations, but takes an optional background channel colour param when you have non-right angle rotations, in case you want to fill in the colour), and then resizes the box accordingly.

Conventions and compatibility

Anyway, please let me know if there's anything else you need - I've tried to to follow the conventions I saw in the repo, as this is my first PR, I may well have missed something obvious.

Also, I might need some pointers how rotation might work with the auto-orientation methods - at present, this doesn't touch exif data.

Ta,

@mrchrisadams
Copy link
Contributor Author

Actually, I've just looked over the history and I realise it's a it messy - I'll rebase locally and push a few tidier commits.

simo97 and others added 2 commits March 7, 2019 10:39
This lefts us rotate an image 90 degree increments
@mrchrisadams
Copy link
Contributor Author

Okay' that should be a tider PR now, without the messy rebase artifacts.

Can you let me know if I should add to the changelog, or is that something the maintainer does when a new release is published?

@tomdyson
Copy link

tomdyson commented Mar 7, 2019

Thanks @mrchrisadams! I've requested a review from @kaedroho.

willow/plugins/pillow.py Outdated Show resolved Hide resolved
willow/plugins/pillow.py Outdated Show resolved Hide resolved
Copy link
Contributor

@kaedroho kaedroho left a comment

Choose a reason for hiding this comment

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

This looks good to me, just a couple of minor comments.

@kaedroho
Copy link
Contributor

kaedroho commented Mar 7, 2019

I have thought about this a bit before and have some ideas for optimisations we can make that might benefit your image editing UI.

--

We could represent orientations as a class that could be added/subtracted, then provide a method that applies them all in a single pass. For example:

image.orient(image.exif_orientation + Orientation.flip_horizontal() + Orientation.rotate(90))

The Orientation class would work out the final dimensions and how to map the pixels across.

--

Both Pillow and Wand have operations which take an affine matrix and transform the image using that matrix. We can represent transformation, rotation, scale and skew (crop being a combo of transformation and scale) using matrices and combine them. This would allow us to perform the EXIF orientation, any custom rotations/cropping and the final cropping/resizing together in a single pass.

--

Ideally, Wagtail would always work from the source image and apply both the changes made by the user in the admin and the developer in the template in one go. The advantages of doing it this way are:

  • If multiple rotations were applied to the image there would be less blurring of the final result
  • It shouldn't be much slower to do it this way as multiple resize/transform operations can be merged and applied together

One disadvantage to this is that we can't merge resize/transform operations if there is a filter/effect applied in between (unless it's an effect that doesn't matter at what point it is applied, such as grayscale).

@mrchrisadams
Copy link
Contributor Author

Hey @kaedroho thanks for the extra comments on the image editing - you okay to make that a separate issue and address in a separate PR?

There's quite a lot going on there, and as I'll be at Wagtail space next week, it might be useful to be able to sketch it out in more detail ideally in person with someone more familiar with Willow than I am right now.

@kaedroho
Copy link
Contributor

kaedroho commented Mar 7, 2019

Hey @kaedroho thanks for the extra comments on the image editing - you okay to make that a separate issue and address in a separate PR?

Sure, I wasn't expecting it to be implemented in this PR. Sorry I didn't clarify that!

@kaedroho kaedroho mentioned this pull request Mar 7, 2019
@kaedroho kaedroho changed the title WIP - Add rotate operation for Willow - Images Add rotate operation for Willow - Images Mar 8, 2019
@kaedroho kaedroho merged commit acc4b7c into wagtail:master Mar 8, 2019
kaedroho added a commit that referenced this pull request Mar 8, 2019
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.

4 participants