#15989 closed defect (bug) (fixed)
image_resize_dimensions() mistakenly defaults crop center as image's center of gravity.
Reported by: | jacof | Owned by: | nacin |
---|---|---|---|
Milestone: | 3.4 | Priority: | normal |
Severity: | major | Version: | 3.1 |
Component: | Post Thumbnails | Keywords: | has-patch commit |
Focuses: | Cc: |
Description
WordPress add_image_size() routine calls ultimately for image_resize_dimensions(), which calculates the cropped image dimensions. (http://core.trac.wordpress.org/browser/tags/3.0.3/wp-includes/media.php#L333)
The function assumes that the cropping center is always the gravity center of the image, whereas the user must be able to select where and how the cropping will be made.
$s_x = floor( ($orig_w - $crop_w) / 2 ); $s_y = floor( ($orig_h - $crop_h) / 2 );
Due to this, custom thumbnails result cropped around wrong centers.
WordPress' Editor inside the Multimedia dialog doesn't affect post thumbnails defined by the user's theme, but only the thumbnails defined in the admin, as it doesn't regenerate them after rescaling the image. A temporary fix is regenerating all of the thumbnails using a third party plugin ("Regenerate Post Thumbnails"). This temporary fix generates (#Thumbs)*(#Images)*(#Resizes)+(#Images) in the Server, which is over demanding.
Fixing the Wordpress Editor to affect post-thumbnails (as it should) would not be a proper fix, since the user must not be constrained to reducing the dimensions of his/her image as a whole in order to produce a correct thumbnail.
The two solutions feasable for this are:
1- Correct the Wordpress Multimedia Editor to regenerate thumbnails after rescaling the image (short-term).
2- Modify image_resize_dimensions() to accept optional $x=0, $y=0 and allowing, ultimately, the selection of those values in the Multimedia Editor by the user, for each image, and for each thumbnail.
Attachments (11)
Change History (35)
#2
@
14 years ago
It would.
Wordpress should at least allow people to hook into this function. The current coding does not allow any correction from the users.
A 3rd way to solve this has come to my mind:
3- Correct Multimedia Editor so a user can choose where to place his thumbnails, internally resize the image accordingly, and regenerating the thumbnail. This method involves no need for changing the image_resize_dimensions() function, and it would be possible to do with a plugin.
#4
@
14 years ago
- Milestone changed from Awaiting Review to Future Release
- Owner set to markjaquith
- Status changed from new to assigned
This would be fixed if we had just-in-time resizing, which would limit you only the image sizes you use.
#7
@
13 years ago
Hi there, this is my first comment and patch!
If it is for image_resize_dimensions to use a filter, why not having it affect the whole output, like on image_downsize?
Implementation on the patch above.
#8
@
13 years ago
We are using the thumbnail API to generate our thumbnails from portrait images. When we noticed the cropping around the center was incorrectly causing the faces of the subject of the photograph to be lost, we investigated the API to see if there was any way to change the center from which the crop was taken. On searching trac, we came across this ticket and it would make a lot of sense for us to use a filter such as the one in the patch, instead of implementing/adding a thumbnail plugin just so we can crop from the top instead of the center.
Is there a plan to include this in core soon? Are there any steps our team can take to help move this along and have it included in core?
Thanks!
#10
@
13 years ago
- Milestone changed from Future Release to 3.4
- Resolution set to fixed
- Status changed from assigned to closed
I like the approach of the patch.
#11
@
13 years ago
- Keywords dev-feedback 2nd-opinion removed
- Resolution fixed deleted
- Status changed from closed to reopened
I like the approach of the patch.
But this doesn't resolve the ticket. :)
#12
@
13 years ago
- Owner changed from markjaquith to nacin
- Status changed from reopened to accepted
Fat-fingered it.
#15
follow-ups:
↓ 16
↓ 17
@
13 years ago
- Keywords commit added
I would only suggest that we move the filter above the two return false's.
#16
in reply to:
↑ 15
@
13 years ago
Replying to nacin:
I would only suggest that we move the filter above the two return false's.
Done in 15989.patch.
#17
in reply to:
↑ 15
@
13 years ago
Replying to nacin and SergeyBiryukov:
I would only suggest that we move the filter above the two return false's.
I would vote against this suggestion for two reasons:
- The two return false's purpose is to validate the input of the function. I believe that input should be validated before reaching the filtering chain. Is there any case that negative numbers are valid input for original or destination image sizes?
- As input validation before filter chaining is already implemented on other cases, like image_downsize, shouldn't we do the same to provide a consistent API behaviour?
Based on the above, I would insist on the media.php.patch patch.
#18
follow-up:
↓ 19
@
13 years ago
You are correct.
The only thing I would change is the default value for the filter should be null. That would allow false to be returned (which is a valid return value for image_resize_dimensions).
#19
in reply to:
↑ 18
@
13 years ago
Replying to nacin:
The only thing I would change is the default value for the filter should be null. That would allow false to be returned (which is a valid return value for image_resize_dimensions).
The media.php.patch modified as requested.
#20
follow-up:
↓ 22
@
13 years ago
In the future, please do not replace existing patches, only upload new ones. It makes it difficult to follow the ticket and figure out why things progressed the way they did. Thanks!
#22
in reply to:
↑ 20
;
follow-up:
↓ 23
@
13 years ago
Replying to nacin:
In the future, please do not replace existing patches, only upload new ones.
Sorry, still a noob here...
Since this patch is now official I wrote the relevant documentation, including an example moving the cropping center to the top of the image.
#23
in reply to:
↑ 22
;
follow-up:
↓ 24
@
13 years ago
Replying to ebababi:
Since this patch is now official I wrote the relevant documentation, including an example moving the cropping center to the top of the image.
Great. It might be good to have the page reflect that it requires WordPress 3.4, though. Not sure the best way to do that on the Codex.
#24
in reply to:
↑ 23
@
13 years ago
Replying to nacin:
Great. It might be good to have the page reflect that it requires WordPress 3.4, though. Not sure the best way to do that on the Codex.
I updated the documentation to reflect the version requirement in the first sentence (it should be obvious).
I was looking for a filter to modify this a few months ago. None exists. It'd be nice if one did.