Conversation
|
|
8fee93f to
34ab58f
Compare
|
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
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? |
|
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. |
| 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"; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
cba984e to
6a7a1d3
Compare
|
Seems to be failing CI. |
Signed-off-by: Aurele Boquet <aurele.boquet@gmail.com>
6a7a1d3 to
81a7656
Compare
|
I had to rebase, there was a refactoring of IvGL which removed the variable |
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