Cropping consistency and Gravity. Allow changing aspect ratio of cropped images

[mickenorlen]

Update 7/7

See comment: #1786 (comment)

---

What is this pull request for?

Closes #1783

• Add consistency to cropping regardless if crop mask is applied or not or using srcset.

• Render cropping. Allow changing aspect ratio of cropped images in render if render_crop=true - with added gravity settings (see screenshot doc for render cropping in action).

• Gravity. The gravity of an essence_picture can now be specified in content settings (1. for devs) image properties (2. for users - if present? in settings) or directly in render (3.). It can be read with proper priority, validation, fallbacks to defaults etc through essence.gravity method. Eg. if you want to use it for aligning background images or something other than its natural use in render_cropping.

• Screenshot doc with explanations and tests of render cropping in action
  https://docs.google.com/document/d/1rRCEBYSnKczOS3HYNHFdy2s-EzY17B0G3ljp1vLMLis/edit?usp=sharing

• TODO

• Add render_gravity column to cms. I added the migration 20200701100718

• I have added tests to cover this change

• I have followed Pull Request guidelines

• I have added a detailed description into each commit message (Not so much but lots of comments in code)

class AddGravityToEssencePicture < ActiveRecord::Migration[6.0]
  def change
    add_column :alchemy_essence_pictures, :render_gravity, :string
  end
end

1. Gravity in content settings

elements.yml

# By default there's no visible change of this PR
# If no gravity is specified it won't be visible to users
# If render_crop != true gravity won't have any effect anyway
# It is only applied when the image aspect ratio changes
- name: no_gravity_visible
  type: EssencePicture

# With default gravity visible in page properties
- name: default_gravity_image
  type: EssencePicture
  settings:
    gravity: true

# With gravity visible in page properties and overwritten layout_defaults
- name: custom_gravity_image
  type: EssencePicture
  settings:
    gravity:
      size: shrink,  # default = grow
      x: left,   # default = center
      y: top   # default = center

2. Gravity in page properties

I serialized the render_gravity (string in db) as JSON so its working as a hash. Then I had to split the params in three and combine them in controller:

essence_pictures_controller.rb

def combine_gravities(essence_picture_params)
  gravity = @essence_picture.render_gravity || {}
  gravity = gravity.symbolize_keys.merge({
    size: essence_picture_params.delete(:render_gravity_size),
    x: essence_picture_params.delete(:render_gravity_x),
    y: essence_picture_params.delete(:render_gravity_y)
  })

  essence_picture_params[:render_gravity] = gravity.reject{ |_k,v| v.blank? }.presence || nil
  essence_picture_params
end

3. Gravity in render

<%= el.render(:blog_post_image, size: '600x400', render_crop: true, gravity: { size: 'shrink' }) %>

---

[codeclimate]

Method crop has 7 arguments (exceeds 4 allowed). Consider refactoring.

[codeclimate]

Method get_crop_area has 6 arguments (exceeds 4 allowed). Consider refactoring.

[tvdeyen]

Thanks. And sorry for letting you wait so long.

Unfortunately I am not sure I fully understand what this rather complex feature is meant for. I read the Google doc. Thanks for that.

Am I assuming correctly that you want to use these values to align the image as background-image and set the background-position and background-size values in CSS accordingly?

I think this is overall a good idea, but the code is very(!) complex. Without tests and more example code (especially on the frontend) I am not able to review it.

Could you provide more examples and tests?

Thanks for your contribution

[tvdeyen]

This probably needs a rebase now that we merged some of the extracted bug fixes.

[mickenorlen]

Rdy for review @tvdeyen

[tvdeyen]

@mickenorlen we switched the default branch to main. I already updated this PR, but you want to pull the main branch and rebase this branch against this as well, so you get future updates. sorry for any inconvenience and thanks for the contribution.

[mickenorlen]

@tvdeyen Force pushed a fresh single squash merge commit into main as rebase was a mess.

[mickenorlen]

I came up with a few small changes that might make this PR a bit easier for you to review. For example breaking out a new render_crop.rb module.. I'll get back to this in a couple of days.

[mickenorlen]

@tvdeyen, After refactoring most logic to alchemy/picture/render_crop.rb (and adding tons of general comments about the feature there) everything came out much cleaner. I also found some general code quality improvements. Should be easier to review now, when you have time. Thanks

[github-actions]

This pull request has not seen any activiy in a long time.
Probably because of missing tests or a necessary rebase.
This PR will be closed in 7 days if no further activity happens.

[tvdeyen]

This is still relevant and and useful feature that we would love to have, but this PR needs to be reworked from scratch since we made a lot of refactorings lately.

[tvdeyen]

Maybe this can be split up into smaller PRs? One that introduces the gravity feature and one that handles aspect ratio changes.

[tvdeyen]

I think this makes sense after we merged the support for active storage (#2288), because image_processing and lib vips have attention and other gravities build in.

[mickenorlen]

I've recently cleaned up the code behind this feature a bit as i used it in my own project.
I've actually renamed "gravity" to "focus" in my new version as i felt it was more intuitive for a user eg:

"The focus in this picture is: x: left/center/right, y: top/center/bot, zoom: in/closest/out" .

Then the developer can make render choices based on this - cropping can be one of them, background-align another etc.

In short i've made a Render::Crop class that takes a wanted size (or aspect_ratio) and changes the cropping area to fit.

class Render::Crop
  def initialize(size:, original_size:, crop_from:, crop_size:, focus:, aspect_ratio: nil)
    ...
    @crop_from = to_crop_from(crop_from) ||  { x: 0, y: 0 }
    @crop_size = to_crop_size(crop_size) || @original_size
  end

  def adjust_crop_area_to_aspect_ratio
    new_crop_size = case @focus[:zoom]
    when :out, :closest
      grow_crop_area
    when :in
      shrink_crop_area
    end

    crop_from = crop_from_after_crop_area_resize(new_crop_size)

    [crop_from, new_crop_size]
  end    
  
  ...
  
  private
  
  # The wanted aspect ratio is taken either from size or a directly from aspect ratio
  def wanted_aspect_ratio
    @wanted_aspect_ratio ||= (@aspect_ratio.presence || aspect_ratio(@size))
  end
end

One thing that was useful for me to implement related to this feature was to make picture size and aspect_ratio methods that always returns something, falling back correctly through render->render_size->settings->original_image. Then you just pass them in to the render_cropper which returns a new crop area. I even used the render_cropper to get default_crop_areas for admin thumbnails etc

Can you give me your thoughts on all this and a quick explanation of how your recent refactorings influence this feature?

[tvdeyen]

I like the idea of a render/crop focus class. Could you elaborate what the values mean? Why is zoom in and zoom out a focus related value? I would rather think of a coordinate instead of a verb/command, but I probably miss some context.

[mickenorlen]

@tvdeyen

I like the idea of a render/crop focus class. Could you elaborate what the values mean? Why is zoom in and zoom out a focus related value? I would rather think of a coordinate instead of a verb/command, but I probably miss some context.

The focus zoom property is perhaps a bit unintuitive and I'm open for suggestions for another wording.

In general focus[:zoom] is specifying the strategy to achieve the wanted aspect ratio:

• out: By zooming out, effectively "growing" the cropped image horizontally or vertically.
• in: by zooming in, effectively "shrinking" the image mask in the opposite direction compared to growing
• closest: Growing one side and shrinking the other 50/50.

With focus[:x] = left the left focus/coordinate of the cropping mask is maintained and the new mask would only grow/shrink on the right if horizontal adjustments are required. Eg if shrinking a wider image, or growing a taller one.

With focus[:y] = top the top focus/coordinate of the cropping mask is maintained and the new mask would only grow/shrink on the bottom if vertical adjustments are required. Eg: if shrinking a taller image, or growing a wider one.

I've updated the comments in the Render::Crop file so you can see the whole class here: https://pastebin.com/78ZRKutk.

In the self.crop method you can see a bit how I prepare the parameters to call the main method adjust_crop_area_to_aspect_ratio. The wanted_aspect_ratio is either derived directly from the aspect_ratio parameter or from the size parameter eg: "400x200".

[github-actions]

This pull request has not seen any activiy in a long time.
Probably because of missing tests or a necessary rebase.
This PR will be closed in 7 days if no further activity happens.

[github-actions]

This pull request has not seen any activiy in a long time.
Probably because of missing tests or a necessary rebase.
Please open a new PR to latest main if you want to continue working on this.
Thanks for the contribution.