Fix #4718 do not resize on open and other zoom fixes #4766

soswow · 2025-05-16T03:57:03Z

Description

See the demo of problems that were fixed and of fixes in action over here: https://www.loom.com/share/3bdee20b32434e0bbba47d393928d4f7

There are fixed for three problems:

Window is not resizing when new image is loaded in (it still does when image being loaded is the only one)
When you using scroll mouse wheel it is taking into account where the cursor is (there is some corner case when it doesn't do it perfectly, that I can't catch when it doesn't follow it exactly, but a bit of panning fixes it)
~~3) Switching between images recenters (if image is smaller) or making some other panning nessery to have image in the viewport. I believe it is doing same stuff what happens when window is resized.~~ This has been reverted.

Checklist:

I have read the contribution guidelines.
I have updated the documentation, if applicable. (Check if there is no
need to update the documentation, for example if this is a bug fix that
doesn't change the API.)
I have ensured that the change is tested somewhere in the testsuite
(adding new test cases if necessary).
If I added or modified a C++ API call, I have also amended the
corresponding Python bindings (and if altering ImageBufAlgo functions, also
exposed the new functionality as oiiotool options).
My code follows the prevailing code style of this project. If I haven't
already run clang-format before submitting, I definitely will look at the CI
test that runs clang-format and fix anything that it highlights as being
nonconforming.

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

linux-foundation-easycla · 2025-05-16T03:57:08Z

The committers listed above are authorized under a signed CLA.

✅ login: soswow / name: Aleksandr Motsjonov (c933689, 00fe749, cf11852, 1c29aef, ef3c806, 93ff5f7, 472fdbb, 8815eee)

soswow · 2025-05-16T04:00:00Z

.gitignore

 /*.exr
 /*.tif
 /*.jpg
+/*.png


since I had my own test images and they were .png I thought it would be ok

soswow · 2025-05-16T04:00:49Z

src/iv/imageviewer.cpp

        add_image(filename);
-        //        int n = m_images.size()-1;
-        //        IvImage *newimage = m_images[n];
-        //        newimage->read_iv (0, false, image_progress_callback, this);


not sure if this and all the other commented parts of the code are there for a reason, so feel free to ask to revert if you believe it is needed

git says they have been unchanged for 17 years, so I think it's safe to say they are not needed. :-)

soswow · 2025-05-16T04:01:16Z

src/iv/imageviewer.cpp

        if (m_images.size() > 1) {
            // Otherwise, add_image already did this for us.
            current_image(m_images.size() - 1);
-            fitWindowToImage(true, true);


that stops window from resizing when new images are loaded in

soswow · 2025-05-16T04:02:13Z

src/iv/imageviewer.cpp


    if (update) {
        glwin->update();
+        glwin->clamp_view_to_window();


that what make sure loaded image (when switching between images for example) is in the view. if images is smaller then viewport this will center it as well

soswow · 2025-05-16T04:03:36Z

src/iv/imageviewer.cpp

+
+    float newzoom = floor2f(current_zoom);
+
+    this->zoomToCursor(newzoom, smooth);


Refactor zoomIn and zoomOut to use common code (new zoomToCursor function) that to be able to zoom at cursor with or without animation.

soswow · 2025-05-16T04:04:42Z

src/iv/ivgl.cpp

+            m_viewer.zoomIn(false);
+        } else {
+            m_viewer.zoomOut(false);
+        }


that fixes the zooming with mouse wheel to use mouse cursor position

soswow · 2025-05-16T04:05:59Z

src/iv/ivgl.h

                            GLenum& gltype, GLenum& glformat,
                            GLenum& glinternal) const;

+    void clamp_view_to_window();


This is the only change I Am not confident about. In order to use clamp_view_to_window I had to move it from protected to public. I don't know if this is a big deal. I seek some guidance here.

not a big deal, this is all internal code anyway, not public classes

lgritz · 2025-05-16T06:02:22Z

Overall, the code looks great (needs a clang-format pass) and you've improved a number of things.

Honestly, the only thing I'm unsure about how you've done is the centering of each image in the view. I think if one is viewing a number of unrelated images of different sizes, that's probably fine. But if you are looking at related images that are occupying the same abstract pixel coordinate system, if you know what I mean, perhaps it's better for them to stay aligned so that as you switch between images, pixel (2,42) on each image is always at the same spot on screen as you cycle through.

I think that the old behavior kept the same coordinate system as you cycled through images instead of repositioning it as you go from one image to another. It's not clear to me whether or not people will view that as a welcome change.

Actually, as I'm thinking about it, there are at least a few different (and contradictory) behaviors people may desire for different use cases, when there are images of different sizes:

A single coordinate system that stays in the same position relative to the view window (I mean you can scroll around, but merely Up/Dn to cycle through the image list doesn't move the coord system).
Images always align themselves individually to be best viewed (centered, as you come to each image? at least until until you use the mouse to drag it to a different position, then maybe that sticks?).
Images align but also zoom to the window size, so that, for example, if you are viewing one 1k image and one 2k image of the same scene, they both appear the same size in the window, so you can flip back and forth between them and they stay aligned. (At least until you zoom in or out, then the zoom level you selected sticks?)

I'm kind of rambling. I don't want to keep expanding the scope of this one PR. Maybe the best policy is to stick to option 1 (as we had before, I think) but then consider a subsequent pass (or at least filing a reminder issue) about allowing users to choose any of those strategies. That is, unless other viewers in general agree on a well established convention for doing this that I'm unaware of, in which case we should align to that for the sake of user expectations.

Thoughts?

soswow · 2025-05-16T06:17:50Z

@lgritz

, if you know what I mean, perhaps it's better for them to stay aligned so that as you switch between images, pixel (2,42) on each image is always at the same spot on screen as you cycle through.

yeah ... When I saw various feature requests that involve some sort of relationship between images in the set (showing a diff or having a split) I already thought about the fact that iv might be often used with images of the same size.

hm ...

I just tried and it works actually well. The only difference from master is when you see image edge in the image (and some black void behind it). Before it would just switch between keeping same position. Now it would make sure whole viewport is filled out. See norated video for illustration: https://imgur.com/a/hY0EVuv

Let me see if I can improve by combing best of both worlds and if not - I Can drop that part for now.

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

lgritz · 2025-05-16T06:57:25Z

yeah ... When I saw various feature requests that involve some sort of relationship between images in the set (showing a diff or having a split) I already thought about the fact that iv might be often used with images of the same size.

And keep in mind an important special case of this: images that are different sizes (in terms of the actual pixel array, what we'd call "data window" for openexr files) but are using a common coordinate system overall and should be aligned to it.

An example of that is that it's common for openexr files to be "shrink wrapped" where it only stores the pixel/data window of the smallest rectangle that completely encloses all the non-black pixels. So if you have two frames from a shot of a space ship that's moving, and those frames are shrink wrapped, maybe on frame 1 the image's data window is (100,100) - (200,200) [thus a "resolution" of 100x100] and on the second frame it is farther right and down an has changed orientation so now the nonzero pixels that are stored are (250,250)-(400,350) [thus a data "resolution" of 150x100, and with a different upper-left origin].

As you flip between those two images, they shouldn't display "on top of" each other, nor should they be aligned to the upper left corner of the screen. One has its upper left corner of pixels at coordinate (100,100) and the other at coordinate (250,250) on a common coordinate system that should not shift around within the viewing window just because you are switching from one image to the other.

I hope that makes some kind of sense.

lgritz · 2025-05-16T07:17:51Z

I'm just thinking aloud here, don't consider this a firm position. :-) But I'm wondering what people think about this strategy I'm about to outline:

Initially, you inventory the images specified on the command line, and take basically the union of their respective "full" resolutions (that's the OIIO term, it's what OpenEXR calls "display window", and is taken from the ImageSpec's full_x, full_y, full_width, full_height). Not the regular pixel data resolution (ImageSpec's x, y, width, height, which correspond to what OpenEXR calls the "data window").

If that combined full size fits on the screen, you start with zoom = 1.0 and make the screen window the size of that combined full size, but if it's too big to fit on the display, you make the window as big as can fit on the display, and set the zoom to whatever makes the combined full size just barely fully fit, centered, in that window (zoom will thus will be something < 1.0). Either way, this establishes a mapping between the pixel coordinate system and the on-screen display showing the images.

At that point, the user dragging with the mouse, or zooming (mouse wheel or clicks or hotkeys) moves or zooms the coordinate system relative to the on-screen viewer window. But up/down to switch between images does not -- they all just display relative to what their ImageSpec says their placement should be on that pixel coordinate system. Opening/loading a new image that we didn't start with also should not change the coordinate system at all at that point. But there is a hotkey ('alt-f', I think) that recenters and/or resizes the mapping between pixel coordinate system and viewer window to optionally center and fill the current viewer window with the currently displayed image. And also hotkey 'f' changes the viewer window to exactly enclose the currently displayed image in its current position and zoom as it appears on screen.

Reading this over, I suspect that this is what I was originally going for all those years ago, though I may not have correctly implemented all those behaviors.

soswow · 2025-05-16T08:27:22Z

src/libtexture/imagecache_pvt.h

🤷 blame clang-format

This reverts commit 1c29aef. Signed-off-by: Aleksandr Motsjonov <[email protected]>

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

lgritz · 2025-05-16T19:07:53Z

Yeah, the behavior looks reasonable to me now. It's certainly a step forward from what we had before.

soswow · 2025-05-16T22:22:28Z

@lgritz I am glad to hear that. Tell me what can I do else? Are there any unit tests/integration tests that needs to be added? I've assumed no coverage there since nothing is failing

lgritz

All LGTM now. Thanks, this is a solid improvement!

…oundation#4766) Fixes AcademySoftwareFoundation#4718 See the demo of problems that were fixed and of fixes in action over here: https://www.loom.com/share/3bdee20b32434e0bbba47d393928d4f7 There are fixed for three problems: 1) Window is not resizing when new image is loaded in (it still does when image being loaded is the only one) 2) When you using scroll mouse wheel it is taking into account where the cursor is (there is some corner case when it doesn't do it perfectly, that I can't catch when it doesn't follow it exactly, but a bit of panning fixes it) ~~3) Switching between images recenters (if image is smaller) or making some other panning nessery to have image in the viewport. I believe it is doing same stuff what happens when window is resized.~~ This has been reverted. --------- Signed-off-by: Aleksandr Motsjonov <[email protected]>

…oundation#4766) Fixes AcademySoftwareFoundation#4718 See the demo of problems that were fixed and of fixes in action over here: https://www.loom.com/share/3bdee20b32434e0bbba47d393928d4f7 There are fixed for three problems: 1) Window is not resizing when new image is loaded in (it still does when image being loaded is the only one) 2) When you using scroll mouse wheel it is taking into account where the cursor is (there is some corner case when it doesn't do it perfectly, that I can't catch when it doesn't follow it exactly, but a bit of panning fixes it) ~~3) Switching between images recenters (if image is smaller) or making some other panning nessery to have image in the viewport. I believe it is doing same stuff what happens when window is resized.~~ This has been reverted. --------- Signed-off-by: Aleksandr Motsjonov <[email protected]> Signed-off-by: Scott Wilson <[email protected]>

…oundation#4766) Fixes AcademySoftwareFoundation#4718 See the demo of problems that were fixed and of fixes in action over here: https://www.loom.com/share/3bdee20b32434e0bbba47d393928d4f7 There are fixed for three problems: 1) Window is not resizing when new image is loaded in (it still does when image being loaded is the only one) 2) When you using scroll mouse wheel it is taking into account where the cursor is (there is some corner case when it doesn't do it perfectly, that I can't catch when it doesn't follow it exactly, but a bit of panning fixes it) ~~3) Switching between images recenters (if image is smaller) or making some other panning nessery to have image in the viewport. I believe it is doing same stuff what happens when window is resized.~~ This has been reverted. --------- Signed-off-by: Aleksandr Motsjonov <[email protected]>

soswow added 5 commits May 16, 2025 12:13

Do not resize window when new image is added.

00fe749

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

Reuse same zooming method between scroll wheel and clicking

ef3c806

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

add .png for ignored image format when found in root

c933689

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

Clamp view to window when switching between images

1c29aef

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

Remove debugging logging

cf11852

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

soswow commented May 16, 2025

View reviewed changes

soswow changed the title ~~Fix 4718 do not resize on open and other zoom fixes~~ Fix #4718 do not resize on open and other zoom fixes May 16, 2025

Fix clang-format issues

472fdbb

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

soswow commented May 16, 2025

View reviewed changes

src/libtexture/imagecache_pvt.h

Copy link

Contributor Author

soswow May 16, 2025

Choose a reason for hiding this comment

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

🤷 blame clang-format

soswow added 2 commits May 16, 2025 18:34

Revert "Clamp view to window when switching between images"

8815eee

This reverts commit 1c29aef. Signed-off-by: Aleksandr Motsjonov <[email protected]>

Fixing linting issues

93ff5f7

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

soswow force-pushed the fix-4718-do-not-resize-on-open-and-other-zoom-fixes branch from f8783aa to 93ff5f7 Compare May 16, 2025 08:37

lgritz approved these changes May 17, 2025

View reviewed changes

lgritz merged commit 044337b into AcademySoftwareFoundation:main May 17, 2025
30 checks passed

lgritz added bug Crash or wrong behavior of an existing feature. iv Image viewer devdays ASWF Dev Days suitable project labels May 17, 2025

lgritz added the devdays25 DevDays 2025 project label May 17, 2025

BrewTestBot mentioned this pull request Oct 2, 2025

openimageio 3.1.6.1 Homebrew/homebrew-core#246692

Merged


		float newzoom = floor2f(current_zoom);

		this->zoomToCursor(newzoom, smooth);

Fix #4718 do not resize on open and other zoom fixes #4766

Fix #4718 do not resize on open and other zoom fixes #4766

Uh oh!

Conversation

soswow commented May 16, 2025 • edited by lgritz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist:

Uh oh!

linux-foundation-easycla bot commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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 May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soswow commented May 16, 2025

Uh oh!

lgritz commented May 16, 2025

Uh oh!

lgritz commented May 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgritz commented May 16, 2025

Uh oh!

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

Uh oh!

Uh oh!

lgritz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

soswow commented May 16, 2025 •

edited by lgritz

Loading

linux-foundation-easycla bot commented May 16, 2025 •

edited

Loading

lgritz commented May 16, 2025 •

edited

Loading

soswow commented May 16, 2025 •

edited

Loading