Skip to content
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

Support "fit to min" in addition to "fit to max". #24

Closed
rich-j opened this issue Jul 17, 2020 · 18 comments
Closed

Support "fit to min" in addition to "fit to max". #24

rich-j opened this issue Jul 17, 2020 · 18 comments

Comments

@rich-j
Copy link

rich-j commented Jul 17, 2020

This widget is awesome! We would like a configuration option to limit scaling to fit the min dimension. The current behavior limits scaling to the max dimension.

When cropping circular (square) avatars from rectangular source images, we would like to limit the scaling fit to the min dimension such that no parts of the resulting image are blank. The current behavior limits the scaling fit to the max dimension which can result in blank areas on the min dimension sides.

For example a rectangular vertical (portrait) image cropped to a square aspect ratio has blank area on left and right sides (i.e. min dimension). The requested option would not allow the image to be scaled smaller than what fits across the min (e.g. horizontal) dimension. Currently the scaling is limited to the max dimension (in this example the vertical dimension) which can result in blank areas on the left and/or right sides.

@xclud
Copy link
Owner

xclud commented Jul 17, 2020

@rich-j Thank you.

If you see blank areas in the result image you've found a bug! Can you share some code and the steps to reproduce the bug?

@rich-j
Copy link
Author

rich-j commented Jul 17, 2020

I don't think it's a bug, crop just defaults to "fit max".

We use a square aspect ratio with the oval crop shape as such:

    child: AspectRatio(
      aspectRatio: 1,
      child: Crop(
        controller: cropController,
        shape: CropShape.oval,
        child: selectedImage,
      ),
    ),

When we give it a rectangular image such as this iOS sample image it defaults the scaling to fit the max dimension (horizontal in this example) resulting in "blank" areas on the top and bottom.

Screen Shot 2020-07-17 at 2 51 20 PM

We don't think this is "wrong" or even a bug - this is an enhancement request.

For this use case of providing a square avatar image from either a portrait or landscape image, we would like to configure crop to to limit the scaling to fit the min dimension. So in the above example the image would be scaled to fill the vertical dimension and centered such that there is non-visible overhang on the sides. The user can then pan the image either left or right to get to the overhang. Of course they can also zoom, pan and rotate further which is what crop does.

@xclud
Copy link
Owner

xclud commented Jul 17, 2020

Did you try fit: BoxFit.cover on selectedImage?

@xclud
Copy link
Owner

xclud commented Jul 17, 2020

btw, you do not need AspectRatio as the parent widget. I don't know how it affects the behavior of the widget. You should set cropController.aspectRatio.

@rich-j
Copy link
Author

rich-j commented Jul 17, 2020

We need the AspectRatio widget to constrain the layout in the dialog. We also have the controller aspectRatio set to 1.0.

    return AlertDialog(
      scrollable: true,
      title: Text("Select avatar image"),
      content: Column(
        mainAxisSize: MainAxisSize.min,
        children: [
          Hero(
            tag: heroKey,
            child: AspectRatio(
              aspectRatio: 1,
              child: Crop(
                controller: cropController,
                shape: CropShape.oval,
                child: selectedImage,
              ),
            ),
          ),
      ... more widgets ...

@rich-j
Copy link
Author

rich-j commented Jul 17, 2020

The fit: BoxFit.cover provides the desired look but not the desired functionality. Functionally we would like the crop widget to be able to "pan" to the "overhang". The BoxFit.cover option crops the overhang. Yes, one can specify the alignment option to something other than the default center but the crop widget no longer has access to the overhang.

@xclud
Copy link
Owner

xclud commented Jul 17, 2020

I didnt understand the functionality you desire. Can you point to a library/demo/video with this functionality. Anything written in Java or Swift or for Windows would help a lot.

@K-Y-Johnson
Copy link

K-Y-Johnson commented Jul 19, 2020

Hello! I'm a UX designer working with rich-j. I hope that by providing imagery our desired functionality will be more clear.

Let me define the images/colors I'm using.

Yellow is a standard rectangular image taken by a camera.

Blue is the widget, a square.

Red is a circular outline within the widget, showcasing what will be cropped to use for an avatar image.

When an image is selected in your widget (in our case, for an avatar), the image is placed within a square for cropping. That square defaults to encasing the entire image, automatically constrained to the size of the image’s largest side. This results in black rectangles (represented by blue here) filling out the rest of the image, since standard images are rectangular, not square.

Essentially, we don't want these black rectangles (again, represented by blue). So instead of defaulting to constrain the image by its longest side, we would like to request an option to constrain it by the shortest side, such that it negates the need for the black rectangles.

The default black rectangles are good for the default option, and the rest of the widget is wonderful; we would just appreciate an option in the code that offers this functionality.

Trying fit: BoxFit.cover like you suggested crops the original image and doesn’t allow access to the outside areas that don’t fit within the square (here shown by the yellow of the image). Essentially, it doesn't enable panning to the area outside the square widget.

Thanks for taking the time to work with us! I hope these images helped.

@xclud
Copy link
Owner

xclud commented Jul 19, 2020

@rich-j Thank you for describing the problem and the images help a lot. The last 2 images are similar. I am not getting what's wrong and what should be fixed/added as an option. This package works as you described unless you've found a bug. Can you shine more light on this?

@K-Y-Johnson
Copy link

K-Y-Johnson commented Jul 19, 2020

Essentially, with the widget as is, it's possible for an avatar image to be generated with black bars in the avatar (here represented by blue).

This is not a bug - your program works great as intended. However, we would appreciate an added option that prevents users from creating an avatar image with these black bars. Currently, a user could save their avatar image to the proportions shown above, containing blue (the black bars). We don't want to allow users the option to save their images with this blue.

The widget defaults to showing the selected image in its entirety, which creates a need for something (the black bars) to fill the rest of the square space, as most pictures are rectangular by nature. In order to not need these black bars (and thus prevent users from creating avatar images with them), we would appreciate an option to constrain the default image by its shortest side. This would cut off visual of parts of the rectangular image that don't fit in the square widget, but since panning and zooming would be allowed as per your current widget, would not be an issue for the user.

I hope this makes more sense?

@rich-j
Copy link
Author

rich-j commented Jul 19, 2020

Another way to consider this enhancement request is to constrain the minimum scale value. Since we're targeting a square cropped result the minimum scale is the aspect ratio for landscape, the reciprocal for portrait. We have added similar code to our initialization:

    final imageAspectRatio = image.width / image.height;
    final minScale = imageAspectRatio < 1.0 ? 1 / imageAspectRatio : imageAspectRatio;
    cropController.scale = minScale;

This starts with the image filling the crop viewport and the user can adjust the image by panning into the offscreen overhang. However, the user can also pan into the areas where there is no image on the sides of the image min dimension resulting in the final cropped image having blank areas. This behavior isn't wrong - we just would like to further constrain the user from creating a final cropped image with blank areas.

Constraining the scale for both min and max (i.e. #15) would be nice. Yes, I recognize the above calculation is very simplified. The general implementation needs to take into account the crop target aspect ratio and rotation.

We did try to implement our own constraints by adding a listener to cropController but the listener only fires during initialization. It's probably a separate enhancement issue for the listener to fire on all user inputs.

@xclud
Copy link
Owner

xclud commented Jul 20, 2020

we would appreciate an added option that prevents users from creating an avatar image with these black bars.

No it doesn't create black bars and referring to your attached image, the blue color never appears.

We don't want to allow users the option to save their images with this blue.

Yes. This is how the package currently works.

I hope this makes more sense?

Unfortunately i did not understand the problem yet. But i will keep working with you until it's fixed :) Please check your inbox.

@K-Y-Johnson
Copy link

K-Y-Johnson commented Jul 21, 2020

When the image first loads, there's no issue if you use the aspect ratio scale initialization as @rich-j shows in the comment above.

However, when you zoom out or pan to one side, there are black bars filling out the image's shortest sides.

This happens with both horizontal pictures (above) and vertical (below).

The longest side is unaffected, and the user is unable to pan past its boundaries. But the user can pan out to save those black bars as part of their avatar image.

Thank you for your patience with us. :) We really appreciate you taking this time.

@K-Y-Johnson
Copy link

K-Y-Johnson commented Jul 22, 2020

To put things in another context, we took a look at your demo app.

This is your original image.

When changing the aspect ratio to 1:1, however, using fit: BoxFit.cover crops out all of the picture that doesn't fit within the 1:1 confines. That loses data that the user may want.

For instance, what if a user has a group photo, but they aren't posed in the middle? Using fit: BoxFit.cover would simply crop them out and render that image unusable for an avatar.

Therefore, we hope for an option for a lossless change in aspect ratio while maintaining the wonderful functionality of your widget.

@xclud
Copy link
Owner

xclud commented Jul 22, 2020

@K-Y-Johnson these black areas doesn't happen on my phone. I have attached a video (in the zip file). It shows the behavior of the package. Are you sure you are using the latest commit and have not modified the source code?

screen.zip

@rich-j
Copy link
Author

rich-j commented Jul 23, 2020

@K-Y-Johnson has added issue #27 which focuses on the "pre-crop" issue. Let's close this issue since it descended into one possible approach of not using fit: BoxFit.cover.

@xclud - please know that we think this is a really good widget and we appreciate your patience with us.

@bkoznov
Copy link

bkoznov commented Sep 28, 2021

@rich-j Would you mind sharing how you were able to get those black bars? Like xclud, I couldn't reproduce, but I would love those as part of the functionality. Thanks!

@rich-j
Copy link
Author

rich-j commented Sep 28, 2021

@bkoznov I don't recall specifics since it's been over a year since I last looked at this issue. The issue is basically due to a mismatch in aspect ratios between the selected image to crop (e.g. 16:9) and the target cropped result (1:1). This issue is closed and is now being tracked in issue #27. I believe that @xclud has reproduced it and a fix is forthcoming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants