spatie / statamic-responsive-images

Responsive images for Statamic 3
MIT License
104 stars 29 forks source link

Image not regenerated when moving focal point #126

Closed hgrimelid closed 2 years ago

hgrimelid commented 2 years ago

Using a fresh installation with Statamic 3.3.9 and Statamic responive images 2.11.2.

I upload an image, then set focal point via asset editor and saving.

When I reload the page with the image (included via the responsive tag), I expect the image to be updated with the new focal point, but that don't happen until I run php please glide:clear. Is this intended behaviour, or is there something I can do to make the image get regenerated after saved/updated? My assets config is the default, with 'cache' => false,

Using the Glide tag the image is regenerated as expected when reloading page.

josephrlee commented 2 years ago

I'm also running into this issue too. I have to clear the image cache to see the updated focal point-based crop.

hgrimelid commented 2 years ago

I created a new Statamic installation and have tested with different versions of Statamic.

It appears that the images are regenerated as expected in 3.3.7 and it stops working in 3.3.8.

Latest version 2.11.2 of statamic-responsive-images is used.

riasvdv commented 2 years ago

Seems like caching has been added to the Glide image generator since 3.3.8:

https://github.com/statamic/cms/blob/b719542205c7848fc13894ff002b1dc3051f9eee/src/Imaging/ImageGenerator.php#L87

anyone know if it's being cleared somewhere? Can't seem to find that 🤔 if so it's an issue with Statamic more than the responsive images addon

hgrimelid commented 2 years ago

Ok, but then it shouldn't work with the Glide tag either?

It seems that the cache is cleared when using the glide tag:

{{ responsive:responsive_field_2 height="200" ratio="16/9" }}
<img src="{{ glide :src="responsive_field_2:src" h=765 w=1360 fit=crop_focal }}"> <!-- this works as expected -->
riasvdv commented 2 years ago

If you add glide:fit="crop_focal" to the responsive tag, does it work as expected then?

hgrimelid commented 2 years ago

I'm afraid not.

dryven commented 2 years ago

We are experiencing this issue, too

taylorcammack commented 2 years ago

Also experiencing this.

stuartcusackie commented 2 years ago

Same here.

stuartcusackie commented 2 years ago

I did a little bit of digging and I assume Statamic clears the cache here or here. I'm still searching for clues and my best lead is that crop parameters are missing from Spatie generated images:

WITHOUT SPATIE: http://site.test/img/asset/aW1hZ2VzL01BWS0yNFRILU5FVy1ob21lcGFnZTItMTY1MzQ4MzI0My5wbmc=?fit=crop-51-48-4.9&w=200&h=100&s=a4e98eed4119b5903156a6a76938a2ba

WITH SPATIE: http://site.test/img/asset/aW1hZ2VzL01BWS0yNFRILU5FVy1ob21lcGFnZTItMTY1MzQ4MzI0My5wbmc=?w=933&bg=FDC5B9&fm=webp&q=90&h=667&s=dc666ce91ca7eed4aa86447e5a5036f1

Trying to figure out why they are removed. These lines look relevant in Statamic: GlideTag@normalizeItem() ImageGenerator@applyDefaultManipulations()

And if I dump the $asset in various files I get different results:

stuartcusackie commented 2 years ago

I found a temporary fix based on my last comment. The crop params are being lost for some reason and they need to be re-retrieved for each asset in the responsive field.

I've tried various combinations in different places and this is the most reliable version that works will all fields. I've tried it on two websites that have a lot of images.

It's not ideal as I am re-retrieving each asset per breakpoint but it's the best I've got! I hope someone else can take it from here.

My fork contains the code below.

GenerateGlideImageJob.php


// FIX: You will need these
use Statamic\Facades\Asset;
use Statamic\Facades\Config;

