microsoft / PowerToys

Windows system utilities to maximize productivity
MIT License
109.67k stars 6.46k forks source link

Image Resizer - Ignore Orientation Not Working #14904

Open spiceygas opened 2 years ago

spiceygas commented 2 years ago

Microsoft PowerToys version

0.51.1

Running as admin

Area(s) with issue?

Image Resizer

Steps to reproduce

According to the documentation, the "ignore orientation" checkbox should affect whether the specified width x height dimensions are strictly enforced or swapped for portrait-oriented pictures. However, it's not working. The resizer always swaps the dimensions.

  1. Testing with an image that is 4000 x 6000 pixels. (Portrait orientation)
  2. Resize the image.
    • Resize Type: Fit
    • Width: 60
    • Height: 40
    • Unit: Pixel
    • [Check] Make pictures smaller but not larger
    • [Check] Ignore the orientation of pictures
    • [Check] Resize the original pictures
    • [Uncheck] Remove metadata that doesn't affect rendering
  3. Resize. The output image has swapped the dimensions, and is 40x60 instead of 60x40. It remains in portrait mode.
  4. Repeat step 2, but this time uncheck the box to "ignore the orientation of pictures."
  5. Resize. The output image has swapped the dimensions, and is 40x60 instead of 60x40. It remains in portrait mode.

The checkbox state is not correctly controlling whether the orientation is forced or rotated.

✔️ Expected Behavior

When unchecked, the documentation says that "ignore the orientation of pictures" should force it to take the literal dimensions provided (60x40).

❌ Actual Behavior

Image Resizer is swapping the dimensions from 60x40 ==> 40x60 regardless of whether or not the checkbox is checked.

Other Software

N/A

bricelam commented 2 years ago

Sounds like a bug. Has the code changed recently? I'm pretty sure I had a test for this.

CleanCodeDeveloper commented 2 years ago

Could it be explained by this documented behavior?

If Ignore the orientation of pictures is checked, the width and height of the specified size may be swapped to match the orientation (portrait/landscape) of the current image. In other words: If checked, the smallest number (in width/height) in the settings will be applied to the smallest dimension of the picture. Regardless if this is declared as width or height. The idea is that different photos with different orientations will still be the same size.

Image Resizer doc, see "Note" Box

spiceygas commented 2 years ago

The same behaviour occurs regardless of whether or not the box is checked. One state should swap to keep orientation, the other shouldn't

However, no matter what you do you get the same outcome. In other words, the checkbox appears to be doing nothing at all.

CleanCodeDeveloper commented 2 years ago

Sounds like a bug. Has the code changed recently? I'm pretty sure I had a test for this.

The code in question was not modified since the initial commit in March 2020. The relevant piece of code is:

if (_settings.IgnoreOrientation
    && !_settings.SelectedSize.HasAuto
    && _settings.SelectedSize.Unit != ResizeUnit.Percent
    && originalWidth < originalHeight != (width < height))
{
    var temp = width;
    width = height;
    height = temp;
}

I'm not sure if we share the same understanding about the feature in question. I have hopes that @bricelam can give us some insights.

In my opinion the checkbox only makes sense if you use absolute values and resize more than one image.

Lets say you have 2 images: 520 x 360px (Landscape) Landscape

360 x 520px (Portrait) Portrait

and want to resize them to halve of the size 520 x 360px --> 260 x 180px (Landscape) 360 x 520px --> 180 x 260px (Portrait)

using the following resize settings: image

Result: First try without enabling the "Ignore the orientation of pictures" checkbox: 260 x 180px (Landscape) Landscape (Custom) (Ignore the orientation of pictures disabled)

260 x 180px (was previously a "Portrait" picture) Portrait (Custom) (Ignore the orientation of pictures disabled)

Second try with "Ignore the orientation of pictures" checkbox enabled: 260 x 180px (Landscape) Landscape (Custom) (Ignore the orientation of pictures enabled)

180 x 260px (Portrait) Portrait (Custom) (Ignore the orientation of pictures enabled)

IMHO the feature works as intended but the docs need some love to avoid further confusion.

spiceygas commented 2 years ago

Question: Did you actually make your pictures by running the test case through the utility? Or did you draw those pictures independently? Because that's not how it behaves right now when I run the test case.

