edemaine / cocreate

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

Zoom trouble #127

Closed edemaine closed 3 years ago

edemaine commented 3 years ago

Zooming far in or out, especially with trackpad, can lead to some unfortunate behavior:

@duetosymmetry Could you take a look at this?

duetosymmetry commented 3 years ago

I've reproduced. The issue is what function to use in when determining newScale: https://github.com/edemaine/cocreate/blob/42838485e6656e7aea1ff442cd37bdc35130c035/client/main.coffee#L990-L994

Depending on the OS sensitivity setting for scroll wheel or track pad, if e.deltaY is greater than 100, scale will go negative. I think we want something that's close to newScale = transform.scale * (1.0 - e.deltaY * 0.01) for small values of e.deltaY, but for large values will asymptote to a max.

How about taking a sigmoid function and scaling/shifting it so that the scale factor (between old scale and new scale) is clamped to lie in some window of some minFac (0 < minFac < 1) to maxFac (1 < maxFac)?

diomidov commented 3 years ago

I think it should be newScale = Math.exp(-transform.scale / c) for some constant c. This way scrolling by 2Δy is the same as scrolling by Δy twice.

duetosymmetry commented 3 years ago

@diomidov thanks for thinking about this mathematically, which I was not doing!

This way scrolling by 2Δy is the same as scrolling by Δy twice.

Or more generally, I think we want the scale transformation to act commutatively, so if we have a scroll distance Δy1 followed by Δy2, it should enact the same transformation as (Δy1+Δy2). This means that zooming in and out the same number of wheel clicks should exactly cancel — which it currently does not!

I think you really meant something like:

 if e.ctrlKey 
   newScale = transform.scale * Math.exp(- e.deltaY * 0.01)
   ...

I will open a PR to fix this.

Thanks for the suggestion!

duetosymmetry commented 3 years ago

BTW in the PR #128, I did not attempt to attempt to clamp the zoom to some reasonable range. Should we attempt to perform clamping so the user can't zoom in/out too far?

edemaine commented 3 years ago

Great, thanks to you both! I will test once I'm back at my computers.

Wrapping in exp should guarantee positivity, so we won't get the flipping issue anymore.

In my user experience, limiting the amount of zoom can be kind of annoying. Given that users can always fall back to "reset zoom" if they get lost, I'd say we can skip clamping, at least for now.

edemaine commented 3 years ago

Experimenting with #128 using a scroll wheel, it does turn out to be way too easy to over zoom/unzoom. Although it's less mathematically natural, I think I preferred the old feeling. Is there a way to just make the unzoom the inverse of the zoom, essentially taking the log of this exponential so that big gestures don't result in crazy zooms? Maybe:

if e.deltaY < 0
  newScale = transform.scale * (1.0 - e.deltaY * 0.01)
else
  newScale = transform.scale / (1.0 + e.deltaY * 0.01)

i.e.

factor = 1.0 + Math.abs(e.deltaY) * 0.01
factor = 1/factor if e.deltaY > 0
newScale = transform.scale * factor

With some simple experimentation, this seems better behaved...

I'm pushing this behavior, but happy to discuss further.