claude generated: experiment with consolidating layers#147
Open
seankmartin wants to merge 6 commits intofeature/edit-modefrom
Open
claude generated: experiment with consolidating layers#147seankmartin wants to merge 6 commits intofeature/edit-modefrom
seankmartin wants to merge 6 commits intofeature/edit-modefrom
Conversation
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
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.
Note not from Claude. I felt like the
SliceViewSpatiallyIndexedSkeletonLayerandMultiscaleSliceViewSpatiallyIndexedSkeletonLayerwere 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:
SpatiallyIndexedSkeletonSliceViewRenderLayerBackend (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
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.
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.
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.