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

Imviz pan/zoom needs improvements (lag in pan) #564

Closed
3 of 5 tasks
pllim opened this issue Apr 23, 2021 · 24 comments · Fixed by glue-viz/glue-jupyter#246 or #864
Closed
3 of 5 tasks

Imviz pan/zoom needs improvements (lag in pan) #564

pllim opened this issue Apr 23, 2021 · 24 comments · Fixed by glue-viz/glue-jupyter#246 or #864
Assignees

Comments

@pllim
Copy link
Contributor

pllim commented Apr 23, 2021

Follow up of #513 . Can be improved in the following ways:

@pllim pllim added this to the Imviz 1.0 milestone Apr 23, 2021
@astrofrog
Copy link
Collaborator

When I am in pan/zoom mode (i.e., push that arrows button), once I left-click on display to enable it, clicking on display again does not disable it. Moving the mouse outside of display also does not disable it. As a result, as I mouse over back to the arrows button to get out of pan/zoom, the image moves with me, panning it to a place that is undesirable. This renders pan/zoom mode unusable.

This is #534 and is fixed with the latest release of bqplot.

There is no "zoom to fit" or "pan back to default position" options.

I've just opened a PR to glue-jupyter to add this tool, and we can then add it here too (basically adding bqplot:home to the list of tools (@pllim - maybe you can do this and open a PR once glue-viz/glue-jupyter#218 is merged?)

In fact, I can't figure out how to zoom. The pan/zoom seems to only pan.

Two finger scrolling or scroll wheel on mouse.

When it does pan, there is a lag in loading the parts of image that come into view.

This is going to be harder to fix though we can try and profile it to see why it's slow. Essentially whenever you pan or zoom, we re-compute a fixed resolution buffer and send it to the front-end. We do this because for large images, sending the whole image to the GPU is not feasible. Essentially we behave a little like Google maps does. Having said this, we could imagine computing a buffer that extends beyond the current image limits so that the experience is a little nicer. I would personally advocate for that being a little lower priority compared to other features but it would be good to improve at some point.

@pllim
Copy link
Contributor Author

pllim commented Apr 27, 2021

Two finger scrolling or scroll wheel on mouse.

I tried the scroll wheel. It didn't work for my mouse. It is a gaming mouse (I didn't buy it, I just inherited it) with the standard two buttons + middle wheel and some other buttons that I never use, but that shouldn't make any difference, right?

@pllim
Copy link
Contributor Author

pllim commented Apr 27, 2021

computing a buffer that extends beyond the current image limits

Yeah, that sounds like a good idea! Though I am not sure about lower priority and I am not the one to make the call. It does get annoying and affects the UX aspect of things, especially if one compares with other existing visualization tools without such limitation.

@pllim
Copy link
Contributor Author

pllim commented Apr 27, 2021

Update: Updating bqplot fixed the click to stop panning problem. But the scroll wheel still does not zoom in/out; it only moves the notebook up/down.

@pllim
Copy link
Contributor Author

pllim commented Apr 29, 2021

Update: Scroll wheel to zoom works fine on the "standalone" app with viola but not in the example notebook.

@pllim pllim changed the title Imviz pan/zoom needs improvements Imviz pan/zoom needs improvements (lag in pan) May 25, 2021
@pllim
Copy link
Contributor Author

pllim commented May 25, 2021

I think just the laggy panning remains, so I updated the title. Thanks for all the help and hard work upstream!

@pllim
Copy link
Contributor Author

pllim commented Jul 27, 2021

At Imviz Hack Hour, user expressed concern on various performance issues. These might be deal breaker for switching away from DS9 to Jdaviz.

@pllim pllim added performance Performance related 🔥 Critical labels Jul 28, 2021
@pllim pllim self-assigned this Aug 2, 2021
@pllim
Copy link
Contributor Author

pllim commented Aug 3, 2021

Re: #564 (comment)

@astrofrog or @maartenbreddels, where is this "Google map buffer" implemented in the stack?