class GenerateGlideImageJob extends GenerateImageJob
{
    protected function imageUrl(): string
    {
        $manipulator = Image::manipulate($this->asset);

        // FIX: Temporary fix for focal points.
        if ((Config::get('statamic.assets.auto_crop') && !array_key_exists('fit', $this->params)) || $this->params['fit'] == 'crop_focal') 
       {
            $object = Asset::find($this->asset['id']);
            $this->params['fit'] = 'crop-'.$object->get('focus', '50-50');
        }

       //....
FlorianMoser commented 2 years ago

The latest Statamic release v3.3.18 allows assets to trigger static caching invalidation. This seems to solve the issue.

ncla commented 2 years ago

Correct me if I am wrong, but I think that PR https://github.com/statamic/cms/pull/5489 only currently relates to static page cache invalidation if an asset has changed. I downloaded the update and the issue is still there.

Regardless, I was ready to jump in and create a PR for this. stuartcusackie's comments have been helpful for my own code deep diving. From my understanding, and I agree with Stuart, we just need to have params parity with the way Statamic Glide tag itself generates these URLs. Take into account statamic.assets.auto_crop config setting on the way.

And if I dump the $asset in various files I get different results:

  • ResponsiveTag.php: focal points are correct
  • Breakpoint.php: focal points are always 50-50-1

I have stumbled upon this issue as well too, there is something weird going on with Asset metadata hydrating. See my comments here: https://github.com/spatie/statamic-responsive-images/issues/143#issuecomment-1172880518

It's not ideal as I am re-retrieving each asset per breakpoint but it's the best I've got! I hope someone else can take it from here.

Don't feel too bad, hehe. Statamic CMS own GlideImageGenerator does this here for fit param. https://github.com/statamic/cms/blob/b719542205c7848fc13894ff002b1dc3051f9eee/src/Imaging/GlideImageManipulator.php#L208-L227

ncla commented 2 years ago

Actually I think there might be a need for a bug report on cms repository. The "auto cropping" feature might be just broken with the latest cache changes they implemented.

From Glide docs:

Note: All Glide generated images are cropped at their focal point, unless you disable the Auto Crop setting. This happens even when you don’t specify a fit parameter. You may override this behavior per-image/tag by specifying the fit parameter as described above.

By default, in the config, it is enabled.

For a fair comparison of the generated URLs, the test should be like this, without adding fit="crop_focal" to the Glide tag, which was done mistakenly in this comment. https://github.com/spatie/statamic-responsive-images/issues/126#issuecomment-1129135782

{{ responsive:image_asset ratio="1/1" }}
{{ glide:image_asset width="300" height="300" }}

For both tags, after I save an asset, the focus does not update. This addon also seems to have relied on this default, auto cropping behavior.

Rias suggestion to add glide:fit="crop_focal" is an clever idea, as it should fix the issue the same way in theory, but it does not work as this addon does not process "crop_focal" fit the way Statamic does (to a crop-x-y value).

I can confirm that downgrading CMS version to 3.3.7 (one version lower then when the new Glide caching was added) makes both of these tags work correctly again as intended. Both update the image when saving asset focus.

ncla commented 2 years ago

Hey folks, v2.13.0 was released just now. Can you try it and see if it fixes it?

stuartcusackie commented 2 years ago

@ncla Initial tests on two websites seem good. Thank you sir!

holgerschillack commented 1 year ago

Hey,

I have an issue where the half-strategy cache get's not invalidated when changing the focal point of an asset.

It works, when the asset is changed or replaced. I'm using the Responsive Images Plugin Version 3.1.3 to display assets and Statamic Pro 3.4.4. Even when I'm setting the invalidation rules in the static_caching.php to rules' => 'all, it only refreshes the image when replaced or changed. Otherwise I have to either delete the static cache manually in the control-panel or i have to clear my browser cache. Both options are not convenient for normal users and not usable for websites which modify their assets quite often.

Do you have any idea where to look?

Or can someone check with static caching, invalidation rules and responsive image plugin with this repro steps?

1) Add an image in the Statamic Control Panel like a portrait of a person 2) Save, publish and check on page if it's displayed 3) Open the asset in the CP and set a zoom focal point onto the face e.g. 4) Save, publish and check on page if it's displayed 5) For me, it's not. And also not changes in the focal point once it's displayed due to forced refresh

This is the usual implementation of our images:

{{ employees:portrait }}
    {{ responsive src="{url}" alt="{title}" class="employee-portrait" width="800" height="500" ratio="3/2" }}
{{ /employees:portrait }}
ncla commented 1 year ago

Please open a new, separate issue as it seems that the original issue is only loosely related to your issue.

For starters, you can check if this issue happens with regular Glide tag with auto crop focus enabled.