iv: Add min/max/avg and sizing to pixel closeup view #4807

soswow · 2025-06-19T12:53:48Z

This changeset has bunch of improvements for pixel closeup window feature:

Min/max/avg values
RGB channels are colored with respective color
Pixel colors in preview window are showing actual colors (previously there was semi-dimming overlay on top of them)
Center pixel is now marked including corner cases when close to the edge
During follow cursor mode window switches the the left from the cursor when too close to right edge, and at the top when too close to bottom edge
Number of previeable pixels is configurable
Number of pixels in a preview used for averaging is configurable
-- When avg subset is smaller then main closeup window there is an indicator showing are that being averaged

Signed-off-by: Aleksandr Motsjonov <[email protected]>

…ntax highlight contents of a string Signed-off-by: Aleksandr Motsjonov <[email protected]>

…sizing-for-color-picker

…nd cyan colors brighter Signed-off-by: Aleksandr Motsjonov <[email protected]>

Signed-off-by: Aleksandr Motsjonov <[email protected]>

…close up view Signed-off-by: Aleksandr Motsjonov <[email protected]>

… + clang-format everything Signed-off-by: Aleksandr Motsjonov <[email protected]>

Signed-off-by: Aleksandr Motsjonov <[email protected]>

…for-color-picker

Signed-off-by: Aleksandr Motsjonov <[email protected]>

soswow · 2025-06-19T12:55:28Z

Let's start the reviewing started. I make code as readable and understandable as possible that includes use of verbose variable names as well as comments.

Happy to squash all the commits

lgritz · 2025-06-19T18:12:18Z

* Happy to squash all the commits

No need to do that in the PR, it all gets squashed when we do the merge.

Signed-off-by: Aleksandr Motsjonov <[email protected]>

soswow · 2025-06-19T22:37:58Z

I never liked that ::setw business that I've introduced. And it's causing issues with different environments/compilers. Let me see if I can have an alternative ...

Signed-off-by: Aleksandr Motsjonov <[email protected]>

antond-weta · 2025-06-19T23:13:00Z

src/iv/ivgl.cpp

+                      center_pix_value_text_color);
+        y_text += text_line_height;
+
+        // TODO Find a nicer way of doing this.


Would it be worth looking at using a table widget to render the table? Something like QTableWidget/QTableView?

Honestly, if you want to just use a mono space font and regular character alignment, that's probably fine if you don't want to go through the trouble of fancier table layout.

I will take a look. There are a lot of extra stuff that I don't like just to manage the column width. If it makes less code why not. But thx for leaving me an option to keep it.

@antond-weta I've consulted with LLM, since I am not expert in Qt. It conclusion is that my current approach might be better since it is fully OpenGL integrated, where is Widget might bring performance and management overhead.

I'm not sure the pixel view window is likely to be a performance-critical thing. I bet a widget is just fine, if it allows for things like easier alignment or automatically adding scroll bars if there are a huge number of channels or very long channel names.

@lgritz scroll bar won't help, since in either of the mode (following corner or in corner) pixel preview window will not allow cursor over it (and so not possible to operate scroll)

The pivot solution would be to show all that in the box left-bttom simiar to what area prop is doing right now.

There will be no width constraint

It makes sense for two tools to have similar UX anyway, since they are doing similar thing.

If this approach is to be done - it should be prob. be done as a next step and not part of this PR, otherwise it will get too big.

I don't think a full solution needs to be part of this PR at all, beyond making sure that the formatting doesn't break down immediately upon getting even a moderately long channel name.

I think either of the following are fine for now:

Make sure the window is wide enough to accommodate 16 characters at least for the channel name, and truncate the name beyond that.

An option somewhere that lets you toggle whether or not the area min/max/avg are shown. Most people don't need it, so maybe those who do are willing to choose either seeing longer channel names or seeing the extra stats, but not both at the same time?

The only thing we must avoid is for the view to look bad any time the channel names are more than a couple characters. It's very common to have longer names for extra channels beyond R, G, B, and A.

@lgritz
Before I go farther with either adding more options to settings (that would control min/max/avg) or increasing box size to fix 6 more characters I did this:

https://www.loom.com/share/710910fdb5274f6b902cb8173560a1e3

