Expand fixed resolution buffer beyond the image boundaries to make panning seamless

[astrofrog]

I think this will resolve spacetelescope/jdaviz#564

[pllim]

Does this go with another fix that disable actual pan until mouse button is up? Or this is it?

[codecov]

Codecov Report

Merging #246 (8fde165) into main (22f46da) will decrease coverage by 0.43%.
The diff coverage is 36.36%.

@@            Coverage Diff             @@
##             main     #246      +/-   ##
==========================================
- Coverage   89.11%   88.68%   -0.44%     
==========================================
  Files          82       82              
  Lines        4044     4074      +30     
==========================================
+ Hits         3604     3613       +9     
- Misses        440      461      +21     

Impacted Files	Coverage Δ
glue_jupyter/bqplot/common/tools.py	70.43% <25.00%> (-2.09%)	⬇️
glue_jupyter/bqplot/image/frb_mark.py	52.63% <25.00%> (-10.79%)	⬇️
glue_jupyter/bqplot/image/state.py	100.00% <100.00%> (ø)
glue_jupyter/bqplot/image/viewer.py	87.30% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22f46da...8fde165. Read the comment docs.

[astrofrog]

I will do a separate PR for the mouse button fix, but you can already try this PR out as it will give you the biggest benefit

[pllim]

What does this mean when the whole image already fits in the view? Does it do unnecessary computation in that case, or is this smart enough?

[astrofrog]

It is smart enough :)

[pllim]

With glue-jupyter 0.7:

With this patch (more performant but display looks grainier):

[rosteen]

Agreed with @pllim - the performance is vastly better, but the display scaling seems strangely different (and I don't see why the small code change here would have affected that). See the two short movies below for an example of what I'm talking about. With this PR, there are a bunch of bright pixels that seem to change location when panning, which is quite annoying.

Screen.Recording.2021-08-03.at.2.46.35.PM.mov

Screen.Recording.2021-08-03.at.2.55.09.PM.mov

[pllim]

Maybe the ny and nx need updating too:

bounds = [(ymin, ymax, ny), (xmin, xmax, nx)]

Min/max changed but ny and nx remained as before, which would explain the degraded resolution.

[pllim]

I dunno, maybe something like this? With this, resolution improved and still appeared somewhat performance on drag/zoom, but I did notice a lag in loading (or has it been always there?). 🤷

--- a/glue_jupyter/bqplot/image/frb_mark.py
+++ b/glue_jupyter/bqplot/image/frb_mark.py
@@ -65,7 +65,8 @@ class FRBImage(ImageGL):
         if xmin is None or xmax is None or ymin is None or ymax is None:
             return

-        ny, nx = self.shape
+        ny = self.shape[0] + int(dy * 0.5)
+        nx = self.shape[1] + int(dx * 0.5)

         # Set up bounds
         bounds = [(ymin, ymax, ny), (xmin, xmax, nx)]

p.s. I also attempted the "only update pan on dragend" but didn't get anywhere. I went as far as setting PanZoom(..., allow_pan=False) and tried to handle the pan manually but didn't work. I'll leave it to the experts...

[astrofrog]

@rosteen @pllim - oops good catch about the resolution, should be fixed now. Let me know what the performance is like.

[rosteen]

Looks like that resolved the problem I was seeing. The resolution looks good now, and the performance is much better than it was. I still get some white space that takes a moment to fill in occasionally when zoomed in and panning, but it is way better than the constant slow drawing previously.

[pllim]

Resolution seems fine now. As @rosteen said, there is still some lag if you pan around or zoom too much real fast, but some improvement is better than no improvement.

[pllim]

Hold on... Toolbar is laggy when I re-enable linking in the Imviz example notebook with this patch. Do you also see the same problem?

[pllim]

Especially the blinking. Blink is now laggy.

[astrofrog]

Yes that could well be - the issue is that the image will now take longer to generate, so blinking could end up being a bit slower. I need to think a little more about how we can improve performance across the board.

[astrofrog]

The issue is that this currently slows down the main display of the image, so it would be better to asynchronously load the image outside the field of view after the main visible image has been updated.

[pllim]

Re: #246 (comment) -- Does that mean you plan to push more commit to this before I review, or is that "future work"? (Tom said this PR is no-go.)

[astrofrog]

I've now made this effectively opt-in for now until we come up with a more elegant solution. To opt in to this, downstream packages should set the viewer.state.image_external_padding property - for instance, to expand the image by 50% in all directions, this value should be set to 0.5.

[pllim]

Thanks, but this isn't really usable for MVP unless you plan to release it in the next 2 days.

[astrofrog]

Watch me 😆

[astrofrog]

See spacetelescope/jdaviz#864 for using this in jdaviz.