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

[Request] `ratio` option for asset transforms #5288

Closed
proimage opened this issue Dec 1, 2019 · 7 comments
Closed

[Request] `ratio` option for asset transforms #5288

proimage opened this issue Dec 1, 2019 · 7 comments

Comments

@proimage
Copy link

@proimage proimage commented Dec 1, 2019

When creating image transforms at the template level (dunno about in the backend; I never use those), I often find myself taking min(...) precautions to prevent image upscaling:

{% set large = {
   width: min(img.width, 1920)
} %}

However, this means that the resulting image width is no longer a known factor when coding the template. It might be 1920px, but if a smaller source image is uploaded, it might be 560px or something. In this specific example, that's "fine", more or less... CSS would be used to ensure that the resulting image is shown at the right size, regardless of its native resolution.

However, what if we need to control the aspect ratio of the resulting image? Consider this example:

{% set wallpaperFullHd = {
   width: min(img.width, 1920),
   height: min(img.height, 1080)
} %}

As long as the source image is larger than 1920x1080, this is still fine. However, what if the source image is something like 1600x1200? The resulting image would be 1600x1080, which is eminently not the aspect ratio we wanted.

Proposed solution: add ratio (eg. ratio: 16/9 or something) and upscale: true/false parameters to image transforms:

{% set wallpaperFullHd = {
   width: 1920,
   height: 1080,
   ratio: 16/9,
   upscale: false
} %}
@brandonkelly brandonkelly added this to the 3.4 milestone Dec 1, 2019
@brandonkelly brandonkelly changed the title [Request] Add additional parameters to image transforms [Request] `ratio` option for asset transforms Dec 1, 2019
@brandonkelly

This comment has been minimized.

Copy link
Member

@brandonkelly brandonkelly commented Dec 1, 2019

Preventing upscaling is already covered by #844, so updated the title to keep this FR focused on the ratio idea.

@brandonkelly

This comment has been minimized.

Copy link
Member

@brandonkelly brandonkelly commented Dec 4, 2019

After talking this over, we’ve realized that A) avoiding upscaling images should be the default behavior; and B) when that happens, maintaining the crop’s aspect ratio would be a must as well. So going to close this issue now, since it will be part of the same effort as #844.

@proimage

This comment has been minimized.

Copy link
Author

@proimage proimage commented Dec 5, 2019

Hmm... I think there's still definitely a use-case for explicit aspect-ratio control, entirely decoupled from the original image's native aspect ratio.

For example, I may want to provide a poster image for a <video>, one which should always be 16:9 to match the majority of videos, regardless of what the ratio of the uploaded image is. Sure, you can calculate for each image width what the corresponding 16:9 ratio height would be, or you can just toss up a ratio: 16/9 line and be done with it. :)

So by all means, do make no-upscaling and aspect-ratio-maintaining the defaults, for sure, but there's still a use for explicit control.

@brandonkelly

This comment has been minimized.

Copy link
Member

@brandonkelly brandonkelly commented Dec 5, 2019

I think you misunderstood – we will use the transform’s width and height params to determine the ratio that should be applied, not the source image’s. See the diagram here. So in your above example, if the transform params were {width: 1600, height: 900}, and a 500⨉500 image were uploaded, it would end up getting cropped to 500⨉281 to match the 16⨉9 aspect ratio of the transform params.

@brandonkelly brandonkelly removed this from the 3.4 milestone Dec 6, 2019
@proimage

This comment has been minimized.

Copy link
Author

@proimage proimage commented Dec 10, 2019

Ok, but what about when you have an N+1 situation, such as when generating multiple sizes for responsive images? Perhaps if we look at a realistic use-case, it will be more apparent.

Say I'm building a site with modular blocks. In the image block, there's a dropdown to control the shape of the image on the frontend, with "Portrait", "Square", "Landscape", and "Leave as-is" as the options.

Performance is important, so in the template, we're not just creating a single transform to get the desired dimensions—we're outputting what I would consider to be a bare-minimum of four image transforms to get a responsive srcset="..." attribute.

Here's the shortest/cleanest/DRYest I can figure out according to the solution you proposed:

{% if block.imageField|length %}
	{% set img = block.imageField.one() %}
	{% set smallWidth, medWidth, largeWidth, xlWidth = 512, 768, 1024, 1600 %}

	{% switch block.imageShape %}
	
		{% case "portrait" %}
			{% set ratio = 9 * 16 %}
			{% set smallHeight = smallWidth / ratio %}
			{% set medHeight = medWidth / ratio %}
			{% set largeHeight = largeWidth / ratio %}
			{% set xlHeight = xlWidth / ratio %}
	
		{% case "square" %}
			{% set smallHeight = smallWidth %}
			{% set medHeight = medWidth %}
			{% set largeHeight = largeWidth %}
			{% set xlHeight = xlWidth %}
	
		{% case "landscape" %}
			{% set ratio = 16 * 9 %}
			{% set smallHeight = smallWidth / ratio %}
			{% set medHeight = medWidth / ratio %}
			{% set largeHeight = largeWidth / ratio %}
			{% set xlHeight = xlWidth / ratio %}
	
		{% case "leaveAsIs" %}
			{% set ratio = null %}
			{% set smallHeight = ratio %}
			{% set medHeight = ratio %}
			{% set largeHeight = ratio %}
			{% set xlHeight = ratio %}		
	
	{% endswitch %}
	
	{% set small = {
		width: smallWidth,
		height: smallHeight
	} %}
	{% set medium = {
		width: medWidth,
		height: medHeight
	} %}
	{% set large = {
		width: largeWidth,
		height: largeHeight
	} %}
	{% set xlarge = {
		width: xlWidth,
		height: xlHeight
	} %}

	{% set srcset %}
		{{- img.url(small) }} {{ img.width(small) }}w,
		{{- img.url(medium) }} {{ img.width(medium) }}w,
		{{- img.url(large) }} {{ img.width(large) }}w,
		{{- img.url(xlarge) }} {{ img.width(xlarge) }}w
	{%- endset %}

	<img srcset="{{ srcset }}">

{% endif %}

And here is the version of the above if we have explicit control over the ratio:

{% if block.imageField|length %}
	{% set img = block.imageField.one() %}
	{% set smallWidth, medWidth, largeWidth, xlWidth = 512, 768, 1024, 1600 %}

	{% switch block.imageShape %}
	
		{% case "portrait" %}
			{% set ratio = 9 * 16 %}
	
		{% case "square" %}
			{% set ratio = 1 %}
	
		{% case "landscape" %}
			{% set ratio = 16 * 9 %}
	
		{% case "leaveAsIs" %}
			{% set ratio = null %}
	
	{% endswitch %}
	
	{% set small = {
		width: smallWidth,
		ratio: ratio
	} %}
	{% set medium = {
		width: medWidth,
		ratio: ratio
	} %}
	{% set large = {
		width: largeWidth,
		ratio: ratio
	} %}
	{% set xlarge = {
		width: xlWidth,
		ratio: ratio
	} %}

	{% set srcset %}
		{{- img.url(small) }} {{ img.width(small) }}w,
		{{- img.url(medium) }} {{ img.width(medium) }}w,
		{{- img.url(large) }} {{ img.width(large) }}w,
		{{- img.url(xlarge) }} {{ img.width(xlarge) }}w
	{%- endset %}

	<img srcset="{{ srcset }}">

{% endif %}
@brandonkelly

This comment has been minimized.

Copy link
Member

@brandonkelly brandonkelly commented Dec 10, 2019

Not sure that extra complexity would be needed. This should also work.

{% if block.imageField|length %}
    {% set img = block.imageField.one() %}
    {% set smallWidth, medWidth, largeWidth, xlWidth = 512, 768, 1024, 1600 %}

    {% switch block.imageShape %}
    
        {% case "portrait" %}
            {% set ratio = 16 / 9 %}
    
        {% case "square" %}
            {% set ratio = 1 %}
    
        {% case "landscape" %}
            {% set ratio = 9 / 16 %}
    
        {% case "leaveAsIs" %}
            {% set ratio = null %}
    
    {% endswitch %}
    
    {% set small = {
        width: smallWidth,
        height: ratio ? smallWidth * ratio
    } %}
    {% set medium = {
        width: medWidth,
        height: ratio ? medWidth * ratio
    } %}
    {% set large = {
        width: largeWidth,
        height: ratio ? largeWidth * ratio
    } %}
    {% set xlarge = {
        width: xlWidth,
        height: ratio ? xlWidth * ratio
    } %}

    {% set srcset %}
        {{- img.url(small) }} {{ img.width(small) }}w,
        {{- img.url(medium) }} {{ img.width(medium) }}w,
        {{- img.url(large) }} {{ img.width(large) }}w,
        {{- img.url(xlarge) }} {{ img.width(xlarge) }}w
    {%- endset %}

    <img srcset="{{ srcset }}">

{% endif %}
@proimage

This comment has been minimized.

Copy link
Author

@proimage proimage commented Dec 11, 2019

...I bow before thy superior Twig-bending powers. ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.