Skip to content

claude generated: experiment with consolidating layers#147

Open
seankmartin wants to merge 6 commits intofeature/edit-modefrom
refactor/skeleton-layers
Open

claude generated: experiment with consolidating layers#147
seankmartin wants to merge 6 commits intofeature/edit-modefrom
refactor/skeleton-layers

Conversation

@seankmartin
Copy link
Copy Markdown

@seankmartin seankmartin commented May 4, 2026

Note not from Claude. I felt like the SliceViewSpatiallyIndexedSkeletonLayer and MultiscaleSliceViewSpatiallyIndexedSkeletonLayer were adding logic which seemed a bit uneeded. This seemed further to be the case when their draw methods became a stub. I asked claude to consolidate the layers in the frontend.ts file based on this.

Rest from claude:

PR Review: feature/edit-mode → refactor/skeleton-layers

What was removed and why

The refactor unifies the 2D and 3D skeleton rendering backends into a single backend class. Previously, there were two
parallel systems:

  • 2D: SliceViewSpatiallyIndexedSkeletonLayer + MultiscaleSliceViewSpatiallyIndexedSkeletonLayer (frontend) +
    SpatiallyIndexedSkeletonSliceViewRenderLayerBackend (backend)
  • 3D: PerspectiveViewSpatiallyIndexedSkeletonLayer (frontend) + SpatiallyIndexedSkeletonRenderLayerBackend (backend)

The new approach merges them. The single SpatiallyIndexedSkeletonRenderLayerBackend now holds both
skeletonGridLevel/skeletonLod (3D) and skeletonGridLevel2d/skeletonLod2d (2D), and selects the correct settings at
chunk priority time using:

const is2dView = pixelSize !== undefined;
const skeletonGridLevel = (is2dView ? this.skeletonGridLevel2d : this.skeletonGridLevel).value;

Is too much removed?

No — the removals are intentional and complete. The SliceViewSpatiallyIndexedSkeletonLayer had an empty draw() method;
its only real job was participating in the chunk visibility tracking pipeline. That responsibility is now explicitly
taken over by SliceViewPanelSpatiallyIndexedSkeletonLayer.attach(), which directly calls the backend RPC:

// src/skeleton/frontend.ts ~3473
backend.rpc.invoke(RENDERED_VIEW_ADD_LAYER_RPC_ID, { ... });
// and later:
baseLayer.rpc!.invoke(SPATIALLY_INDEXED_SKELETON_RENDER_LAYER_UPDATE_SOURCES_RPC_ID, { ... });

This is a tighter but more direct coupling — removing the indirection through the old SliceView layer system.

Things worth scrutinizing

  1. Chunk stats silencing for 2D (getSpatialSkeletonChunkStats):
    kind === "2d" now always returns {0, 0}. The comment says this avoids double-counting since the 2D backend is shared.
    This is logically correct, but it means the loading indicator only reflects 3D chunk loading. If 2D and 3D have
    different LOD/grid settings and 2D is still loading while 3D is done, spatialSkeletonVisibleChunksLoaded could report
    true prematurely. Worth testing with visible 2D skeletons and slow connections.

  2. SliceViewPanelSpatiallyIndexedSkeletonLayer.backend is now public:
    backend: ChunkRenderLayerFrontend was added explicitly. This is needed for the new RPC calls in attach(), so it's
    intentional — but it's a broader exposure than before.

  3. Debounce duplication in the backend:
    SpatiallyIndexedSkeletonRenderLayerBackend now has two separate debounce handlers — one for skeletonLod and one for
    skeletonLod2d — both identical in structure. They could share an abstraction, but this is a minor style issue, not a
    bug.

Overall verdict

The changes make clear architectural sense. The motivation is to eliminate redundant layer classes (300+ lines
removed) by recognizing that the 2D chunk-loading logic was essentially the same as 3D but with different parameters —
so it belongs in the shared backend, not in separate frontend layer classes. The replacement mechanism (panel layer
calling the unified backend's RPC directly) appears complete.

The one functional risk to validate is whether the silenced 2D chunk stats causes any observable regression in the
loading indicator behavior.

@seankmartin seankmartin marked this pull request as ready for review May 5, 2026 17:18
@seankmartin seankmartin requested a review from afonsobspinto May 5, 2026 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant