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

POC: canvas rotation #1384

Closed
wants to merge 5 commits into from
Closed

Conversation

kecnry
Copy link
Member

@kecnry kecnry commented Jun 9, 2022

TODO

  • Noticeable lag when pan/zoom in the rotated canvas. Is this something we can fix? (seems to have significantly improved after rebasing on recent performance updates upstream)
  • cross-browser support for the correct dragging direction. Will need significant testing across different browsers/versions and perhaps considering workarounds upstream in glue/bqplot or worse case scenario we could detect compatibility and display a message in the plugin
  • track down and fix the 1/3 image disappearing bug. Probably was introduced somewhere in the limits logic since it does not seem to happen on main.
  • test/fix pan stuttering, if possible (probably more performance improvements upstream)
  • update the limits logic (actual bqplot limits need to be adjust so that the displayed image roughly matches the requested limits, with actual limits extending outside the window)
  • cleanup plugin UI (live update instead of button press? option to set north to up?)
  • Have zoom box in Compass follow canvas rotation. (Disabled for now.)
  • Home button no longer make sense when canvas is rotated. Is this something we can address? Should rotation reset or just set limits such that the whole image is in-frame?
  • Port some of the relevant docs from Rotate image in Imviz #1340 into docs/imviz/plugins.rst. This should wait till after Doc Changes for June 2022 #1393 is merged to avoid conflicts.
  • Port relevant tests from Rotate image in Imviz #1340, if possible.
  • Any other TODOs in the diff

Description

This pull request rotates the entire viewer widget.

  • NOTE: currently this is hardcoded to 45 degrees for all viewers. This will need to be controlled by some state object... but we don't have access to the viewer state at this level, so will likely need to be a per-viewer dictionary in the app state that is then controlled from a plugin in the UI.
  • see notes about black background color (will need upstream changes in glue/glue-jupyter or some hacky JS to avoid white spaces when zooming out or panning outside data limits within the viewer).
Screen.Recording.2022-06-09.at.11.58.51.AM.mov

This would supersede #1340

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
  • Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
  • Do the proposed changes follow the STScI Style Guides?
  • Are tests added/updated as required? If so, do they follow the STScI Style Guides?
  • Are docs added/updated as required? If so, do they follow the STScI Style Guides?
  • Did the CI pass? If not, are the failures related?
  • Is a change log needed? If yes, is it added to CHANGES.rst?
  • Is a milestone set?
  • After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

@github-actions github-actions bot added the embed Regarding issues with front-end embedding label Jun 9, 2022
@codecov
Copy link

codecov bot commented Jun 9, 2022

Codecov Report

Merging #1384 (9bb8aa4) into main (3db487f) will decrease coverage by 0.05%.
The diff coverage is 68.00%.

❗ Current head 9bb8aa4 differs from pull request most recent head c0931d0. Consider uploading reports for the commit c0931d0 to get more accurate results

@@            Coverage Diff             @@
##             main    #1384      +/-   ##
==========================================
- Coverage   85.12%   85.07%   -0.06%     
==========================================
  Files          91       93       +2     
  Lines        8314     8339      +25     
==========================================
+ Hits         7077     7094      +17     
- Misses       1237     1245       +8     
Impacted Files Coverage Δ
...configs/imviz/plugins/rotate_image/rotate_image.py 61.11% <61.11%> (ø)
jdaviz/configs/imviz/plugins/viewers.py 82.08% <66.66%> (-0.28%) ⬇️
jdaviz/app.py 92.78% <100.00%> (+0.02%) ⬆️
jdaviz/configs/imviz/plugins/__init__.py 100.00% <100.00%> (ø)
...viz/configs/imviz/plugins/rotate_image/__init__.py 100.00% <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 3db487f...c0931d0. Read the comment docs.

@pllim pllim mentioned this pull request Jun 14, 2022
21 tasks
@kecnry kecnry force-pushed the canvas-rotation branch from 0513cee to 3522d16 Compare June 21, 2022 12:21
@pllim pllim added this to the 2.7 milestone Jun 21, 2022
@pllim pllim added the imviz label Jun 21, 2022
kecnry added 2 commits June 21, 2022 17:08
* can currently control with: `imviz.app._viewer_item_by_id('imviz-0')['rotation'] = 60`
@pllim pllim self-assigned this Jun 21, 2022
Copy link
Contributor

@pllim pllim left a comment

Choose a reason for hiding this comment

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

As it stands, in the Imviz example notebook (2 images loaded, linked by pixel), when I rotate the canvas by, say, 45 degrees, the zoom box in Compass and the Home button no longer make sense to me. I also see noticeable lag when pan/zoom in the rotated canvas (and can hear my CPU churning away) but not sure what caused it. I'll add to the "todo" things to look at.

Screenshot 2022-06-21 181731

@pllim pllim force-pushed the canvas-rotation branch from 3522d16 to 924bced Compare June 21, 2022 22:21
until we can figure out how to do it properly
@@ -78,6 +94,41 @@ module.exports = {
* between a user closing a tab or a re-render. However, when the user closes a tab, the
* source of the event is a vue component. We can use that distinction as a close signal. */
source.$root && this.closefn(viewerId);
},
resizeViewer(viewer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I rotate to 45 degrees and then rotate back, but the Compass zoom box starts acting weird (i.e., it is showing the zoom limits that is a little off from what I am looking at). Since Compass grabs the numbers straight of viewer.state.x/y_min/max attributes, something isn't sync-ing right after this operation but I don't know what. Is it the aspect ratio?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe the WCS distortion in the Imviz example notebook data is causing trouble.

This needs clean-up for production.
[ci skip]
@pllim pllim mentioned this pull request Jun 24, 2022
9 tasks
@rosteen rosteen modified the milestones: 2.7, 2.8 Jul 6, 2022
@pllim pllim mentioned this pull request Jul 7, 2022
@pllim

This comment was marked as outdated.

@pllim pllim closed this Jul 7, 2022
@rosteen

This comment was marked as resolved.

@kecnry kecnry mentioned this pull request Jan 26, 2023
20 tasks
@mariobuikhuizen

This comment was marked as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
embed Regarding issues with front-end embedding imviz
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants