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

Allow to override min_size via command-line argument #114

Merged
merged 8 commits into from
Dec 22, 2021

Conversation

sbesson
Copy link
Member

@sbesson sbesson commented Aug 20, 2021

The current implementation stops downsampling the image as soon as both X and Y dimensions are smaller than 256. In some cases, we want this cut-off values to be configurable.

This commit makes the minimal change by keeping the default as 256 but allowing to override it via command-line option.

Tests are also added to cover the min-size arguments with various scenarios

The current implementation stops downsampling the image as soon as both X and
Y dimensions are smaller than 256. In some cases, we want this cut-off values
to be configurable.
This commit makes the minimal change by keeping the default as 256 but allowing
to override it via command-line option.
Copy link
Member

@melissalinkert melissalinkert left a comment

Choose a reason for hiding this comment

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

👍 tests look much nicer now, thanks.

I do wonder though if the --min-size 30 tests expose undesired behavior; an 8x16 image doesn't match with the Minimum size of the largest XY dimension being 30. That isn't an issue introduced in this PR, so I'm happy for this to be merged and we can discuss the correct behavior separately.

@sbesson
Copy link
Member Author

sbesson commented Aug 23, 2021

Heh. I was hesitant to start this conversation but since you brought it up, I am also not convinced by the --min-size terminology which is confusing as proven by your example. To be precise, the parameter actually specifies a maximum allowed size rather than a minimal value but it applies to the smallest resolution.

If we were to agree on the thumbnail nomenclature, I would propose something along the lines of --max-thumbnail-size. Should we brainstorm for additional terms before we introduce the option in the command-line?

@melissalinkert
Copy link
Member

I'm fine with --max-thumbnail-size. --max-apex-size is the only other option that comes to mind (to avoid confusion between the smallest resolution and any separate thumbnail image), but that's probably not any clearer overall.

@chris-allan
Copy link
Member

My inclination is to call this --target-min-size or similar and refer to what the "target" is explicitly in the documentation. As mentioned this is a desired minimum size of longest side of the highest level of the pyramid rather than a hard limit.

@chris-allan
Copy link
Member

75b2863 conceptually looks great to me. Will defer to @melissalinkert for anything else here.

@melissalinkert
Copy link
Member

Looks good to me too, thanks!

@chris-allan chris-allan merged commit b2b2395 into glencoesoftware:master Dec 22, 2021
@sbesson sbesson deleted the min_size branch October 17, 2022 13:51
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