craftcms / cms

Build bespoke content experiences with Craft.
https://craftcms.com
Other
3.28k stars 634 forks source link

Image transforms don't include focalpoints by default #7116

Open Mosnar opened 4 years ago

Mosnar commented 4 years ago

Description

There appears to be some inconsistencies with how focalpoints are used. If I recall, focalpoints used to be applied by default anytime you asked for a URL with specific dimensions. This no longer appears to be the case (assuming it ever was). To make matters more confusing, it appears that calling getSrcset does include the focalpoint by default.

Steps to reproduce

We're using the following code to generate an image tag:

{% do image.setTransform({ width: 352, height: 172 }) %}
        {{ tag('img', {
            class: 'lazyload',
            'data-src': image.url,
            width: image.width,
            height: image.height,
            'data-srcset': image.getSrcset(['1.5x', '2x'])
        }) }}

The result is that the src has a url with a 0.5,0.5 focalpoint set; however, each of the srcsets has a focalpoint set correctly to 0.9415,0.9476.

Additional info

Mosnar commented 4 years ago

I lied, this is a different issue. Will open a new ticket if necessary.

Mosnar commented 4 years ago

I actually had it backwards originally - this issue has been updated with the actual weird behavior I was seeing.

andris-sevcenko commented 4 years ago

I'm unable to reproduce this. All the links get generated with the focal point when I try this.

Can you try with a different image and/or cleaning the caches?

Do you have any plugins installed?

Mosnar commented 4 years ago

@andris-sevcenko After some fussing with it, this does indeed appear to be an issue with Imager (not talking about Imager X).

I did some light debugging and discovered that the transform provided to the EVENT_GET_ASSET_URL event has the transform position set to "center-center" by default, therefore, Imager assumes a focalpoint should not be used. Craft on the other hand disregards "center-center" crop modes and assumes a focal point should be used. For example, using:

{% do image.setTransform({ width: 352, height: 172, position: 'center-center' }) %}
{{ image.url }}

will produce a 352x172px image with the focal point set to the hand-selected focalpoint rather than .5,.5.

https://github.com/craftcms/cms/blob/0e2e263d0cd17e1954b85f768f1127e0880cc570/src/services/AssetTransforms.php#L1566-L1582

My naive reaction is that the focalpoint should be set on the initial transform earlier on rather than the center-center default position. This would allow Craft & plugins to have a better understanding of exactly what the intentions of the transform are.

I'll leave it to you to decide if this is a bug or expected behavior.

andris-sevcenko commented 4 years ago

Craft on the other hand disregards "center-center" crop modes and assumes a focal point should be used.

Not entirely correct. In the code snippet, you linked, if the Asset has a focal point, transform positioning is disregarded, which is the correct behavior. Transforms are way more general than an asset, and, all things considered, can never in advance predict which area if any given Asset is more important. Before there were focal points, the transform position signaled how to crop an image but now it should be considered as a fallback if the Asset does not have a focal point set.

To sum up - even if your transform has, for example, "top-left" set, Asset having a focal point will override that, because the Asset has more awareness, which part of the image is important, as opposed to a general transform, that is applied to multiple images.

Does that make sense?

andris-sevcenko commented 3 years ago

@Mosnar any follow-up on this?

Mosnar commented 3 years ago

@andris-sevcenko Thanks for the explanation, that does make sense. Supposing I wanted to hook into the Assets::EVENT_GET_ASSET_URL - is there a better approach to standardizing what steps should be taken to ensure focal points behave consistently across plugins and core or should implementations always be aware that focalpoints needs to be pulled off of the source asset and merged into the transformed image?