Skip to content

fix(iv): 'save window' saves only the visible region - #5479

Open
orelsin wants to merge 1 commit into
AcademySoftwareFoundation:mainfrom
orelsin:fix/iv-save-window-roi
Open

orelsin wants to merge 1 commit into
AcademySoftwareFoundation:mainfrom
orelsin:fix/iv-save-window-roi

Conversation

@orelsin

@orelsin orelsin commented Sep 17, 2026

Copy link
Copy Markdown

Issue: "Save Window As..." called ImageBuf::write() on the whole image, which made it do exactly the same thing as "Save As...".

Add IvGL::get_visible_image_roi(), which maps the two opposite corners, sorts out which is the min and which is the max, and intersects the result with the data window so only real pixels come back.
saveWindowAs() then writes ImageBufAlgo::cut() of that region instead of the whole buffer, returns an error if nothing is visible.

Closes #4716

Assisted-by: Claude Code / Claude Opus 5

@linux-foundation-easycla

linux-foundation-easycla Bot commented Sep 17, 2026

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

  • ✅ login: orelsin / name: Aurele Boquet (6a7a1d3)

@orelsin
orelsin force-pushed the fix/iv-save-window-roi branch 2 times, most recently from 8fee93f to 34ab58f Compare September 17, 2026 15:04
@lgritz

lgritz commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

Need CLA and formatting fix.

I built it on my side, and it did not behave quite like I expected.

Maybe we should establish a common understanding of what this does. What do you think is the difference between

  • save as
  • save window as
  • save selection as

Trying them on my end, I could discern no difference at all. Now I'm a little confused about what we originally wanted and/or really should do.

I have some ideas, but before I pollute the space with my notions, I'd like to hear what you and others think.

What was your recipe for testing this? Do you have a sequence of actions that results in different saved results for the three menu choices?

@lgritz

lgritz commented Sep 18, 2026

Copy link
Copy Markdown
Collaborator

Wait! I take it back!

I was testing the wrong branch.

This does seem to work -- "save window as" does save the part of the image that's visible in the window as a separate image. So you can zoom, adjust window boundaries, etc., and it acts as a cropping tool.

Comment thread src/iv/imageviewer.cpp Outdated
ROI roi;
glwin->get_visible_image_roi(roi);
if (!roi.defined() || roi.width() <= 0 || roi.height() <= 0) {
std::cerr << "Save failed: no pixels of the image are visible\n";

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We're trying to slowly get rid of << stream I/O.

Would you mind changing these to

OIIO::print(stderr, "...");

But actually, in the long run, we don't really even want that. I think that stderr doesn't go anywhere useful in a Windows GUI app, and even on Linux/MacOS, if you don't happen to have the terminal window that launched iv visible, you won't see the error.

I think that in a GUI app, it's better to have a pop-up dialog when something goes wrong. That's kind of out of scope for this PR's limited goals, so if you don't want to go that far, it's ok. But you should at least put a

    // TODO: May not be visible. This should be replaced with a GUI dialog.

comment in these places where you print to stderr.

Thanks.

@orelsin orelsin Sep 18, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, I had the same thought, but I told myself it was out of scope for this issue. I’d be happy to do it, should I create another merge request for this, or can I include it in this one?
Same question for ‘Save selection as…’, which currently works exactly the same as ‘Save as…’

I will take care of the CLA and the formatting issue

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

For NEW code or lines you have to change anyway, I would like you to use the preferred OIIO::print rather than introduce new cerr << lines. But I don't want you to go hunting for other instances unrelated to the purpose of this PR. (If somebody wants to make an isolated PR where they do track those down and bring them to modern idioms, that would be fine, just not mixed with other tasks.)

For this PR, I would be happy with the // TODO to explain where it should really be a dialog box. But let's get this PR wrapped up simply, and if you want to go back and do the dialogs separately, that would be very welcome also.

@orelsin
orelsin force-pushed the fix/iv-save-window-roi branch from cba984e to 6a7a1d3 Compare September 18, 2026 14:13
@lgritz

lgritz commented Sep 21, 2026

Copy link
Copy Markdown
Collaborator

Seems to be failing CI.

Signed-off-by: Aurele Boquet <aurele.boquet@gmail.com>
@orelsin
orelsin force-pushed the fix/iv-save-window-roi branch from 6a7a1d3 to 81a7656 Compare September 21, 2026 12:44
@orelsin

orelsin commented Sep 21, 2026

Copy link
Copy Markdown
Author

I had to rebase, there was a refactoring of IvGL which removed the variable m_current_image and replaced it with an image-slot array. I changed the cerr << statements to OIIO::print(stderr, ‘...’) and added the TODOs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

iv: "Save window" doesn't work -- it saves the whole image

2 participants