Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Save as Figure #465

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Save as Figure #465

wants to merge 13 commits into from

Conversation

will-moore
Copy link
Member

@will-moore will-moore commented Mar 14, 2024

To test:

  • Open 1 or more images, zoom, pan, change rendering settings and add ROIs.
  • File > Save Viewport(s) as Figure
  • Enter a name for figure
  • Click on link in the success dialog -> Open figure in new tab
  • Only the ROIs (Shapes) visible at the time of export (and intersect with the viewport) will be included in the Figure.
  • Pixel sizes, timestamps, Dataset names etc should be included in the figure

Screenshot 2024-03-19 at 12 34 15

Screenshot 2024-03-19 at 12 35 08

@will-moore will-moore requested a review from pwalczysko March 28, 2024 17:56
@pwalczysko
Copy link
Member

pwalczysko commented Apr 4, 2024

Bug: The panel in the newly created figure is shifted with respect to the ROIs.

  1. Create a "figure from viewport" with couple of precisely drawn freehand polygon ROIs (e.g. "segmentation" of nuclei in a large image) - you can either draw these ROIs manually at the time of opening the image in iviewer, or have them already drawn and saved, or, draw them and save these ROIs. Click the "figure from viewport" menu item, select the name for the new figure and navigate to this figure using the link in the dialog.
  2. In the newly created "figure from viewport" - when inspected in OMERO.figure, observe that the ROIs which were in the viewport when the "figure from viewport" was created are positioned as expected.
  3. Still in OMERO.figure, select the panel, go to Labels tab, click Edit in the ROI section and add the ROIs which are saved from step 1. above in OMERO (these are, or rather should be, the same ROIs which you just transported onto your figure using the new "figure from viewport" feature in iviewer
  4. Observe that the ROIs which you are newly adding now are slightly shifted, always towards bottom right - see screenshot.
  5. Save your figure, close it. Reopen it.
  6. Repeat the addtion of the ROIs step in OMERO.figure as per Ad 3. above - observe again that the newly added ROIs are shifted one more time towards bottom right.
    Screenshot 2024-04-04 at 12 30 03

@pwalczysko
Copy link
Member

RFE:

Some misformatting of the dialog - note the inverted commas are only on one side of the ID (there probably should not be any inverted commas)

Screenshot 2024-04-04 at 11 40 19

Copy link
Member

@pwalczysko pwalczysko left a comment

Choose a reason for hiding this comment

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

Two problems noted, the shift of the pane with respect to ROI position probably needs a full rethink.

@pwalczysko
Copy link
Member

To comment further on the ROI-shifting problem #465 (comment) - this behaviour is present only when working with figures created by the feature implemented in the present PR #465 - the figures which are created within OMERO.figure itself do not have any problems with shifted ROIs, i.e. the ongoing work on ome/omero-figure#477 does not seem to be cause of this, cc @jburel

@will-moore
Copy link
Member Author

@pwalczysko thanks for the testing. Re: #465 (comment) - I think that what is going on here is that OMERO.figure is trying not to add duplicate ROIs on top of one-another. If a Shape already exists when you try to add one, the Shape is automatically offset to the lower-right. In the same way that when you copy and paste a Shape it is offset to the lower-right. If you first delete the Shapes in OMERO.figure before loading and adding them, you should see that they are added in the right place.

@pwalczysko
Copy link
Member

@pwalczysko thanks for the testing. Re: #465 (comment) - I think that what is going on here is that OMERO.figure is trying not to add duplicate ROIs on top of one-another. If a Shape already exists when you try to add one, the Shape is automatically offset to the lower-right. In the same way that when you copy and paste a Shape it is offset to the lower-right. If you first delete the Shapes in OMERO.figure before loading and adding them, you should see that they are added in the right place.

Ah, sorry, thanks for explanation. Indeed, this is as you say, the shapes are shifted only because they are already added once. This behaviour is not different from what is in Figure by default and is not introduced by this PR.

@pwalczysko
Copy link
Member

I will wait for tomorrow's build for retest of the double-quotes efda997

@imagesc-bot
Copy link

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/omero-iviewer-multiple-images-views-exporting-to-omero-figure/94675/2

@pwalczysko
Copy link
Member

Double-quotes are fixed.

The only remaining issue is that points are ignored.
As this is done without warning, I wonder if we should add one ?
If points are not supported in Figure, then this is fine... but still they are supported in iviewer - if someone gets into great lenght and creates a lot of points, then they might be unpleasantly surprised not to find them in figure later...

Copy link
Member

@pwalczysko pwalczysko left a comment

Choose a reason for hiding this comment

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

@will-moore
Copy link
Member Author

@pwalczysko Points are now converted to Ellipses in figure...

Screenshot 2024-04-15 at 17 59 47

@pwalczysko
Copy link
Member

@pwalczysko Points are now converted to Ellipses in figure...

I am really not sure about this. If somebody has already some Ellipses (which were meant to be ellipses) on an image, and adds points, then, in Figure, they get a surprise.

Maybe we can discuss in a meeting tomorrow - my preference would be to drop the points if there are any and warn the user. Offering some distortion of the ROIs by default is not the best practice imho.

@will-moore
Copy link
Member Author

@jburel Just to clarify, do we need to have Points support in OMERO.figure before this PR is approved?

@pwalczysko suggested at #465 (comment) that a warning might be enough, although I think he's changing his mind?!

If we need to wait for Points in OMERO.figure, that needs the Vite PR to be merged, then a follow-up PR. Then iviewer will depend on Points support in OMERO.figure, so we'll need to only enable "Save as Figure" if we have the most recent version of Figure. This will have to be detected in some manual way (we don't want to add omero-figure as a python dependency of iviewer).
Currently we detect if Figure is installed and don't show the "Save As Figure" option if it's not found, but detecting the version will be a bit harder.

@jburel
Copy link
Member

jburel commented Apr 19, 2024

I don't think we are in any rush to have this PR included and release.
My preference, as mentioned earlier today, will be to:

  • release Figure with Vite
  • add support for point in figure and release
  • include this feature and release iviewer. As part of the release, we recommend to upgrade to the version of figure with point support. (I am not expecting a detection of the version). I think we should avoid having a release of iviewer with the feature implemented knowing that the shapes are silently converted. Points are more common than ellipse so I will not ignore them as suggested above

@jburel
Copy link
Member

jburel commented Jun 19, 2024

While using figure with this PR, I don't think it will solve the problems we are currently facing with big images

  • The viewport is then a "crop" in figure, preventing the user to pan to a new location if a small mistake was made
  • The "context" of that viewport is also not present. So the user does not know the part of the image the crop is related to.

I feel that going back and forth between the apps to manipulate the images is not going to work for big images unfortunately

@will-moore
Copy link
Member Author

@jburel There is no cropping here. All panels in Figure can be zoomed and panned over the whole image.
You may get the impression of a crop when you pan the viewport, but that is just because the rendered region is restricted around the viewport and doesn't cover the whole image. When you pan and drop, it re-renders around the new viewport region.

It's possible to create an Inset panel in the normal way (maybe even with a shortcut in the new workflow) to show where the viewport is, but that is outside the scope of this PR.

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.

4 participants