edemaine / cocreate

Cocreate Shared Whiteboard/Drawing
MIT License
209 stars 27 forks source link

Touch input pinch to zoom, #13 #111

Closed duetosymmetry closed 2 years ago

duetosymmetry commented 3 years ago

This PR will add touch-based pinch-to-zoom and simultaneously pan.

The desired behavior is as follows:

The current code uses some unsavory global variables for state and needs to be refactored into an object. Having never programmed in coffeescript before, I kindly request some guidance from @edemaine or another expert. There may be lots of non-idiomatic code in this PR.

I also haven't really studied the structure of the rest of the code, so I don't know if I'm stepping on the feet of existing code.

Finally, I haven't thought too carefully about how should the input state machine work. Currently, the user can start out drawing with one finger, but then the second finger can trigger zoom/pan behavior. This seems undesirable, and suggestions are welcome for how to avoid this, or general discussion how should multitouch input be handled.

duetosymmetry commented 3 years ago

All relevant code has been refactored into the touchStateMachine object, I think this is ready for review.

duetosymmetry commented 3 years ago

Added text to doc/README.md. Rebased on master.

duetosymmetry commented 3 years ago

I've been using cocreate from a tablet a lot, and I'd love to be able to use two-finger pinch-to-zoom. Any thoughts on the approach implemented here, or how it might be better structured?

duetosymmetry commented 3 years ago

Rebased on master to fix merge conflicts

duetosymmetry commented 3 years ago

@edemaine Can I request a code review or feedback if this whole implementation needs an overhaul?

edemaine commented 3 years ago

Sorry again for the delay. I just did some testing and code cleanup. Overall, this is really cool, and I'd love to get it merged in. But first we need to deal with a few things:

  1. I don't like how this behaves when in a drawing tool and touchDraw (new name for allowTouch/allowTouchDraw) is true. One or more draw gestures will start, get interrupted by pan/zoom, and generally makes a mess.

    There's a general challenge here of how to distinguish the first finger that comes into contact, when it could be the beginning of a draw operation or the beginning of a pair of fingers to start a zoom operation. I see two options:

    1. Disable multifinger gestures when touchDraw is true, or perhaps more precisely, when restrictTouchDraw returns false. This seems simplest, and is my preference (for reasons described below). Is it sufficient for your purposes? (Do you use a pen on your tablet? Or could you stand switching to pan mode?) I added a reset method to the state machine that would enable this; it could be called whenever touchDraw or the current mode changes (so restrictTouchDraw might change).
    2. When drawing with touch, don't actually start a draw operation (retroactively at the initial pointerdown) until a certain distance has been traversed. This is probably what most touch-based interfaces do, but it would be a much bigger code rewrite... It also means that users could no longer do "multifinger drawing" (multiple fingers drawing at once), which is fun when playing with kids, so I'd want to preserve it behind a toggle at least.
  2. There's buggy behavior when using this in pan mode, because both the multitouch state machine and the pan mode store an initial translation. Depending on the exact lifting sequence, this can lead to a big wrong translation at the end of a multitouch session.

    I wonder if the "right" way to think about this is that the "multitouch state machine" should really be (replace) the pan mode. Then all we need to add is "jumping into this mode temporarily" when doing multitouch gestures in a non-pan mode, when restrictTouchDraw returns true. One approach to this temporary jumping is how Cocreate already jumps into pan mode when holding spacebar; it's a bit hacky, though, and because it's an actual mode transition it would prevent things like: start drawing with pen/mouse, use multifinger gesture to zoom, and continue drawing the same object with pen/mouse. We also need the state machine to even detect a multitouch gesture, so something closer to the current approach seems necessary anyway...

    I've long wanted to convert modes into classes (or objects), so that they each have their own state instead of the global pointers object. This may be a case where we'd also like either subclassing or multiple instances of the same class, because pan mode and the multitouch state machine are similar but different: pan responds to all events, not just touch, and translates even with one-pointer gestures, while the multitouch state machine only responds to two-pointer (two-finger) gestures. So their behavior is largely shared code, with some boolean parameters to affect behavior.

  3. We/I should probably change the touchDraw icon to also include a stroke underneath the extended finger, to more accurately represent "drawing via touch" instead of just "touch".

Let me know your thoughts, especially on 1 and 2, and then we can figure out how to proceed.

duetosymmetry commented 3 years ago

