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

Inset feature #549

Merged
merged 21 commits into from
Sep 6, 2024
Merged

Inset feature #549

merged 21 commits into from
Sep 6, 2024

Conversation

will-moore
Copy link
Member

@will-moore will-moore commented Apr 8, 2024

Fixes #319

This replaces the tricky "create Inset" workflow (copy Rectangle and paste as Crop region or vice versa) with a single button click, and the Inset panel and Rectangle should stay in sync when either are updated.

To test:

  • Select one or more panels and in the right-tab, under ROIs, click the "Create Inset" button
  • This will create a new Inset Panel and a corresponding Rectangle:

Screenshot 2024-04-26 at 11 06 04

  • When you Pan, Zoom, rotate or Resize the Inset Panel, the Rectangle will update in sync (NB: we previously handled rotation when copying crop region and pasting as ROI by creating a Polygon instead of rotated Rectangle)
  • When you edit the Rectangle (via the ROI dialog) the Inset Panel will update accordingly
  • If you copy and paste the Inset panel to create e.g. split-view panels, each of these panels is "linked" to the original Rectangle (updating any one of them will update the Rectangle and vice-versa)
  • If you copy and paste the parent panel that contains the Rectangle, both Rectangles in these panels are linked to the Inset Panel(s)
  • Save the Figure, then re-open (can simply refresh the page) and check that the Rectangle and Inset panels are still kept in sync.

@will-moore will-moore marked this pull request as ready for review April 26, 2024 10:04
@ome ome deleted a comment from snoopycrimecop Apr 29, 2024
@ome ome deleted a comment from snoopycrimecop Apr 29, 2024
@ome ome deleted a comment from snoopycrimecop Apr 29, 2024
@pwalczysko
Copy link
Member

Tested on https://merge-ci.openmicroscopy.org/web/figure/file/5875/ - lgtm actually. The only (slight) question is the copying of the zoomed in viewports -> might be a bit confusing that all the "daughter" viewports are linked to the parent viewport and only the panning is going to make a change and sync -> but I do not have any better suggestion, and indeed, this is in line with the iviewer behaviour and also behaviour in figure itself (selection does not cause change, but change inside selection does)

@snoopycrimecop snoopycrimecop mentioned this pull request Jul 11, 2024
@Rdornier Rdornier mentioned this pull request Aug 8, 2024
@will-moore
Copy link
Member Author

Thinking about discussion at #578, using a panel border to indicate which inset panel is from which rectangle. cc @Rdornier

I'm not sure whether this is really needed since most real figures only have 1 or two insets and even with 2 or 3 insets it's usually clear where each has come from.

A panel border could help, but again, in most real figures the inset panels don't have borders, so I wouldn't really want to add one by default to inset panels.
One possibility is that we ONLY show a temporary (dashed?) border when the parent panel is selected:
E.g. this is how the figure would look normally:

Screenshot 2024-08-09 at 15 47 38

And when you select the parent panel, you see e.g.
Screenshot 2024-08-09 at 16 04 55

NB: any panel border needs a bit of work to avoid shifting the panel (right and down) and changing the size of the internal viewport area.

I'm not convinced that this is really needed / worth the effort.

However, I think we do need to make it easier to pick the colour for the newly created Inset rectangles: currently they are created with the current ROI colour for that panel, but if there are no ROIs on the panel then you can't change this colour, and if there ARE other ROIs on the panel, changing the colour will also change the ROIs colour.

@Rdornier
Copy link
Contributor

Hello,

A panel border could help, but again, in most real figures the inset panels don't have borders, so I wouldn't really want to add one by default to inset panels.

Ok, that's fair enough. We can forget about having a default panel color border there. It's better to make it a separate feature.

I'm not sure whether this is really needed since most real figures only have 1 or two insets and even with 2 or 3 insets it's usually clear where each has come from.

For me, having the possibility to identify the inset remains important. Panel color border wasn't maybe the good idea but maybe labeling insets would probably make more sense (i.e. A/B/C or 1/2/3). This would imply to also add a label to the corresponding ROI but I think this feature is not yet implemented (see #457).

However, I think we do need to make it easier to pick the colour for the newly created Inset rectangles

Ok

@will-moore
Copy link
Member Author

Updated form to allow you to choose position of Inset, as well as colour and line-width for the ROI:

Screenshot 2024-08-23 at 11 36 10

Anybody want to test?

@Rdornier
Copy link
Contributor