@maartenbreddels
Copy link
Collaborator

The zooming might still have been buggy (see bqplot/bqplot#1359) but this should be fixed in bqplot/bqplot#1391

@astrofrog
Copy link
Collaborator

The main component of the lag is due to the debounced function in glue-jupyter:

https://github.com/glue-viz/glue-jupyter/blob/1806445d913f2724664d2c1880165e558d169508/glue_jupyter/utils.py#L119-L144

which is used in the image mark:

https://github.com/glue-viz/glue-jupyter/blob/1806445d913f2724664d2c1880165e558d169508/glue_jupyter/bqplot/image/frb_mark.py#L40-L42

An immediate fix is to reduce the delay_seconds to say 0.1 which makes things feel a bit snappier. The behavior currently is that if you pan and stay clicked then the image will update after that delay - if the delay is set to zero then the issue is that the image is updated too many times while panning which then causes a different kind of lag where the image will continue to pan after the mouse movements has stopped.

I'm not sure what the best way forward here is - there are several options:

  • Reduce the delay to 0.1 or so and be done with it. That is fine but we'll run into issues if the calculation of the fixed resolution buffer takes longer than 0.1 seconds we'll run into laggy pan/zoom again.

  • Make it so that the image only gets updated when the user releases the mouse button, not during the panning. To me this would be the preferred solution - we only update during panning if the user has the button pressed but they don't move the mouse for a little while which is actually not super common behavior. So we could just update the image at the end of a pan event.

  • Keep the current behavior but also trigger drawing as soon as the mouse button is released.

Personally I think the middle option might be the best but I'm interested in other opinions.

@pllim
Copy link
Contributor Author

pllim commented Aug 3, 2021

Personally, I like the pan to update while dragging, especially if I am scanning for UFOs, I mean, stars. I think other tools also allow update while dragging, so middle option might be a non-starter.

@astrofrog
Copy link
Collaborator

Ah of course another option is to go with the middle option but we compute a fixed resolution buffer that goes beyond the initial boundaries, which I think would give the illusion of continuous updates.

@pllim
Copy link
Contributor Author

pllim commented Aug 3, 2021

👍 to middle option + illusion.

@astrofrog
Copy link
Collaborator

@pllim - can you try out glue-viz/glue-jupyter#246?

@astrofrog
Copy link
Collaborator

I still need to then make it update on button release but at least I think it is an improvement

@pllim
Copy link
Contributor Author

pllim commented Aug 17, 2021

On 2021-08-17, @astrofrog said this should be on hold for now until #667 is resolved to avoid premature optimization.

@pllim
Copy link
Contributor Author

pllim commented Sep 13, 2021

Linking stuff is done. @astrofrog , we can revisit this now. FYI.

@pllim
Copy link
Contributor Author

pllim commented Sep 14, 2021

PO said this is no longer MVP, so removing milestone.

@pllim
Copy link
Contributor Author

pllim commented Sep 15, 2021

The remaining work mentioned here is not critical for MVP. Follow-up issue opened at glue-viz/glue-jupyter#261

@stscijgbot-jwql
Copy link

This issue is tracked on JIRA as JDAT-1642.

@stscijgbot-jwql
Copy link

Comment by Duy Nguyen on JIRA:

Current theory is that with every action requires WCS recalculations on every pixel

@stscijgbot-jwql
Copy link

Comment by Duy Nguyen on JIRA:

Pey-Lian Lim: Linking was done in a general sense (for other contexts outside of astro) and adapted for WCS. A refactor of the general linking algorithm might be necessary

For the time being, we can alleviate this by sidestepping the issue: link on pixel space for most cases

@stscijgbot-jwql
Copy link

Comment by Pey-Lian Lim on JIRA:

Tom has a fix at glue-viz/glue-jupyter#246 but it is blocked by JDAT-1598, which causes blink to be very slow (6 seconds!) with this fix.

@stscijgbot-jwql
Copy link

Comment by Pey-Lian Lim on JIRA:

Tom implemented the buffer.

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