From 1951fc0e47cc44d4b9092607f91c5efdb882a15a Mon Sep 17 00:00:00 2001 From: David Sheldrick Date: Mon, 18 Mar 2024 16:03:44 +0000 Subject: [PATCH] Fix lag while panning + translating at the same time (#3186) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before ![Kapture 2024-03-18 at 09 42 33](https://github.com/tldraw/tldraw/assets/1242537/d27c5852-9514-4e44-8b75-d2cb2571362a) After ![Kapture 2024-03-18 at 09 41 27](https://github.com/tldraw/tldraw/assets/1242537/f5cbebfd-a45c-48d9-915b-18823f4555ff) The probelm was manifesting because our camera updates were not throttled and our render tick was on a different tick timeline to our tick manager. Fixing the latter gets rid of the lag without requiring us to throttle the camera updates. ### Change Type - [x] `sdk` — Changes the tldraw SDK - [ ] `dotcom` — Changes the tldraw.com web app - [ ] `docs` — Changes to the documentation, examples, or templates. - [ ] `vs code` — Changes to the vscode plugin - [ ] `internal` — Does not affect user-facing stuff - [x] `bugfix` — Bug fix ### Test Plan 1. Add a step-by-step description of how to test your PR here. 2. - [ ] Unit Tests - [ ] End to end tests ### Release Notes - Add a brief release note for your PR here. --------- Co-authored-by: Steve Ruiz --- packages/editor/src/lib/editor/Editor.ts | 3 +- .../src/lib/editor/managers/TickManager.ts | 33 ++--- packages/tldraw/src/test/TestEditor.ts | 25 +++- packages/tldraw/src/test/translating.test.ts | 126 ++++++++++++++++++ packages/utils/api-report.md | 2 +- packages/utils/src/lib/throttle.ts | 30 +++-- 6 files changed, 190 insertions(+), 29 deletions(-) diff --git a/packages/editor/src/lib/editor/Editor.ts b/packages/editor/src/lib/editor/Editor.ts index b22a3b4bc..5ce42d153 100644 --- a/packages/editor/src/lib/editor/Editor.ts +++ b/packages/editor/src/lib/editor/Editor.ts @@ -207,6 +207,7 @@ export class Editor extends EventEmitter { this.getContainer = getContainer ?? (() => document.body) this.textMeasure = new TextManager(this) + this._tickManager = new TickManager(this) class NewRoot extends RootState { static override initial = initialState ?? '' @@ -666,7 +667,7 @@ export class Editor extends EventEmitter { readonly disposables = new Set<() => void>() /** @internal */ - private _tickManager = new TickManager(this) + private readonly _tickManager /** * A manager for the app's snapping feature. diff --git a/packages/editor/src/lib/editor/managers/TickManager.ts b/packages/editor/src/lib/editor/managers/TickManager.ts index f1bc83363..a3de4492e 100644 --- a/packages/editor/src/lib/editor/managers/TickManager.ts +++ b/packages/editor/src/lib/editor/managers/TickManager.ts @@ -1,21 +1,31 @@ +import { throttleToNextFrame as _throttleToNextFrame } from '@tldraw/utils' import { Vec } from '../../primitives/Vec' import { Editor } from '../Editor' +const throttleToNextFrame = + typeof process !== 'undefined' && process.env.NODE_ENV === 'test' + ? // At test time we should use actual raf and not throttle, because throttle was set up to evaluate immediately during tests, which causes stack overflow + // for the tick manager since it sets up a raf loop. + function mockThrottle(cb: any) { + const frame = requestAnimationFrame(cb) + return () => cancelAnimationFrame(frame) + } + : _throttleToNextFrame + export class TickManager { constructor(public editor: Editor) { this.editor.disposables.add(this.dispose) this.start() } - raf: any + cancelRaf?: null | (() => void) isPaused = true last = 0 - t = 0 start = () => { this.isPaused = false - cancelAnimationFrame(this.raf) - this.raf = requestAnimationFrame(this.tick) + this.cancelRaf?.() + this.cancelRaf = throttleToNextFrame(this.tick) this.last = Date.now() } @@ -27,25 +37,18 @@ export class TickManager { const now = Date.now() const elapsed = now - this.last this.last = now - this.t += elapsed - this.editor.emit('frame', elapsed) - - if (this.t < 16) { - this.raf = requestAnimationFrame(this.tick) - return - } - - this.t -= 16 this.updatePointerVelocity(elapsed) + this.editor.emit('frame', elapsed) this.editor.emit('tick', elapsed) - this.raf = requestAnimationFrame(this.tick) + this.cancelRaf = throttleToNextFrame(this.tick) } // Clear the listener dispose = () => { this.isPaused = true - cancelAnimationFrame(this.raf) + + this.cancelRaf?.() } private prevPoint = new Vec() diff --git a/packages/tldraw/src/test/TestEditor.ts b/packages/tldraw/src/test/TestEditor.ts index bb8e8a2a0..edbb67700 100644 --- a/packages/tldraw/src/test/TestEditor.ts +++ b/packages/tldraw/src/test/TestEditor.ts @@ -3,6 +3,7 @@ import { BoxModel, Editor, HALF_PI, + IdOf, Mat, PageRecordType, ROTATE_CORNER_TO_SELECTION_CORNER, @@ -205,6 +206,8 @@ export class TestEditor extends Editor { y: +camera.y.toFixed(2), z: +camera.z.toFixed(2), }).toCloselyMatchObject({ x, y, z }) + + return this } expectShapeToMatch = ( @@ -218,6 +221,25 @@ export class TestEditor extends Editor { return this } + expectPageBoundsToBe = (id: IdOf, bounds: Partial) => { + const observedBounds = this.getShapePageBounds(id)! + expect(observedBounds).toCloselyMatchObject(bounds) + return this + } + + expectScreenBoundsToBe = ( + id: IdOf, + bounds: Partial + ) => { + const pageBounds = this.getShapePageBounds(id)! + const screenPoint = this.pageToScreen(pageBounds.point) + const observedBounds = pageBounds.clone() + observedBounds.x = screenPoint.x + observedBounds.y = screenPoint.y + expect(observedBounds).toCloselyMatchObject(bounds) + return this + } + /* --------------------- Inputs --------------------- */ protected getInfo = (info: string | T): T => { @@ -294,8 +316,9 @@ export class TestEditor extends Editor { You can use this helper to force the tick, which will then process all the updates. */ forceTick = (count = 1) => { + const tick = (this as any)._tickManager as { tick(): void } for (let i = 0; i < count; i++) { - this.emit('tick', 16) + tick.tick() } return this } diff --git a/packages/tldraw/src/test/translating.test.ts b/packages/tldraw/src/test/translating.test.ts index 7d038c216..e99b6541c 100644 --- a/packages/tldraw/src/test/translating.test.ts +++ b/packages/tldraw/src/test/translating.test.ts @@ -1876,3 +1876,129 @@ it('clones a single shape simply', () => { expect(editor.getEditingShape()).toBe(undefined) expect(editor.getHoveredShape()).toBe(sticky2) }) + +describe('Moving the camera while panning', () => { + it('moves things while dragging', () => { + editor.createShape({ + type: 'geo', + id: ids.box1, + x: 0, + y: 0, + props: { geo: 'rectangle', w: 100, h: 100, fill: 'solid' }, + }) + + editor + .expectShapeToMatch({ id: ids.box1, x: 0, y: 0 }) + .expectToBeIn('select.idle') + .pointerMove(40, 40) + .pointerDown() + .expectToBeIn('select.pointing_shape') + .pointerMove(50, 50) // move by 10,10 + .expectToBeIn('select.translating') + .expectShapeToMatch({ id: ids.box1, x: 10, y: 10 }) + .wheel(-10, -10) // wheel by -10,-10 + .forceTick() // needed + .expectShapeToMatch({ id: ids.box1, x: 20, y: 20 }) + .wheel(-10, -10) // wheel by -10,-10 + .forceTick() // needed + .expectShapeToMatch({ id: ids.box1, x: 30, y: 30 }) + }) + + it('FAILING EXAMPLE: preserves screen point while dragging', () => { + editor.createShape({ + type: 'geo', + id: ids.box1, + x: 0, + y: 0, + props: { geo: 'rectangle', w: 100, h: 100, fill: 'solid' }, + }) + + editor + .expectCameraToBe(0, 0, 1) + .expectShapeToMatch({ id: ids.box1, x: 0, y: 0 }) + .expectPageBoundsToBe(ids.box1, { x: 0, y: 0 }) + .expectScreenBoundsToBe(ids.box1, { x: 0, y: 0 }) + .expectToBeIn('select.idle') + .pointerMove(40, 40) + .pointerDown() + .expectToBeIn('select.pointing_shape') + .pointerMove(50, 50) // move by 10,10 + .expectToBeIn('select.translating') + + // we haven't moved the camera from origin yet, so the + // point / page / screen points should all be identical + .expectCameraToBe(0, 0, 1) + .expectShapeToMatch({ id: ids.box1, x: 10, y: 10 }) + .expectPageBoundsToBe(ids.box1, { x: 10, y: 10 }) + .expectScreenBoundsToBe(ids.box1, { x: 10, y: 10 }) + + // now we move the camera by -10,-10 + // since we're dragging, they should still all move together + .wheel(-10, -10) + + // ! This is the problem here—the screen point has changed + // ! because the camera moved but the resulting pointer move + // ! isn't processed until after the tick + .expectCameraToBe(-10, -10, 1) + .expectScreenBoundsToBe(ids.box1, { x: 0, y: 0 }) // should be 10,10 + + // nothing else has changed yet... until the tick + .expectShapeToMatch({ id: ids.box1, x: 10, y: 10 }) + .expectPageBoundsToBe(ids.box1, { x: 10, y: 10 }) + + .forceTick() // needed + + // The camera is still the same... + .expectCameraToBe(-10, -10, 1) + + // But we've processed a pointer move, which has changed the shapes + .expectShapeToMatch({ id: ids.box1, x: 20, y: 20 }) + .expectPageBoundsToBe(ids.box1, { x: 20, y: 20 }) + + // ! And this has fixed the screen point + .expectScreenBoundsToBe(ids.box1, { x: 10, y: 10 }) + }) + + it('Correctly preserves screen point while dragging', async () => { + editor.createShape({ + type: 'geo', + id: ids.box1, + x: 0, + y: 0, + props: { geo: 'rectangle', w: 100, h: 100, fill: 'solid' }, + }) + + editor + .expectCameraToBe(0, 0, 1) + .expectShapeToMatch({ id: ids.box1, x: 0, y: 0 }) + .expectPageBoundsToBe(ids.box1, { x: 0, y: 0 }) + .expectScreenBoundsToBe(ids.box1, { x: 0, y: 0 }) + .expectToBeIn('select.idle') + .pointerMove(40, 40) + .pointerDown() + .expectToBeIn('select.pointing_shape') + .pointerMove(50, 50) // move by 10,10 + .expectToBeIn('select.translating') + + // we haven't moved the camera from origin yet, so the + // point / page / screen points should all be identical + .expectCameraToBe(0, 0, 1) + .expectShapeToMatch({ id: ids.box1, x: 10, y: 10 }) + .expectPageBoundsToBe(ids.box1, { x: 10, y: 10 }) + .expectScreenBoundsToBe(ids.box1, { x: 10, y: 10 }) + + // now we move the camera by -10,-10 + // since we're dragging, they should still all move together + .wheel(-10, -10) + .forceTick() + // wait for a tick to allow the tick manager to dispatch to the translating tool + + // The camera has moved + .expectCameraToBe(-10, -10, 1) + .expectShapeToMatch({ id: ids.box1, x: 20, y: 20 }) + .expectPageBoundsToBe(ids.box1, { x: 20, y: 20 }) + + // Screen bounds / point is still the same as it was before + .expectScreenBoundsToBe(ids.box1, { x: 10, y: 10 }) + }) +}) diff --git a/packages/utils/api-report.md b/packages/utils/api-report.md index ae37fa6c1..5eb4b1373 100644 --- a/packages/utils/api-report.md +++ b/packages/utils/api-report.md @@ -315,7 +315,7 @@ export { structuredClone_2 as structuredClone } export function throttle any>(func: T, limit: number): (...args: Parameters) => ReturnType; // @internal -export function throttleToNextFrame(fn: () => void): void; +export function throttleToNextFrame(fn: () => void): () => void; // @internal (undocumented) export function validateIndexKey(key: string): asserts key is IndexKey; diff --git a/packages/utils/src/lib/throttle.ts b/packages/utils/src/lib/throttle.ts index c39eca537..323236ec4 100644 --- a/packages/utils/src/lib/throttle.ts +++ b/packages/utils/src/lib/throttle.ts @@ -74,23 +74,31 @@ export function fpsThrottle(fn: () => void) { * Calls the function on the next frame. The target frame rate is 60fps. * If the same fn is passed again before the next frame, it will still be called only once. * @param fn - the fun to call on the next frame - * @returns + * @returns a function that will cancel the call if called before the next frame * @internal */ -export function throttleToNextFrame(fn: () => void) { +export function throttleToNextFrame(fn: () => void): () => void { if (isTest()) { - return fn() + fn() + return () => { + // noop + } } - if (fpsQueue.includes(fn)) { - return + if (!fpsQueue.includes(fn)) { + fpsQueue.push(fn) + if (!started) { + started = true + // We set last to Date.now() - targetTimePerFrame - 1 so that the first run will happen immediately + last = Date.now() - targetTimePerFrame - 1 + } + tick() } - fpsQueue.push(fn) - if (!started) { - started = true - // We set last to Date.now() - targetTimePerFrame - 1 so that the first run will happen immediately - last = Date.now() - targetTimePerFrame - 1 + return () => { + const index = fpsQueue.indexOf(fn) + if (index > -1) { + fpsQueue.splice(index, 1) + } } - tick() }