[pull] main from tldraw:main#562
Merged
Merged
Conversation
In order to preserve the user's original intent when creating GitHub issues via the issue skill, this PR adds a convention to include the user's description verbatim at the top of the issue body, annotated with a human emoji and separated from researched content by a horizontal rule. ### Change type - [x] `other` ### Test plan - [ ] Run `/issue` with a description and verify the user's text appears verbatim at the top of the created issue ### Code changes | Section | LOC change | | -------------- | ---------- | | Config/tooling | +8 / -0 |
In order to avoid redundant work in the spatial index when shape props change without affecting bounds, this PR makes `SpatialIndexManager` skip rbush mutation on prop-only diffs and exposes a private `_boundsEpoch` that only ticks when the index actually changes. While drawing on a populated page, every pointer move emits one updated shape (the active draw shape with new `segments`) through the store diff. Previously the index would unconditionally `upsert` the shape — an O(log N) tree rebalance — and bump its return value, invalidating downstream consumers like `getShapeIdsInsideBounds`. It also walked every indexed shape allocating a `Box` per shape during the transitive-bounds sweep. Three changes: 1. `processIncrementalUpdate` compares incoming bounds to the indexed bounds and skips `upsert` when they're equal, removing the rebalance for prop-only updates. 2. The computed returns a private `_boundsEpoch` that only ticks when an add/remove/upsert actually happened, so consumers like `notVisibleShapes` no longer invalidate when nothing in the rbush moved. Bookkeeping is centralised in a small `rebuildAndBumpEpoch` helper. 3. The transitive-bounds sweep iterates the rbush's element map directly and compares against the raw `SpatialElement` instead of allocating a `Box` per shape and a fresh shape-id array. Existence checks use `RBushIndex.has(id)` instead of `getBounds(id) !== undefined`. The transitive sweep itself still runs unconditionally — it has to, because some diffs (e.g. a geo's `rectangle`→`ellipse` flip at the same width/height) leave the source shape's axis-aligned bounds unchanged but shift a bound arrow's intersection points. There's a regression test covering exactly that case. ### Change type - [x] \`improvement\` ### Test plan 1. Open a page with a few hundred shapes. 2. Start drawing a freehand stroke and move the pointer continuously. 3. Confirm no perf regression in interaction; existing culling behavior (shapes outside the viewport hidden) is unchanged when panning/zooming or when shapes are added, moved, or deleted. - [x] Unit tests - [ ] End to end tests ### Release notes - Improve drawing performance on pages with many shapes by skipping spatial index updates when only shape props change. ### Code changes | Section | LOC change | | --------- | ---------- | | Core code | +100 / -28 | | Tests | +240 / -0 |
In order to stop the canvas culling derivation from invalidating on every shape prop change, this PR makes `notVisibleShapes` skip per-shape subscriptions for shapes whose `ShapeUtil` uses the default `canCull` (which is always-true). Previously the derivation read `editor.getCurrentPageShapes()`, which calls `store.get(id)` for every shape on the page. Each shape record became a parent of the derivation, so any prop change anywhere — even an onscreen color flip during a draw stroke — invalidated `notVisibleShapes` and cascaded to `CullingController`, `Shape` consumers, and `PerformanceManager`. The new flow: 1. Subscribe to `getCurrentPageShapeIds()` (a Set; only changes on add/remove/reparent) instead of `getCurrentPageShapes()`. 2. For each offscreen id, peek the record via `store.unsafeGetWithoutCapture` to get `type` without subscribing. 3. If `util.canCull === ShapeUtil.prototype.canCull` (the default), push the id with no per-shape subscription. 4. Only when a shape's util overrides `canCull` do we call `editor.getShape(id)` (which subscribes) and invoke `canCull(shape)`. Across the repo only two real overrides of `canCull` exist (`ConditionalCullingExample` and `SizeFromDomExample`), plus test-only shapes — so >99% of shapes in the wild now contribute zero per-shape subscriptions to this derivation. ### Findings: interaction with #8799 `notVisibleShapes` had two propagation paths that fired on any shape change: 1. `getCurrentPageShapes()` → `notVisibleShapes` (5000 per-shape `store.get` subs) 2. `spatialIndex` → `notVisibleShapes` (via `getShapeIdsInsideBounds` — bumped its computed value on every shape diff) Either path alone is sufficient to invalidate the derivation. Benchmark on a 5000-shape page (500 onscreen, 4500 offscreen, 60 onscreen prop flips), counting `notVisibleShapes` body executions: | Configuration | body runs | | --- | --- | | main | 60 | | this PR alone | 60 (spatial-index path still propagates) | | #8799 alone | 60 (`getCurrentPageShapes` path still propagates) | | this PR + #8799 | **0** | This PR and #8799 are complementary. Together they drop body runs to zero on prop-only changes; either alone leaves the other path firing. Read-time average for `getNotVisibleShapes()` over 60 iterations on the combined config is ~620ms vs ~687ms on main (~10% faster); standalone this PR is a smaller ~3-7% win driven by removing per-shape capture overhead. ### Change type - [x] `improvement` ### Test plan 1. Open the conditional culling example, scroll a `preventCulling` shape offscreen, toggle the prop, confirm it remains visible/culled correctly. - [x] Unit tests ### Release notes - Reduce `notVisibleShapes` recomputation cost by skipping per-shape subscriptions for shapes with default `canCull`. ### Code changes | Section | LOC change | | --------------- | ---------- | | Core code | +30 / -21 | | Tests | +39 / -0 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )