ome / omero-figure

An OMERO.web app for creating Figures from images in OMERO
http://figure.openmicroscopy.org
GNU Affero General Public License v3.0
15 stars 31 forks source link

Weird translation #580

Closed Rdornier closed 3 months ago

Rdornier commented 4 months ago

Fixes issue #506

will-moore commented 4 months ago

Thanks for the PR fix. I'm away till August so won't be able to look more just yet...

will-moore commented 3 months ago

Thanks for tracking down a fix for this. The change you've made here has some issues when you select 2 panels that have different zoom values. In that case the zoom average is a very different value from the existing zoom value of each panel, so that m.set(zoom) actually changes the figure quite a lot by updating those panels. Actually, it shouldn't ever be necessary to call model.set() in a render() method, since that will update the figure while the user is just viewing the figure (instead of making any desired changes).

However, the instructions for reproducing the bug and the change you've made (that did fix it nicely for a single panel) did allow me to work out the core issue, which is that the cropToRectangle() sets the zoom to a float (instead of an integer) and this isn't handled well by the panning behaviour.

One possible fix that works is to make sure that cropToRectangle() saves the zoom as an Integer:

--- a/src/js/models/panel_model.js
+++ b/src/js/models/panel_model.js
@@ -610,7 +610,7 @@
             // zoom to correct percentage
             var xPercent = this.get('orig_width') / coords.width,
                 yPercent = this.get('orig_height') / coords.height,
-                zoom = Math.min(xPercent, yPercent) * 100;
+                zoom = parseInt(Math.round(Math.min(xPercent, yPercent) * 100));

However, I don't think that we should rely on zoom always being an Integer, so I looked for the bug in panning and tracked the fix down to model.get_vp_big_image_css(). Since the zoom value that is passed into that function is always an Integer (it comes from zoom_avg), when this.get('zoom') is a float then zooming is True, which it shouldn't be when we're panning. If we always compare the difference it fixes the issue:

--- a/src/js/models/panel_model.js
+++ b/src/js/models/panel_model.js
@@ -790,7 +790,7 @@
             // Used for static rendering, as well as during zoom, panning, panel resizing
             // and panel re-shaping (stretch/squash).

-            var zooming = zoom !== this.get('zoom');
+            var zooming = Math.abs(zoom - this.get('zoom')) > 1;
             var panning = (x !== undefined && y!== undefined);
Rdornier commented 3 months ago

Thanks @will-moore for your deeper look ; it wasn't easy to find ! I've corrected the code according to your inputs and it also works for me 👍

will-moore commented 3 months ago

Thanks - merging. NB: you've got the same change to .gitignore in #581 but you can resolve that when fixing merge conflicts there...