There’s not much code… well easier to review! Anyway here’s a few things I could notice…
This signature is of course more generic than necessary (only being used once with some concrete types), but I guess it’s good for practice of writing such generic function signatures anyway…
fn downsample_image<T>(
img: &T,
config: &Config,
) -> ImageBuffer<T::Pixel, Vec<<T::Pixel as Pixel>::Subpixel>>
where
T: GenericImageView,
<T as GenericImageView>::Pixel: 'static,
{
…
}
here’s an idea to refactor this function signature nonetheless, without actually changing its generality, but IMO it’s perhaps nicer (since it’s more compact):
- using
impl Trait
can be convenient and easy to read
- since the return type depends on the
Pixel
associated type of T
, we need an explicit generic arg for that nonetheless
- giving it its own generic parameter
P
, constrained with Pixel = P
, has the additional benefit of avoiding the need for the unwieldy <T::Pixel as Pixel>::Subpixel
syntax
so here’s the result:
fn downsample_image<P: Pixel + 'static>(
img: &impl GenericImageView<Pixel = P>,
config: &Config,
) -> ImageBuffer<P, Vec<P::Subpixel>> {
…
}
let ratio = img.dimensions().0 as f32 / (config.width / config.amount as u16) as f32;
let new_width = (img.dimensions().0 as f32 / ratio) as u32;
let new_height = (img.dimensions().1 as f32 / ratio) as u32;
I see you use floating points for calculating rations. I personally am a fan of using integer arithmetic when floats aren’t really needed, though for images I guess it doesn’t matter. When using floats, it’s generally better to use f64
though, just as a reasonable default; f32
’s use case is mainly when you have so many of them that the memory matters.
Another kind of thing you could pay attention to here if you wanted is to use checked arithmetic and checked fallible conversions (e.g. from float back to u32
), because your program probably behaves more reasonable if it errors on overflows instead of the default wrapping behavior (in optimized builds).
img.enumerate_rows()
// map each row to a string of ASCII characters (terminating with a newline)
.map(|(_, row)| {
row.map(|(_, _, lumi)| repeat_char(lumi_8_to_char(lumi.0[0]), horiz_repeat_count))
.collect::<String>()
+ "\n"
})
// collect all the rows into a single string
.collect::<String>()
you use enumerate_rows
but don’t make use of the indices… just use rows
instead 
img.rows()
// map each row to a string of ASCII characters (terminating with a newline)
.map(|row| {
row.map(|lumi| repeat_char(lumi_8_to_char(lumi.0[0]), horiz_repeat_count))
.collect::<String>()
+ "\n"
})
// collect all the rows into a single string
.collect::<String>()
row.map(|lumi| repeat_char(lumi_8_to_char(lumi.0[0]), horiz_repeat_count))
.collect::<String>()
+ "\n"
…
/// Takes a char and return a `String` consisting of that char repeated `n` times
fn repeat_char(c: char, n: usize) -> String {
std::iter::repeat(c).take(n).collect()
}
allocating a new String
like this for each individual character isn’t necessary. You can just flat_map
to an iterator of char
instead:
row.flat_map(|lumi| repeat_char(lumi_8_to_char(lumi.0[0]), horiz_repeat_count))
.collect::<String>()
+ "\n"
…
fn repeat_char(c: char, n: usize) -> impl Iterator<Item = char> {
std::iter::repeat(c).take(n)
}
A new string per line … well I guess that’s less bad, but if you wanted, you can also take the flat_map
one step further and only .collect
once:
img.rows()
// map each row to a line of ASCII characters (terminating with a newline)
.flat_map(|row| {
row.flat_map(|lumi| repeat_char(lumi_8_to_char(lumi.0[0]), horiz_repeat_count))
.chain(std::iter::once('\n'))
})
// collect all the rows into a single string
.collect::<String>()
Also, we have (fairly new) the
function, so (in the flat_map
ped use case) your helper can be skipped entirely 
row.flat_map(|lumi| std::iter::repeat_n(lumi_8_to_char(lumi.0[0]), horiz_repeat_count))
Looking at code like this
let width_mult_factor = match config.mode {
HorizontalAdjustmentMode::Stretch => config.amount,
_ => 1,
};
note that it would generally be preferred to write out the other case explicitly, too.
You’ll gain the guarantees of exhaustiveness checks, so future refactors to HorizontalAdjustmentMode
would make the compiler point out all the relevant places in the code you’d need to touch.
Pair this with a consistent ordering of the match arms, i.e.
let width_mult_factor = match config.mode {
HorizontalAdjustmentMode::Stretch => config.amount,
HorizontalAdjustmentMode::Repeat => 1,
};
and
let horiz_repeat_count = match config.mode {
HorizontalAdjustmentMode::Stretch => 1,
HorizontalAdjustmentMode::Repeat => config.amount,
} as usize;
and it becomes more clear that these two sections of code are deliberately implementing a different logic.
Another minor thing: It looks like img_to_ascii
doesn’t need ownership of its argument (only calling the rows
iterator)
fn img_to_ascii(img: image::GrayImage, config: &Config) -> String {
so you can make the argument by-reference
fn img_to_ascii(img: &image::GrayImage, config: &Config) -> String {
// generate and print ASCII art
println!("{}", img_to_ascii(&downsampled_image, &config));
It doesn’t matter because you don’t need the ownership in main
anymore either, but correctly declaring the ownership story of functions helps not to get confused in any future refactors.