Basically two main changes - truncated name up to 10 characters and limiting size of each float value to 5 characters. You can see in the video how precision changes according to how many digits in whole part. So, it would be 123.4, 12.34, 1.234 etc. I wonder what you think of this approach?

I think that's probably fine for now. If people complain, we can adjust in a follow-up.

I think the N significant digits approach is fine -- after all, if values are > 1.0, it has to be a floating point format anyway (either half or float). I think 4 digits, as you illustrate here, is sufficient for half, and matches what we use now.

lgritz · 2025-06-19T23:17:46Z

Aha, it looks like you're doing exactly what I was going to suggest (but haven't had time today to comment again): replace the setw business with the new format calls.

lgritz · 2025-06-19T23:25:41Z

If the Strutil::fmt::format business is rubbing you wrong by being too verbose, I don't have a problem with using Strutil::fmt::format; (inside the scope of a function, not globally) and then you can just say format(...) instead of needing multiple layers of namespacing.

lgritz · 2025-06-20T17:54:34Z

src/iv/ivgl.cpp

+        if (stat.name[0] == 'R') {
+            channelColor = QColor(255, 50, 50);
+        } else if (stat.name[0] == 'G') {
+            channelColor = QColor(100, 255, 90);
+        } else if (stat.name[0] == 'B') {
+            channelColor = QColor(107, 188, 255);


To my old eyes, the blue (which is definitely blue-ish, but not super saturated) is very readable and pleasant. The green somewhat less so, and the red somewhat straining to read because it's so saturated and with high contrast. Maybe make the G and B colors also a bit less saturated?

All three should have a similar level of saturation, perceptually, to ease readability. You want enough color that it clearly connotes red, green, and blue, but not so much color contrast that it becomes a strain to read.

Amendment: your colors actually look pretty good when the image is black or quite dark, but when the image is fairly light, saturated-red-on-white is harder to read.

And we haven't even got into territory of a11y :-D Alright. let me play with that. and see what I came out with that would be better. I did spent a bit of time trying to find balance between saturation and lightness, but more can be done, I agree.

what I came up with so far. They are farily desaturated (like blue is) but prettu light.

I think that's much more readable!

lgritz · 2025-06-20T18:10:31Z

The code all looks great. I built it locally and ran it, and like it a lot overall. These comments are purely suggestions from a UX perspective.

I already commented inline about the color saturation issue.

I think it would help to have a bit more visual separation between the value of the center point (which is the main information most people are looking for) and the sampling window min/max/average. Just to kind of illustrate, here's what I mean

         val    min    max    avg
     R: 0.000  0.000  0.000  0.000
     G: 0.000  0.000  0.000  0.000
     B: 0.000  0.000  0.000  0.000

possible alternative:

         val     region min    max    avg
     R: 0.000         0.000  0.000  0.000
     G: 0.000         0.000  0.000  0.000
     B: 0.000         0.000  0.000  0.000

Try seeing what happens with longer channel names, and more channels:

 $ oiiotool --create:type=half 512x512 10 --chnames R,G,B,A,long_name.R,long_name.G,long_name.B,very_long_name.R,very_long_name.G,very_long_name.B -o channels.exr
 $ iv channels.exr

You can see that now there are some issues with alignment and things running off the edge of the window.

soswow · 2025-06-21T03:51:16Z

I didn't realize there can channels names longer then one character o_O
Yeah, all of these problems are due to limited realestate I have =
Not sure yet how this can be solved, tbh

Signed-off-by: Aleksandr Motsjonov <[email protected]>

soswow · 2025-07-13T11:43:54Z

@lgritz I can't come up with a good idea how to add that extra spacing you want.

The space is the problem. One solution is to make whole thing bigger (wider)

lgritz · 2025-07-13T14:56:46Z

I think the normalized values are the important data, it should be first, and use normalized for the min/max/avg. The encoded value can be in parenthesis:

      Val             min    max    avg
R:  0.0305 (19978)   0.300  0.309  0.306

Maybe there should be an option somehow for whether or not to show the min/max/avg. If those are not present, the channel names can be a lot longer before there's any horizontal spacing problems. Maybe people are ok making the tradeoffs -- you either get to see long channel names clearly, or min/max/avg, but not both?

Signed-off-by: Aleksandr Motsjonov <[email protected]>

lgritz · 2025-07-15T17:24:20Z

Is this PR ready to do a final review and merge now?

Does it need a git rebase main to ensure that there are no conflicts?

Signed-off-by: Aleksandr Motsjonov <[email protected]>

soswow · 2025-07-16T00:26:17Z

Is this PR ready to do a final review and merge now?

I guess.
I've made some final changes trying to accomodate your suggestion around visually separating center value value stats.
See demo with voice over here https://www.loom.com/share/0b19cfe33d6a4d9baadfd921c6cf1970

I think the normalized values are the important data, it should be first
I know that Anton more interested in raw value, for example. This can be def. improved by having an option in settings. With that opition we would be able to unify experience between this picker and area probe, which currently shows only normilized value even for RAW file, for example. This feature can def. wait for follow up PR.

lgritz · 2025-07-16T05:24:36Z

src/iv/ivgl.cpp

+            min = format("{:<5}", min_val);
+            max = format("{:<5}", max_val);
+            avg = format("{:<5}", avg_val);
+        } break;


Tiny nit: This is a little bit odd looking and made me wonder what was going on here when I first saw it. Perhaps

Suggested change

} break;

break;

}

would be slightly less surprising???
(And of course the other similar places as well.)

Fixed that. Btw, I wound it like that already =)

lgritz · 2025-07-16T05:30:10Z

We are SO CLOSE.

On the code: Looks fine. I made one minor comment/suggestion, but that's all.

On the UX: I think it's really attractive, a big improvement over what we had before, and I think we should just stick to this for now, and at least let people try it out and see if there's anything that needs adjustment later. (My own preference would be to list min/max/avg as normalized values, I'm not sure what the benefit is of showing encoded values there? But I'm willing to let people try it and see, maybe it's just me who thinks that way.)

On the correctness: I downloaded the patch and tried it on my end. Works fine on files with integer pixels, but I tried an exr file with half pixels, and I think the values are totally wrong. It only displayed things very close to 0.000, even though the values were quite bright. Maybe you are somehow treating half as if it were uint16 and doing an extra /65535? I'm only guessing, I didn't try to figure it out from the code.

antond-weta · 2025-07-16T05:49:23Z

I'm not sure what the benefit is of showing encoded values there?

I would like iv to be a functional raw image viewer. I really hope we will get there!
The more I use other raw image viewers, the more I learn that I can't trust them. Looking at a raw file (when debugging raw conversion), I only care about the encoded values. For any other file type, if the Mac Preview is not good enough, I launch Nuke.

I do understand that my use case is far from common. I'll see myself out.

lgritz · 2025-07-16T06:11:42Z

@antond-weta I should clarify.

I, too, find value in the raw encoded value of the actual pixel, in addition to the normalized value. Because it's useful to know what the encoded value was.

But I'm not as sure that it's as useful for the regional min/max/avg to show an encoded value, compared to knowing those normalized value. Especially for the average, which may well be a value that isn't encoded in the file at all. That's what I was talking about, sorry if it was unclear.

Signed-off-by: Aleksandr Motsjonov <[email protected]>

soswow · 2025-07-17T00:44:12Z

@lgritz I've fixed HALF problem. In order to do that I had to actually understand what I was doign with values :-D while doing this I noticed word Iterator in the ImageBuf::ConstIterator which allowed finally to extract a bunch of logic, which I tried to do couple of times.
So, now it's all extracted and I am very happy.

As for stats values being encoded values or normilized - I want to hear if @antond-weta interested in those being encoded for his use case. If he wants that - I will introduce some more configuration setting in the next PR where I can 1) toggle stats completely on/off (and allow longer channel names when off) as well as 2) choose norm or encoded values for stats.

antond-weta · 2025-07-17T00:46:48Z

Thanks Larry,

Some potential use cases:
Min/avg encoded value is useful for checking the black levels.
Max/avg is useful for checking the clipping levels without pixel hunting on the specular highlights is there is no substantial size clipping area in the image.
Avg over a grey card gives the codevalue the camera puts the avg scene luminance at in spot metering mode.

soswow · 2025-07-17T01:15:51Z

hm. I just realized that even for RAW currently "encoded" is not actual values but interpreted to 16 bit scale:

In here, my image is CR3 made with R6, which produces 14 bit depth images ... but iv shows 65535 as clipping value, which is 16bit value. So, I can see how encoded value here is not much better then normilized one.

lgritz · 2025-07-17T01:25:55Z

hm. I just realized that even for RAW currently "encoded" is not actual values but interpreted to 16 bit scale:

Yeah, let's sweep that under the rug for now so we can merge what we have. There's room for further refinement for sure.

The OIIO way is to have the app side of the API only deal with 8/16/32 bit types, no "exotic" bit depths, so all values are scaled up to the next byte/word size. This is just to reduce the space of possible cases apps need to deal with. When this happens -- such as a 12 bit DPX being presented to the app with 16-bit range -- it is customary for the reader plugin to set a special attribute "oiio:BitsPerSample" to the true bit depth that was stored in the file. So we could check that attribute and re-scale again for the sake of the values we print out as the encoded values.

Of course, once you peel that layer off, there are others as well: if viewing PNG files (which require "unassociated alpha"), should we be showing the encoded value as the original un-premultiplied color (again, OIIO automatically premultiplies so that every app doesn't need to figure out how to handle that)?

lgritz

LGTM, I'm satisfied with this. Is this ready to merge, @soswow ?

soswow · 2025-07-17T01:30:02Z

@lgritz I am. Do you want to do some more testing on your end? Just in case. And if you don't see anything else, let's merge it.

lgritz · 2025-07-17T01:30:18Z

Thanks Larry,

Some potential use cases:

Yeah, all very legit.

I was thinking more from a lookdev/lighting background, maybe the values you see as the regional min/max are going to be things you want to type into a lookdev slider or shader code, etc., so those would all require normalized values.

So a few different use cases (a.k.a. pools of users) to potentially consider.

lgritz · 2025-07-17T01:34:53Z

@lgritz I am. Do you want to do some more testing on your end? Just in case. And if you don't see anything else, let's merge it.

I built on my end and tried on a couple files (one half, one uint8). Both seemed to work great!

I really like the use of color in the display. Very cool.

…Foundation#4807) This changeset has bunch of improvements for pixel closeup window feature: - Min/max/avg values - RGB channels are colored with respective color - Pixel colors in preview window are showing actual colors (previously there was semi-dimming overlay on top of them) - Center pixel is now marked including corner cases when close to the edge - During follow cursor mode window switches the the left from the cursor when too close to right edge, and at the top when too close to bottom edge - Number of previeable pixels is configurable - Number of pixels in a preview used for averaging is configurable -- When avg subset is smaller then main closeup window there is an indicator showing are that being averaged --------- Signed-off-by: Aleksandr Motsjonov <[email protected]>

soswow added 14 commits June 6, 2025 22:32

Add min/max/average and control over size of color picker preview window

9245574

Signed-off-by: Aleksandr Motsjonov <[email protected]>

Merge branch 'main' into move-glsl-into-files

757bb2e

Signed-off-by: Aleksandr Motsjonov <[email protected]>

After merge-conflict fix

d536287

Signed-off-by: Aleksandr Motsjonov <[email protected]>

Merge branch 'main' into add-avg-and-sizing-for-color-picker

f464d18

Add raw string syntax modifier for VSCode and Cursor to be able to sy…

9fe6c27

…ntax highlight contents of a string Signed-off-by: Aleksandr Motsjonov <[email protected]>

Merge branch 'add-syntax-modifer-to-shader-strings' into add-avg-and-…

32f0535

…sizing-for-color-picker

Add separate settings control for avg value section and make yellow a…

f07fd4e

…nd cyan colors brighter Signed-off-by: Aleksandr Motsjonov <[email protected]>

Add tooltip to new settings controls

4d8550e

Signed-off-by: Aleksandr Motsjonov <[email protected]>

Add coordinates of measured pixel

c5e1f5e

Signed-off-by: Aleksandr Motsjonov <[email protected]>

shifting current measured pixel indicator when looking at edges with …

2a525f1

…close up view Signed-off-by: Aleksandr Motsjonov <[email protected]>

Fix problem with showing min/max calues outside data window for float…

ff11760

… + clang-format everything Signed-off-by: Aleksandr Motsjonov <[email protected]>

Keep closeup window within window bounds

716c76c

Signed-off-by: Aleksandr Motsjonov <[email protected]>

Merge remote-tracking branch 'upstream/main' into add-avg-and-sizing-…

4ea3ff9

…for-color-picker

Tidying up: small refactoring, better variable names and comments

e70fa34

Signed-off-by: Aleksandr Motsjonov <[email protected]>

Include explicit header to fix std::setw compilation error

e841931

Signed-off-by: Aleksandr Motsjonov <[email protected]>

soswow force-pushed the add-avg-and-sizing-for-color-picker branch from 43cf9ac to e841931 Compare June 19, 2025 22:20

Do not use std:setw streaming approach but simple string formattting

730918c

Signed-off-by: Aleksandr Motsjonov <[email protected]>

soswow force-pushed the add-avg-and-sizing-for-color-picker branch from 825c2b4 to 730918c Compare June 19, 2025 23:07

antond-weta reviewed Jun 19, 2025

View reviewed changes

lgritz reviewed Jun 20, 2025

View reviewed changes

soswow added 2 commits July 13, 2025 17:39

Do local using of format

87866e7

Signed-off-by: Aleksandr Motsjonov <[email protected]>

Make channel colors more uniform in perfecption

4f794a2

Signed-off-by: Aleksandr Motsjonov <[email protected]>

lgritz added the iv Image viewer label Jul 13, 2025

Add support for longer channel names

c4a2619

Signed-off-by: Aleksandr Motsjonov <[email protected]>

soswow force-pushed the add-avg-and-sizing-for-color-picker branch from f3931f3 to c4a2619 Compare July 15, 2025 00:36

soswow added 3 commits July 16, 2025 10:10

Add visual separation between center value and aggregate ones

9958420

Signed-off-by: Aleksandr Motsjonov <[email protected]>

Merge branch 'main' into add-avg-and-sizing-for-color-picker

ecc2f56

Fix formatting

3816999

Signed-off-by: Aleksandr Motsjonov <[email protected]>

lgritz reviewed Jul 16, 2025

View reviewed changes

Handle half-format properly and DRY stats calc code

a34ecd3

Signed-off-by: Aleksandr Motsjonov <[email protected]>

lgritz approved these changes Jul 17, 2025

View reviewed changes

lgritz changed the title ~~Add min/max/avg and sizing to closeup window~~ iv: Add min/max/avg and sizing to pixel closeup view Jul 17, 2025

lgritz merged commit 64ee518 into AcademySoftwareFoundation:main Jul 17, 2025
31 checks passed

BrewTestBot mentioned this pull request Oct 2, 2025

openimageio 3.1.6.1 Homebrew/homebrew-core#246692

Merged

iv: Add min/max/avg and sizing to pixel closeup view #4807

iv: Add min/max/avg and sizing to pixel closeup view #4807

Uh oh!

Conversation

soswow commented Jun 19, 2025

Uh oh!

soswow commented Jun 19, 2025

Uh oh!

lgritz commented Jun 19, 2025

Uh oh!

soswow commented Jun 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soswow Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgritz commented Jun 19, 2025

Uh oh!

lgritz commented Jun 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgritz commented Jun 20, 2025

Uh oh!

soswow commented Jun 21, 2025

Uh oh!

soswow commented Jul 13, 2025

Uh oh!

lgritz commented Jul 13, 2025

Uh oh!

lgritz commented Jul 15, 2025

Uh oh!

soswow commented Jul 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgritz commented Jul 16, 2025

Uh oh!

antond-weta commented Jul 16, 2025

Uh oh!

lgritz commented Jul 16, 2025

Uh oh!

soswow commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antond-weta commented Jul 17, 2025

Uh oh!

soswow commented Jul 17, 2025

Uh oh!

lgritz commented Jul 17, 2025

soswow Jul 14, 2025 •

edited

Loading

soswow commented Jul 17, 2025 •

edited

Loading