junaidbhura / auto-cloudinary

Super simple Cloudinary auto-upload implementation for WordPress.
https://wordpress.org/plugins/auto-cloudinary/
MIT License
41 stars 9 forks source link

default crop #2

Closed petersplugins closed 6 years ago

petersplugins commented 6 years ago

Referring to https://wordpress.org/support/topic/add-option-to-ignore-size/

I digged a bit deeper into your code and found the root cause of my problem. You generally use fill for the crop. This in my opinion only fits for thumbnails but usually not for all other image sizes.

My suggestion would be to alter the function filter_image_downsize() and check if crop is used for the current image size.

I've tested the following solution and it seems to work fine:

Beginning from line 81 change

$args = array();
if ( 'full' !== $size ) {
  $args = array(
    'transform' => array(
      'width'  => $dimensions['width'],
      'height' => $dimensions['height'],
      'crop'   => apply_filters( 'cloudinary_default_crop', 'fill' ),
    ),
  );
}

to

$args = array();
if ( 'full' !== $size ) {
  $crop = 'fit';
  if ( (bool) get_option( $size . '_crop' ) ) {
    $crop = 'fill';
  }
  $args = array(
    'transform' => array(
      'width'  => $dimensions['width'],
      'height' => $dimensions['height'],
      'crop'   => apply_filters( 'cloudinary_default_crop', $crop ),
    ),
  );
}

What do you think about that?

Regards, Peter

junaidbhura commented 6 years ago

Thanks Peter. I'll have to investigate this some more to see which crop works best. In the mean time, could you share actual / real image URLs from the site you are working on, so I can see some samples of fill vs fit in your case?

petersplugins commented 6 years ago

Sorry, the site is under development, so I can't share it.

I'm using the_post_thumbnail( 'large' ); to show the thumbnail image in large size. Image size large is defined as 1024x1024. Original image size is 1200x600. This means using image size large results in an image with 1024x512. Activating your plugin changes all images to cropped 1024x1024.

One of the URLs created by your plugin e.g. is http://res.cloudinary.com/wpwissen/image/upload/w_1024,h_1024,c_fill/v1523264375/wpw-uploads/welche-plugins-brauche-ich.jpg

The changed code now gives me for the same image http://res.cloudinary.com/wpwissen/image/upload/w_1024,h_1024,c_fit/v1523264375/wpw-uploads/welche-plugins-brauche-ich.jpg - which respects the aspect ratio. This is the same result as without using your plugin.

As you can see the only difference is that crop fill was changed to fit.

The original image is http://res.cloudinary.com/wpwissen/image/upload/v1523264375/wpw-uploads/welche-plugins-brauche-ich.jpg

To test the code I've posted I've changed my theme to use the_post_thumbnail( 'thumbnail' );.

Size thumbnail is defined as 150x150 cropped, which is WP default.

The generated URL is https://res.cloudinary.com/wpwissen/w_150,h_150,c_fill/wpw-uploads/welche-plugins-brauche-ich.jpg

As you can see it uses fill because my code checks the crop option for the used image size.

Peter

petersplugins commented 6 years ago

PS:

To make the code work really perfect you should also check the $_wp_additional_image_sizes array which contains image sizes added by the add_image_size() function.

See https://codex.wordpress.org/Function_Reference/get_intermediate_image_sizes#Examples

junaidbhura commented 6 years ago

I think I understand what you mean now. The problem is not the crop, but the height parameter. Using your example:

http://res.cloudinary.com/wpwissen/image/upload/w_1024,h_1024,c_fit/v1523264375/wpw-uploads/welche-plugins-brauche-ich.jpg

and

http://res.cloudinary.com/wpwissen/image/upload/w_1024,c_fill/v1523264375/wpw-uploads/welche-plugins-brauche-ich.jpg

Both achieve the same result.

The problem occurs when:

  1. The full image is smaller than the thumbnail size being requested
  2. The thumbnail size being requested is not hard cropped

The default WordPress behavior for the above is to ignore the height and just crop based on width. So in this plugin, we should

  1. Consider the crop parameter of the thumbnail size, and see if the aspect ratio is available within the crop
  2. If the aspect ratio is available, add the height parameter in the URL, so Cloudinary can crop the image
  3. If the aspect ratio is not available, do not add the height parameter, so Cloudinary can just resize the image

Does that sound alright? 😊

junaidbhura commented 6 years ago

Hey @petersplugins do you have any thoughts on this?

junaidbhura commented 6 years ago

Hey @petersplugins I've created a new branch on this repo: https://github.com/junaidbhura/auto-cloudinary/tree/2-hard-crop

I've thought about this some more, and I think your approach of having a different crop for images with hard crop would probably be better. I've handled this a little differently. I've added a new filter called cloudinary_default_hard_crop. So now there are two filters:

cloudinary_default_hard_crop : (defaults to the value of the default crop) This is the default crop for all images that are set to "hard crop" cloudinary_default_crop : (defaults to 'fill') This is the default crop for all images

In order to be flexible, and ensure backwards compatibility for folks already using this plugin:

  1. If a person has only added cloudinary_default_crop, it will be used for both hard and soft crops
  2. If a person has added both cloudinary_default_crop and cloudinary_default_hard_crop, it will use each of them separately
  3. If a person has only added cloudinary_default_hard_crop, it will use each of them separately

Here are the relevant commits: https://github.com/junaidbhura/auto-cloudinary/commit/7ce5766edfda6d9fdf298d0d39b83548f3e5bc50, https://github.com/junaidbhura/auto-cloudinary/commit/dd5da91ec667a9e440d797bb60b624577cbd61ef

Could you test this out at your end by adding the following filters and see if it solves your problem?

add_filter( 'cloudinary_default_crop', function() {
    return 'fit';
} );

add_filter( 'cloudinary_default_hard_crop', function() {
    return 'fill';
} );

I'll have to do a full test before merging this. I'll wait for your feedback, thanks!  😊

petersplugins commented 6 years ago

@junaidbhura, please excuse me for not replying, but right now I just do not have time to dig into it. I have now quickly installed the new branch and inserted the two filters and it works as expected. Unfortunately I'm missing the time for more accurate tests at the moment but it seems to be all right. If I comment out the two filters then the images are displayed wrong again. As far as I can estimate yet the solution is perfect. Thank you for your great work!

Many greetings from Austria to Australia Peter

junaidbhura commented 6 years ago

Hey @petersplugins thanks for taking the time to test this! Greetings back from Australia 😊

I'll test this out some more at my end and merge it once it's all good!