Thanks for taking a look! First, a general comment about the overall approach. I recently tried GIMP on a multitouch input device, and saw that they had the following behavior. Each input has its own tool bound to it. So, for example, if I have a mouse or touchpad ('pointer'), style ('pen'), and 'touch', then there could be three different tools bound. Touch could be bound to crop tool, stylus to calligraphic tool, and pointer to airbrush, or something like that. That model seems powerful but potentially confusing.

  1. Thanks for noting this, it's definitely bad behavior. IMO the most versatile approach would be something like the following. If a touchPointer travels a threshold distance (like eraseDist or dragDist), switch the state machine into a new state, 'usingTool' or something like that [note that while I called it a state machine, I haven't actually made a state diagram and implemented different states yet; it's currently just the binary @doingPanZoom]. If any tool is being used by a touch pointer, we should block two-finger pan/zooming. This way, if somebody starts drawing, as many fingers as you want can start to draw. Only leave this state if all touchPointers are lifted. However, if two pointers go down before either one has traversed a threshold distance, transition to state 'doingPanZoom'. Then, only the first two touchPointers which were touched will control the zooming/panning. Additional touch input should be ignored. If either of the original two touchPointers are lifted, we have to leave the state and do something sensible with the remaining still-down pointers. I'm not sure what to do with them -- the easiest thing to do is continue to ignore them until all of them are lifted. Probably this still seems a little too fuzzy, so I think we should make a state diagram so we force ourselves to specify behavior for down, up, and move events in each state, and the transitions between them. Regarding how big of a rewrite it would be if touch inputs pend until traveling a threshold distance: there is less work to do if the touchStateMachine keeps track of the distance before dispatching to the tool

  2. Yes, this definitely needs additional states. Not all tools make sense for multiple touch inputs at the same time (e.g. does it make sense for the text tool, which only has one input field?). I'd say either continue forwarding just the first pointer to the tool, ignoring the others; or end the pan tool action and transition to two-finger pan/zoom. Either way, we should specify this when we make the state machine more precise.

  3. Sounds good, were you thinking of something like these? [1], [2]

edemaine commented 3 years ago
  1. Your approach sounds reasonable, though as you say, it probably needs some refinement and testing before we can be certain it'll work. But this is surely how most iPad apps work, so I'm hopeful it'll work well. Some thoughts:
    • It's maybe hard to set thresholds in general (I usually find the current dragDist too small, but not sure how system-dependent that is). But in this case, we can measure the size of the finger via the PointerEvent's width and height, and maybe set the threshold relative to that. Will still need some testing, but hopefully we can give the feeling of real-time drawing because by the time your finger is out of the way, we've passed the threshold.
    • The touch state machine will need to track all events that happen before a state change (not just the distance traveled), so that it can retroactively pass all the events to the tool. You're right that this should be possible, though; we can just send those events to currentTool when it's time. (Currently, events aren't particularly time sensitive. At some distant future, we might care about the timeline for some kind of freehand smoothing, but we could always record the times in those cases.) We could even imagine extra-coalescing pointermove events so that they're dealt with more efficiently.
    • In pan mode, we probably don't want to wait for a critical distance traveled, so that one-finger panning feels responsive. Easy enough.
    • I still have some worry that two people drawing freehand, picking up and dropping their respective fingers, might accidentally start a multitouch gesture... So we might still want a multitouch gesture toggle. But this would certainly be less likely and less crucial. (Could probably be relegated to a settings page once we have one...)
    • All this said, I feel like this approach is substantially more complicated, while the current PR is close, so it might be a good idea to delay this touchDraw-supporting approach for a future PR, and try to get the current behavior enabled when touchDraw is false. Thoughts?
  2. A simple approach for now would be to not activate the touch state machine in pan mode, and give full control to the pan tool. This seems like what we want (in some sense) even with the complex implementation of 1. And then duplicate the multitouch code into the pan tool so that it can pan and zoom, or ideally find a way to deduplicate/share the core code (maybe a new Board.scale helper?).
  3. Yeah, those look nice, especially the first. I was thinking of modifying the FontAwesome icon to add a path, along those lines. EDIT: Done.
edemaine commented 3 years ago

Forgot to respond about this:

recently tried GIMP on a multitouch input device, and saw that they had the following behavior. Each input has its own tool bound to it. So, for example, if I have a mouse or touchpad ('pointer'), style ('pen'), and 'touch', then there could be three different tools bound. Touch could be bound to crop tool, stylus to calligraphic tool, and pointer to airbrush, or something like that. That model seems powerful but potentially confusing.

Neat that GIMP was so bold to do this. (Of course, GIMP is somewhat infamous for being difficult to use...) I had a similar idea in #39 (which is probably where this discussion "belongs", but not important), for how to assign different operations to different tips/buttons on a pen (e.g. to use the opposite end of a pen to erase, and maybe also for doing different things with left and right mouse). I think at the level of a pen (or a mouse) it makes sense. My main worry for doing it with pen vs. touch is that a comfortable use case is to select tools with touch of the finger with one hand, and draw using the pen (or mouse) in the other hand. I routinely do that with a keyboard (left hand on keyboard selecting tools, right hand using mouse/pen/whatever), so I could imagine a tablet user doing so as well (left hand on tool set, right hand drawing with touch/pen/whatever). This workflow wouldn't work if touch has its own tool separate from pen/mouse tool...

Roughly, touchDraw set to false is like a simple version of this, which forces touch to act like a two-finger pan tool (while all other pointer types can be different tools), which is probably the most important/common case that a user would want to assign a tool specifically to touch (and they have other pointer types for other tools). But I'm open to hear other use cases that might make sense... e.g. would you want to select with touch and draw with pen? Unclear, especially that using a pen with your palm on the tablet can easily accidentally register a touch event (which is why I'd only want two-finger options to trigger anything when touchDraw is false).

duetosymmetry commented 3 years ago

I also use the 'Squid' app on a tablet, which similarly allows the user to bind touch, stylus, stylus's eraser, stylus with button... all to different tools. This is probably much easier for a single user who carefully configures their environment rather than multiple users all interacting with the same display [are there multi-touch displays with active styli that will identify which of multiple styli are inputting?]. The way I use it (the default, IIRC) is for single-finger touch to be an eraser (since I do not have an eraser or button on my stylus). Two-finger touch is still pan/zoom, that can't be altered in Squid.

All this said, I feel like this approach is substantially more complicated, while the current PR is close, so it might be a good idea to delay this touchDraw-supporting approach for a future PR, and try to get the current behavior enabled when touchDraw is false. Thoughts?

Wise idea. Better to make some partial progress than try to solve all problems at once.

A simple approach for now would be to not activate the touch state machine in pan mode, and give full control to the pan tool. This seems like what we want (in some sense) even with the complex implementation of 1. And then duplicate the multitouch code into the pan tool so that it can pan and zoom,

I think it seems easier to keep this code within the touch state machine; i.e. to say if the current tool is pan, and we are handling touch events, for the state machine to be responsible for both the panning and zooming. Or do you think that makes the code too difficult to follow?

ideally find a way to deduplicate/share the core code

Ok, this would help with my above worry about code readability.

edemaine commented 3 years ago

I also use the 'Squid' app on a tablet, which similarly allows the user to bind touch, stylus, stylus's eraser, stylus with button... all to different tools. This is probably much easier for a single user who carefully configures their environment rather than multiple users all interacting with the same display

Maybe one day the touchDraw toggle could have a right/long-press option for selecting a preferred tool for touch...

[are there multi-touch displays with active styli that will identify which of multiple styli are inputting?]

Certainly Wacom tablets can distinguish between different styli. I don't know whether that comes through as distinguishable PointerEvent inputs (e.g. consistent pointerIds or buttons) — I will have to get a second pen to test.

The way I use it (the default, IIRC) is for single-finger touch to be an eraser (since I do not have an eraser or button on my stylus). Two-finger touch is still pan/zoom, that can't be altered in Squid.

Interesting — do you ever accidentally erase something when doing a two-finger pan/zoom gesture?

I think it seems easier to keep this code within the touch state machine; i.e. to say if the current tool is pan, and we are handling touch events, for the state machine to be responsible for both the panning and zooming. Or do you think that makes the code too difficult to follow?

I guess that makes sense, though it does seem weird at first. In this view, the touch state machine is essentially an "overlay mode" that affects all modes, and some modes slightly differently. And then pan mode doesn't actually do anything (that's the weird part). At the least, we'll want a code comment in the pan mode to this effect.

I could see going either way on this. It'd probably also be easy enough to call similar functions between the touch state machine / overlay and the pan tool, with different parameters, like I mentioned above. But feel free to try whichever you'd like first.

edemaine commented 2 years ago

In commits f657258af649a84b7ec699679b61b19ad7d63c1f, 064b6e96894927c700518065b08e535f87eab95a, f1a01cd16a34ad260601a8894d8fe5c471f4d811 I ported this code to modern Cocreate and implemented the plan I'd previously stated:

Thanks again for writing the initial code here! I've marked f657258af649a84b7ec699679b61b19ad7d63c1f in particular as co-authored by you. Happy to finally have multitouch pan/zoom in Cocreate! Tablet users will be very happy.

duetosymmetry commented 2 years ago

Hooray! Sorry I got distracted with other things, glad it finally made it in though