mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
102.51k stars 35.37k forks source link

controls end event broken for mousewheel #20636

Open drcmda opened 3 years ago

drcmda commented 3 years ago

Describe the bug

Currently mousewheel is treated like this in controls:

function onMouseWheel(event) {
  if ( scope.enabled === false || scope.enableZoom === false || ( state !== STATE.NONE && state !== STATE.ROTATE ) ) return
  event.preventDefault()
  event.stopPropagation()
  scope.dispatchEvent(startEvent)
  handleMouseWheel(event)
  scope.dispatchEvent(endEvent)
}

That means that start and stop for zoom cannot be relied upon as it behaves differently from all other gestures, pan and rotate for instance. This makes it hard to detect when the control has stopped on the parental level.

We started to notice this because we render on demand. Controls tell the state model that the scene is moving or not. Zoom caused issues when the canvas flips on-demand-rendering on and off.

To Reproduce

https://codesandbox.io/s/r3f-basic-demo-forked-omcxr

Open the dev console and pan, you will see 1 start, 1 stop. Zoom and it will spit out hundreds of start/stops.

Expected behavior

start and stop should not leak internals, they should tell the user when controls are active and when not. As long as controls are dollying, either because mousewheel is firing in sequence or because damping is enabled, stop should not be called. In this case it has to anticipate browser behaviour and debounce the tail call. It could do the same to start with a leading call but it's not as important imo.

import debounce from "lodash-es/debounce"

const debouncedEnd = debounce(() => scope.dispatchEvent(endEvent), 100)

function onMouseWheel(event) {
  if ( scope.enabled === false || scope.enableZoom === false || ( state !== STATE.NONE && state !== STATE.ROTATE ) ) return
  event.preventDefault()
  event.stopPropagation()
  scope.dispatchEvent(startEvent)
  handleMouseWheel(event)
  debouncedEnd()
}
Mugen87 commented 3 years ago

const debouncedEnd = debounce(() => scope.dispatchEvent(endEvent), 100)

How do you know an optimal value for the rate? I mean is 100 ms better than 500ms?

I can only test with a mouse which wheel has discrete positions. Are events fired differently with other mice like a magic mouse?

I'm not sure about this feature request since users might depend on the current behavior. Since you can use debounce() on app level, you at least have a workaround at hand.

drcmda commented 3 years ago

This is what happens when you scroll:

start 
end 
start 
end 
start 
end 
start 
end 
start 
end 
start 
end 
start 
end 
start 
end 
start 
end 
start 
...

I'm not sure about this feature request since users might depend on the current behavior.

Three just removed Geometry, Face3, switched to WebGL2, etc ..., ... Imo this is a bug report, not a feature request. :-P

The debounce could be anything, just to help it connect frames. It could also be fixed by checking movement diff, perhaps better than a debounce? If it's actively moving, something needs to tell it that it can't call the stopped-event.

We can make the PR if you decide to give it a shot.

gsimone commented 3 years ago

I remember fixing something like this in some controls months ago, but it was on rotation movements. We ended up checking total movement in the last frame and calling stop after it got under a threshold.

Do you think that could work for this @Mugen87? I can make a PR for all controls, if you 👍 it

pailhead commented 2 years ago

Shouldn't the debounce be the frame rate? If two consecutive frames detect a mouse zoom, the second one should not fire an end, the next one could.

mrdoob commented 2 years ago

If two consecutive frames detect a mouse zoom, the second one should not fire

Why?

pailhead commented 2 years ago

Because at that point you don’t know if the next frame will also be zooming right?

Think about it like this, as you fire frames, each can be a vertex. If two vertices (frames) happen one after another you create a line segment. If a third one shows up, you create another segment. Now your zoom is lasting three frames and two segments (edges). You keep going until you miss a frame, at which point you could end the “line” or your multi frame zoom.

The point is I think this needs to be resolved in the past, at a minimum one frame behind, so you know if you need to keep on going or stop at the present time.

I think its easier to do this by tracking frames. If fps turns out to be a concern, this could be a fixed loop. So you could technically resolve this over several “ticks” between rendering two frames.

pailhead commented 2 years ago

How do you know an optimal value for the rate? I mean is 100 ms better than 500ms

100ms is definitely better than 500ms. If you want to do something on “end” you probably don’t want that to happen half a second later.

16ms would probably be better. Even if you render at 32, you should know (before you render) if your zoom is going on or has ended.

pailhead commented 2 years ago

image

pailhead commented 2 years ago

image

Here, in the 3rd case:

pailhead commented 2 years ago

I think this diagram may also be useful, all these red vertices are processed right before the frame is drawn (because update is conflated with render) image

so in the 3rd case, even though we have two mouse events in f1 and two in f2, we accumulate those and compute it then. So the key is to have this one "frame" in the past, where we can say :

hey did we finish that start from X frames ago, in the last frame? yes? fire end no, keep going

drcmda commented 2 years ago

so glad this is moving forward again, @pailhead do you think this is fixable then?

pailhead commented 2 years ago

I feel that a small debounce would be a great start here?

makc commented 2 years ago

@pailhead both the start and the end, since there is nothing else you can do. the browser just wont tell you when the wheel stops turning.

pailhead commented 2 years ago

It’s discrete.

makc commented 2 years ago

@pailhead actually, if we assume that event.deltaY is proportional to wheel rotation speed, we can model its rotation and estimate the time remaining until complete stop. This model would have to take into account sudden deltaY increases due to repeated spins, etc, but probably still doable? Maybe. Not sure if you get wheel events fast enough for this estimation to be accurate enough.

wheel go brrr

softyoda commented 1 year ago

Hi, just a quick question as this thread seem related to #13080 and #12179 and also #13342 is there any integration of zoom-damping on desktop (with a mouse). I know there is dolly (with middle mouse click drag) witch seem to smoothly zoom, maybe touch-control also have smooth zoom (haven't tested).

Current behavior when zooming with a mouse wheel feel like 2000's design, with jumping making the update aggressive, this could be solved using tweening multiplied each update with a zoom-speed factor, but I don't know if this is currently implemented nor if it will be in the future ?