Skip to content

Allow arbitrary gravity shifts when cropping#205

Closed
ghost wants to merge 1 commit into
masterfrom
unknown repository
Closed

Allow arbitrary gravity shifts when cropping#205
ghost wants to merge 1 commit into
masterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Apr 29, 2015

In an CMS type application I develop I need to determine the gravity for the image for many different crop sizes.
For example, the person's head in the photography should always be visible and the thumbnail on the main page is different then the thumbnail in the list of articles. If it is placed in the upper left corner then I would have to choose gravity "north" or "west" dependent on the aspect ratio. This is the reason why set of "centre", "north", "east", etc. is not enough.

Additionally, I am trying to add visual tool to the editor to choose the most important part of the photo (always visible), with a single click. For this purpose I need to set arbitrary value for the gravity, e.g. in units of 0-100% in horizontal and vertical dimension.

@lovell
Copy link
Copy Markdown
Owner

lovell commented Apr 29, 2015

Hello Marcin, thank you very much for your work on this enhancement.

I understand the need for additional north-east, south-east, south-west and north-west gravities. This would bring things line with *magick's -gravity flag. Perhaps these additional compass points should be exposed in the sharp.gravity object?

Are you able to provide some functional tests to help me understand what the slightly more complex variable (0.0 - 1.0) crop gravity might provide beyond the existing extract feature?

@ghost
Copy link
Copy Markdown
Author

ghost commented Apr 29, 2015

I think the gravity feature does not provide new feature beyond extract in general. You can always first extract area with desired aspect ratio, then scale. With gravity you do not have to check the size of the photo first. For this reason adding noth-east, etc. would be valuable, because it does not require a priori knowledge of aspect ratio.
In case of slightly more complex gravity with horizontal and vertical shift between 0-100% the additional benefit is smaller - but in my case I need it :) My use case:
Choose the position of the face in the portrait aspect ratio photo [http://en.wikipedia.org/wiki/Portrait#/media/File:VanGogh_1887_Selbstbildnis.jpg] - lets say 50% from left (horizontal center) 30% from top (shifted). For the crop into landscape banner (e.g. aspect ratio 4:3) gravity center could cut upper part of the head, and gravity top would leave to much empty space above and cut the lower part of the face. I want to give full control to the editor of the post, so the photo will look good anywhere in the CMS system.
Just like in the case of the simple gravity - you can always do it with extract, but it is harder. You have to check the size of the photo first.

Gravity centre:
vangogh_1887_selbstbildnis-center
Gravity top:
vangogh_1887_selbstbildnis-top

@lovell
Copy link
Copy Markdown
Owner

lovell commented Apr 29, 2015

Thanks for providing more info about "variable gravity" @ndziorlg. Your scenario is similar to #130 and your addition would seem to make more sense as an improvement to the extract method. Any thoughts @jonathanong as originator of that request?

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 93.01% when pulling 969e882 on ndziorlg:master into 783826a on lovell:master.

@lovell
Copy link
Copy Markdown
Owner

lovell commented May 20, 2015

@ndziorlg Sorry for the delay getting back to this PR and thanks again for your contributions.

Are you able to separate the features in your excellent work and, at least initially, include only the addition of new northEast, southEast, southWest and northWest attributes to the existing sharp.gravity Object, preferably with associated tests?

The next release is taking shape on the knife branch, so please use that as the PR target.

This should keep separate and therefore allow any deliberation on adding the % to crop vs extract (vs crop+extract) to continue unabated.

@martinheidegger
Copy link
Copy Markdown

👍

@lovell
Copy link
Copy Markdown
Owner

lovell commented Jun 2, 2015

Hi @martinheidegger, thanks for the +1. Are you able to provide more details of which feature(s) you're referring to as there are a couple of improvements under discussion here?

@martinheidegger
Copy link
Copy Markdown

@lovell Just read the code and really liked what I saw. Having the same properties as GraphicsMagick is a good step ahead. Since sharp's api is already quite departed from gm in other regards it doesn't hurt to me to see the percentage option open. I found the solution clean and superior, so: 👍

@lovell
Copy link
Copy Markdown
Owner

lovell commented Jun 2, 2015

@martinheidegger Thank you!

@lovell
Copy link
Copy Markdown
Owner

lovell commented Sep 26, 2015

I'm still very happy to accept a PR that adds support for north-west etc. gravities part of this PR.

Our golang sibling has just added this via DAddYE/vips@6e59422

The addition of #236 should provide enough API for the variable cropping/extracting feature.

@brandonaaron
Copy link
Copy Markdown
Contributor

Might take a look at pulling in the additional gravities soon if there still isn't movement here.

@brandonaaron
Copy link
Copy Markdown
Contributor

I briefly looked at using the approach laid out here but it wasn't passing unit tests. In favor of time, I simply extended the existing approach to support these extra gravity settings in pull request #291.

@lovell
Copy link
Copy Markdown
Owner

lovell commented Oct 27, 2015

#291 has been merged, adding support for the additional gravity values, thanks @brandonaaron.

@lovell
Copy link
Copy Markdown
Owner

lovell commented Nov 7, 2015

v0.11.4 is now available via npm and includes the additional corner gravities part of this PR.

The variable gravity idea should be made possible via #236 - please feel free to continue the discussion there.

Thanks again for your time and suggestions @ndziorlg!

@lovell lovell closed this Nov 7, 2015
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