In your first scenario (without checking the box), the pictures you show both come out in landscape format. You labeled the second output picture in the first scenario incorrectly as 180x260, but the picture you showed is 260x180.

In my steps to reproduce (#3 and #5 in the initial post) you get the exact same dimensions REGARDLESS of whether or not you check the box. How you're describing the behaviour is NOT how it works right now (v0.51.1).

It seems that one of the checkbox states should keep the orientation (and letterbox the picture), while the other should swap the orientation parameters to accommodate landscape/portrait. And that's what you described. But it's not what's happening right now.

Jay-o-Way commented 2 years ago

@CleanCodeDeveloper thanks for the message. What you say is correct and what you show is the intended result. I think the problem for @spiceygas is just that it doesn't seem to work over there. So the thing is, we need to know why, and I am willing to label this issue with Needs-Repro.

Jay-o-Way commented 2 years ago

@spiceygas is this problem occuring to all images you have tried, or maybe one or a few specific?

CleanCodeDeveloper commented 2 years ago

Question: Did you actually make your pictures by running the test case through the utility? Or did you draw those pictures independently? Because that's not how it behaves right now when I run the test case.

I used the Image Resizer v0.51.1 for this.

In your first scenario (without checking the box), the pictures you show both come out in landscape format. You labeled the second output picture in the first scenario incorrectly as 180x260, but the picture you showed is 260x180.

Fixed this in my comment, thanks for pointing out.

But it's not what's happening right now.

Can you please confirm that you used the exact same settings (Resize type: Fill, Width: 260, Height: 180, Unit: Pixel)?

@Jay-o-Way: Could you try to reproduce the issue using my sample images?

Jay-o-Way commented 2 years ago

Could you try to reproduce the issue using my sample images?

Fit 260 × 180 pixels

Fit

Fill 260 × 180 pixels

Orentation test - FILL

Works fine here...

(off-topic) @bricelam Notice the increase from 15.3 to 21.3 kB in the only cropped image? I suppose this is normal?

bricelam commented 2 years ago

Notice the increase from 15.3 to 21.3 kB in the only cropped image? I suppose this is normal?

Weird. I know nothing about PNG encoding. 🤷‍♂️ Could be some quirk with the compression. Or maybe some metadata is inadvertently being duplicated.... no idea.

Jay-o-Way commented 2 years ago

@spiceygas can you share your original images and a screenshot of the exact GUI with settings?

spiceygas commented 2 years ago

Ok, using the image you provided above (portrait, dimensions = 360 x 520). My goal is to have it resize down to 120 x 80 with cropping.

Scenario 1: Fill, 120 x 80 pixel, no boxes checked Result: Landscape oriented photo, 120 x 80.

Scenario 2: Fill, 120 x 80 pixel, ignore orientation of pictures Result: Portrait oriented picture, 80 x 120.

Scenario 3: Fit, 120 x 80 pixel, no boxes checked Result: Portrait oriented photo, 55 x 80

Scenario 4: Fit, 120 x 80 pixel, ignore orientation of pictures Result: Portrait oriented picture, 80 x 116

Summary

  1. I think there's some human error here on my part. I thought I had tried all of these permutations, but clearly I missed one of them. Scenario 1 matches my use case.
  2. Is it correct that 3 of 4 scenarios output portrait-oriented images? If so, then this issue can be closed as user error.
Jay-o-Way commented 2 years ago

Thanks for the message.

  1. That is always a possibility :-)
  2. Yes, that's correct. Only "fill" will crop.
cjwijtmans commented 1 year ago

still not fixed.

CleanCodeDeveloper commented 1 year ago

still not fixed.

We were not able to reproduce the issue above (see conversation above). I believe the feature works as intended but the docs need some love to avoid further confusion.

cjwijtmans commented 1 year ago

still not fixed.

We were not able to reproduce the issue above (see conversation above). I believe the feature works as intended but the docs need some love to avoid further confusion.

Try again. It is not doing what i tell it to. Portrait images are incorrectly cropped even when i tell it to ignore orientation or not. When i tell it to fill 200x50 it should produce that image and not 50x200.

CleanCodeDeveloper commented 1 year ago

Try again. It is not doing what i tell it to. Portrait images are incorrectly cropped even when i tell it to ignore orientation or not. When i tell it to fill 200x50 it should produce that image and not 50x200.

That's what I meant with documentation. If i remember correctly, the ignore orientation option does not have any effect until you resize multiple images at the same time. If you still feel this is a bug please provide detailed repro steps.

Jay-o-Way commented 1 year ago

the ignore orientation option does not have any effect until you resize multiple images at the same time.

@CleanCodeDeveloper meh, the devil is in the details here. The setting applies the highest value to the largest dimension, but the number of images doesn't matter.

Jay-o-Way commented 1 year ago

@cjwijtmans i think the FILL setting is crucial here. Imagine you have a portrait image and you want to fill it (e.g.) 400×300px. What would be the expected result? So if I'm correct, Fill will always crop and the "ignore" setting should only be visible for Fit. Pinging @bricelam

bricelam commented 1 year ago

Here is my brain dump from the last time I thought about the interaction between these two settings: https://github.com/bricelam/ImageResizer/issues/38#issuecomment-368962399

cjwijtmans commented 1 year ago

@cjwijtmans i think the FILL setting is crucial here. Imagine you have a portrait image and you want to fill it (e.g.) 400×300px. What would be the expected result? So if I'm correct, Fill will always crop and the "ignore" setting should only be visible for Fit. Pinging @bricelam

The issue is not the cropping. The issue is that when i tell it to create a 500x400 image it will create a 400x500 instead because portrait. Its essentially not doing what i am telling it to. I have resorted to manually cropping instead which is fine. But its still a bug.

bricelam commented 1 year ago

@cjwijtmans Do you have Ignore the orientation of pictures checked? If so, why? Does it work as expected when you uncheck it?

bricelam commented 1 year ago

This comment illustrates the intent of that checkbox:

You may have landscape and portrait photos in 16:9 ratio that you want to fill/crop to 4"x6". The portrait photos should be 6" high and the landscape ones should be 6" wide.

cjwijtmans commented 1 year ago

@cjwijtmans Do you have Ignore the orientation of pictures checked? If so, why? Does it work as expected when you uncheck it?

Simply read my comments again it contains your answer.

bricelam commented 1 year ago

@cjwijtmans I'm having a hard time following your comments. All I'm hearing is "it's wrong", "it's not fixed", and "it's not doing what I tell it to."

Can you provide some clear repro steps along with your expectations? For example, what are the dimensions of the input images, what are all the settings you've specified? What are the output dimensions your expecting for each of the input images?

TheJoeFin commented 12 months ago

image

I have made this to describe the current behavior of the "ignore orientation checkbox" specifically the confusion around the Unchecked Image 2 quadrant. I also think the term ignore orientation is confusing and should probably be a toggle between fit or fill. But then that opens the option to change how you fill: center zoom, top left, bottom right... etc.

Does this lay everything out clearly? How do we want to solve for this? /needinfo

Jay-o-Way commented 12 months ago

Your graphic is good, @TheJoeFin . The confusion lies in the fact that - when checked - the words "width" and "height" do not apply anymore. The highest input value applies to the longest dimension for each image file. Using words like "long(est) side" and "short(est) side" sounds a bit odd, but are much more accurate.

I think the UI should have a choice between

Yet, then the code should have more logic to handle all this (like, check which value is actually bigger). This is indeed very complicated. 🤯

bricelam commented 12 months ago

I agree, it's complicated. And it stems from users having an overly-simplified mental model. If they specify 5"x7", they expect all their photos to be 5x7 regardless of whether they were oriented portrait or landscape when they took them. But when they're resizing just one digital image, they don't expect the dimensions they specify to suddenly be flipped because that checkbox is checked.

Maybe a warning icon with a tooltip should show up when:

  1. They've only selected one image
  2. The checkbox is checked
  3. The input dimensions are oriented different from the source dimmensions
microsoft-github-policy-service[bot] commented 11 months ago

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 5 days. It will be closed if no further activity occurs within 5 days of this comment.

microsoft-github-policy-service[bot] commented 11 months ago

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 5 days. It will be closed if no further activity occurs within 5 days of this comment.

microsoft-github-policy-service[bot] commented 11 months ago

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 5 days. It will be closed if no further activity occurs within 5 days of this comment.