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.

Sign up for GitHub

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

Jump to bottom

34 - Image Pan and Zoom #4736

Merged
merged 96 commits into from
Mar 23, 2022
Merged

34 - Image Pan and Zoom #4736

merged 96 commits into from
Mar 23, 2022

Conversation

michaelrogers
Copy link
Contributor

@michaelrogers michaelrogers commented Jan 19, 2022 •
edited by nikhilmandlik
Loading

Fixes #34

TODO:

Describe your changes:

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Is this change backwards compatible? For example, developers won't need to change how they are calling the API or how they've extended core plugins such as Tables or Plots.

Author Checklist

  • Changes address original issue?
  • Unit tests included and/or updated with changes? N/A
  • Command line build passes?
  • Has this been smoke tested?
  • Testing instructions included in associated issue?

Reviewer Checklist

  • Changes appear to address issue?
  • Changes appear not to be breaking changes?
  • Appropriate unit tests included?
  • Code style and in-line documentation are appropriate?
  • Commit messages meet standards?
  • Has associated issue been labelled unverified? (only applicable if this PR closes the issue)
  • Has associated issue been labelled bug? (only applicable if this PR is for a bug fix)

@codecov
Copy link

codecov bot commented Jan 19, 2022 •
edited
Loading

Codecov Report

Merging #4736 (7e895d3) into master (3a2381b) will decrease coverage by 0.25%.
The diff coverage is 26.82%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4736      +/-   ##
==========================================
- Coverage   50.42%   50.17%   -0.26%     
==========================================
  Files         511      513       +2     
  Lines       18665    18820     +155     
  Branches     1644     1664      +20     
==========================================
+ Hits         9411     9442      +31     
- Misses       8837     8960     +123     
- Partials      417      418       +1
Impacted Files Coverage Δ
src/plugins/imagery/components/ImageControls.vue 22.58% <22.58%> (ø)
src/plugins/imagery/components/ImageryView.vue 32.80% <26.16%> (-5.21%) ⬇️
src/plugins/imagery/lib/eventHelpers.js 36.11% <36.11%> (ø)
...ettingsSynchronizer/URLTimeSettingsSynchronizer.js 96.96% <0.00%> (-1.02%) ⬇️
...c/plugins/persistence/couch/CouchObjectProvider.js 81.77% <0.00%> (-0.45%) ⬇️
src/api/objects/ObjectAPI.js 93.37% <0.00%> (+2.20%) ⬆️

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 3a2381b...7e895d3. Read the comment docs.

@michaelrogers
Copy link
Contributor Author

michaelrogers commented Jan 20, 2022 •
edited
Loading

@charlesh88 - In this image you can see the temporary mocked up UI for interacting with the zoom level. There is a zoom in, zoom out, and reset button. The current zoom scale is called out if the zoom is greater than 1; in the photo that is visible as x2.00. Happy to make adjustments to its positioning and structure to integrate better with the existing UI.

Screen Shot 2022-01-20 at 8 54 32 AM

There is currently some overlap between how the mouse click (zoom in) and alt/option + mouseclick (zoom out) behaves with the way pan is handled on the plots. In plots, panning is initiated with alt/option keypress. We might consider moving the zoom reverse to another keybinding to avoid any confusion there.

Screen Shot 2022-01-28 at 10 00 58 AM

Another concern is the current mocked up UI is visible when imagery is included in a layout. This might not be a concern if the UI is activated on hover like the contrast/brightness sliders.

michaelrogers and others added 14 commits January 20, 2022 11:09
@michaelrogers
Remove unused draw library files
1188663
@shefalijoshi
Update version number (#4759)
457cd42
@michaelrogers
Separated the translation calculations externally
0d06308
@akhenry
Alternative implementation of zoomImage
d53ce2d
@michaelrogers
Add panning handlers
d1a899c
@michaelrogers
Change click listener to mousedown
30a7846
@michaelrogers
Pan capability
bfc7f2d
@michaelrogers
Capture initial wheelzoom coordinates by using debounced flag
a20da34
@scottbell
update copyright (#4775)
169c23d
@michaelrogers
Add debounced method to track wheel zoom sequence
fb84c05
@akhenry
Observe changes to sub-objects in flexible layouts. (#4780)
205dc67 
* Get child of flex layout as mutable if possible

* Fix bug when no default notebook defined
@shefalijoshi
Follow domain object changes for Independent time conductor (#4783)
2f292fb 
* Track if domain object changes when independent time conductor is in use.
@shefalijoshi
fix typo when using fallback template (#4784)
44f5372
@michaelrogers
Code cleanup
a5c734b
@michaelrogers michaelrogers marked this pull request as ready for review January 27, 2022 19:44
@shefalijoshi
Request data when switching between real time and fixed. (#4807)
2329076 
* Request data when switching between real time and fixed.
Also add some null checks
* Address review comments - add more conditional logic to optimize time conductor.
@shefalijoshi
Revert "Request data when switching between real time and fixed. (#4807…
2732e9f 
…)" (#4809)

This reverts commit 2329076.
@charlesh88
Copy link
Contributor

@michaelrogers Looking at this now.

@charlesh88
Styling for mct34
daaf7f0 
- Moved controls into local controls holder;
- `<a>` tags converted to `button`;
- WIP! Have not linted or made sure I didn't break tests yet;
@charlesh88
Copy link
Contributor

@michaelrogers Have pushed to the branch:

  • I've done some visual styling on the buttons and gotten them how I want them.
  • Not sure what the lock button is supposed to do, and hope I didn't inadvertently break it.
  • Think we mis-communicated a bit about how the image zoom should behave within the image holder. Let's talk asap Monday 1/31.

@michaelrogers
Copy link
Contributor Author

@charlesh88 - Thanks for pushing the updates. The new UI layout looks sharp.

  • The lock button is for persisting the zoom translation and scaling. This would allow the operator to resume a local clock view and stream in new images while focused on a fixed point.
  • Happy to sync with you on 1/31 to iron out the details.

shefalijoshi
shefalijoshi requested changes Feb 18, 2022
View reviewed changes
Copy link
Contributor

@shefalijoshi shefalijoshi left a comment

Choose a reason for hiding this comment

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

The code logic looks solid! 💯
There needs to be some code reorganization so that it's more readable. Added comments to that effect.
Thank you for doing this!

@michaelrogers michaelrogers mentioned this pull request Mar 1, 2022
Closed
5 tasks
michaelrogers and others added 2 commits March 16, 2022 14:53
@michaelrogers @shefalijoshi
Mct 34 revisions (#4937)
f097f25 
* Zoom pan controls moved to component
* Implement adjustments to encapsulate state into ImageryControls
* Track modifier key pressed for layouts
* Removed trailing space
* toggleLock -> toggleZoomLock
* Required zoomFactor, remove default
* Added back in click listener for pan zoom click

Co-authored-by: Joshi <[email protected]>
@michaelrogers
Merge branch 'master' into mct34
b06c0bb
nikhilmandlik
nikhilmandlik reviewed Mar 22, 2022
View reviewed changes
Copy link
Contributor

@nikhilmandlik nikhilmandlik left a comment

Choose a reason for hiding this comment

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

Small suggestions/changes, Almost ready to merge 👍

@michaelrogers
Copy link
Contributor Author

@nikhilmandlik - Thanks, I've updated a few of the items but had a question about the best way to proceed for:

#4736 (comment)

nikhilmandlik
nikhilmandlik approved these changes Mar 23, 2022
View reviewed changes
Copy link
Contributor

@nikhilmandlik nikhilmandlik left a comment

Choose a reason for hiding this comment

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

LGTM

@michaelrogers michaelrogers enabled auto-merge (squash) March 23, 2022 19:17
@michaelrogers michaelrogers merged commit 0f9e727 into master Mar 23, 2022
@unlikelyzero unlikelyzero deleted the mct34 branch March 23, 2022 22:45
@michaelrogers michaelrogers mentioned this pull request Apr 1, 2022
Merged
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@unlikelyzero unlikelyzero unlikelyzero left review comments

@nikhilmandlik nikhilmandlik nikhilmandlik approved these changes

@shefalijoshi shefalijoshi shefalijoshi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Image Telemetry] Support pan-zoom
9 participants
@michaelrogers @charlesh88 @akhenry @nikhilmandlik @shefalijoshi @unlikelyzero @scottbell @johnhill2 @jvigliotta