servdhost / craft-asset-storage

MIT License
10 stars 1 forks source link

Imager-X adapter bug when transform mode is "fit" #76

Closed johndwells closed 7 months ago

johndwells commented 8 months ago

We have a site using Imager-X to transform images. We have one particular transform rule that defines width & height, and mode "fit".

The transformed image downscales to be less than the rule dimensions.

To replicate:

Take an image, say 400px wide by 300px tall.

Transform it to:

When we use craft (locally) as the Imager-X transformer, the transformed image is 200px wide by 150px tall as expected.

Similarly, if we transform the image without using Imager-X, and just use craft native transforms, the result image is again as expected, 200px wide by 150px tall.

However when we use servd as the Imager-X transformer, the transformed image is 150px wide by 113px tall.

I'm xdebugging and best I can tell it's occurring around here: https://github.com/servdhost/craft-asset-storage/blob/craft-4/src/AssetsPlatform/ImageTransforms.php#L140-L151

I have tried a few different ways to set upscale on that transform model, but nothing I do seems to take.

Craft version: 4.7.3 Servd Asset Storage version: 3.5.10 Imager-X version: 4.3.1

mattgrayisok commented 8 months ago

Hey John. I'm struggling to recreate this. There is some tidying up I've noticed whilst looking (the preventUpscaling logic is being applied twice for ImagerX transforms) but it shouldn't change the behaviour.

The linked code has a couple of ifs() that should never be true when the transform options are smaller than the source asset dimensions:

$targetWidth > $asset->width

$targetHeight > $asset->height

So that code block shouldn't be able to do anything given the description, but if you could supply me with the following I'll have another go at recreating to see if I can see where the bug is:

  1. The config from config/imager-x.php
  2. The code being used in twig to output the URL
  3. The resulting Servd asset platform URL which is dumped into the HTML
  4. The source asset's dimensions
mattgrayisok commented 8 months ago

I've had a closer look into how we're managing disabled upscaling and have decided to make some changes to push the upscaling decision and associated calculations out to our asset platform. Removing the code you highlighted entirely.

There's a couple of updates for us to push out, but once they're released, if upscaling is disabled the Servd plugin will switch to using fit=min instead of fit=crop (former has no upscaling) and then pass along the originally requested width and height. The asset platform will then return an image which is sized appropriately.

mattgrayisok commented 7 months ago

This should now be resolved. Upscale=false calculations have now been passed over to the asset platform instead of being calculated by our plugin and the asset platform has been updated to fully support 'min', 'max' and 'fillmax' fit modes as per Imgix's implementation. Our Imager integration has also been updated to convert Imager's transform params into these new fit modes.

Released in 3.5.13 and 4.0.3