Points render path: composite + PointsDataEngine + size/overdraw controls - #80
Merged
Conversation
Move the data-loading/caching/orchestration logic out of the vis SpatialCanvas/useLayerData god-hook into a framework-agnostic LayerDataEngine in @spatialdata/layers, leaving a thin React binding. Groundwork for the FBO-splat points path and the MDV active-link. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Brings the @spatialdata/core points loading layer onto current main as the dependency root of the render-path reconstruction (see docs/plans/layer-data-engine-decomposition.md): - points loaders/tiling/features/limits/load-options + points worker - bounded (loadPointsInBounds) and capped preload paths on VPointsSource; related VShapesSource/VTableSource updates - vendored parquet-wasm (local files + parquetWasmLoader) replacing the parquet-wasm npm dependency; dropped from core, docs, and the workspace catalog; lockfile regenerated - core tsconfig gains WebWorker/DOM libs for the worker Typecheck clean; 120 core tests pass. No layers/vis changes yet. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
main is bare of the points render path, so the resolver-module move can't be step 1; core I/O must land first as the dependency root. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Second step of the render-path reconstruction (see docs/plans/layer-data-engine-decomposition.md): - Bring the @spatialdata/layers points render strategies onto main: preloadedScatterStrategy, mortonTiledStrategy, geoArrowStrategies, pointsLoader/pointsLoaderAdapter, PointsLayer, pointsScatterLayer, and the tile-debug modules; layers now depends on @spatialdata/core. - Relocate pointsLoadPlan and resolvePointsRenderResource from vis/SpatialCanvas INTO layers (they are framework-agnostic, importing only core/layers). resolvePointsRenderResource's cross-package @spatialdata/layers import becomes a relative import; both are re-exported from layers' barrel. This is the proof-of-direction placement — vis will import them from layers. - Move their unit tests into layers/tests; tighten one intentionally-partial PointsTilingMetadata fixture that vis had not been typechecking. Typecheck clean; 72 layers tests pass (120 core tests still green). vis not yet updated — it will consume these from @spatialdata/layers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The core points loader now returns PointsLoadResult with ArrayLike<number>[] columns (may be TypedArrays), which the legacy SpatialCanvas points renderer consumed as number[][]. Widen PointData.data to match — the renderer only indexes the columns, and the type already carried a TODO to move to TypedArrays. Unblocks the vis build against the reconstructed core. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Root cause: defaultEnabled was `typeof window !== 'undefined'`, so the points worker auto-enabled in every browser. loadPoints() then takes the worker branch (await decodeParquetGeometryCappedInWorker) which has no timeout and only falls back to the main thread on a *rejection*. Wherever the worker isn't functionally wired — e.g. the Vite dev demo serving core from source, where the @vite-ignore './points-worker.js' URL doesn't resolve to a runnable worker — the worker loads but never replies, so the promise never settles and the layer sits on "Loading layer data..." forever. Fix: default the worker to off; hosts opt in via enablePointsWorker() / setPointsWorkerDefaultEnabled(true) once the worker bundle is wired. The default path is now main-thread decode, which always completes. Verified in the vis demo: transcripts geometry reaches "ready", 67 parquet row-group range reads issued (previously zero), no hang, no console errors. Follow-up (not this fix): a dead/opted-in worker should still fall back via a timeout rather than hang; and points load but don't yet visually render on the legacy renderer path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The button's enablement (selectedLayerCanCenter) was gated solely on hasRenderableLayerData, which reads useLayerData's imperatively-mutated loadedDataRef cache. Mutating a ref does not trigger a re-render, so after a layer's geometry finished loading the button stayed disabled until some unrelated re-render (hover, resize) happened to recompute the gate. Gate on the reactive layer load-state instead: image/labels on the `image` resource, shapes/points on `geometry`. layerLoadStates is real React state that flips to 'ready' in lockstep with the load (the same freshness trick deriveBlockingState already relies on), so the button now enables immediately. hasRenderableLayerData is kept as an OR fallback for cached-without-status data. Verified in the vis demo: enabling `transcripts` now enables "Center on layer" as soon as geometry is ready (no interaction needed), and clicking it frames the point cloud. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Grilling outcome — MVP = filter/colour(+highlight)/identify on the layers/PointsLayer composite, reached via a points-only parity-first LayerDataEngine slice. Records the Points Feature vs table-feature distinction, Feature Code rules, and the serializable-colour / runtime-highlight boundary in CONTEXT.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the legacy flat renderPointsLayer call with the @spatialdata/layers PointsLayer composite, fed by resolvePointsRenderResource using the already- cached x/y batch as the resolver's `preloaded` input. Same I/O, same flat- colour scatter (verified at parity in the preview: cloud draws, Center-on- layer works, row-group range reads intact) — but now on the composite that will gain filter/colour/highlight, and where the future FBO strategy slots in. First increment of the points MVP parity slice (docs/plans/points-mvp-and- roadmap.md step 1). The engine extraction (1b) follows. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
preloadedScatterStrategy passed the composite PointsLayer's own id straight
to its ScatterplotLayer sublayer. A sublayer sharing its parent's id makes
deck.gl re-initialise an already-initialised layer — assert(!this.internalState)
in ScatterplotLayer._initialize — flooding the console every frame once the
composite was wired live (step 1a). Derive `${id}-scatter` like the morton
strategy already does.
Found while diagnosing the pan-flash (the visible bug); this assertion was the
noisy console symptom underneath. Regression test asserts the sublayer id is
namespaced below the composite id (red without the fix).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
getLayers() runs on every viewer render — including every pan/zoom frame, via the pickingEnabled interaction gate — and resolved a fresh PointsRenderResource (new loader identity) each call. The PointsLayer composite resets its async- loaded batch whenever the loader identity changes, so the layer blanked for a frame on every pan: visible flashing on and off. Memoize the resource per element by pointsRenderResourceSignature so re-renders reuse a stable loader identity; evict on element unload. Verified in preview: an interaction burst that previously triggered composite resets now triggers none, and the cloud stays drawn while panning/zooming. This per-element stable-resource cache is exactly what the LayerDataEngine will own in step 1b — a stepping stone, not throwaway. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Move the points cache, the stable render-resource memo, and the async preload orchestration out of the vis useLayerData god-hook into a framework-agnostic PointsDataEngine in @spatialdata/layers (the points-only sub-engine of the LayerDataEngine decomposition). The hook's points path is now a thin binding: it forwards status into layerLoadStates via onStatus and re-renders on the engine's subscribe. - New: layers/src/engine/PointsDataEngine.ts — hasData/getData/getResource (memoized, the pan-flash guard) / ensureLoaded (idempotent) / evict / subscribe. - 6 headless unit tests (impossible against the hook): single-load idempotency, stable-resource identity, error status, evict, subscribe. - Hook: removed loadedDataRef.points, stablePointsResourceRef, the inline resolve+memo, and the inline loadPoints branch (-76/+49 lines). Parity verified in preview: transcripts loads, Geometry ready, Center works, cloud renders, zero deck assertions through a zoom burst. Scope is the preloaded flat path only; tiling/catalog/filter/tile-debug remain dark, to be wired INTO this engine in MVP steps 2-4. Per-type sub-engine; parity-to-current only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A per-layer Point size slider (0.1-12px) in the points properties panel, bound to PointsLayerConfig.pointSize via actions.updateLayer. Point radius is the cheapest lever on scatter overdraw (fill cost scales with radius squared), so this gives direct control over the dominant perf/visual tradeoff on large transcript layers. Verified in preview: 8px fills solid, 0.3px reveals the granular density texture; no console errors. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…w control The preloaded scatter path now sizes points in world (common) units instead of fixed pixels, so the GPU scales them with zoom: points shrink when you zoom out — exactly where scatter overdraw is worst — and grow when you zoom in to reveal individual transcripts. radiusMinPixels/radiusMaxPixels clamp the projected radius so points never vanish or bloat. No viewZoom threading / per-frame React re-renders (the reason we chose this over reviving zoomScaledPointSize). The Morton tile path keeps fixed pixel sizing (tiles are viewport-bounded). The pointSize slider now controls world radius; dropped the misleading 'px' suffix. Verified in preview: framed view is granular/low-overdraw, zooming in enlarges points, the slider bites across the mid-zoom range; no console errors; layers 79 tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (67)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The 1b wiring created the engine via a lazy-initialised useRef and read
.current during render, which trips react-hooks/refs ("Cannot access refs
during render") and left pointsEngine out of five dependency arrays. Create it
with a useState lazy initializer instead: a stable value that is safe to read
during render and to list as a dependency. Added pointsEngine to the load
effect and the getLayers / hasRenderableLayerData / getWorldBoundsForLayer /
reloadElement callbacks. `pnpm lint:react` clean; behaviour unchanged (engine
still created once).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Merged
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
What this lands
Reconstructs the points / transcript rendering path across
core+layers+vis, on the clean architecture from ADR 0002 / ADR 0003, and delivers user-visible point-size + overdraw controls. It also lands the first step of theLayerDataEnginedecomposition (plan).This is intentionally a milestone PR, not the full feature set — feature filtering, colour-by-feature, tooltips, and the Morton tiled render path are present in
core/layersbut not yet wired into the UI. They're the next PR (roadmap).User-visible
@spatialdata/layersPointsLayercomposite.Architecture (the design-bearing parts to review)
resolvePointsRenderResource— the store-agnostic boundary that turns aPointsElementinto a render resource (ADR 0003).PointsDataEngine(layers/src/engine/) — framework-agnostic, React-free, unit-tested. Owns the points cache, the stable render-resource memo, and async load orchestration. First sub-engine of theLayerDataEnginedecomposition; the vis hook is now a thin binding.@spatialdata/corepoints I/O — bounded/capped loading onPointsElement, Morton tiling metadata, dictionary-aware feature catalog, an opt-in points worker, and vendored parquet-wasm (replaces the npm/CDN dep) with row-group range reads.Fixes (surfaced while wiring this up)
Not in this PR (follow-up)
Feature filtering, colour-by-feature, per-point tooltips, and the Morton tiled render path (points currently load as a capped 4M-row preload). Scoped in
docs/plans/points-mvp-and-roadmap.md.Testing
core120,layers79 (incl. 6 headlessPointsDataEnginetests),vis42 — all green; all three packages typecheck clean.Reviewer note
It's a large diff because reconstructing points rendering is irreducible across three packages (none renders points alone). ~3k lines are vendored parquet-wasm and much of
coreis the I/O foundation. The design-bearing changes arePointsDataEngine, the composite/resolver wiring invis, and the world-unit sizing — the core foundation and vendored wasm are largely mechanical / ADR-sanctioned. Commits are ordered to tell the story (docs → core → layers → vis wiring → fixes → engine → size controls).🤖 Generated with Claude Code