From cd02d03d063b50d93d840aa8194aeced43a6a9c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mitja=20Bezen=C5=A1ek?= Date: Thu, 21 Mar 2024 11:05:44 +0100 Subject: [PATCH] Revert perf changes (#3217) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Step 1 of the master plan πŸ˜‚ ![CleanShot 2024-03-19 at 16 05 08](https://github.com/tldraw/tldraw/assets/2523721/7d2afed9-7b69-4fdb-8b9f-54a48c61258f) This: - Reverts #3186 - Reverts #3160 (there were some conflicting changes so it's not a straight revert) - Reverts most of #2977 ### Change Type - [ ] `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 - [x] `internal` β€” Does not affect user-facing stuff - [ ] `bugfix` β€” Bug fix - [ ] `feature` β€” New feature - [ ] `improvement` β€” Improving existing features - [x] `chore` β€” Updating dependencies, other boring stuff - [ ] `galaxy brain` β€” Architectural changes - [ ] `tests` β€” Changes to any test code - [ ] `tools` β€” Changes to infrastructure, CI, internal scripts, debugging tools, etc. - [ ] `dunno` β€” I don't know --- apps/examples/package.json | 4 +- packages/editor/src/lib/components/Shape.tsx | 122 ++++++++++-------- .../default-components/DefaultCanvas.tsx | 32 +++-- packages/state/api-report.md | 3 - packages/state/package.json | 3 - .../state/src/lib/core/EffectScheduler.ts | 5 +- packages/state/src/lib/react/index.ts | 1 - .../state/src/lib/react/useLayoutReaction.ts | 10 -- .../state/src/lib/react/useStateTracking.ts | 58 ++++++++- .../src/lib/react/useTrackedScheduler.ts | 61 --------- packages/state/src/lib/react/useValue.ts | 15 +-- packages/state/tsconfig.json | 1 - .../src/lib/shapes/draw/toolStates/Drawing.ts | 62 +++------ .../tools/SelectTool/childStates/Brushing.ts | 9 +- .../tools/SelectTool/childStates/Cropping.ts | 12 +- .../SelectTool/childStates/DraggingHandle.tsx | 12 +- .../tools/SelectTool/childStates/Resizing.ts | 7 +- .../tools/SelectTool/childStates/Rotating.ts | 10 +- .../childStates/ScribbleBrushing.ts | 13 +- .../SelectTool/childStates/Translating.ts | 9 +- packages/tldraw/src/test/TestEditor.ts | 14 +- packages/tldraw/src/test/drawing.test.ts | 2 - packages/tldraw/src/test/translating.test.ts | 73 +---------- yarn.lock | 1 - 24 files changed, 179 insertions(+), 360 deletions(-) delete mode 100644 packages/state/src/lib/react/useLayoutReaction.ts delete mode 100644 packages/state/src/lib/react/useTrackedScheduler.ts diff --git a/apps/examples/package.json b/apps/examples/package.json index 578995d19..4fa778353 100644 --- a/apps/examples/package.json +++ b/apps/examples/package.json @@ -29,8 +29,8 @@ "dev": "vite --host", "build": "vite build", "lint": "yarn run -T tsx ../../scripts/lint.ts", - "e2e": "NODE_ENV=test && playwright test -c ./e2e/playwright.config.ts", - "e2e-ui": "NODE_ENV=test && playwright test --ui -c ./e2e/playwright.config.ts" + "e2e": "playwright test -c ./e2e/playwright.config.ts", + "e2e-ui": "playwright test --ui -c ./e2e/playwright.config.ts" }, "dependencies": { "@playwright/test": "^1.38.1", diff --git a/packages/editor/src/lib/components/Shape.tsx b/packages/editor/src/lib/components/Shape.tsx index 0823f49a4..3762290b3 100644 --- a/packages/editor/src/lib/components/Shape.tsx +++ b/packages/editor/src/lib/components/Shape.tsx @@ -1,7 +1,7 @@ -import { useLayoutReaction, useStateTracking } from '@tldraw/state' +import { useQuickReactor, useStateTracking } from '@tldraw/state' import { IdOf } from '@tldraw/store' import { TLShape, TLShapeId } from '@tldraw/tlschema' -import { memo, useCallback, useLayoutEffect, useRef } from 'react' +import { memo, useCallback, useRef } from 'react' import { ShapeUtil } from '../editor/shapes/ShapeUtil' import { useEditor } from '../hooks/useEditor' import { useEditorComponents } from '../hooks/useEditorComponents' @@ -54,60 +54,68 @@ export const Shape = memo(function Shape({ height: 0, }) - useLayoutReaction('set shape stuff', () => { - const shape = editor.getShape(id) - if (!shape) return // probably the shape was just deleted + useQuickReactor( + 'set shape stuff', + () => { + const shape = editor.getShape(id) + if (!shape) return // probably the shape was just deleted - const prev = memoizedStuffRef.current + const prev = memoizedStuffRef.current - // Clip path - const clipPath = editor.getShapeClipPath(id) ?? 'none' - if (clipPath !== prev.clipPath) { - setStyleProperty(containerRef.current, 'clip-path', clipPath) - setStyleProperty(bgContainerRef.current, 'clip-path', clipPath) - prev.clipPath = clipPath - } + // Clip path + const clipPath = editor.getShapeClipPath(id) ?? 'none' + if (clipPath !== prev.clipPath) { + setStyleProperty(containerRef.current, 'clip-path', clipPath) + setStyleProperty(bgContainerRef.current, 'clip-path', clipPath) + prev.clipPath = clipPath + } - // Page transform - const transform = Mat.toCssString(editor.getShapePageTransform(id)) - if (transform !== prev.transform) { - setStyleProperty(containerRef.current, 'transform', transform) - setStyleProperty(bgContainerRef.current, 'transform', transform) - prev.transform = transform - } + // Page transform + const transform = Mat.toCssString(editor.getShapePageTransform(id)) + if (transform !== prev.transform) { + setStyleProperty(containerRef.current, 'transform', transform) + setStyleProperty(bgContainerRef.current, 'transform', transform) + prev.transform = transform + } - // Width / Height - // We round the shape width and height up to the nearest multiple of dprMultiple - // to avoid the browser making miscalculations when applying the transform. - const bounds = editor.getShapeGeometry(shape).bounds - const widthRemainder = bounds.w % dprMultiple - const heightRemainder = bounds.h % dprMultiple - const width = widthRemainder === 0 ? bounds.w : bounds.w + (dprMultiple - widthRemainder) - const height = heightRemainder === 0 ? bounds.h : bounds.h + (dprMultiple - heightRemainder) + // Width / Height + // We round the shape width and height up to the nearest multiple of dprMultiple + // to avoid the browser making miscalculations when applying the transform. + const bounds = editor.getShapeGeometry(shape).bounds + const widthRemainder = bounds.w % dprMultiple + const heightRemainder = bounds.h % dprMultiple + const width = widthRemainder === 0 ? bounds.w : bounds.w + (dprMultiple - widthRemainder) + const height = heightRemainder === 0 ? bounds.h : bounds.h + (dprMultiple - heightRemainder) - if (width !== prev.width || height !== prev.height) { - setStyleProperty(containerRef.current, 'width', Math.max(width, dprMultiple) + 'px') - setStyleProperty(containerRef.current, 'height', Math.max(height, dprMultiple) + 'px') - setStyleProperty(bgContainerRef.current, 'width', Math.max(width, dprMultiple) + 'px') - setStyleProperty(bgContainerRef.current, 'height', Math.max(height, dprMultiple) + 'px') - prev.width = width - prev.height = height - } - }) + if (width !== prev.width || height !== prev.height) { + setStyleProperty(containerRef.current, 'width', Math.max(width, dprMultiple) + 'px') + setStyleProperty(containerRef.current, 'height', Math.max(height, dprMultiple) + 'px') + setStyleProperty(bgContainerRef.current, 'width', Math.max(width, dprMultiple) + 'px') + setStyleProperty(bgContainerRef.current, 'height', Math.max(height, dprMultiple) + 'px') + prev.width = width + prev.height = height + } + }, + [editor] + ) // This stuff changes pretty infrequently, so we can change them together - useLayoutEffect(() => { - const container = containerRef.current - const bgContainer = bgContainerRef.current + useQuickReactor( + 'set opacity and z-index', + () => { + const container = containerRef.current + const bgContainer = bgContainerRef.current - // Opacity - setStyleProperty(container, 'opacity', opacity) - setStyleProperty(bgContainer, 'opacity', opacity) + // Opacity + setStyleProperty(container, 'opacity', opacity) + setStyleProperty(bgContainer, 'opacity', opacity) - // Z-Index - setStyleProperty(container, 'z-index', index) - setStyleProperty(bgContainer, 'z-index', backgroundIndex) - }, [opacity, index, backgroundIndex]) + // Z-Index + setStyleProperty(container, 'z-index', index) + setStyleProperty(bgContainer, 'z-index', backgroundIndex) + }, + [opacity, index, backgroundIndex] + ) const annotateError = useCallback( (error: any) => editor.annotateError(error, { origin: 'shape', willCrashApp: false }), @@ -169,14 +177,18 @@ const CulledShape = function CulledShape({ shapeId }: { shape const editor = useEditor() const culledRef = useRef(null) - useLayoutReaction('set shape stuff', () => { - const bounds = editor.getShapeGeometry(shapeId).bounds - setStyleProperty( - culledRef.current, - 'transform', - `translate(${toDomPrecision(bounds.minX)}px, ${toDomPrecision(bounds.minY)}px)` - ) - }) + useQuickReactor( + 'set shape stuff', + () => { + const bounds = editor.getShapeGeometry(shapeId).bounds + setStyleProperty( + culledRef.current, + 'transform', + `translate(${toDomPrecision(bounds.minX)}px, ${toDomPrecision(bounds.minY)}px)` + ) + }, + [editor] + ) return
} diff --git a/packages/editor/src/lib/components/default-components/DefaultCanvas.tsx b/packages/editor/src/lib/components/default-components/DefaultCanvas.tsx index f0990e938..0fe9d9c79 100644 --- a/packages/editor/src/lib/components/default-components/DefaultCanvas.tsx +++ b/packages/editor/src/lib/components/default-components/DefaultCanvas.tsx @@ -1,4 +1,4 @@ -import { react, useLayoutReaction, useValue } from '@tldraw/state' +import { react, useQuickReactor, useValue } from '@tldraw/state' import { TLHandle, TLShapeId } from '@tldraw/tlschema' import { dedupe, modulate, objectMapValues } from '@tldraw/utils' import classNames from 'classnames' @@ -43,21 +43,25 @@ export function DefaultCanvas({ className }: TLCanvasComponentProps) { useGestureEvents(rCanvas) useFixSafariDoubleTapZoomPencilEvents(rCanvas) - useLayoutReaction('position layers', () => { - const { x, y, z } = editor.getCamera() + useQuickReactor( + 'position layers', + () => { + const { x, y, z } = editor.getCamera() - // Because the html container has a width/height of 1px, we - // need to create a small offset when zoomed to ensure that - // the html container and svg container are lined up exactly. - const offset = - z >= 1 ? modulate(z, [1, 8], [0.125, 0.5], true) : modulate(z, [0.1, 1], [-2, 0.125], true) + // Because the html container has a width/height of 1px, we + // need to create a small offset when zoomed to ensure that + // the html container and svg container are lined up exactly. + const offset = + z >= 1 ? modulate(z, [1, 8], [0.125, 0.5], true) : modulate(z, [0.1, 1], [-2, 0.125], true) - const transform = `scale(${toDomPrecision(z)}) translate(${toDomPrecision( - x + offset - )}px,${toDomPrecision(y + offset)}px)` - setStyleProperty(rHtmlLayer.current, 'transform', transform) - setStyleProperty(rHtmlLayer2.current, 'transform', transform) - }) + const transform = `scale(${toDomPrecision(z)}) translate(${toDomPrecision( + x + offset + )}px,${toDomPrecision(y + offset)}px)` + setStyleProperty(rHtmlLayer.current, 'transform', transform) + setStyleProperty(rHtmlLayer2.current, 'transform', transform) + }, + [editor] + ) const events = useCanvasEvents() diff --git a/packages/state/api-report.md b/packages/state/api-report.md index 1ad2b1eed..f58a65e49 100644 --- a/packages/state/api-report.md +++ b/packages/state/api-report.md @@ -128,9 +128,6 @@ export function useComputed(name: string, compute: () => Value, deps: any // @public (undocumented) export function useComputed(name: string, compute: () => Value, opts: ComputedOptions, deps: any[]): Computed; -// @internal (undocumented) -export function useLayoutReaction(name: string, effect: () => void): void; - // @public (undocumented) export function useQuickReactor(name: string, reactFn: () => void, deps?: any[]): void; diff --git a/packages/state/package.json b/packages/state/package.json index c6037198c..003a5ddd2 100644 --- a/packages/state/package.json +++ b/packages/state/package.json @@ -52,9 +52,6 @@ "node_modules/(?!(nanoid)/)" ] }, - "dependencies": { - "@tldraw/utils": "workspace:*" - }, "devDependencies": { "@types/lodash": "^4.14.188", "@types/react": "^18.2.47", diff --git a/packages/state/src/lib/core/EffectScheduler.ts b/packages/state/src/lib/core/EffectScheduler.ts index 1f890cfdd..bf99d5d80 100644 --- a/packages/state/src/lib/core/EffectScheduler.ts +++ b/packages/state/src/lib/core/EffectScheduler.ts @@ -51,7 +51,6 @@ class __EffectScheduler__ { lastTraversedEpoch = GLOBAL_START_EPOCH private lastReactedEpoch = GLOBAL_START_EPOCH - private hasPendingEffect = true private _scheduleCount = 0 /** @@ -95,7 +94,6 @@ class __EffectScheduler__ { /** @internal */ scheduleEffect() { this._scheduleCount++ - this.hasPendingEffect = true if (this._scheduleEffect) { // if the effect should be deferred (e.g. until a react render), do so this._scheduleEffect(this.maybeExecute) @@ -108,7 +106,7 @@ class __EffectScheduler__ { /** @internal */ readonly maybeExecute = () => { // bail out if we have been detached before this runs - if (!this._isActivelyListening || !this.hasPendingEffect) return + if (!this._isActivelyListening) return this.execute() } @@ -144,7 +142,6 @@ class __EffectScheduler__ { try { startCapturingParents(this) const result = this.runEffect(this.lastReactedEpoch) - this.hasPendingEffect = false this.lastReactedEpoch = getGlobalEpoch() return result } finally { diff --git a/packages/state/src/lib/react/index.ts b/packages/state/src/lib/react/index.ts index 8b8457e4a..c56b51d82 100644 --- a/packages/state/src/lib/react/index.ts +++ b/packages/state/src/lib/react/index.ts @@ -1,7 +1,6 @@ export { track } from './track' export { useAtom } from './useAtom' export { useComputed } from './useComputed' -export { useLayoutReaction } from './useLayoutReaction' export { useQuickReactor } from './useQuickReactor' export { useReactor } from './useReactor' export { useStateTracking } from './useStateTracking' diff --git a/packages/state/src/lib/react/useLayoutReaction.ts b/packages/state/src/lib/react/useLayoutReaction.ts deleted file mode 100644 index 323e77db6..000000000 --- a/packages/state/src/lib/react/useLayoutReaction.ts +++ /dev/null @@ -1,10 +0,0 @@ -import React from 'react' -import { useTrackedScheduler } from './useTrackedScheduler' - -/** @internal */ -export function useLayoutReaction(name: string, effect: () => void): void { - const scheduler = useTrackedScheduler(name, effect) - - // eslint-disable-next-line react-hooks/exhaustive-deps - React.useLayoutEffect(scheduler.maybeExecute) -} diff --git a/packages/state/src/lib/react/useStateTracking.ts b/packages/state/src/lib/react/useStateTracking.ts index 2153a9dc1..d72e12e60 100644 --- a/packages/state/src/lib/react/useStateTracking.ts +++ b/packages/state/src/lib/react/useStateTracking.ts @@ -1,6 +1,60 @@ -import { useTrackedScheduler } from './useTrackedScheduler' +import React from 'react' +import { EffectScheduler } from '../core' /** @internal */ export function useStateTracking(name: string, render: () => T): T { - return useTrackedScheduler(name, render).execute() + // This hook creates an effect scheduler that will trigger re-renders when its reactive dependencies change, but it + // defers the actual execution of the effect to the consumer of this hook. + + // We need the exec fn to always be up-to-date when calling scheduler.execute() but it'd be wasteful to + // instantiate a new EffectScheduler on every render, so we use an immediately-updated ref + // to wrap it + const renderRef = React.useRef(render) + renderRef.current = render + + const [scheduler, subscribe, getSnapshot] = React.useMemo(() => { + let scheduleUpdate = null as null | (() => void) + // useSyncExternalStore requires a subscribe function that returns an unsubscribe function + const subscribe = (cb: () => void) => { + scheduleUpdate = cb + return () => { + scheduleUpdate = null + } + } + + const scheduler = new EffectScheduler( + `useStateTracking(${name})`, + // this is what `scheduler.execute()` will call + () => renderRef.current?.(), + // this is what will be invoked when @tldraw/state detects a change in an upstream reactive value + { + scheduleEffect() { + scheduleUpdate?.() + }, + } + ) + + // we use an incrementing number based on when this + const getSnapshot = () => scheduler.scheduleCount + + return [scheduler, subscribe, getSnapshot] + }, [name]) + + React.useSyncExternalStore(subscribe, getSnapshot, getSnapshot) + + // reactive dependencies are captured when `scheduler.execute()` is called + // and then to make it reactive we wait for a `useEffect` to 'attach' + // this allows us to avoid rendering outside of React's render phase + // and avoid 'zombie' components that try to render with bad/deleted data before + // react has a chance to umount them. + React.useEffect(() => { + scheduler.attach() + // do not execute, we only do that in render + scheduler.maybeScheduleEffect() + return () => { + scheduler.detach() + } + }, [scheduler]) + + return scheduler.execute() } diff --git a/packages/state/src/lib/react/useTrackedScheduler.ts b/packages/state/src/lib/react/useTrackedScheduler.ts deleted file mode 100644 index d0f7de761..000000000 --- a/packages/state/src/lib/react/useTrackedScheduler.ts +++ /dev/null @@ -1,61 +0,0 @@ -import { fpsThrottle } from '@tldraw/utils' -import React from 'react' -import { EffectScheduler } from '../core' - -/** @internal */ -export function useTrackedScheduler(name: string, exec: () => T): EffectScheduler { - // This hook creates an effect scheduler that will trigger re-renders when its reactive dependencies change, but it - // defers the actual execution of the effect to the consumer of this hook. - - // We need the exec fn to always be up-to-date when calling scheduler.execute() but it'd be wasteful to - // instantiate a new EffectScheduler on every render, so we use an immediately-updated ref - // to wrap it - const execRef = React.useRef(exec) - execRef.current = exec - - const [scheduler, subscribe, getSnapshot] = React.useMemo(() => { - let scheduleUpdate = null as null | (() => void) - // useSyncExternalStore requires a subscribe function that returns an unsubscribe function - const subscribe = (cb: () => void) => { - scheduleUpdate = cb - return () => { - scheduleUpdate = null - } - } - - const scheduler = new EffectScheduler( - `useStateTracking(${name})`, - // this is what `scheduler.execute()` will call - () => execRef.current?.(), - // this is what will be invoked when @tldraw/state detects a change in an upstream reactive value - { - scheduleEffect: fpsThrottle(() => { - scheduleUpdate?.() - }), - } - ) - - // we use an incrementing number based on when this - const getSnapshot = () => scheduler.scheduleCount - - return [scheduler, subscribe, getSnapshot] - }, [name]) - - React.useSyncExternalStore(subscribe, getSnapshot, getSnapshot) - - // reactive dependencies are captured when `scheduler.execute()` is called - // and then to make it reactive we wait for a `useEffect` to 'attach' - // this allows us to avoid rendering outside of React's render phase - // and avoid 'zombie' components that try to render with bad/deleted data before - // react has a chance to umount them. - React.useEffect(() => { - scheduler.attach() - // do not execute, we only do that in render - scheduler.maybeScheduleEffect() - return () => { - scheduler.detach() - } - }, [scheduler]) - - return scheduler -} diff --git a/packages/state/src/lib/react/useValue.ts b/packages/state/src/lib/react/useValue.ts index 4e8d7fa1f..8d4384a5b 100644 --- a/packages/state/src/lib/react/useValue.ts +++ b/packages/state/src/lib/react/useValue.ts @@ -1,5 +1,4 @@ /* eslint-disable prefer-rest-params */ -import { throttleToNextFrame } from '@tldraw/utils' import { useMemo, useRef, useSyncExternalStore } from 'react' import { Signal, computed, react } from '../core' @@ -82,16 +81,10 @@ export function useValue() { const { subscribe, getSnapshot } = useMemo(() => { return { subscribe: (listen: () => void) => { - return react( - `useValue(${name})`, - () => { - $val.get() - listen() - }, - { - scheduleEffect: throttleToNextFrame, - } - ) + return react(`useValue(${name})`, () => { + $val.get() + listen() + }) }, getSnapshot: () => $val.get(), } diff --git a/packages/state/tsconfig.json b/packages/state/tsconfig.json index 3e0ffe912..662a89b2d 100644 --- a/packages/state/tsconfig.json +++ b/packages/state/tsconfig.json @@ -2,7 +2,6 @@ "extends": "../../config/tsconfig.base.json", "include": ["src"], "exclude": ["node_modules", "dist", ".tsbuild*"], - "references": [{ "path": "../utils" }], "compilerOptions": { "outDir": "./.tsbuild", "rootDir": "src" diff --git a/packages/tldraw/src/lib/shapes/draw/toolStates/Drawing.ts b/packages/tldraw/src/lib/shapes/draw/toolStates/Drawing.ts index 8269718f3..499c19713 100644 --- a/packages/tldraw/src/lib/shapes/draw/toolStates/Drawing.ts +++ b/packages/tldraw/src/lib/shapes/draw/toolStates/Drawing.ts @@ -51,18 +51,11 @@ export class Drawing extends StateNode { markId = null as null | string - // Used to track whether we have changes that have not yet been pushed to the Editor. - isDirty = false - // The changes that have not yet been pushed to the Editor. - shapePartial: TLShapePartial | null = null - override onEnter = (info: TLPointerEventInfo) => { this.markId = null this.info = info this.canDraw = !this.editor.getIsMenuOpen() this.lastRecordedPoint = this.editor.inputs.currentPagePoint.clone() - this.shapePartial = null - this.isDirty = false if (this.canDraw) { this.startShape() } @@ -106,18 +99,10 @@ export class Drawing extends StateNode { this.mergeNextPoint = false } - this.processUpdates() + this.updateShapes() } } - override onTick = () => { - if (!this.isDirty) return - this.isDirty = false - if (!this.shapePartial) return - - this.editor.updateShapes([this.shapePartial], { squashing: true }) - } - override onKeyDown: TLEventHandlers['onKeyDown'] = (info) => { if (info.key === 'Shift') { switch (this.segmentMode) { @@ -132,7 +117,7 @@ export class Drawing extends StateNode { } } } - this.processUpdates() + this.updateShapes() } override onKeyUp: TLEventHandlers['onKeyUp'] = (info) => { @@ -154,7 +139,7 @@ export class Drawing extends StateNode { } } - this.processUpdates() + this.updateShapes() } override onExit? = () => { @@ -296,12 +281,7 @@ export class Drawing extends StateNode { this.initialShape = this.editor.getShape(id) } - /** - * This function is called to process user actions like moving the mouse or pressing a key. - * The updates are not directly propagated to the Editor. Instead they are stored in the `shapePartial` - * and only sent to the Editor on the next tick. - */ - private processUpdates() { + private updateShapes() { const { inputs } = this.editor const { initialShape } = this @@ -316,16 +296,12 @@ export class Drawing extends StateNode { if (!shape) return - // We default to the partial, as it might have some segments / points that the editor - // does not know about yet. - const segments = this.shapePartial?.props?.segments || shape.props.segments + const { segments } = shape.props const { x, y, z } = this.editor.getPointInShapeSpace(shape, inputs.currentPagePoint).toFixed() const newPoint = { x, y, z: this.isPen ? +(z! * 1.25).toFixed(2) : 0.5 } - this.isDirty = true - switch (this.segmentMode) { case 'starting_straight': { const { pagePointWhereNextSegmentChanged } = this @@ -394,7 +370,9 @@ export class Drawing extends StateNode { ) } - this.shapePartial = shapePartial + this.editor.updateShapes([shapePartial], { + squashing: true, + }) } break } @@ -452,7 +430,7 @@ export class Drawing extends StateNode { ) } - this.shapePartial = shapePartial + this.editor.updateShapes([shapePartial], { squashing: true }) } break @@ -594,7 +572,7 @@ export class Drawing extends StateNode { ) } - this.shapePartial = shapePartial + this.editor.updateShapes([shapePartial], { squashing: true }) break } @@ -639,19 +617,13 @@ export class Drawing extends StateNode { ) } + this.editor.updateShapes([shapePartial], { squashing: true }) + + // Set a maximum length for the lines array; after 200 points, complete the line. if (newPoints.length > 500) { - // It's easier to just apply this change directly, so we will mark that the shape is no longer dirty. - this.isDirty = false - // Also clear the changes as they were flushed. - // The next pointerMove will establish a new partial from the new shape created below. - this.shapePartial = null + this.editor.updateShapes([{ id, type: this.shapeType, props: { isComplete: true } }]) - if (shapePartial?.props) { - shapePartial.props.isComplete = true - this.editor.updateShapes([shapePartial]) - } - - const { currentPagePoint } = inputs + const { currentPagePoint } = this.editor.inputs const newShapeId = createShapeId() @@ -675,10 +647,8 @@ export class Drawing extends StateNode { this.initialShape = structuredClone(this.editor.getShape(newShapeId)!) this.mergeNextPoint = false - this.lastRecordedPoint = inputs.currentPagePoint.clone() + this.lastRecordedPoint = this.editor.inputs.currentPagePoint.clone() this.currentLineLength = 0 - } else { - this.shapePartial = shapePartial } break diff --git a/packages/tldraw/src/lib/tools/SelectTool/childStates/Brushing.ts b/packages/tldraw/src/lib/tools/SelectTool/childStates/Brushing.ts index 5cc228b00..b150b7aab 100644 --- a/packages/tldraw/src/lib/tools/SelectTool/childStates/Brushing.ts +++ b/packages/tldraw/src/lib/tools/SelectTool/childStates/Brushing.ts @@ -27,7 +27,6 @@ export class Brushing extends StateNode { brush = new Box() initialSelectedShapeIds: TLShapeId[] = [] excludedShapeIds = new Set() - isDirty = false isWrapMode = false // The shape that the brush started on @@ -55,7 +54,6 @@ export class Brushing extends StateNode { ) this.info = info - this.isDirty = false this.initialSelectedShapeIds = this.editor.getSelectedShapeIds().slice() this.initialStartShape = this.editor.getShapesAtPoint(currentPagePoint)[0] this.hitTestShapes() @@ -68,14 +66,10 @@ export class Brushing extends StateNode { override onTick = () => { moveCameraWhenCloseToEdge(this.editor) - if (this.isDirty) { - this.isDirty = false - this.hitTestShapes() - } } override onPointerMove = () => { - this.isDirty = true + this.hitTestShapes() } override onPointerUp: TLEventHandlers['onPointerUp'] = () => { @@ -105,7 +99,6 @@ export class Brushing extends StateNode { private complete() { this.hitTestShapes() - this.isDirty = false this.parent.transition('idle') } diff --git a/packages/tldraw/src/lib/tools/SelectTool/childStates/Cropping.ts b/packages/tldraw/src/lib/tools/SelectTool/childStates/Cropping.ts index aa2bcbc94..0bd0abc49 100644 --- a/packages/tldraw/src/lib/tools/SelectTool/childStates/Cropping.ts +++ b/packages/tldraw/src/lib/tools/SelectTool/childStates/Cropping.ts @@ -26,7 +26,6 @@ export class Cropping extends StateNode { } markId = '' - isDirty = false private snapshot = {} as any as Snapshot @@ -41,19 +40,11 @@ export class Cropping extends StateNode { this.markId = 'cropping' this.editor.mark(this.markId) this.snapshot = this.createSnapshot() - this.isDirty = false this.updateShapes() } - override onTick = () => { - if (this.isDirty) { - this.isDirty = false - this.updateShapes() - } - } - override onPointerMove: TLEventHandlers['onPointerMove'] = () => { - this.isDirty = true + this.updateShapes() } override onPointerUp: TLEventHandlers['onPointerUp'] = () => { @@ -215,7 +206,6 @@ export class Cropping extends StateNode { private complete() { this.updateShapes() - this.isDirty = false if (this.info.onInteractionEnd) { this.editor.setCurrentTool(this.info.onInteractionEnd, this.info) } else { diff --git a/packages/tldraw/src/lib/tools/SelectTool/childStates/DraggingHandle.tsx b/packages/tldraw/src/lib/tools/SelectTool/childStates/DraggingHandle.tsx index e4fc689f0..90c678dc9 100644 --- a/packages/tldraw/src/lib/tools/SelectTool/childStates/DraggingHandle.tsx +++ b/packages/tldraw/src/lib/tools/SelectTool/childStates/DraggingHandle.tsx @@ -39,7 +39,6 @@ export class DraggingHandle extends StateNode { isPrecise = false isPreciseId = null as TLShapeId | null pointingId = null as TLShapeId | null - isDirty = false override onEnter: TLEnterEventHandler = ( info: TLPointerEventInfo & { @@ -51,7 +50,6 @@ export class DraggingHandle extends StateNode { ) => { const { shape, isCreating, handle } = info this.info = info - this.isDirty = false this.parent.setCurrentToolIdMask(info.onInteractionEnd) this.shapeId = shape.id this.markId = isCreating ? `creating:${shape.id}` : 'dragging handle' @@ -167,15 +165,8 @@ export class DraggingHandle extends StateNode { } } - override onTick = () => { - if (this.isDirty) { - this.isDirty = false - this.update() - } - } - override onPointerMove: TLEventHandlers['onPointerMove'] = () => { - this.isDirty = true + this.update() } override onKeyDown: TLKeyboardEvent | undefined = () => { @@ -192,7 +183,6 @@ export class DraggingHandle extends StateNode { override onComplete: TLEventHandlers['onComplete'] = () => { this.update() - this.isDirty = false this.complete() } diff --git a/packages/tldraw/src/lib/tools/SelectTool/childStates/Resizing.ts b/packages/tldraw/src/lib/tools/SelectTool/childStates/Resizing.ts index 69a921c81..b2ea81950 100644 --- a/packages/tldraw/src/lib/tools/SelectTool/childStates/Resizing.ts +++ b/packages/tldraw/src/lib/tools/SelectTool/childStates/Resizing.ts @@ -75,15 +75,10 @@ export class Resizing extends StateNode { override onTick = () => { moveCameraWhenCloseToEdge(this.editor) - if (!this.isDirty) return - this.isDirty = false - this.updateShapes() } - isDirty = false - override onPointerMove: TLEventHandlers['onPointerMove'] = () => { - this.isDirty = true + this.updateShapes() } override onKeyDown: TLEventHandlers['onKeyDown'] = () => { diff --git a/packages/tldraw/src/lib/tools/SelectTool/childStates/Rotating.ts b/packages/tldraw/src/lib/tools/SelectTool/childStates/Rotating.ts index 4527301e8..e14b836e1 100644 --- a/packages/tldraw/src/lib/tools/SelectTool/childStates/Rotating.ts +++ b/packages/tldraw/src/lib/tools/SelectTool/childStates/Rotating.ts @@ -22,7 +22,6 @@ export class Rotating extends StateNode { info = {} as Extract & { onInteractionEnd?: string } markId = '' - isDirty = false override onEnter = ( info: TLPointerEventInfo & { target: 'selection'; onInteractionEnd?: string } @@ -66,15 +65,8 @@ export class Rotating extends StateNode { this.snapshot = {} as TLRotationSnapshot } - override onTick = () => { - if (this.isDirty) { - this.isDirty = false - this.update() - } - } - override onPointerMove = () => { - this.isDirty = true + this.update() } override onKeyDown = () => { diff --git a/packages/tldraw/src/lib/tools/SelectTool/childStates/ScribbleBrushing.ts b/packages/tldraw/src/lib/tools/SelectTool/childStates/ScribbleBrushing.ts index 75e83c1ca..4d869e018 100644 --- a/packages/tldraw/src/lib/tools/SelectTool/childStates/ScribbleBrushing.ts +++ b/packages/tldraw/src/lib/tools/SelectTool/childStates/ScribbleBrushing.ts @@ -24,8 +24,6 @@ export class ScribbleBrushing extends StateNode { initialSelectedShapeIds = new Set() newlySelectedShapeIds = new Set() - isDirty = false - override onEnter = () => { this.initialSelectedShapeIds = new Set( this.editor.inputs.shiftKey ? this.editor.getSelectedShapeIds() : [] @@ -33,7 +31,6 @@ export class ScribbleBrushing extends StateNode { this.newlySelectedShapeIds = new Set() this.size = 0 this.hits.clear() - this.isDirty = false const scribbleItem = this.editor.scribbles.addScribble({ color: 'selection-stroke', @@ -54,15 +51,8 @@ export class ScribbleBrushing extends StateNode { this.editor.scribbles.stop(this.scribbleId) } - override onTick = () => { - if (this.isDirty) { - this.isDirty = false - this.updateScribbleSelection(true) - } - } - override onPointerMove = () => { - this.isDirty = true + this.updateScribbleSelection(true) } override onPointerUp = () => { @@ -168,7 +158,6 @@ export class ScribbleBrushing extends StateNode { private complete() { this.updateScribbleSelection(true) - this.isDirty = false this.parent.transition('idle') } diff --git a/packages/tldraw/src/lib/tools/SelectTool/childStates/Translating.ts b/packages/tldraw/src/lib/tools/SelectTool/childStates/Translating.ts index 2b4c75d5b..9e5c89ced 100644 --- a/packages/tldraw/src/lib/tools/SelectTool/childStates/Translating.ts +++ b/packages/tldraw/src/lib/tools/SelectTool/childStates/Translating.ts @@ -35,7 +35,6 @@ export class Translating extends StateNode { isCloning = false isCreating = false - isDirty = false onCreate: (shape: TLShape | null) => void = () => void null dragAndDropManager = new DragAndDropManager(this.editor) @@ -51,7 +50,6 @@ export class Translating extends StateNode { const { isCreating = false, onCreate = () => void null } = info this.info = info - this.isDirty = false this.parent.setCurrentToolIdMask(info.onInteractionEnd) this.isCreating = isCreating this.onCreate = onCreate @@ -100,14 +98,10 @@ export class Translating extends StateNode { this.updateParentTransforms ) moveCameraWhenCloseToEdge(this.editor) - if (this.isDirty) { - this.isDirty = false - this.updateShapes() - } } override onPointerMove = () => { - this.isDirty = true + this.updateShapes() } override onKeyDown = () => { @@ -172,7 +166,6 @@ export class Translating extends StateNode { protected complete() { this.updateShapes() - this.isDirty = false this.dragAndDropManager.dropShapes(this.snapshot.movingShapes) this.handleEnd() diff --git a/packages/tldraw/src/test/TestEditor.ts b/packages/tldraw/src/test/TestEditor.ts index edbb67700..b1d204c6d 100644 --- a/packages/tldraw/src/test/TestEditor.ts +++ b/packages/tldraw/src/test/TestEditor.ts @@ -311,18 +311,6 @@ export class TestEditor extends Editor { /* ------------------ Input Events ------------------ */ - /** - Some of our updates are not synchronous any longer. For example, drawing happens on tick instead of on pointer move. - 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++) { - tick.tick() - } - return this - } - pointerMove = ( x = this.inputs.currentScreenPoint.x, y = this.inputs.currentScreenPoint.y, @@ -332,7 +320,7 @@ export class TestEditor extends Editor { this.dispatch({ ...this.getPointerEventInfo(x, y, options, modifiers), name: 'pointer_move', - }).forceTick() + }) return this } diff --git a/packages/tldraw/src/test/drawing.test.ts b/packages/tldraw/src/test/drawing.test.ts index 76bfd06cf..eb96dcecb 100644 --- a/packages/tldraw/src/test/drawing.test.ts +++ b/packages/tldraw/src/test/drawing.test.ts @@ -211,7 +211,6 @@ for (const toolType of ['draw', 'highlight'] as const) { expect(point1.x).toBe(1) editor.keyDown('Meta') - editor.forceTick() const shape2 = editor.getCurrentPageShapes()[0] as DrawableShape const segment2 = last(shape2.props.segments)! const point2 = last(segment2.points)! @@ -237,7 +236,6 @@ for (const toolType of ['draw', 'highlight'] as const) { expect(point1.x).toBe(1) editor.keyDown('Meta') - editor.forceTick() const shape2 = editor.getCurrentPageShapes()[0] as DrawableShape const segment2 = last(shape2.props.segments)! const point2 = last(segment2.points)! diff --git a/packages/tldraw/src/test/translating.test.ts b/packages/tldraw/src/test/translating.test.ts index e99b6541c..a0292426e 100644 --- a/packages/tldraw/src/test/translating.test.ts +++ b/packages/tldraw/src/test/translating.test.ts @@ -137,14 +137,14 @@ describe('When translating...', () => { const before = editor.getShape(ids.box1)! - editor.forceTick(5) + jest.advanceTimersByTime(100) editor // The change is bigger than expected because the camera moves .expectShapeToMatch({ id: ids.box1, x: -160, y: 10 }) // We'll continue moving in the x postion, but now we'll also move in the y position. // The speed in the y position is smaller since we are further away from the edge. .pointerMove(0, 25) - editor.forceTick(2) + jest.advanceTimersByTime(100) editor.pointerUp() const after = editor.getShape(ids.box1)! @@ -159,16 +159,16 @@ describe('When translating...', () => { editor.user.updateUserPreferences({ edgeScrollSpeed: 1 }) editor.pointerDown(50, 50, ids.box1).pointerMove(1080, 50) + jest.advanceTimersByTime(100) editor - .forceTick(4) // The change is bigger than expected because the camera moves - .expectShapeToMatch({ id: ids.box1, x: 1140, y: 10 }) + .expectShapeToMatch({ id: ids.box1, x: 1160, y: 10 }) .pointerMove(1080, 800) - .forceTick(6) + jest.advanceTimersByTime(100) editor - .expectShapeToMatch({ id: ids.box1, x: 1280, y: 845.68 }) + .expectShapeToMatch({ id: ids.box1, x: 1300, y: 845.68 }) .pointerUp() - .expectShapeToMatch({ id: ids.box1, x: 1280, y: 845.68 }) + .expectShapeToMatch({ id: ids.box1, x: 1300, y: 845.68 }) }) it('translates multiple shapes', () => { @@ -1897,68 +1897,11 @@ describe('Moving the camera while panning', () => { .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', @@ -1990,8 +1933,6 @@ describe('Moving the camera while panning', () => { // 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) diff --git a/yarn.lock b/yarn.lock index a1dffab9c..c81a8d937 100644 --- a/yarn.lock +++ b/yarn.lock @@ -7399,7 +7399,6 @@ __metadata: version: 0.0.0-use.local resolution: "@tldraw/state@workspace:packages/state" dependencies: - "@tldraw/utils": "workspace:*" "@types/lodash": "npm:^4.14.188" "@types/react": "npm:^18.2.47" "@types/react-test-renderer": "npm:^18.0.0"