Rdornier commented Aug 26, 2024

Hi @will-moore

I've tested it and it works pretty well ! It's a super nice improvement and I'm sure users will strongly appreciate.

I noticed a weird behavior of the inset line width drop-down

  1. When changing to another linewidth, the menu doesn't always expend horizontally

image

  1. Modifying the inset line width prevents from modifying it again. I wasn't able to choose 3pt and then 4pt ; it fixes the line width to the first selection 3pt. To reset it, I can either select another image or changing the line width of the ROIs

I don't know if it was intended but I noticed that if you delete an inset, the corresponding ROI is not deleted from the parent image (and vice-versa). Do you plan to delete both parts of the link (inset and ROI) ?

Regarding the big images, if I create an inset on that image, without any zoom, it works correctly but a zoom was already set on the parent image, the inset panel is not representing the correct part of the parent image. This is maybe linked to #580

image

Last point, which is more a suggestion : it could be great to label both ROI and inset with the same letter/number.

Rémy.

@will-moore
Copy link
Member Author

@Rdornier I fixed the drop-down behaviour (see last commit above). You'll need the same change on #578

@pwalczysko
Copy link
Member

@Tom-TBT Do you want to have a look please ?

@Tom-TBT
Copy link
Contributor

Tom-TBT commented Aug 30, 2024

This PR is amazing.

I tried to break it, without success:

  • with rotation in the source or inset
  • moving the ROI in source or inset, before and after rotation
  • by cascading insets
  • changing the size of the rectangle
  • editing the image ids (if changing only one, the inset still works but the images don't match. It's not a bug)

I find the creation of an inset intuitive (for sure easier than before). Also, panning in the inset or changing the rectangle in the ROI edit menu gives the same expected results.

It works really well.

Ah! Finally, I found a bug: when creating the inset from a rotated image, both the inset and the ROI have a rotation (only the new inset should inherit rotation, not the ROI on the source image). Right after panning in the inset, the ROI is updated to what it should be (or the other way around by changing the rectangle in ROI edit, the inset is updated correctly).

image

@will-moore
Copy link
Member Author

Thanks @Tom-TBT - good catch! I fixed that now, along with tiny bit of code cleanup.

@will-moore
Copy link
Member Author

@Rdornier, @Tom-TBT Can you give a final test of this? Thanks!

@Rdornier
Copy link
Contributor

Rdornier commented Sep 4, 2024

Hi @will-moore

It works nicely. However,

Regarding the big images, if I create an inset on that image, without any zoom, it works correctly but a zoom was already set on the parent image, the inset panel is not representing the correct part of the parent image. This is maybe linked to #580

This is not fixed with the more recent code

image

I don't know if it was intended but I noticed that if you delete an inset, the corresponding ROI is not deleted from the parent image (and vice-versa). Do you plan to delete both parts of the link (inset and ROI) ?

What about this point ?

Last point, which is more a suggestion : it could be great to label both ROI and inset with the same letter/number.

and this one ?

@will-moore
Copy link
Member Author

@Rdornier Thanks for that.

  • I can't seem to reproduce the issue you're seeing there. What's the size of that image? Is it rotated? When you pan the viewport of the inset panel, does the ROI update correctly, or does the mismatch persist? What about if you update the ROI - does the inset panel update correctly?
  • I wasn't sure about delete... I'm kinda thinking that someone may want to delete just the ROI or Inset Panel independently of each other. But maybe I'm just being lazy! I'll think about it a bit more...
  • Apologies - I should have responded to that labels suggestion earlier. It is a nice idea, but we still don't support labels on ROIs yet, so I think that's going to be quite a bit more work.

@will-moore
Copy link
Member Author

Build failing with...

#12 [omero 2/2] RUN dnf install -y -q     git &&     dnf clean all
...
#12 9.488  From       : /etc/pki/rpm-gpg/PGDG-RPM-GPG-KEY-RHEL
#12 10.54 Error: Failed to download metadata for repo 'pgdg16': repomd.xml GPG signature verification error: Bad GPG signature
#12 ERROR: process "/bin/sh -c dnf install -y -q     git &&     dnf clean all" did not complete successfully: exit code: 1

@Rdornier
Copy link
Contributor

Rdornier commented Sep 4, 2024

I can't seem to reproduce the issue you're seeing there. What's the size of that image? Is it rotated? When you pan the viewport of the inset panel, does the ROI update correctly, or does the mismatch persist? What about if you update the ROI - does the inset panel update correctly?

The image is 46622 x 63895 pixels, not rotated. If I pan the viewport or if I update the ROI, the mismatch persists.
It seems that it is only a vertical mismatch.
And I have to correct something ; it also appears if the parent image is not zoomed.

I wasn't sure about delete... I'm kinda thinking that someone may want to delete just the ROI or Inset Panel independently of each other. But maybe I'm just being lazy! I'll think about it a bit more...

Ok

Apologies - I should have responded to that labels suggestion earlier. It is a nice idea, but we still don't support labels on ROIs yet, so I think that's going to be quite a bit more work.

Well, I see... maybe we can first work on ROI label support and, when it is ready, include it for insets creation, in another release, depending when you planned to release the inset feature.

@Tom-TBT
Copy link
Contributor

Tom-TBT commented Sep 5, 2024

On my side everything looks fine, I tried with large images too and neither can I reproduce Remy's issue.
Can you share the image @Rdornier?

@Rdornier
Copy link
Contributor

Rdornier commented Sep 5, 2024

I tried with large images too and neither can I reproduce Remy's issue.

Ok, that's weird. Maybe it is coming from my side only. I'll check

Can you share the image

Yes, sure. You can find it on zenodo : https://zenodo.org/records/10629087
It is the Brightfield H-DAB dataset

@pwalczysko
Copy link
Member

Build failing with...

#12 [omero 2/2] RUN dnf install -y -q     git &&     dnf clean all
...
#12 9.488  From       : /etc/pki/rpm-gpg/PGDG-RPM-GPG-KEY-RHEL
#12 10.54 Error: Failed to download metadata for repo 'pgdg16': repomd.xml GPG signature verification error: Bad GPG signature
#12 ERROR: process "/bin/sh -c dnf install -y -q     git &&     dnf clean all" did not complete successfully: exit code: 1

@will-moore This failure is probably just a blip. Please see that the tests are passing on my fork where I have just forked, checked out your branch and pushed to my fork with no changes.. I would suggest you simply rerun the tests (I do not seem to have the perms to do that).

@jburel
Copy link
Member

jburel commented Sep 5, 2024

Build failing with...

#12 [omero 2/2] RUN dnf install -y -q     git &&     dnf clean all
...
#12 9.488  From       : /etc/pki/rpm-gpg/PGDG-RPM-GPG-KEY-RHEL
#12 10.54 Error: Failed to download metadata for repo 'pgdg16': repomd.xml GPG signature verification error: Bad GPG signature
#12 ERROR: process "/bin/sh -c dnf install -y -q     git &&     dnf clean all" did not complete successfully: exit code: 1

Where did you get that error? local build?

@will-moore
Copy link
Member Author

@Rdornier Thanks for the zenodo link.
I tried and I really couldn't see any errors (yet)!

Screenshot 2024-09-06 at 09 58 06

@Rdornier
Copy link
Contributor

Rdornier commented Sep 6, 2024

Thanks for checking !
Hum... Ok, so it seems that the issue is coming from my side.
As I'm working with Docker to develop on omero-figure, I'll have a look to the configs, re-build container... and see if it solves this problem.

@will-moore
Copy link
Member Author

@Rdornier - with that last commit, when you delete the Inset Panel, the corresponding ROI will also get deleted.
When you delete the Inset ROI, the Inset Panel doesn't get deleted. But that was a fair bit harder to implement and probably not as important?

@Rdornier
Copy link
Contributor

Rdornier commented Sep 6, 2024

when you delete the Inset Panel, the corresponding ROI will also get deleted.

Perfect, thank you ! I've just tested It and it works as expected.

When you delete the Inset ROI, the Inset Panel doesn't get deleted. But that was a fair bit harder to implement and probably not as important?

Ok, that's fair. This way is not as important and as obvious as the other one. We can keep it like this.

Regarding the bug on the big image, I re-build my entire docker-compose, which now have the latest image of omero-web, figure..., and I cannot reproduce the error neither. I don't know exactly what was going wrong but, now it works !
So, all good from my side.

@will-moore will-moore merged commit f9a321d into ome:master Sep 6, 2024
1 check passed
@will-moore
Copy link
Member Author

Thanks for all the testing. Merging...

@will-moore will-moore added this to the 7.2.0 milestone Oct 3, 2024
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.

New workflow: inset handling
5 participants