atanunq / viuer

Rust library for displaying images in the terminal.
MIT License
241 stars 43 forks source link

iTerm2 printer is wrong aspect ratio due to manually passing in `width` and `height` #18

Closed thorlucas closed 2 years ago

thorlucas commented 3 years ago

I posted this issue initially at: https://github.com/atanunq/viu/issues/65

Comparing this source with imgcat's source makes it clear that the issue is viuer is attempting to manually calculate image size, which does not take into account iTerm2s specific line height/etc settings.

A simple fix to this is to simply not print width={};height={} when no width and height are specified, and allow iTerm2 to instead automatically calculate those.

I'm working on a PR right now for this.

thorlucas commented 3 years ago

So I started working on the PR here (https://github.com/thorlucas/viuer), but I've ran into a bit of an issue: if we let iTerm2 do the calculations on best fit itself (which produces more accurate results than viu's find_best_fit), then we don't have a width and height to return as a ViuResult.

As you can see in my fork, I currently just hacked it together to return "fake" results. I get w, h from find_best_fit but then just ignore those and don't use them in the actual printing. If I had to guess, I would assume that find_best_fit is just sort of trying to calculate assuming that a terminal cell has a specific dimension, which produces the wrong result.

There are two options here as I see it:

  1. Fix find_best_fit by taking this into account. This seems unnecessarily hard (if it's even possible?) and also does computation that is not required (since iTerm2 does this for us).
  2. Update the Result to return ViuResult<(Option<u32>, Option<u32>)> instead, returning only the values we know (which would be the user provided w and h values).

What is ViuResult<(u32, u32)> even used for, anyway? Is there a reason that it is important to return the used width and height?

atanunq commented 3 years ago

Thank you for raising the issue and the detailed write up. It's great to be able to follow your chain of thought.

Across the project I have always assumed a terminal cell has a width to height ratio of 1:2. Unfortunately, it seems that this is not the case.

The ViuResult<(u32, u32)> is used by viu here to know how many rows to go back up when printing GIFs. This may be possible through other means, such as saving & restoring the cursor, for example. Even if doable, though, I think it is important to be aware of the size of the printed image. Having this information will be crucial when trying to place an image only in a particular part of the screen. I think this should remain the default viuer behaviour.

However, a fix for this would be nice to have. It can be put behind a configuration flag, unset by default, and set in viu. Maybe a flag that makes iTerm printer's print_buffer return a fallback value, e.g Ok((0, 0)) and not use width={};height={}. This is fine because for GIFs viu doesn't need to know the printed image's height - it sends the whole GIF to iTerm anyway and lets it handle the looping.

Do you think that makes sense? Would like to hear your thoughts on this

atanunq commented 3 years ago

Any update on this one? I'd rather not implement it unless someone finds it useful