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

Weird translation #580

Merged
merged 4 commits into from
Aug 9, 2024
Merged

Weird translation #580

merged 4 commits into from
Aug 9, 2024

Conversation

Rdornier
Copy link
Contributor

Fixes issue #506

@will-moore
Copy link
Member

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

@will-moore
Copy link
Member

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
Copy link
Contributor Author

Rdornier commented Aug 8, 2024

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 will-moore merged commit 430fc3f into ome:master Aug 9, 2024
1 check passed
@will-moore
Copy link
Member

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

@Rdornier Rdornier mentioned this pull request Aug 26, 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.

2 participants