feat(viewer): add showPcbNotes toggle to 3D renderer & SVG export (hidden by default)#720
feat(viewer): add showPcbNotes toggle to 3D renderer & SVG export (hidden by default)#720rushabhcodes wants to merge 1 commit intotscircuit:mainfrom
showPcbNotes toggle to 3D renderer & SVG export (hidden by default)#720Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| manifoldJSModule: ManifoldToplevel | null, | ||
| circuitJson: AnyCircuitElement[], | ||
| visibility: LayerVisibilityState, | ||
| showPcbNotes = false, |
There was a problem hiding this comment.
Pull request overview
Adds a showPcbNotes flag to control whether pcb_note_* annotation elements are rendered in the 3D board textures (default off), and propagates this option through the viewer components and Manifold/JSCAD texture pipelines.
Changes:
- Introduce
showPcbNotes(defaultfalse) and propagate it through CadViewer → renderers → board texture creation. - Update combined board texture generation to conditionally include pcb note textures.
- Add a Bun test and Storybook controls/docs updates for the new toggle.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/pcb-notes-visibility.test.ts | Adds test coverage for pcb note visibility default/override behavior. |
| stories/PcbNoteLine.stories.tsx | Updates Storybook story to expose showPcbNotes as a controllable arg. |
| src/three-components/JscadBoardTextures.tsx | Passes showPcbNotes into texture generation and refactors cleanup loops. |
| src/textures/create-combined-board-textures.ts | Adds showPcbNotes option and gates pcb note texture generation. |
| src/hooks/useManifoldBoardBuilder.ts | Threads showPcbNotes into Manifold board texture creation. |
| src/convert-circuit-json-to-3d-svg.ts | Adds showPcbNotes option and filters pcb note elements when disabled. |
| src/CadViewerManifold.tsx | Adds prop and passes it down into Manifold builder hook. |
| src/CadViewerJscad.tsx | Adds prop and passes it down into JscadBoardTextures. |
| README.md | Documents the new showPcbNotes option/prop. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - `backgroundColor`: Background color in hex format (default: "#ffffff") | ||
| - `padding`: Padding around the board (default: 20) | ||
| - `zoom`: Zoom level (default: 1.5) | ||
| - `showPcbNotes`: Whether to render `pcb_note_*` elements (default: `false`) |
There was a problem hiding this comment.
This documentation line adds showPcbNotes to convert3dCircuitToSvg, but the codebase appears to export convertCircuitJsonTo3dSvg (see src/index.tsx) and tests import that name. Please update the README function name/import path (or add an alias export) so the documented API matches what consumers can actually call, including the new showPcbNotes option.
| import type { AnyCircuitElement, PcbBoard } from "circuit-json" | ||
| import { JSDOM } from "jsdom" | ||
| import { createCombinedBoardTextures } from "../src/textures" | ||
| import { applyJsdomShim } from "../src/utils/jsdom-shim" |
There was a problem hiding this comment.
In this repo's other Bun tests, imports of internal TS modules include the .ts extension (e.g. ../src/utils/jsdom-shim.ts). This test uses an extensionless import, which can break under stricter ESM resolution/tooling and is inconsistent with the existing test suite style. Align the import with the other tests for consistency.
| import { applyJsdomShim } from "../src/utils/jsdom-shim" | |
| import { applyJsdomShim } from "../src/utils/jsdom-shim.ts" |
| const filteredCircuitJson = showPcbNotes | ||
| ? circuitJson | ||
| : circuitJson.filter( | ||
| (element) => !(element.type as string).startsWith("pcb_note_"), | ||
| ) | ||
|
|
There was a problem hiding this comment.
convertCircuitJsonTo3dSvg now accepts showPcbNotes, but enabling it doesn't actually render pcb_note_* elements in the SVG output (the code only filters them out when false). This makes the new option effectively a no-op for SVG export and doesn't match the stated behavior. Consider either implementing note rendering in the SVG scene when showPcbNotes is true, or removing/renaming the option from this converter to avoid misleading API surface.
seveibar
left a comment
seveibar
left a comment
There was a problem hiding this comment.
circuit-to-canvas should have this option, the number of hacks in this pr isn't good.
This pull request introduces support for optionally rendering
pcb_note_*elements (such as annotation lines or notes) in 3D PCB visualizations, controlled by a newshowPcbNotesoption. By default, these note elements are hidden unless explicitly enabled. The change is integrated across the rendering pipeline, React components, and documentation, and is validated by new tests.Default (false)
true
https://linear.app/tscircuit/issue/TSC-407/pcb-notes-shouldnt-show-in-3d-view-3d-snapshot-from-circuit-json-to
Feature: PCB Notes Visibility Control
showPcbNotesoption to theconvertCircuitJsonTo3dSvgfunction and related APIs, allowing users to toggle the rendering ofpcb_note_*elements in SVG/3D outputs. This option defaults tofalseto preserve existing behavior. [1] [2] [3] [4]CadViewerJscad,CadViewerManifold, andJscadBoardTextures) and hooks (useManifoldBoardBuilder) to accept and propagate theshowPcbNotesflag throughout the rendering stack. [1] [2] [3] [4] [5] [6] [7] [8]showPcbNotesflag, ensuring that note elements are only rendered in board textures when this option is enabled. [1] [2]Documentation & Storybook
README.mdand Storybook stories to document the newshowPcbNotesprop and demonstrate its usage and default behavior. [1] [2] [3]Testing
pcb_note_*elements are hidden by default in board textures and only rendered whenshowPcbNotesis explicitly enabled.Code Quality
JscadBoardTextures, replacingforEachwithfor...ofloops for clarity and consistency.