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

Pageimage::maxSize() creates image of wrong aspect ratio #709

Closed
Toutouwai opened this Issue Sep 24, 2018 · 11 comments

Comments

Projects
None yet
4 participants
@Toutouwai

Toutouwai commented Sep 24, 2018

Pageimage::maxSize() creates an image with the wrong aspect ratio if the image is smaller than the maxSize height or width.

I think the bug may have been introduced in this commit: processwire/processwire@6c9b475
The bug doesn't occur in PW 3.0.107.

I understand the idea of cropping to the desired aspect ratio for size(), but maxSize() shouldn't do this because it isn't used to enforce a particular crop ratio.

In the screenshots below the source image is 450x600.

<img src="<?= $page->images->first->maxSize(800,700)->url ?>" alt="">

Result:
2018-09-24_151725

Should be (screenshot from PW 3.0.107):
2018-09-24_152032

Setup/Environment

  • ProcessWire version: 3.0.114

ryancramerdesign added a commit to processwire/processwire that referenced this issue Sep 25, 2018

Fix issue processwire/processwire-issues#709 where Pageimage::maxSize…
…() could crop when it shouldn't when source image smaller than maxSize requested dimensions
@ryancramerdesign

This comment has been minimized.

Show comment
Hide comment
@ryancramerdesign

ryancramerdesign Sep 25, 2018

Contributor

Thanks @Toutouwai I've pushed a fix for this issue.

Contributor

ryancramerdesign commented Sep 25, 2018

Thanks @Toutouwai I've pushed a fix for this issue.

@Toutouwai

This comment has been minimized.

Show comment
Hide comment
@Toutouwai

Toutouwai Sep 25, 2018

Thanks @ryancramerdesign, but I'm not sure about that solution. It means that the image doesn't go through the image sizer at all, so something like this doesn't work as expected:

<img src="<?= $page->images->first->maxSize(800, 700, ['quality' => 10])->url ?>" alt="">

And as @horst-n has explained in this closed issue, the original image should never be returned by a method that normally returns a variation.

Toutouwai commented Sep 25, 2018

Thanks @ryancramerdesign, but I'm not sure about that solution. It means that the image doesn't go through the image sizer at all, so something like this doesn't work as expected:

<img src="<?= $page->images->first->maxSize(800, 700, ['quality' => 10])->url ?>" alt="">

And as @horst-n has explained in this closed issue, the original image should never be returned by a method that normally returns a variation.

ryancramerdesign added a commit to processwire/processwire that referenced this issue Sep 26, 2018

@ryancramerdesign

This comment has been minimized.

Show comment
Hide comment
@ryancramerdesign

ryancramerdesign Sep 26, 2018

Contributor

I prefer not to have ImageSizer create new images unnecessarily that are exact copies of the original, especially with the maxSize() method. That just seems like an unnecessary redundancy. But if passing in a populated $options array, then it should let the size() method make that call since some $options require creation of a new image regardless. I have updated it to correct that. Thanks.

Contributor

ryancramerdesign commented Sep 26, 2018

I prefer not to have ImageSizer create new images unnecessarily that are exact copies of the original, especially with the maxSize() method. That just seems like an unnecessary redundancy. But if passing in a populated $options array, then it should let the size() method make that call since some $options require creation of a new image regardless. I have updated it to correct that. Thanks.

@Toutouwai

This comment has been minimized.

Show comment
Hide comment
@Toutouwai

Toutouwai Sep 26, 2018

But if passing in a populated $options array, then it should let the size() method make that call since some $options require creation of a new image regardless. I have updated it to correct that.

This won't catch options defined in $config->imageSizerOptions though. I think those should be honoured by maxSize() too.

I prefer not to have ImageSizer create new images unnecessarily that are exact copies of the original

I thought the same thing too initially, which is why I opened the issue referenced above when I noticed that size() was creating a new image even if the image was already at the requested size.

But from @horst-n's comments I now see why image sizer methods returning the original image is not a good thing, because people may be following the approach of uploading originals at 100% quality but don't want to actually serve that original source image but only serve variations from it. If the image sizer methods sometimes return the original then that approach becomes unreliable.

Another thing is that the principle of not creating new images that are exact copies of the original isn't followed by other image sizer methods. For example, this would return a variation of 800px wide image even though the variation is a copy of the original.

<img src="<?= $page->images->first->width(2000, ['upscaling' => false])->url ?>" alt="">

So I don't think we should be introducing a special case for maxSize(), especially since this method used to return a variation until recently.

Maybe others can offer an opinion here?

Toutouwai commented Sep 26, 2018

But if passing in a populated $options array, then it should let the size() method make that call since some $options require creation of a new image regardless. I have updated it to correct that.

This won't catch options defined in $config->imageSizerOptions though. I think those should be honoured by maxSize() too.

I prefer not to have ImageSizer create new images unnecessarily that are exact copies of the original

