Prevent canvas micro-panning on point add #5742

brisvag · 2023-04-18T14:12:11Z

Description

Currently, when adding points interactively the vispy camera is still getting moved by mouse events.

This is not a problem with a mouse - since normally you don't move the mouse while clicking - but it's pretty frustrating with a tablet, since the cursor moves slightly every time you press down or release the stylus. This results on the canvas moving around a tiny bit every time you click.

This was a know issue, since there is a mouse-movement threshold in place beyond which a point doesn't get added; however this does not prevent the canvas from moving even within that threshold.

This PR prevents the vispy camera from receiving the event if this threshold is not yet surpassed.

Type of change

Bug-ix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

psobolewskiPhD · 2023-04-18T14:20:31Z

On the other hand, the current behavior is pretty sweet on a trackpad where click is click and pan is a drag gesture.
I think the issue here and the other one related to using a Wacom is probably really vispy not handling/using QTabletEvents
https://doc.qt.io/qt-6/qtwidgets-widgets-tablet-example.html
https://doc.qt.io/qtforpython/PySide6/QtGui/QTabletEvent.html

codecov · 2023-04-18T14:37:02Z

Codecov Report

Merging #5742 (3a59de1) into main (1da0350) will increase coverage by 0.01%.
The diff coverage is 95.77%.

@@            Coverage Diff             @@
##             main    #5742      +/-   ##
==========================================
+ Coverage   89.86%   89.87%   +0.01%     
==========================================
  Files         610      610              
  Lines       52121    52133      +12     
==========================================
+ Hits        46837    46856      +19     
+ Misses       5284     5277       -7

Impacted Files	Coverage Δ
napari/_vispy/canvas.py	`93.05% <50.00%> (-0.41%)`	⬇️
napari/layers/points/_points_mouse_bindings.py	`96.51% <85.71%> (-1.05%)`	⬇️
napari/utils/_proxies.py	`94.11% <85.71%> (-0.89%)`	⬇️
...layers/points/_tests/test_points_mouse_bindings.py	`100.00% <100.00%> (ø)`

... and 7 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

brisvag · 2023-04-18T14:47:05Z

On the other hand, the current behavior is pretty sweet on a trackpad where click is click and pan is a drag gesture.

Yeah... what we really need is for the threshold to also apply to vispy camera events :/

I think the issue here and the other one related to using a Wacom is probably really vispy not handling/using QTabletEvents

How would this solve the problem? Or are you saying that we could differentiate the two cases and disable panning only if it's qtabletevents and not normal mouse events?

psobolewskiPhD · 2023-04-18T16:14:04Z

I assume proper handling of the actual tablet/stylus would help resolve the issue because the tap event would be handled as that and not as a click-slight drag. Basically, I'm assuming at the moment everything is just being treated as mouse events—same with my trackpad, only the gestures that the OS translates to mouse events work.

jni · 2023-04-19T00:42:35Z

I assume this interactivity was enabled so that one can click and drag to pan, and just click to add ponts. I don't think this a good reason to keep it, since we have the hold space to pan zoom feature.

This was added in #2148. Space to pan is good but it is much less ergonomic than just using the mouse — there's a reason why that PR was made. So I don't want to "just" revert that change.

The good solution would be to not forward events to vispy camera unless we go beyond that threshold; unfortunately, I think this requires the famous refactoring of event system so that napari comes first, and then vispy, rather than the other way around.

I'm pro-this. 😂 You might not need a complete refactoring, as shown by the recent PRs for keeping mouse zoom working in all modes. #5701 #5726

jni

Sorry I thought this was an issue not a PR 😂

brisvag · 2023-04-19T10:21:56Z

Ok, I found another solution using the event.handled attribute.

Unfortunately it required some changes to the ReadOnlyProxy to give it exceptions. I could not simply remove the ReadOnlyProxy, because all the vents suddenly broke (really hard to debug why :/)

The basic idea is: mark event as handled if we're within a small threshold of movement, and on the vispy canvas side simply ignore the event if it's marked as handled.

Note that this works because the order of callbacks is set up so that vispy acts last.

cc @Czaki @jni

This reverts commit 1e25701.

brisvag · 2023-04-19T13:46:55Z

These tests are so annoying :/ They are failing claiming that a point is added when it shouldn't even though that doesn't happen when actually using napari. I think it's because all the ReadOnlyWrapper logic is flawed, and I can't find out how exactly.

@kevinyamauchi it seems you touched these tests before; maybe you have some suggestions?

brisvag · 2023-04-19T17:32:48Z

@jni the changes should satisfy you now :)

alisterburt

Really nice - tested manually and all working as expected from my side, could you add a test? With a test I think this is ready to go!

stale

brisvag · 2023-04-24T15:23:17Z

napari/layers/points/_tests/test_points_mouse_bindings.py

    # Simulate click
-    event = ReadOnlyWrapper(
-        Event(type='mouse_press', position=known_non_point)
+    event = read_only_event(
+        type='mouse_press', position=known_non_point, pos=np.array([0, 0])
    )
    mouse_press_callbacks(layer, event)

    known_non_point_end = [40, 60]

    # Simulate drag end
-    event = ReadOnlyWrapper(
-        Event(
-            type='mouse_move', is_dragging=True, position=known_non_point_end
-        )
+    event = read_only_event(
+        type='mouse_move',
+        is_dragging=True,
+        position=known_non_point_end,
+        pos=np.array([4, 4]),
    )
    mouse_move_callbacks(layer, event)

    # Simulate release
-    event = ReadOnlyWrapper(
-        Event(
-            type='mouse_release',
-            position=known_non_point_end,
-            pos=np.array([4, 4]),
-        )
+    event = read_only_event(
+        type='mouse_release',
+        position=known_non_point_end,
+        pos=np.array([4, 4]),


The add/not-add-point behaviour is already tested here. Testing whether the camera moves is quite a bit harder cause we need a fully working vispy canvas, and I don't seem to be able to get that to work. I'm using make_napari_viewer and then checking if viewer.window.qt_viewer.camera.center changes before and after the callback. For some reason, this never changes, despite the fact that when I'm actually testing manually, this value is changed as appropriate. Any suggestions? Otherwise I suggest we get this in without adding an extra test, since it's a small bug fix.

alisterburt

happy for this to go in if main behaviour is already covered in tests!

…t-mouse-pan

# Description In the merge of #5432 we lost the change from #5742 to allow `event.handled` to be set despite the readonly proxy. This reinstates it.

Currently, when adding points interactively the vispy camera is still getting moved by mouse events. This is not a problem with a mouse - since normally you don't move the mouse while clicking - but it's pretty frustrating with a tablet, since the cursor moves slightly every time you press down or release the stylus. This results on the canvas moving around a tiny bit every time you click. This was a know issue, since there is a mouse-movement threshold in place beyond which a point doesn't get added; however this does not prevent the canvas from moving even within that threshold. This PR prevents the vispy camera from receiving the event if this threshold is not yet surpassed.  - [x] Bug-ix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update

psobolewskiPhD · 2024-05-02T22:07:30Z

I ran into this -- I think -- recently in a workshop.
I've never been using a mouse much before, but for this I was using mouse and external monitor.
When I clicked to add a point instead the canvas moves -- happens more frequently if I'm going fast -- I was just trying to spam a few points for an example. Reaching over to the trackpad when I tap I could never make that happen.
Any thoughts @brisvag ? Must be difference in location between press and release. Maybe lack of accounting for hi-dpi? Could it be coordinate related? I'm using a vanilla Points Layer in gui, with no scaling.

brisvag · 2024-05-03T07:08:01Z

As a quick check, try setting the DRAG_DIST_THRESHOLD to like 10 and see if it still happens!

psobolewskiPhD · 2024-05-03T21:51:29Z

Thanks, that helps, but, I need to set to at least 20 to mostly prevent the effect when labeling and 50 still feels better.
As a test, open the Brick sample and try to quickly label each brick with a point.
I guess it's an edge case, but it's weird. Also I never think to drag the canvas in Add mode. I use spacebar to toggle -- zoom works naturally.
(napari is so sweet on a track pad I never realized how janky things can be with a mouse)

brisvag · 2024-05-06T08:25:28Z

With the current value on main, I can go as fast as I can and Istill never move the canvas ^^' Either you're very fast, or I'm very slow, or something else is afoot, probably the hidpi. But so HI as to require a whopping 50 pixels?

psobolewskiPhD · 2024-05-06T13:05:29Z

@brisvag What kind of mouse do you have?
I looked into it a bit more, it's not so much how quickly you move the mouse, per se, it's how long is the duration between mouse press and release, when clicking.
I tested two mice:
My work Microsoft mouse (yes, it's crappy) has a slow click, no matter what you do with your finger -- I get the panning instead of points frequently.
At home I have access to a gaming Logitech mouse and the click is massively faster and rarely get the issue at 5.
And when using my trackpad I guess it's just the tap event that triggers a point, so equivalent to point on press and not on release, so it can't happen.

brisvag · 2024-05-06T14:14:08Z

I have a logitech B100 according to the sticker, should be a generic office mouse afaik ^^'

Czaki · 2024-05-06T14:46:15Z

@psobolewskiPhD could you somehow log what are mean movement of your mouse movement?

psobolewskiPhD · 2024-05-06T15:11:31Z

like a screen recording?

Czaki · 2024-05-06T15:55:24Z

napari/layers/points/_points_mouse_bindings.py

-
-    while event.type != 'mouse_release':
+    while event.type == 'mouse_move':
+        dist = np.linalg.norm(start_pos - event.pos)


Like print this dist and test how it behaves when you perform long and short moves.

psobolewskiPhD · 2024-05-06T20:17:35Z

When I go slow, I get 0.0 and 1.0 basically exclusively.
When I got quickly I get all sorts:

1.4142135623730951
5.0990195135927845
19.026297590440446
29.017236257093817
48.010415536631214

The values appear to be in screen pixels?
Here I was very zoomed in - which made the issue much worse:

11.045361017187261
19.1049731745428
34.713109915419565
4.123105625617661
9.219544457292887
12.36931687685298
16.278820596099706
25.317977802344327
42.20189569201838

psobolewskiPhD · 2024-05-06T20:18:16Z

Basically I have to be very careful to ensure the full release of the mouse button before moving. Very deliberate.

disable mouse pan on point add

1e25701

github-actions bot assigned brisvag Apr 18, 2023

jni previously requested changes Apr 19, 2023

View reviewed changes

prevent canvas from moving

4f94a54

github-actions bot added the qt Relates to qt label Apr 19, 2023

brisvag force-pushed the fix/point-mouse-pan branch from 34d5bf1 to 640d212 Compare April 19, 2023 11:44

Revert "disable mouse pan on point add"

640d212

This reverts commit 1e25701.

brisvag changed the title ~~Disable mouse pan on point add~~ Prevent canvas micro-panning on point add Apr 19, 2023

brisvag added the bugfix PR with bugfix label Apr 19, 2023

brisvag added 2 commits April 19, 2023 15:38

fix tests

02e02cc

remove useless line

e85eeec

github-actions bot added the tests Something related to our tests label Apr 19, 2023

brisvag added 2 commits April 19, 2023 18:11

fix test missing pos

b0b9daa

be super safe

471dbca

brisvag requested a review from jni April 19, 2023 17:32

Merge branch 'main' into fix/point-mouse-pan

d889e4b

alisterburt self-requested a review April 22, 2023 11:37

alisterburt reviewed Apr 22, 2023

View reviewed changes

brisvag mentioned this pull request Apr 24, 2023

Changing brush size with mouse movement while holding specified modifiers #5753

Merged

7 tasks

brisvag commented Apr 24, 2023

View reviewed changes

alisterburt approved these changes Apr 24, 2023

View reviewed changes

alisterburt added this to the 0.4.18 milestone Apr 24, 2023

Merge branch 'main' of https://github.com/napari/napari into fix/poin…

3a59de1

…t-mouse-pan

brisvag added the ready to merge Last chance for comments! Will be merged in ~24h label Apr 25, 2023

brisvag merged commit fb7da0a into napari:main Apr 26, 2023
34 checks passed

brisvag deleted the fix/point-mouse-pan branch April 26, 2023 15:18

brisvag removed the ready to merge Last chance for comments! Will be merged in ~24h label Apr 26, 2023

brisvag mentioned this pull request May 2, 2023

Readd exception to event proxy #5797

Merged

psobolewskiPhD mentioned this pull request May 3, 2023

[main] TypeError when adding points by GUI #5801

Closed

brisvag added a commit that referenced this pull request May 3, 2023

Readd exception to event proxy (#5797)

0ffc9e8

# Description In the merge of #5432 we lost the change from #5742 to allow `event.handled` to be set despite the readonly proxy. This reinstates it.

brisvag mentioned this pull request May 24, 2023

Fix shapes interactivity with scale != 1 (selection, rotate/resize) #5802

Merged

4 tasks

Czaki mentioned this pull request Jun 7, 2023

v0.4.18 - release process #5911

Closed

jni mentioned this pull request Jun 23, 2023

[0.4.18rc1] Dragging Points layer in add-points mode emits TypeError #5993

Closed

Czaki reviewed May 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent canvas micro-panning on point add #5742

Prevent canvas micro-panning on point add #5742

brisvag commented Apr 18, 2023 •

edited

Loading

psobolewskiPhD commented Apr 18, 2023

codecov bot commented Apr 18, 2023 •

edited

Loading

brisvag commented Apr 18, 2023

psobolewskiPhD commented Apr 18, 2023

jni commented Apr 19, 2023

jni left a comment

brisvag commented Apr 19, 2023

brisvag commented Apr 19, 2023

brisvag commented Apr 19, 2023

alisterburt left a comment

brisvag Apr 24, 2023

alisterburt left a comment

psobolewskiPhD commented May 2, 2024

brisvag commented May 3, 2024

psobolewskiPhD commented May 3, 2024 •

edited

Loading

brisvag commented May 6, 2024

psobolewskiPhD commented May 6, 2024 •

edited

Loading

brisvag commented May 6, 2024

Czaki commented May 6, 2024

psobolewskiPhD commented May 6, 2024

Czaki May 6, 2024

psobolewskiPhD commented May 6, 2024

psobolewskiPhD commented May 6, 2024

Prevent canvas micro-panning on point add #5742

Prevent canvas micro-panning on point add #5742

Conversation

brisvag commented Apr 18, 2023 • edited Loading

Description

Type of change

psobolewskiPhD commented Apr 18, 2023

codecov bot commented Apr 18, 2023 • edited Loading

Codecov Report

brisvag commented Apr 18, 2023

psobolewskiPhD commented Apr 18, 2023

jni commented Apr 19, 2023

jni left a comment

Choose a reason for hiding this comment

brisvag commented Apr 19, 2023

brisvag commented Apr 19, 2023

brisvag commented Apr 19, 2023

alisterburt left a comment

Choose a reason for hiding this comment

brisvag Apr 24, 2023

Choose a reason for hiding this comment

alisterburt left a comment

Choose a reason for hiding this comment

psobolewskiPhD commented May 2, 2024

brisvag commented May 3, 2024

psobolewskiPhD commented May 3, 2024 • edited Loading

brisvag commented May 6, 2024

psobolewskiPhD commented May 6, 2024 • edited Loading

brisvag commented May 6, 2024

Czaki commented May 6, 2024

psobolewskiPhD commented May 6, 2024

Czaki May 6, 2024

Choose a reason for hiding this comment

psobolewskiPhD commented May 6, 2024

psobolewskiPhD commented May 6, 2024

brisvag commented Apr 18, 2023 •

edited

Loading

codecov bot commented Apr 18, 2023 •

edited

Loading

psobolewskiPhD commented May 3, 2024 •

edited

Loading

psobolewskiPhD commented May 6, 2024 •

edited

Loading