fix: board width and height should be undefined when outline is provided #1614
Conversation
Fixes tscircuit#1516 When a board has an outline property, width and height are now left as undefined instead of being calculated from the outline bounds. This removes ambiguity about whether the board shape is defined by width/height or by the outline. Changes: - Board.doInitialPcbComponentRender: Don't calculate width/height when outline exists - Board.doInitialPcbBoardAutoSize: Skip width/height update when outline is present - Added repro70-board-outline.test.tsx to verify width/height are undefined with outline - Updated board-outline-dimension.test.tsx to expect undefined instead of 16
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
- Removed 'as any' typecast from pcb_board.insert() as requested by maintainer - Updated board-outline-dimension snapshot to reflect new behavior - TypeScript errors will be resolved when circuit-json types are updated to allow undefined for width/height
MustafaMulla29
left a comment
MustafaMulla29
left a comment
There was a problem hiding this comment.
Type check and other workflows are failing
nailoo
left a comment
nailoo
left a comment
There was a problem hiding this comment.
Easy to fix, please update snapshots
|
@MustafaMulla29 @nailoo , the type script error is due to the typescript definition not being defined, the issue was to keep the width and height undefined when using outline property. Since Rishabh requested to not to type cast to any, the problem arises that width and height cannot be undefined as they expect a number. For the bun test failing, I have talked to Rishabh on discord about the same, and am waiting for his reply. Here the issue turned out to be more than updating snapshots, at first I did indeed updated the snapshots but the difference between the snapshots are causing the error. After implementing the change to set width/height as undefined when outline is present, I discovered breaking issues throughout the codebase: Problems found:
The Issue: this particular file is causing the snapshot issue: tests/features/pcb-pack-layout/pcb-pack-cad-component-rotation.test.tsx 
this screenshot, shows it properly, before the height and width being undefined the snapshot is the left image, after fixing the issue mentioned in #1516 in the Boards.ts script the updated snapshot is on the right. Please note that my code is not breaking the elements, as in this code: tests/repros/repro62-xiao-board-rp2040-pcb-packing.test.tsx proof : only the two mentioned code files uses "xiao board rp2040 pcb packing", that is what I am getting when the bun test fails for merging. So if any of you guys can help me with these two issues, it would be very much appreciated. |
|
need to also add the shape property as the original comment stated |
let me add that, in that case |
…used rendering and testing issues with custom board outlines. By explicitly tracking board geometry type, downstream tools can correctly handle both rectangular boards (with width/height) and outlined boards (with custom point arrays) without ambiguity.
Added shape property to PCBBoard type to distinguish between rectangular and outlined boards, ensuring proper handling of board geometry throughout the rendering pipeline.
Changes Made:
Type Definition Enhancement (circuit-json.d.ts):
Extended PCBBoard interface with optional shape property ("rectangular" | "outlined")
Enables explicit identification of board geometry type in circuit JSON
Board Component Update (Board.ts):
Added shape property assignment during PCB board initialization
Automatically sets shape: "outlined" when outline is provided, otherwise "rectangular"
Ensures board shape metadata is consistently present in generated circuit JSON
3D Snapshot Testing Fix (extend-expect-3d-matcher.ts):
Updated camera positioning logic to handle outlined boards separately from rectangular boards
For outlined boards, calculates dimensions from outline points instead of using width/height
Fixes camera positioning issues when rendering 3D snapshots of custom board shapes
Test Snapshot Update (footprint-library-map3.test.tsx):
Updated test expectation to include shape: "rectangular" in snapshot
Ensures test compatibility with new board shape property
|
Hi! While implementing the shape property for outlined boards, I ran into a TypeScript compilation error. The issue is that the PCBBoard interface in the circuit-json package currently requires width and height to be numbers. However, when a board uses a custom outline, the width and height properties shouldn't be present at all - the board dimensions come from the outline points instead. Right now, I'm trying to conditionally set width and height to undefined when an outline is present, but TypeScript is rejecting this because it expects those fields to always be numbers, not undefined. Could you update the circuit-json package to make the width and height properties optional on the PCBBoard interface? This would allow rectangular boards to specify width and height normally, while outlined boards can omit these properties and rely solely on the outline. This is currently blocking the PR because the type-check tests are failing, which is in turn causing build test failure |
|
This PR has been automatically marked as stale because it has had no recent activity. It will be closed if no further activity occurs. |
- Upgraded circuit-json to 0.0.303 (includes optional width/height and shape property) - Added shape property to Board: "rect" for rectangular, "polygon" for outlined boards - Fixed 22 TypeScript errors by adding proper undefined checks for width/height - Updated get-board-polygon.ts, getSimpleRouteJsonFromCircuitJson.ts, is-route-outside-board.ts - Updated test fixtures and snapshots to handle optional width/height - Board info silkscreen now skips boards without width/height
Co-authored-by: Rishabh Gupta <38923768+imrishabh18@users.noreply.github.com>
Co-authored-by: Rishabh Gupta <38923768+imrishabh18@users.noreply.github.com>
| if (!pcbBoard) return | ||
|
|
||
| // Only add board information for rectangular boards with width/height | ||
| if (!pcbBoard.width || !pcbBoard.height) return |
|
this issue is valid but i don't think it's important enough where we can risk the upstream breakage w.r.t. undefined width and height. Reduce scope to just add the |
In that case, should I also revert the changes made in this pr: tscircuit/circuit-json#339 by opening another pr ? @seveibar |
|
This PR has been automatically marked as stale because it has had no recent activity. It will be closed if no further activity occurs. |
… into reproduce-issue-1516
|
@Enity300 no it's not necessary to revert the changes, i'm saying that we can leave tscircuit/core out-of-spec. But you can add |
Understood, I have not reverted any changes made to circuit-json, but have only updated boards.ts for shape property and have reduced the scope of undefined for now @seveibar |
5 participants
Fixes #1516
Issue: When outline property is opted, previously width and height were also being updated to what the outline bounds tells it. This was needed to be fixed to not cause any ambiguity for the user in the future.
Fix: When a board has an outline property, width and height are now left as undefined instead of being calculated from the outline bounds. This removes ambiguity about whether the board shape is defined by width/height or by the outline.
Changes: