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

Mirroring support #439

Merged
merged 16 commits into from
Jun 6, 2023
Merged

Mirroring support #439

merged 16 commits into from
Jun 6, 2023

Conversation

barrettMCW
Copy link
Contributor

We co-register our WSIs against MRIs. Being able to mirror the image is pretty important for that.
I did a little CSS trickery then modified pixel values based off what way it's flipped. DragPan, ShapePopup, and Drawing in general work identically when mirrored. (as far as I can tell)
Thanks

@will-moore
Copy link
Member

Hi, apologies for not reviewing sooner.
Can you briefly say what I'm expecting to see when mirroring is enabled? A button or popup option or is the mirroring simply enabled always?
Thanks,

@barrettMCW
Copy link
Contributor Author

No problem!
Enabling mirroring adds two buttons to the top left set of buttons, under the plus and minus. These buttons will flip the viewport in the x or y axis with css and modifies pointer event coords as needed.
The mirroring buttons themselves are enabled by adding ENABLE_MIRRORING=true to initparams or setting omero.web.iviewer.enable_mirror=true in configs(url)
mirrored-drawing

@will-moore
Copy link
Member

Ah, yes - it's working now. I though I had the config set but it turned out I didn't....

In general this looks great...

The only significant issue I've seen so far is that when I click a different image thumbnail on the left to open a new Image, that doesn't show the mirror control buttons. It's like those buttons only get shown correctly when the page loads, not when each image loads.

I also noticed that when I drag (pan) the image while the Shape pop-up is showing, the Shape pop-up movement is not reflected, so if the image is reflected then the pop-up flies in the wrong direction. But this is very minor.

@barrettMCW
Copy link
Contributor Author

Good catch! I figured out how to properly add a control (with globals). I have been using BirdsEye as a guide, but I can't seem to figure out what's going on there. As far as I can tell birdseye is a non-default control, but I can't find where it gets added to the MDI images (or any image that isn't the init_requested.) Any ideas on where that happens or another place I should add the control?
I was not able to replicate the shape popup issues. I used to have them (when I recorded the gif) but I added some code to hopefully fix that before my initial commit. Is it persisting on your end?
Sorry about the true != 'True' thing, it got me too lol

@barrettMCW
Copy link
Contributor Author

found it!

Copy link
Member

@will-moore will-moore left a comment

Choose a reason for hiding this comment

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

This is working now (the controls are shown on every viewer).

The popup is being positioned correctly when it is initially shown, and when the Shape is dragged.
But when the image is panned, it can move in the wrong direction.

E.g. when the shape popup is shown:
Screenshot 2023-01-12 at 17 30 15

Then I pan the image up (and the popup moves DOWN):
Screenshot 2023-01-12 at 17 30 38

This is probably kinda low-level OpenLayers behaviour so it may not be easy/possible to fix (and if not, then don't worry as it's not a blocker).
Otherwise looking good!

README.rst Outdated
When working with other images (coregistering MRIs for example), it is necessary to be able to mirror an image.
There is now experimental support for runtime image mirroring. To enable mirroring set enable_mirror to true.

$ omero config set omero.web.iviewer.enable__mirror true
Copy link
Member

Choose a reason for hiding this comment

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

You have a double underscore in enable__mirror which I think is why my config didn't work before.

@barrettMCW
Copy link
Contributor Author

Ah I see what you mean. I wasn't able to find an obvious place to fix. I'll make a pr if I find something in the future.
Thanks!

@pwalczysko
Copy link
Member

I have a problem with a follow-up function which is not taking into account the flipped image.

  1. open an image
  2. flip it
  3. go File > Save Viewport as png
  4. observe that the resulting png is not flipped (the flipping got ignored)

Not sure that this is a blocker, but a user will certainly be surprised.

@pwalczysko
Copy link
Member

Similarly to #439 (comment) , when a Projection is made out of the flipped image, the Projection is non-flipped (although the Preview of it is flipped)

@pwalczysko
Copy link
Member

Thinking and playing further, I think that the Save Viewport... behaviour is not very good (say, worse than the projection) in this sense:

  1. Open and image, and zoom in and rotate it using either the top-left controls for zooming or mouse wheel and the mouse drag for rotation
  2. Then File > Save Viewport...
  3. Observe that the rotation is respected in the saved PNG
  4. Flip the image
  5. File > Save Viewport...
  6. Observe the flipping is ignored

Thus, some controls in the top-left get respected, some ignored... Not explicable in intuitive way...

@barrettMCW
Copy link
Contributor Author

Great finds!
I agree, save viewport should do that. I got saving mirrored canvases to work, so that will be coming. Gonna finish up that then I'll download some z-stack data to look at projections.

@barrettMCW
Copy link
Contributor Author

Saving mirrored viewport works now!
I was looking at the projections and it seems doable but I think maybe we shouldn't save it as mirrored directly.
The other controls do not effect how the projection saves. I want to add a "ground truth" option in the future that saves an orientation (zoom, rotation, mirror) to move to when opening that image. ( so once we coreg we view the slides in the proper orientation by default ) I think it would be better to add that and copy over current orientation as ground truth?

@pwalczysko
Copy link
Member

Saving mirrored viewport works now! I was looking at the projections and it seems doable but I think maybe we shouldn't save it as mirrored directly. The other controls do not effect how the projection saves. I want to add a "ground truth" option in the future that saves an orientation (zoom, rotation, mirror) to move to when opening that image. ( so once we coreg we view the slides in the proper orientation by default ) I think it would be better to add that and copy over current orientation as ground truth?

Thank you @barrettMCW , this all sounds very reasonable and encouraging to me.

And yes, the Save Viewport... is working as expected. I am suggesting that this great improvement should be merged as is - the bird's eye view is of course good to have, but if it is not soon-to-solve problem, it seems to me that the benefits outweight the drawbacks.

@barrettMCW
Copy link
Contributor Author

Thank you! Do you mean shape popup? I didn't see any problems with birdseye.

@pwalczysko
Copy link
Member

Thank you! Do you mean shape popup? I didn't see any problems with birdseye.

No, I mean bridseye view, bottom-right in central pane of iviewer (on many of the screenshots here in this PR, the birdseye view is collapsed, you just have to click onto the arrow in bottom-right to expand it).

Screenshot 2023-01-16 at 15 43 07

You can see that if I flip the image, the birdseye is still in original orientation. Now, when I use the birdseye to "jump" between places on a large image, the discrepancy between the two views is of course confusing. I have also checked that e.g. rotation of the image rotates the birdseye view accordingly. I was thinking you were discussing it with @will-moore in #439 (comment) ?

@barrettMCW
Copy link
Contributor Author

I was looking at the non-flipped birdseye as a feature rather than a bug, as the red box is correct, although now seeing that rotating the image rotates the entire birdseye makes me feel I should flip it. Shouldn't be hard

@barrettMCW
Copy link
Contributor Author

barrettMCW commented Jan 16, 2023

Ahh, that was me talking about trying to copy how birdseye was implemented to make sure it's always added. But yeah I'll change that quick, good catch

@pwalczysko
Copy link
Member

Ummm, really sorry for not discoverign it earlier. When I rotate&flip horizontally then the panning can become very hard. I can record a video if need be, but maybe you can reproduce ? Just rotate some 45 degrees, then flip horizontally and try to pan please.

@barrettMCW
Copy link
Contributor Author

ickyyyy, i see it. Birdseye is almost good then I'll fix that. I've got a couple ideas churning so I think it'll be doable

@barrettMCW
Copy link
Contributor Author

barrettMCW commented Jan 16, 2023

I have working mirrored rotation but I am not happy about it. I unrotate the current and previous coords, mirror new coords and redo the rotation on new coords. It works, barring light stuttering and the occasional jump to a side then jump back. I'm sure there's a faster way to calculate it without unrotating it but I only took trig once in highschool :/
There is an if statement so performance should only suffer while rotated

@will-moore
Copy link
Member

@barrettMCW I just found this: https://stackoverflow.com/questions/63638347/inverting-the-y-axis, and the codesandbox link there is working. I wonder if that approach for flipping will address some of these issues you're seeing since OpenLayers may understand at a lower level that the image is inverted/mirrored?

I wouldn't worry about save Projection handling the flipped image - that seems like really edge case.

A couple of other places you may want to handle the mirrored state:

But these are not blockers, so it's up to you (could also come in a follow-up PR)

@barrettMCW
Copy link
Contributor Author

I remember seeing that codepen from way back when I started this, but I couldn't figure it out. I tried again, but no luck. I think the problem is that I'm doing addCoordinateTransforms(view.getProjection(), new Projection...) instead of (tiles.getProjection(), view.getProjection()), but I cannot find the OmeroImage or Ol3.Tile projections. If our image layer doesn't have a projection I'm not sure that method will work. Cache and Viewport links are good adds, as that's probably the mechanism I'll use for ground truth. I'll get started on that shortly

@barrettMCW
Copy link
Contributor Author

I think this is it. Cached settings works. ViewportAsUrl probably works. I didn't build and put it on a test server to test the url itself but it recognizes the index-dev.html initparams and builds a url with mirror parameters, which I imagine means it will work. I discovered rotating is a little wonky as well. I think it's much more manageable than the other things I correct for. I think we just leave it for now and I'll take another crack at mirroring with openlayers when I start base truth.

@pwalczysko
Copy link
Member

I think this is it. Cached settings works. ViewportAsUrl probably works. I didn't build and put it on a test server to test the url itself but it recognizes the index-dev.html initparams and builds a url with mirror parameters, which I imagine means it will work. I discovered rotating is a little wonky as well. I think it's much more manageable than the other things I correct for. I think we just leave it for now and I'll take another crack at mirroring with openlayers when I start base truth.

Thanks a lot.
Testing on our server has shown that:

  1. the cached settings work, as you indicated, the workflow is to open an image, flip it, switch off a channel, click on another thumb in LHP of iviewer and then click back onto the first image thumb -> should still be flipped and channel off, works as expected
  2. the url does not work, https://merge-ci.openmicroscopy.org/web/webclient/img_detail/105665/?dataset=25019&z=0&t=0&x=33059&y=27449&zm=34&c=-1|0:255$FF0000,2|0:255$00FF00,3|0:255$0000FF&m=c&maps=[{"inverted":{"enabled":false},"quantization":{"family":"linear","coefficient":1}},{"inverted":{"enabled":false},"quantization":{"family":"linear","coefficient":1}},{"inverted":{"enabled":false},"quantization":{"family":"linear","coefficient":1}}]&fx=true&fy=false is the link I am producing from the flipped and zoomed-in image, when I paste it in a new tab, I get a non-flipped image

@will-moore
Copy link
Member

Testing with the dev build

enableMirror: "True"
initialFlipX: true
initialFlipY: null

and when built, with URL containing &fx=true&fy=false

enableMirror: "True"
initialFlipX: "true"
initialFlipY: "false"

So it looks like you need to convert the "true" string into true boolean

@barrettMCW
Copy link
Contributor Author

Sorry about that, using a string bool for initparam works. Thanks for your help guys!

@pwalczysko
Copy link
Member

Confirming that Get link to viewport now works with the mirroring as expected.

Ready to merge fmpov.

@snoopycrimecop snoopycrimecop mentioned this pull request Apr 17, 2023
@jburel jburel merged commit f7e917b into ome:master Jun 6, 2023
@will-moore
Copy link
Member

@barrettMCW Apologies for the delay, but this has now been released in omero-iviewer 0.13.0.
Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants