fix(viewer): route grid lines to their own overlay channel on #3368's single-owner base - #3387
fix(viewer): route grid lines to their own overlay channel on #3368's single-owner base#3387BIMvoice wants to merge 3 commits into
Conversation
…lic path (#3368) Two independent sources fed the ifcGrid-visibility toggle: useSymbolicAnnotations (section-clipped, origin-shift-rebased grid buckets) and useGridLines3D (raw parseGridLines wasm output, unclipped, rebased only by RTC). Both uploaded to their own renderer line-overlay channel, so every axis drew twice, #862's grid section-clipping was inert, and a federated/re-aligned model with nonzero originShift could show the two copies at different elevations. Grid lines now draw only from useSymbolicAnnotations; useGridLines3D is removed. parseGridLines/parseGridAxes stay as published @ifc-lite/geometry SDK surface, unrelated to this viewer-internal wiring. Claude-Session: https://claude.ai/code/session_01QPHChk3Ve9N519A4kY7436
`grid-overlay-single-owner.test.ts` read `Viewport.tsx` and regex-matched it to assert the redundant hook was no longer imported and no second 'grid' line-overlay channel was uploaded. `check-source-text-assertions.mjs` refuses new assertions of that shape -- its `@source-text-assertion-ok` hatch is for anchor guards, which these are not -- so the Node tests lane failed on the step named 'Check for new source-text assertions'. The import assertion was redundant anyway: `useGridLines3D.ts` is deleted in this branch, so an import of it does not compile and typecheck already enforces it. The file-absence check stays -- `existsSync` is not a text predicate, and it is what stops the hook being restored and re-wired. What is genuinely lost is a guard against a NEW second channel wired from somewhere else. That is left to review rather than kept as an assertion the house rule bans.
…gle-owner base (#3359, #3368) Rebuilds #3381 on top of #3376's single-owner grid pipeline instead of alongside it. #3376 collapsed grid-line ownership onto `useSymbolicAnnotations` and deleted the redundant `useGridLines3D` extractor; #3381 (mutually exclusive with #3376 as originally written) kept `useGridLines3D` and merged its output with the symbolic path via a new `mergeGridLineChannels`. Two grid-line sources that must stay in agreement is exactly the failure mode #3376 exists to retire, so this keeps #3376's single owner and ports only #3381's channel split onto it. `useSymbolicAnnotations` previously lifted IfcAnnotation curves AND IfcGridAxis lines into one buffer uploaded to the renderer's `annotation` line-overlay channel. With annotations off and the grid on, that buffer carried only grid content, but `CHANNEL_EXPANDS_MODEL_BOUNDS.annotation` is `true` (so an annotation-only model can still be framed) while `.grid` is `false` (grid axes routinely extend past the model envelope, issue #967) — the grid-only upload grew the scene bounds the `grid` channel exists to protect. Every camera fit and empty-space orbit gesture (whose pivot falls back to the scene-bounds centroid) then reframed onto the grid. `useSymbolicAnnotations` now returns `{ annotation, grid }` separately (`buildSymbolicLineChannels` in the new `symbolic-line-channels.ts`, split out for unit testing with no React/store/WASM dependency and to keep the hook file under the module-size budget), and `Viewport.tsx` uploads each buffer to its like-named channel. There is no `useGridLines3D` and no merge step — one source, one channel. Claude-Session: https://claude.ai/code/session_01QPHChk3Ve9N519A4kY7436
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughChangesGrid overlay pipeline
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The PR separates grid overlays from model-bound annotations and reports targeted tests and validation checks passing; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Viewport
participant useSymbolicAnnotations
participant RendererOverlays
participant Camera
Viewport->>useSymbolicAnnotations: obtain separate annotation and grid buffers
useSymbolicAnnotations-->>Viewport: return lifted and clipped line buffers
Viewport->>RendererOverlays: upload annotation buffer to annotation channel
Viewport->>RendererOverlays: upload grid buffer to grid channel
RendererOverlays->>Camera: synchronize bounds only for expanding channels
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 45.45% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 9 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
Viewer benchmark✅ No threshold regressions detected. 01_Snowdon_Towers_Sample_Structural(1).ifcBaseline recorded 2026-07-01T20:31:05.538Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.
AC20-FZK-Haus.ifcBaseline recorded 2026-07-01T20:30:59.972Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.
Refresh the baseline from a CI run: dispatch the Benchmark workflow with |
|
Superseded by #3394.
#3394 (branch For the record, #3368 is still not fixed on Not closing this — leaving it to the maintainer's judgment. |
|
Closing as superseded by #3394. This carries two changesets, and The remaining half, the #3368 single-owner dedupe, is what #3394 does on the correct post-#3381 base. I am reviewing #3394 now. No work lost. Closing #3376 for the same reason. |
…#3394) The viewport fed the 'grid' line-overlay channel from two independent sources: useSymbolicAnnotations's grid buckets (section-clipped and origin-shift-rebased) and useGridLines3D (the wasm parseGridLines API, unclipped and rebased only by RTC), merged with mergeGridLineChannels. Every IfcGridAxis drew twice, #862's grid section-clipping was inert (the unclipped copy always drew the full grid), and a federated or re-aligned model with a nonzero originShift could show the two copies at different elevations. Grid lines now draw only from useSymbolicAnnotations's 'grid' channel. useGridLines3D and mergeGridLineChannels are removed as dead code. parseGridLines/parseGridAxes stay as published @ifc-lite/geometry SDK surface. Supersedes #3376 and #3387, both stale against main after #3381 (#3359) changed which grid extractor main keeps and added the annotation/grid channel split this rebuilds on top of. Claude-Session: https://claude.ai/code/session_01QPHChk3Ve9N519A4kY7436
2 participants
Summary
References #3368 and #3359. Supersedes #3381 by rebuilding it on top of #3376's single-owner grid pipeline instead of alongside it — #3376 and the original #3381 are mutually exclusive as written (#3376 deletes
useGridLines3D; #3381 keeps it and merges its output with the symbolic path via a newmergeGridLineChannels).useGridLines3D— one grid-line owner, not two independent extractors that have to agree (they didn't: Two extractors both own IfcGridAxis lines, so grid section-clipping is inert and the copies can disagree in Y #3368 documents them disagreeing in both content, clipping, and elevation rebase).useSymbolicAnnotationsnow returns{ annotation, grid }instead of oneFloat32Array, and the viewer uploads grid content to its own'grid'line-overlay channel instead of sharing the'annotation'buffer.useGridLines3Dand nomergeGridLineChannelsin this branch — nothing to merge, because there is only one grid-line source.Why the split still matters with one source
CHANNEL_EXPANDS_MODEL_BOUNDS.annotationistrue(an annotation-only model must still be frameable) but.gridisfalse(grid axes routinely extend past the model envelope, issue #967). With annotations off and the grid on, the single shared buffer carried only grid content but still reached the bounds-expandingannotationchannel — inflating the scene bounds that the empty-space orbit-pivot fallback (camera.getSceneBounds()centroid, seeuseMouseControls.ts/useTouchControls.ts) falls back to, so orbiting swung the camera toward the grid instead of the model. This is independent of how many hooks feed the buffer, so it survives #3376's consolidation.Test plan
apps/viewer: targetednode:testrun ofuseSymbolicAnnotations.test.ts, the newuseSymbolicAnnotations.gridChannelSplit.test.ts,useSymbolicAnnotations.cacheKeyFrame.test.ts,useSymbolicAnnotations.frameRace.test.ts,useSymbolicAnnotations.retryStorm.test.ts,useSymbolicAnnotationsForDrawing.classGate.test.tsx, and Two extractors both own IfcGridAxis lines, so grid section-clipping is inert and the copies can disagree in Y #3368'sgrid-overlay-single-owner.test.ts— 34/34 pass, 0 failures.packages/renderer:renderer-overlays-grid-channel-reframe.test.ts(new, ported from fix(viewer): route grid-only annotation content to its own line-overlay channel #3381) +renderer-overlays-line-channels.test.ts— 15/15 pass, confirmingCHANNEL_EXPANDS_MODEL_BOUNDSisannotation: true/grid: falseand that a grid-only upload to'grid'leaves the orbit-pivot fallback untouched while the same content on'annotation'(today's pre-fix wiring) drags it.tsc --noEmitclean forapps/viewerandpackages/renderer.node scripts/check-module-size.mjsexits 0 (Viewport.tsxtrimmed to 1839 lines, budget 1840).node scripts/check-source-text-assertions.mjsexits 0 (no new source-text assertions).git status --porcelainclean before push.apps/viewer/src/hooks/useGridLines3D.tsandapps/viewer/src/components/viewer/merge-grid-line-channels.tsdo not exist on this branch.🤖 Generated with Claude Code
https://claude.ai/code/session_01QPHChk3Ve9N519A4kY7436
Summary by CodeRabbit