Skip to content

fix(renderer): derive SECTION_2D_UNIFORM_SLOT_COUNT from the slot index - #3385

Open
BIMvoice wants to merge 1 commit into
mainfrom
fix-3342
Open

fix(renderer): derive SECTION_2D_UNIFORM_SLOT_COUNT from the slot index#3385
BIMvoice wants to merge 1 commit into
mainfrom
fix-3342

Conversation

@BIMvoice

@BIMvoice BIMvoice commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • SECTION_2D_UNIFORM_SLOT_COUNT (packages/renderer/src/shaders/section-2d-overlay.wgsl.ts) sized the shared section-2D-overlay uniform buffer with a hand-written 6. SECTION_2D_UNIFORM_SLOT_INDEX, defined just above it, maps each draw site to its slot and currently has 6 keys — the two agree today, but nothing enforces it. A new key added to the index type-checks (a property access forces the key to exist) while the count stays whatever was last typed, leaving the buffer one slot short of what the new draw site addresses.
  • SECTION_2D_UNIFORM_SLOT_COUNT is now Object.keys(SECTION_2D_UNIFORM_SLOT_INDEX).length, so the two can no longer drift.
  • Added a test in section-2d-overlay-lifecycle.test.ts pinning the count equal to the index's key count, as a backstop in case a future refactor reintroduces a literal.

Verification the test is non-vacuous

Confirmed by hand before committing (not left in the diff): with the old literal 6 restored and a 7th key added to the index, the new test failed (not ok 1 - equals the number of entries in SECTION_2D_UNIFORM_SLOT_INDEX). Reverting the perturbation and re-applying the derive fix turned it green again.

Other literals checked

Grepped every .ts/.tsx reference to both symbols. No other hand-written literal is tied to the slot count — the other consumers (section-2d-overlay.ts, section-2d-overlay-lifecycle.test.ts) all read SECTION_2D_UNIFORM_SLOT_COUNT itself, so they automatically track the fix.

Test plan

  • cd packages/renderer && npx tsx --test src/*.test.ts — 1144 pass, 0 fail, 1 pre-existing skip
  • node scripts/check-module-size.mjs — OK
  • node scripts/check-source-text-assertions.mjs — OK

Refs #3342

https://claude.ai/code/session_01QPHChk3Ve9N519A4kY7436

Summary by CodeRabbit

  • Bug Fixes
    • Improved reliability of 2D overlay rendering by keeping uniform slot configuration automatically synchronized.
  • Tests
    • Added coverage to detect mismatches between configured uniform slots and their index definitions.

SECTION_2D_UNIFORM_SLOT_COUNT sized the shared section-2D-overlay
uniform buffer with a hand-written 6, while SECTION_2D_UNIFORM_SLOT_INDEX
maps each draw site to its slot. Nothing tied the two together: a new
key added to the index type-checks fine (a property access needs the
key to compile) but the count doesn't grow with it, so the buffer stays
one slot short and the new draw site's bind-group offset lands one
record past the end.

Derive the count from the index instead, and pin the two equal with a
test so a future divergence fails a test rather than only surfacing as
a WebGPU validation error on the new site.

Refs #3342
@BIMvoice
BIMvoice requested a review from louistrue as a code owner August 28, 2026 08:08
@coderabbitai

coderabbitai Bot commented Aug 28, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 2d273b4d-b842-4b99-8376-d0a489676fa7

📥 Commits

Reviewing files that changed from the base of the PR and between 5a431e5 and 3e09926.

📒 Files selected for processing (3)
  • .changeset/section-2d-slot-count-derived.md
  • packages/renderer/src/section-2d-overlay-lifecycle.test.ts
  • packages/renderer/src/shaders/section-2d-overlay.wgsl.ts

Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The renderer now derives SECTION_2D_UNIFORM_SLOT_COUNT from SECTION_2D_UNIFORM_SLOT_INDEX. A test verifies that both values remain equal. A patch changeset documents the update.

Changes

Renderer slot count

Layer / File(s) Summary
Derive and verify slot count
packages/renderer/src/shaders/section-2d-overlay.wgsl.ts, packages/renderer/src/section-2d-overlay-lifecycle.test.ts, .changeset/section-2d-slot-count-derived.md
The slot count is derived from the slot index length. A test checks their equality. The changeset records the patch release.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Merge Risk: ⚪ Minimal · up to 3e099

The uniform buffer slot count is now derived from the slot index and covered by a regression test; no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: louistrue

Poem

A rabbit counts slots in a tidy row
No hardcoded six remains below
The index and count now agree
Tests guard their harmony
Hop, hop, shipped cleanly!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: deriving SECTION_2D_UNIFORM_SLOT_COUNT from the slot index in the renderer.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

Viewer benchmark

✅ No threshold regressions detected.

01_Snowdon_Towers_Sample_Structural(1).ifc

Baseline recorded 2026-07-01T20:31:05.538Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.

Metric Current Baseline Delta Threshold Status
firstBatchWaitMs 1853ms 2905ms -36.2% +50%
firstVisibleGeometryMs 2717ms 3652ms -25.6% +50%
streamCompleteMs 3101ms 3598ms -13.8% +50%
spatialReadyMs 1460ms 1032ms +41.5% +50%
metadataCompleteMs 2025ms 3063ms -33.9% +50%
totalWallClockMs 3600ms 3700ms -2.7% +50%

AC20-FZK-Haus.ifc

Baseline recorded 2026-07-01T20:30:59.972Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.

Metric Current Baseline Delta Threshold Status
firstBatchWaitMs 373ms 1075ms -65.3% +50%
firstVisibleGeometryMs 1537ms 1572ms -2.2% +50%
streamCompleteMs 1554ms 1980ms -21.5% +50%
spatialReadyMs 1081ms 915ms +18.1% +50%
metadataCompleteMs 1270ms 1392ms -8.8% +50%
totalWallClockMs 1800ms 3300ms -45.5% +50%

Refresh the baseline from a CI run: dispatch the Benchmark workflow with record_baseline, download the benchmark-baseline artifact, and commit baseline.json (see tests/benchmark/README.md).

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