Use Pointer Lock For Scrubbing #6239
Conversation
- Changed some const functions to regular functions. - Removed the redundant y dragging for scrubbing as it wasn't used. - Added some additional React refs to help keep track of what has occurred. - Reworked the old scrub event handlers with the pointer lock based event handlers.
#13892 Bundle Size — 62.49MiB (~+0.01%).bf83092(current) vs 6339ece master#13891(baseline) Warning Bundle contains 70 duplicate packages – View duplicate packages Bundle metrics
|
| Current #13892 |
Baseline #13891 |
|
|---|---|---|
 Initial JS |
45.63MiB(+0.01%) |
45.62MiB |
 Initial CSS |
0B |
0B |
 Cache Invalidation |
21.52% |
22.16% |
| Chunks |
30 |
30 |
| Assets |
33 |
33 |
| Modules |
4373 |
4373 |
| Duplicate Modules |
519 |
519 |
| Duplicate Code |
31.65%(+0.03%) |
31.64% |
| Packages |
472 |
472 |
| Duplicate Packages |
70 |
70 |
Bundle size by type 2 changes 1 regression 
1 improvement
| Current #13892 |
Baseline #13891 |
|
|---|---|---|
| JS |
62.48MiB (~+0.01%) |
62.48MiB |
| HTML |
11.05KiB (-0.33%) |
11.09KiB |
Bundle analysis report Branch refactor/use-pointer-lock-for-sc... Project dashboard
Generated by RelativeCI Documentation Report issue
| scrubbingCleanupCallbacks.current.push(() => { | ||
| window.removeEventListener('mouseup', scrubOnMouseUp) | ||
| }) | ||
| scrubbingCleanupCallbacks.current.push(() => { | ||
| window.removeEventListener('mousemove', scrubOnMouseMove) | ||
| }) | ||
| scrubbingCleanupCallbacks.current.push(() => { | ||
| document.removeEventListener('pointerlockchange', checkPointerLockChange, true) | ||
| }) |
There was a problem hiding this comment.
Is there a reason to push these callbacks to a ref of an array like this? It seems to me that in both cleanup sites (checkPointerLockChange and scrubOnMouseUp) we could just call a single function that removes these 3 event listeners regardless of the circumstances
There was a problem hiding this comment.
I referenced this in the description, the issue was that I kept ending up with circular references between the callbacks, as their dependencies need to be declared before the callbacks which use them. This means that the function which triggers the cleanup doesn't depend on the event listeners, which snips a convenient gap in the circle.
There was a problem hiding this comment.
Ohhhh.... ew.... ok, can you add that in a comment here please too
There was a problem hiding this comment.
Fixed in 8e2c64b.
That to me sounds like it should be addressed before merging this PR, as I don't believe that's an issue with the existing implementation (though I may be wrong) |
Interesting, definitely should be addressed in this, I don't remember ever seeing this but I wonder if the browser is still tracking the real position and I've coincidentally dragged around the other fields. |
I've created a new bug for this as it's a pre-existing issue and can be triggered in other ways: #6249 |
- Changed some const functions to regular functions. - Removed the redundant y dragging for scrubbing as it wasn't used. - Added some additional React refs to help keep track of what has occurred. - Reworked the old scrub event handlers with the pointer lock based event handlers.
Problem:
Utopia input labels are scrubbable, but this scrubbing suffers from a number of issues:
Fix:
The previous scrubbing implementation has been completely replaced in favour of an implementation which uses pointer locking to achieve the same effect. This has resulted in some slightly more complicated handling around tracking the accumulated movement.
Handling the "cleanup" of event handlers is also a little more complicated than before because it repeatedly tended towards callbacks that were circularly referenced.
Commit Details:
Manual Tests:
I hereby swear that:
Fixes #6196