search

Tom94 / tev

High dynamic range (HDR) image viewer for graphics people
BSD 3-Clause "New" or "Revised" License
1.02k stars 86 forks source link

WIP: Statistics only over selected regions of the image #215

Closed iRath96 closed 6 months ago

iRath96 commented 6 months ago

Hi there,

this PR adds the possibility to hold alt/option while drag clicking to mark a region in the image over which statistics in the MultiGraph should be computed. To reset the region to the full image, you can hold alt/option and right click. This is useful to quickly compare averages/histograms of specific regions only, or look at the error between two images in a specific region only.

⚠️ I added the necessary code to the OpenGL backend, but was not able to test it, since #version 110 is not supported on my system (same for GLES). The Metal backend is working.

Let me know if you're interested in such a feature at all. In that case could also invest some time to find an OpenGL system to test on.

Thanks!

Tom94 commented 6 months ago

Hi, thanks for the PR — this is a nice feature to have for sure! I am traveling over the new year break, so will need another week or two until I have a chance to review in depth.

Quickly scanning over the code on my phone, it already looks in good shape. Just some high level comments:

  1. cropMin, cropMax, and isCropped are passed around quite a bit. Why not make a Rect2i { Vector2i min, max; } (or Box2i) struct near where matrix and vector ops are defined and pass around a std::optional of that?
  2. Permitting the storage of swapped min/max points (and fixing it up on demand) feels janky. I’d prefer if the member variables would always be in a valid state. Ideally also a std::optional.
  3. I think I’d prefer alt+left click (without drag) as the reset mechanism. That way, the left mouse button is uniformly responsible for the crop window. And the right button remains free for future functionality.

thanks again!

iRath96 commented 6 months ago

Hi, thanks for the great feedback! I think I'll also need a week to get around to implement this, so no hurry. By then I should also be able to test the OpenGL side of things. Until then, have a nice travel and a good start into 2024 👍🏻

iRath96 commented 6 months ago

@Tom94 hope you had a great new year break!

I have implemented your feedback and have tested the PR on macOS 14.2 (with Metal backend) and Ubuntu 22.04 (with OpenGL backend) now. The code is now ready for review whenever you have time 👍🏻 In particular Windows and GLES might still need testing.

While testing, I noticed that tev (even before this PR) would compute the statistics of a 8192x8192 white image as "min: 1; max: 1; mean: 0.25". I have changed the mean accumulator to double precision to avoid such inaccuracies (which also occur at smaller resolution, albeit less noticeably), at the cost of a bit higher computational burden. If that is not acceptable, I can of course revert this change.

Tom94 commented 6 months ago

Thanks! Happy new year to you, too. The fix for round-off error during the mean computation is great -- I suppose a more robust fix would be a hierarchical sum reduction, but double should be sufficient for all images sizes tev can load already.

I made a few adjustments and additions to the PR; here's a clip of how things behave now:

https://github.com/Tom94/tev/assets/4923655/a127f4a1-a52a-410c-93a1-aaf81d8064aa

I don't have a Windows machine to test nearby, so won't merge just yet.

Tom94 commented 6 months ago

Confirmed working on Windows & GLES 2 based Arm64 Ubuntu.

I also made a last-minute change to the keyboard shortcut: it's now Hold-C + Drag rather than Alt. Reason being that Alt+Click/Drag is used by many window managers as the default shortcut to drag windows around and to resize them. (See also tools like AltDrag for Windows.)

Thanks again for the contribution!