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

Add keyboard control for zoom #44

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ctrueden
Copy link

@ctrueden ctrueden commented Oct 8, 2024

This patch adds keyboard shortcuts for zooming the canvas in and out using +/= and -/_ respectively. It also tweaks the hover logic to save the last computed data point, so that the canvas can be zoomed more precisely at that coordinate.

Copy link

codecov bot commented Oct 8, 2024

Codecov Report

Attention: Patch coverage is 33.33333% with 8 lines in your changes missing coverage. Please review.

Project coverage is 77.62%. Comparing base (10abe13) to head (5d2e83f).

Files with missing lines Patch % Lines
src/ndv/viewer/_viewer.py 33.33% 4 Missing ⚠️
src/ndv/viewer/_backends/_pygfx.py 25.00% 3 Missing ⚠️
src/ndv/viewer/_backends/_vispy.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #44      +/-   ##
==========================================
- Coverage   77.90%   77.62%   -0.29%     
==========================================
  Files          13       13              
  Lines        1856     1868      +12     
==========================================
+ Hits         1446     1450       +4     
- Misses        410      418       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tlambert03
Copy link
Member

hell yes :)

there is a slight typing issue here, which requires some explanation. the self._canvas issue you've found is an interface that needs to be implemented for each backend (vispy and pygfx). self._canvas._camera.zoom is actually a vispy-specific API.

So, we either need to add a zoom() method to that PCanvas interface (to be implemented by both backends), or we need to use the existing self._canvas.set_range() method instead (which, then leads to the question "how do I get the current range 😂 ... lemme dig a moment)(

This adds a zoom function to the PCanvas and implements for the two
current backends. For vispy, the zoom happens under the cursor, like how
the mouse wheel behaves. But for pygfx, I did not yet figure out how to
do that. Do we need to translate the focused point to the origin, then
change zoom, then translate it back?
@tlambert03
Copy link
Member

thanks for the update @ctrueden. Couple remaining issues. (Depending on your degree of interest, I'm happy to fix them from here):

  • the pygfx backend seems to A) zoom the wrong direction and B) not zoom around the mouse position. (To see this, pip install pygfx and then use NDV_CANVAS_BACKEND=pygfx python examples/numpy_arr.py in order to pick backend)
  • this breaks in 3D mode. I'm not sure what the appropriate behavior should be in 3D mode. Options include:
    • simply doing nothing (i.e. use an isinstance(camera, ...) check on the backend and just return if it's not the right type of camera)
    • do try to zoom in and out, but then deal with center not being as easy/relevant in the 3d camera modes (for example, using the mouse wheel in 3d mode doesn't pay attention to the mouse position)

@ctrueden
Copy link
Author

ctrueden commented Oct 9, 2024

@tlambert03 Thanks. Sorry about the pygfx wrong-direction zoom; I didn't notice that when I tested briefly. I did know about the mouse position limitation though—I wrote about it in the commit message.

In my view, the +/- keys should behave the same as the mouse wheel scroll (in both 2D and 3D), so of course the first place I looked for implementing +/- was how the mouse wheel code works. But I, embarrassingly, could not figure out how/where the mouse wheel code is... reading the source, all I could see was stuff about a selection, which did not seem relevant. I concluded that maybe the mouse wheel behavior is handled upstream in the backends themselves, rather than in ndv? But I didn't yet dive into a debugger to confirm that. If you know how it works already, I think it makes sense for you to do your thing and wrap up this work. But if you would also need to dig, I can try to make time to do it myself some time soon. Just let me know your preference!

@ctrueden ctrueden marked this pull request as draft October 9, 2024 17:24
@tlambert03
Copy link
Member

I concluded that maybe the mouse wheel behavior is handled upstream in the backends themselves, rather than in ndv?

indeed! all of the mouse interactions are currently just passed to the backends. This was done for now so as to avoid duplicating all of the mouse/controller logic that has already been implemented in these backends. It does of course mean that cases like this can be a little hard to figure out exactly what is handling mouse interactions. For now, until we fully abstract away all mouse interactions and key interactions, this is the state of things. We can certainly intercept mouse/key interaction and do something else, but by default, we just pass them through to the backends (which, thankfully, do mostly the same thing with them)

I'm also more than happy to dig into this (i appreciate what you've already done :)). not sure when I'll get to it with busy upcoming weeks, but whoever gets there first can update the other. thanks @ctrueden 🙏

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