I thought the same thing too initially, which is why I opened the issue referenced above when I noticed that size() was creating a new image even if the image was already at the requested size.

But from @horst-n's comments I now see why image sizer methods returning the original image is not a good thing, because people may be following the approach of uploading originals at 100% quality but don't want to actually serve that original source image but only serve variations from it. If the image sizer methods sometimes return the original then that approach becomes unreliable.

Another thing is that the principle of not creating new images that are exact copies of the original isn't followed by other image sizer methods. For example, this would return a variation of 800px wide image even though the variation is a copy of the original.

<img src="<?= $page->images->first->width(2000, ['upscaling' => false])->url ?>" alt="">

So I don't think we should be introducing a special case for maxSize(), especially since this method used to return a variation until recently.

Maybe others can offer an opinion here?

@horst-n

This comment has been minimized.

Show comment
Hide comment
@horst-n

horst-n Sep 27, 2018

@Toutouwai summarized it perfectly.
All resize methods must behave in the same way. They always have to return a variation and never should return the original image! Only way to get the original image has to be the direct call to it, without any resize method!

horst-n commented Sep 27, 2018

@Toutouwai summarized it perfectly.
All resize methods must behave in the same way. They always have to return a variation and never should return the original image! Only way to get the original image has to be the direct call to it, without any resize method!

ryancramerdesign added a commit to processwire/processwire that referenced this issue Sep 27, 2018

@ryancramerdesign

This comment has been minimized.

Show comment
Hide comment
@ryancramerdesign

ryancramerdesign Sep 27, 2018

Contributor

Okay I trust whatever Horst thinks is best here then, since he's the expert on this stuff. But also want to mention this is not as simple as just one use case, and also not what was originally intended for these methods. If you look at the maxWidth() or maxHeight() methods, those work the way they were supposed to (and the phpdoc indicates the behavior as well). All the methods used to return the original if it was already at the right size and didn't need further adjustments per $options. I didn't realize this had changed before this issue report. Maybe it's a better default for PW to have than the original intention, or maybe it's necessary now with all the $options that are now possible, I really don't know. I just don't like redundancy and the potential for re-compression of already prepared images. But if it is what's necessary, I'm okay with it too.

I primarily just use the size() method with no $options, but I tell clients (in the field description or notes) to upload images in a specific dimension if they want their original version to be used. When a designer is preparing images in Photoshop exactly how they want, and they don't want it to go through re-compression, they ensure it's at the right dimension ahead of time, so that PW will use their prepared image. While the less skilled users can let PW resize it to the right dimension for them. So that behavior appears to have been lost at some point, and I didn't realize it. I trust Horst if he says it's better this way, but need to support the original intention here too, since that's how I use it at least.

I've added an allowOriginal option (default=false) to the size() which makes it use the original image if already at the requested dimension and no other $options present. Also added it to maxSize() so that it returns the original image if already at or below the requested dimensions.

Contributor

ryancramerdesign commented Sep 27, 2018

Okay I trust whatever Horst thinks is best here then, since he's the expert on this stuff. But also want to mention this is not as simple as just one use case, and also not what was originally intended for these methods. If you look at the maxWidth() or maxHeight() methods, those work the way they were supposed to (and the phpdoc indicates the behavior as well). All the methods used to return the original if it was already at the right size and didn't need further adjustments per $options. I didn't realize this had changed before this issue report. Maybe it's a better default for PW to have than the original intention, or maybe it's necessary now with all the $options that are now possible, I really don't know. I just don't like redundancy and the potential for re-compression of already prepared images. But if it is what's necessary, I'm okay with it too.

I primarily just use the size() method with no $options, but I tell clients (in the field description or notes) to upload images in a specific dimension if they want their original version to be used. When a designer is preparing images in Photoshop exactly how they want, and they don't want it to go through re-compression, they ensure it's at the right dimension ahead of time, so that PW will use their prepared image. While the less skilled users can let PW resize it to the right dimension for them. So that behavior appears to have been lost at some point, and I didn't realize it. I trust Horst if he says it's better this way, but need to support the original intention here too, since that's how I use it at least.

I've added an allowOriginal option (default=false) to the size() which makes it use the original image if already at the requested dimension and no other $options present. Also added it to maxSize() so that it returns the original image if already at or below the requested dimensions.

@horst-n

This comment has been minimized.

Show comment
Hide comment
@horst-n

horst-n Sep 28, 2018

@ryancramerdesign Your described usecase, when designers preparing exact images in Photoshop, is not clean. Then you would and should not use resize methods at all.
When images are already prepared for output, (compressed etc), they are only useful for being displayed. This in mind, they never should be passed into a resize method. If you do so, created variations from those have lost all of the benefit of the prepairing, PLUS variations derived from them are bigger in filesize, compared to the exact same variations derived from the same originals in best quality (uncompressed).
So, in regard of your personal usecase, what doesn't fit into none of the two strict usecases, it is a good workaround to add the ´´´allowOriginal´´´ option.

horst-n commented Sep 28, 2018

@ryancramerdesign Your described usecase, when designers preparing exact images in Photoshop, is not clean. Then you would and should not use resize methods at all.
When images are already prepared for output, (compressed etc), they are only useful for being displayed. This in mind, they never should be passed into a resize method. If you do so, created variations from those have lost all of the benefit of the prepairing, PLUS variations derived from them are bigger in filesize, compared to the exact same variations derived from the same originals in best quality (uncompressed).
So, in regard of your personal usecase, what doesn't fit into none of the two strict usecases, it is a good workaround to add the ´´´allowOriginal´´´ option.

@Toutouwai

This comment has been minimized.

Show comment
Hide comment
@Toutouwai

Toutouwai Sep 28, 2018

I primarily just use the size() method with no $options, but I tell clients (in the field description or notes) to upload images in a specific dimension if they want their original version to be used.

Where I (or a client) want to use a pre-optimised image I use this hook method:

// Only resize image if not at requested size
$wire->addHook('Pageimage::sizeIfNeeded', function(HookEvent $event) {
    /* @var Pageimage $pageimage */
    $pageimage = $event->object;
    $width = (int) $event->arguments(0);
    $height = (int) $event->arguments(1);
    if($width && $pageimage->width !== $width || $height && $pageimage->height !== $height) {
        $event->return = $pageimage->size($width, $height);
    } else {
        $event->return = $pageimage;
    }
});

Toutouwai commented Sep 28, 2018

I primarily just use the size() method with no $options, but I tell clients (in the field description or notes) to upload images in a specific dimension if they want their original version to be used.

Where I (or a client) want to use a pre-optimised image I use this hook method:

// Only resize image if not at requested size
$wire->addHook('Pageimage::sizeIfNeeded', function(HookEvent $event) {
    /* @var Pageimage $pageimage */
    $pageimage = $event->object;
    $width = (int) $event->arguments(0);
    $height = (int) $event->arguments(1);
    if($width && $pageimage->width !== $width || $height && $pageimage->height !== $height) {
        $event->return = $pageimage->size($width, $height);
    } else {
        $event->return = $pageimage;
    }
});
@szabeszg

This comment has been minimized.

Show comment
Hide comment
@szabeszg

szabeszg Sep 28, 2018

when designers preparing exact images in Photoshop, is not clean. Then you would and should not use resize methods at all.

While in this workflow is preferable, there are use cases when there is no clear border between images uploaded by designers versus clients. For example I have a project where both my client and me work on the content (including images). I prefer preparing images myself for frontend display because I can often find a better image type/compression than the system. My client cannot do that, so they must rely on ProcessWire. Also, when my client makes mistakes, I fix it.

So in such a case described above, I would even love to be able to specify in the admin what the image API should do to the images, on an image-by-image level, overwriting the default.

szabeszg commented Sep 28, 2018

when designers preparing exact images in Photoshop, is not clean. Then you would and should not use resize methods at all.

While in this workflow is preferable, there are use cases when there is no clear border between images uploaded by designers versus clients. For example I have a project where both my client and me work on the content (including images). I prefer preparing images myself for frontend display because I can often find a better image type/compression than the system. My client cannot do that, so they must rely on ProcessWire. Also, when my client makes mistakes, I fix it.

So in such a case described above, I would even love to be able to specify in the admin what the image API should do to the images, on an image-by-image level, overwriting the default.

@horst-n

This comment has been minimized.

Show comment
Hide comment
@horst-n

horst-n Sep 29, 2018

@szabeszg: Yep, I know the reallife usecases very well for sure! :)
But the core resize system should be strictly organized, so that inspections / decisions in mixed (designers / clients) usecases should be implemented by the devs on demand. Like with a hook shown from @Toutouwai, or with Ryans new implemented option "allowOriginal".

OT: Regarding fixing client mistakes: I have one site where I email me a detailed message everytime when the client uploads new images and the mismatch is to big. (Small mismatches are handled by the templatefiles).

horst-n commented Sep 29, 2018

@szabeszg: Yep, I know the reallife usecases very well for sure! :)
But the core resize system should be strictly organized, so that inspections / decisions in mixed (designers / clients) usecases should be implemented by the devs on demand. Like with a hook shown from @Toutouwai, or with Ryans new implemented option "allowOriginal".

OT: Regarding fixing client mistakes: I have one site where I email me a detailed message everytime when the client uploads new images and the mismatch is to big. (Small mismatches are handled by the templatefiles).

@Toutouwai

This comment has been minimized.

Show comment
Hide comment
@Toutouwai

Toutouwai Oct 2, 2018

Thanks for the fix. I found that a similar issue occurs for size() though - will open a new issue for that.

Toutouwai commented Oct 2, 2018

Thanks for the fix. I found that a similar issue occurs for size() though - will open a new issue for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment