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

add public resize_to_fit and resize_to_fill utility functions #1356

Closed
wants to merge 1 commit into from

Conversation

filnet
Copy link
Contributor

@filnet filnet commented Nov 11, 2020

Proposed API would look like this:

/// Resizes the given size to fit into a new size while preserving the aspect ratio.
/// The returned size can be smaller than the requested size in one or the other dimension.
pub fn resize_to_fit(size: (u32, u32), new_size: (u32, u32)) -> (u32, u32) {
    resize_dimensions(size.0, size.1, new_size.0, new_size.1, false)
}

/// Resizes the given size to fill a new size while preserving the aspect ratio.
/// The returned size can be larger than the requested size in one or the other dimension.
pub fn resize_to_fill(size: (u32, u32), new_size: (u32, u32)) -> (u32, u32) {
    resize_dimensions(size.0, size.1, new_size.0, new_size.1, true)
}

Feel free to comment/suggest alternatives.

If this API is validated, I'll rewrite the functions to get rid of the old resize_dimensions function.

@filnet filnet force-pushed the make_resize_public branch from d55086c to e815b6a Compare November 11, 2020 16:16
@@ -24,14 +24,26 @@ where
a
}

/// Resizes the given size to fit into a new size while preserving the aspect ratio.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably word this "Returns the dimensions..." Using the word "Resize" makes it sound like the function actually resamples a specific image.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have been rewriting that sentence a few times... And English is not my mother tongue.
Feel free to suggest a full sentence.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about:

Returns the dimensions of the largest possible image that would fit inside a canvas of canvas_size, while keeping the same aspect ratio from original_size. The returned size will be smaller than the canvas size in at most one dimension.

fn dimensions_to_fit_canvas(original_size: (u32, u32), canvas_size: (u32, u32)) -> (u32, u32) {...}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trouble with the use of canvas is that the fill mode can return a size larger than the canvas.
The same problem affects @HeroicKatora's suggestion to use cover. cover will crop enlarged images to fit the canvas.
But the current fill mode will not crop. Might be confusing...

DynImage::resize_to_fit() will actually fill then crop, meaning fill is still needed.

I also did a bit of searching for prior art.
The best I found so far is https://legacy.imagemagick.org/Usage/resize/#resize. It does not exhibit a usable API but lists all the resize modes possible and provides wording.
I was hoping to find something immediately adoptable but no luck so far.

Anyways, to move on, what about introducing the enum suggested by @HeroicKatora :

fn dimensions_to_fit_canvas(original_size: (u32, u32), canvas_size: (u32, u32), resize_mode: ResizeMode) -> (u32, u32) {...}

Where ResizeMode can be :

  • FIT (or CONTAIN)
  • FILL (or COVER)
  • FILL_OVERFLOW (or COVER_NO_CROP) // current fill mode

Might be a bit over engineered and does not address the canvas/background-size issues though...

@filnet
Copy link
Contributor Author

filnet commented Nov 11, 2020

Note that DynImage::resize_to_fill() has a slightly different behavior.
In addition to resizing it will also crop the image so that it does not exceed the new dimensions.

Copy link
Member

@HeroicKatora HeroicKatora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In css these size specifiers are called contain and cover instead of fit and fill. Those are relatively clear and pretty popular prior art.

@@ -4,4 +4,5 @@ pub mod utils;

mod rect;
pub use self::rect::Rect;
pub(crate) use self::utils::resize_dimensions;

pub use self::utils::{resize_to_fit, resize_to_fill};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd just implement them here instead of in utils and not re-export them to have them visible in two places.

@filnet
Copy link
Contributor Author

filnet commented Nov 18, 2020

I am sleeping on it for a bit. I don't like to force solutions.

I'll do another round of prior art research and will come back with another proposal.

@HeroicKatora
Copy link
Member

@filnet Did you find the time for the research :)

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

Successfully merging this pull request may close these issues.

3 participants