Conversation
…ay channel (#3359) `useSymbolicAnnotations` lifted IfcAnnotation curves AND IfcGridAxis lines into one buffer uploaded to the renderer's `annotation` line-overlay channel. With "Show IFC Annotations" off and the IfcGrid toggle on, that buffer carried only grid content, but `CHANNEL_EXPANDS_MODEL_BOUNDS.annotation` is `true` (grid axes routinely extend past the model envelope, which is exactly why `grid: false` exists — issue #967), so every camera fit and empty-space orbit pivot (falls back to the scene-bounds centroid) reframed around the inflated bounds instead of the model. Root cause: the annotation overlay channel was keyed by SOURCE HOOK, not by CONTENT — the renderer's bounds-expansion policy is keyed by channel, but one hook fed two content kinds into the same channel. `useSymbolicAnnotations` now returns `{ annotation, grid }` separately (the merge logic lives in the new `symbolic-line-channels.ts`, split out to keep the hook file under the module-size budget and independently unit testable), and `Viewport.tsx` uploads each to its like-named renderer channel, merging the grid buffer with the existing `useGridLines3D` output (`merge-grid-line-channels.ts`) pending #3368's single-owner dedupe. Tests: - apps/viewer/src/hooks/useSymbolicAnnotations.gridChannelSplit.test.ts: pins that a grid-only ParseResult produces an empty `annotation` buffer. - packages/renderer/src/renderer-overlays-grid-channel-reframe.test.ts: demonstrates the reframe mechanism with a real Camera + ModelBoundsTracker from a non-default camera pose — grid content on the `annotation` channel drags the empty-space orbit-pivot fallback (`camera.getSceneBounds()` centroid, the same value `useMouseControls.ts`/`useTouchControls.ts` fall back to) far from the model; on `grid` it does not move at all. Related but distinct from #3368 (PR #3376), which deduplicates two grid line sources on the SAME channel — it does not change which channel grid content reaches, so it does not resolve this issue. Claude-Session: https://claude.ai/code/session_01QPHChk3Ve9N519A4kY7436
|
Warning Review limit reached
This review includes 7 billable files and costs up to $1.75. Or wait 59 minutes for your next included review. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
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 |
|
#3376 and #3381 are mutually exclusive designs over the same code — one must be rewritten on top of the other, and merge order will not resolve it. Both fix a real, different bug, but they take opposite architectural directions:
Consequences either way:
The underlying question is a design call, not a mechanic: should grid geometry have one owner (#3376) or two that are merged (#3381)? #3359's actual defect — grid content landing on the To be clear about how this happened: these were worked in parallel, and the #3359 investigation concluded #3376 was "complementary" without noticing the two designs contradict. That assessment was wrong, and this is being flagged before either merges rather than after. Happy to rewrite whichever you prefer on top of the other — my read is that #3376's single-owner direction is the sounder base, since it removes the second source rather than teaching two sources to agree, and this repo has repeatedly paid for two implementations that must agree with nothing enforcing it. |
|
@coderabbitai review |
|
2 participants
Summary
Fixes #3359.
useSymbolicAnnotationslifted IfcAnnotation curves AND IfcGridAxis lines into ONE buffer uploaded to the renderer'sannotationline-overlay channel. With "Show IFC Annotations" off and the IfcGrid toggle on, that buffer carried only grid content — butCHANNEL_EXPANDS_MODEL_BOUNDS.annotation(packages/renderer/src/renderer-overlays.ts) istrue, the opposite ofgrid: false, which exists precisely because grid axes routinely extend past the model envelope (issue #967). So a grid-only upload on theannotationchannel grew the scene boundsgrid: falseexists to protect, and any later camera fit or empty-space orbit gesture — whose pivot falls back to the scene-bounds centroid (useMouseControls.ts/useTouchControls.ts) — reframed around the inflated bounds instead of the model.Root cause: the channel a buffer reached was decided by which HOOK produced it, not by what CONTENT it held — the renderer's bounds-expansion policy is keyed by channel, but
useSymbolicAnnotationsfed two content kinds into the same one.Relationship to #3368 / PR #3376: that PR removes the duplicate, unclipped
useGridLines3Dupload so grid lines draw only from the symbolic (clipped) path — a dedupe on the SAME (annotation) channel. It does not change which channel grid content reaches, so it does not resolve this issue; this PR is complementary and still needed after #3376 lands (I merged the two buffers'gridchannel here so it works correctly with or without #3376 merged first).Fix
useSymbolicAnnotationsnow returns{ annotation, grid }as two separate buffers instead of one. The merge logic (previously inline) is split into a new pure function,buildSymbolicLineChannelsinapps/viewer/src/hooks/symbolic-line-channels.ts— split out so it is unit-testable with no React/store/WASM dependency, and to keep the hook file under the module-size budget.Viewport.tsxnow uploadsannotationto the'annotation'channel andgridto the'grid'channel (merged with the existinguseGridLines3Doutput via the newmerge-grid-line-channels.ts, pending #3368's single-owner dedupe).Test plan
apps/viewer/src/hooks/useSymbolicAnnotations.gridChannelSplit.test.ts(new): pins that a grid-onlyParseResult(annotation buckets empty) produces an emptyannotationbuffer and a populatedgridbuffer — RED before the fix (stashed source, restored by SHA):buildSymbolicLineChannelsdid not exist, so the grid-only content had no way to avoid the single shared buffer. GREEN after.packages/renderer/src/renderer-overlays-grid-channel-reframe.test.ts(new): demonstrates the reframe mechanism directly against realCamera+ModelBoundsTracker+RendererOverlayscode (no fakes for those two), starting from a camera pose distinct from both the model centre and the origin so a spurious reframe is observable. Grid-only content uploaded to'annotation'(today's pre-fix wiring) drags the empty-space orbit-pivot fallback (camera.getSceneBounds()centroid) far from the model; the same content uploaded to'grid'(the fix's wiring) leaves it untouched.useSymbolicAnnotations*.test.ts,useSymbolicAnnotationsForDrawing.classGate.test.tsx,dxf-lines-3d-upload.test.ts,store/teardown-registry.test.ts— 41/41 pass.renderer-overlays-grid-channel-reframe.test.ts,renderer-overlays-line-channels.test.ts,renderer-overlays-section.test.ts,model-bounds-tracker.test.ts— 46/46 pass.pnpm typecheck(turbo, full workspace): clean.node scripts/check-module-size.mjs: exit 0 (Viewport.tsxtrimmed back to its existing 1840-line budget;useSymbolicAnnotations.tsshrunk by splittingbuildSymbolicLineChannelsinto its own file).node scripts/check-source-text-assertions.mjs: exit 0, no new assertions.git status --porcelainclean before push.https://claude.ai/code/session_01QPHChk3Ve9N519A4kY7436