From 0b44a8b47a3924c2a1b9c94cb30761f60f6f563b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mitja=20Bezen=C5=A1ek?= Date: Wed, 17 Apr 2024 13:39:09 +0200 Subject: [PATCH] Fix culling. (#3504) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes culling for cases when another user would drag shapes inside your viewport. We weren't correctly calculating the culling status for arrows that might be bound to those shapes and also for shapes within dragged in groups / frames. ### 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 - [x] `bugfix` — Bug fix - [ ] `feature` — New feature - [ ] `improvement` — Improving existing features - [ ] `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 ### Test Plan 1. Open the same room in two browsers / tabs. 2. Have some shapes that are visible in one browser, but not the other. 3. Drag these shapes so that they are visible in the other browser as well. 4. They should correctly get unculled. 5. Do this by dragging shapes that have arrows bound to them (arrows should uncull), groups (shapes within them should uncull), frames. - [x] Unit Tests - [ ] End to end tests ### Release Notes - Fix culling. --- .../editor/derivations/notVisibleShapes.ts | 70 +++---------------- .../tldraw/src/test/getCulledShapes.test.tsx | 53 ++++++++++++++ 2 files changed, 62 insertions(+), 61 deletions(-) diff --git a/packages/editor/src/lib/editor/derivations/notVisibleShapes.ts b/packages/editor/src/lib/editor/derivations/notVisibleShapes.ts index 461835500..25a676706 100644 --- a/packages/editor/src/lib/editor/derivations/notVisibleShapes.ts +++ b/packages/editor/src/lib/editor/derivations/notVisibleShapes.ts @@ -1,5 +1,5 @@ -import { RESET_VALUE, computed, isUninitialized } from '@tldraw/state' -import { TLPageId, TLShapeId, isShape, isShapeId } from '@tldraw/tlschema' +import { computed, isUninitialized } from '@tldraw/state' +import { TLShapeId } from '@tldraw/tlschema' import { Box } from '../../primitives/Box' import { Editor } from '../Editor' @@ -21,15 +21,10 @@ function isShapeNotVisible(editor: Editor, id: TLShapeId, viewportPageBounds: Bo */ export const notVisibleShapes = (editor: Editor) => { const isCullingOffScreenShapes = Number.isFinite(editor.renderingBoundsMargin) - const shapeHistory = editor.store.query.filterHistory('shape') - let lastPageId: TLPageId | null = null - let prevViewportPageBounds: Box function fromScratch(editor: Editor): Set { const shapes = editor.getCurrentPageShapeIds() - lastPageId = editor.getCurrentPageId() const viewportPageBounds = editor.getViewportPageBounds() - prevViewportPageBounds = viewportPageBounds.clone() const notVisibleShapes = new Set() shapes.forEach((id) => { if (isShapeNotVisible(editor, id, viewportPageBounds)) { @@ -38,68 +33,21 @@ export const notVisibleShapes = (editor: Editor) => { }) return notVisibleShapes } - return computed>('getCulledShapes', (prevValue, lastComputedEpoch) => { + return computed>('getCulledShapes', (prevValue) => { if (!isCullingOffScreenShapes) return new Set() if (isUninitialized(prevValue)) { return fromScratch(editor) } - const diff = shapeHistory.getDiffSince(lastComputedEpoch) - if (diff === RESET_VALUE) { - return fromScratch(editor) - } + const nextValue = fromScratch(editor) - const currentPageId = editor.getCurrentPageId() - if (lastPageId !== currentPageId) { - return fromScratch(editor) - } - const viewportPageBounds = editor.getViewportPageBounds() - if (!prevViewportPageBounds || !viewportPageBounds.equals(prevViewportPageBounds)) { - return fromScratch(editor) - } - - let nextValue = null as null | Set - const addId = (id: TLShapeId) => { - // Already added - if (prevValue.has(id)) return - if (!nextValue) nextValue = new Set(prevValue) - nextValue.add(id) - } - const deleteId = (id: TLShapeId) => { - // No need to delete since it's not there - if (!prevValue.has(id)) return - if (!nextValue) nextValue = new Set(prevValue) - nextValue.delete(id) - } - - for (const changes of diff) { - for (const record of Object.values(changes.added)) { - if (isShape(record)) { - const isCulled = isShapeNotVisible(editor, record.id, viewportPageBounds) - if (isCulled) { - addId(record.id) - } - } - } - - for (const [_from, to] of Object.values(changes.updated)) { - if (isShape(to)) { - const isCulled = isShapeNotVisible(editor, to.id, viewportPageBounds) - if (isCulled) { - addId(to.id) - } else { - deleteId(to.id) - } - } - } - for (const id of Object.keys(changes.removed)) { - if (isShapeId(id)) { - deleteId(id) - } + if (prevValue.size !== nextValue.size) return nextValue + for (const prev of prevValue) { + if (!nextValue.has(prev)) { + return nextValue } } - - return nextValue ?? prevValue + return prevValue }) } diff --git a/packages/tldraw/src/test/getCulledShapes.test.tsx b/packages/tldraw/src/test/getCulledShapes.test.tsx index 53c99bd35..eaf3ed701 100644 --- a/packages/tldraw/src/test/getCulledShapes.test.tsx +++ b/packages/tldraw/src/test/getCulledShapes.test.tsx @@ -136,3 +136,56 @@ it('correctly calculates the culled shapes when adding and deleting shapes', () const culledShapeFromScratch = editor.getCulledShapes() expect(culledShapesIncremental).toEqual(culledShapeFromScratch) }) + +it('works for shapes that are outside of the viewport, but are then moved inside it', () => { + const box1Id = createShapeId() + const box2Id = createShapeId() + const arrowId = createShapeId() + + editor.createShapes([ + { + id: box1Id, + props: { w: 100, h: 100, geo: 'rectangle' }, + type: 'geo', + x: -500, + y: 0, + }, + { + id: box2Id, + type: 'geo', + x: -1000, + y: 200, + props: { w: 100, h: 100, geo: 'rectangle' }, + }, + { + id: arrowId, + type: 'arrow', + props: { + start: { + type: 'binding', + isExact: true, + boundShapeId: box1Id, + normalizedAnchor: { x: 0.5, y: 0.5 }, + isPrecise: false, + }, + end: { + type: 'binding', + isExact: true, + boundShapeId: box2Id, + normalizedAnchor: { x: 0.5, y: 0.5 }, + isPrecise: false, + }, + }, + }, + ]) + + expect(editor.getCulledShapes()).toEqual(new Set([box1Id, box2Id, arrowId])) + + // Move box1 and box2 inside the viewport + editor.updateShapes([ + { id: box1Id, type: 'geo', x: 100 }, + { id: box2Id, type: 'geo', x: 200 }, + ]) + // Arrow should also not be culled + expect(editor.getCulledShapes()).toEqual(new Set()) +})