The image style "Focal Point Scale and Crop" does not work as expected. No matter where the focal point is (except Y = 0) the crop area stays the same.

My settings for the "Focal Point Scale and Crop" effect: 350px (both width and height).
Test image dimensions: 4912x7360

The main problem lies in the FocalPointEffectBase class:

public function applyEffect(ImageInterface $image) {
    // @todo: Get the original image in case there are multiple scale/crop effects?
    $this->originalImage = clone $image;
    return TRUE;
  }

So later in "calculateAnchor" method in lines 235, 238 and 239 there's code, which tries to get the width and height of the original Image. But because we resize the image before cropping, the cloned object is not really the "original" image anymore, cause the properties height and width do get changed "too" (they are just referenced). That leads to unchanged $focal_point variable, because $this->originalImage->getWidth() is actually the same as $image_size['width'], which itself is $image->getWidth().

    $focal_point['x'] = (int) round($focal_point['x'] / $this->originalImage->getWidth() * $image_size['width']); // is like (500/1000) * 1000 = 500
    $focal_point['y'] = (int) round($focal_point['y'] / $this->originalImage->getHeight() * $image_size['height']);

This error leads to a faulty calculated anchor and thus a faulty crop area.

CommentFileSizeAuthor
#8 crop_anchor_is_lost-2782073-8.patch54.07 KBbleen
#6 crop_anchor_is_lost-2782073-6.patch2.31 KBbleen
#6 interdiff.txt2.31 KBbleen
#2 focal_point-2782073-2.patch2.05 KBalex0412
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alex0412 created an issue. See original summary.

alex0412’s picture

alex0412 CreditAttribution: alex0412 commented
FileSize
focal_point-2782073-2.patch2.05 KB

Here's a basic patch, which saves the width and height of the original image in a new property (because that's all we need for the cropping, not the entire object). I've left the clone there because I'm not sure if it's needed somewhere else. The maintainers sure have a better overview of that ;-)

ahebrank’s picture

ahebrank CreditAttribution: ahebrank at NewCity commented
Title: Focal Point Scale and Crop does not work as expected » Crop anchor is lost after scaling
Priority: Normal » Major

Changing the title to get some attention, since this needs a quick fix. Scale and crop does not work in the current 8.x releases. The patch above works for me.

bleen’s picture

bleen CreditAttribution: bleen at NBCUniversal commented
Status: Active » Needs review

Unit tests need to run. They are failing locally ...

Status: Needs review » Needs work

The last submitted patch, 2: focal_point-2782073-2.patch, failed testing.

bleen’s picture

bleen CreditAttribution: bleen at NBCUniversal commented
Status: Needs work » Needs review
FileSize
interdiff.txt2.31 KB
crop_anchor_is_lost-2782073-6.patch2.31 KB

Ok ... this is why the unit tests were failing...

What I don't understand now is why the unit tests are passing... If the crops are wrong, then these tests should fail. At minimum, if this patch changes the crops in any way, then these tests should fail.

ahebrank’s picture

ahebrank CreditAttribution: ahebrank at NewCity commented

Just taking a quick look at the testing and learning a bit about PHP object copying, I'm speculating that the issue is hidden from testing by how the two images are defined. In testing:

    $original_image = $this->prophesize(ImageInterface::class);
    $original_image->getWidth()->willReturn($original_image_size['width']);
    $original_image->getHeight()->willReturn($original_image_size['height']);

    $image = $this->prophesize(ImageInterface::class);
    $image->getWidth()->willReturn($resized_image_size['width']);
    $image->getHeight()->willReturn($resized_image_size['height']);

whereas in the plugin proper,

  $this->originalImage = clone $image;

results in what's apparently only a shallow copy of the $image -- meaning that the image dimensions of $originalImage end up changing with each effect applied.

bleen’s picture

bleen CreditAttribution: bleen at NBCUniversal commented
FileSize
crop_anchor_is_lost-2782073-8.patch54.07 KB

Interesting ... well, I spent a bunch of time this afternoon completely tearing apart the "calculateAnchor" method and breaking it down into its compnent pieces so I could write much more fine-grained tests.

I think this fortifies things pretty well ...

I'm not bothering with an iterdiff because just about everything changed...

Please test and report back.

hctom’s picture

hctom
Primary language German
CreditAttribution: hctom at undpaul commented

The patch from #8 applies cleanly and solves my problems I had, when my images were autorotated by the image_effects module based in available EXIF data. I don't change the status yet, because I did not check the code so far, but just wanted to report, that it definitely solves the problem.

  • bleen committed f3765d7 on 8.x-1.x 
    Issue #2782073 by bleen, alex0412: Fixed crop anchor is lost after...
bleen’s picture

bleen CreditAttribution: bleen at NBCUniversal commented
Status: Needs review » Fixed

I went ahead and committed this

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.