-
Notifications
You must be signed in to change notification settings - Fork 88
Fix schpinspacing width #1578
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fix schpinspacing width #1578
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
hey @seveibar can i get a review on this, i reproduced this issue and then tried the fix. Please let me know if this is correct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no point in requesting a review with failing tests
also rename your test appropriately for the directory you put it in
|
also you need snapshot tests |
|
@RaghavArora14 request your assigned maintainer for review. Work with them directly, join discord if needed https://tscircuit.com/join https://docs.google.com/spreadsheets/d/1BU96V97WA6Xv9WyjIyN1qmdWvoJgLEir8mdqz--4B_o/edit?usp=sharing |
|
okay thanks, will ask the maintainer soon after fixing this up |
- Changed width/height calculation to use fixed MIN_PADDING (0.4) instead of schPinSpacing * 2 - This ensures box dimensions only change based on pin positions, not pin spacing - Added tests to verify width/height remain constant when schPinSpacing changes - Updated snapshots for affected tests Fixes tscircuit#1576
ce97b6f to
f81b8b0
Compare
|
hey @ShiboSoftwareDev can i get a review on this please, all the tests are now passing, have reproduced the issue and added test snapshots too |
|
also had created a seperate branch to reproduce the issue before fixing it and that was causing some issues with the format check so had to force push sorry if that causes any issues, lmk how i can fix those |
|
combine your repros into a single circuit in a single file |
Removed comments explaining fixed padding for schWidth and schHeight.
|
okay resolved the comments issue, will add them into a single repro file and ask for a review is that cool? |
57d814a to
c336810
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is repro3 messed up?
Because the fix changed how box dimensions are calculated. The repro3 test uses schPinSpacing={0.75} which is larger than the default 0.2. |
|
i think repro03 is showing that we introduced a bug no? Shouldn't it have a wider box because it has pin labels? |
Repro3 doesn't have pin labels, so the box width is correctly calculated based on pin positions (sideLengths) + fixed padding. The box is narrower now because the old code incorrectly added schPinSpacing to the padding - the fix ensures padding stays constant at 0.4mm regardless of pin spacing. If users need a wider box, they can explicitly set schWidth prop or the code already handles pin labels by adding extra width when labels are present |
|
in repro3 i could add schWidth={3} or whatever width looks good. This overrides the automatic calculation and gives full control over the box size. Alternatively, i could increase DEFAULT_SCHEMATIC_BOX_PADDING_MM from 0.4 to something like 0.6 or 0.8 if you think the boxes look too cramped overall - that would affect all components without explicit widths. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you broke more snapshots, your logic doesn't account for pin labels
|
The core issue was that the original code used params.schPinSpacing * 2 for padding, which made box dimensions depend on schPinSpacing (the bug). My fix changed this to a fixed DEFAULT_SCHEMATIC_BOX_PADDING_MM (0.4mm), which correctly makes padding independent of schPinSpacing. However, this created a problem: repro3 has schPinSpacing={0.75}, so it previously got 1.5mm of padding and now only gets 0.4mm, making it appear too narrow. I tried several approaches to fix this: (1) adding a minimum width check of 1.0mm for chips with left/right pins - this broke 28 snapshots because it overrode the pin label width calculations, (2) calculating default label width based on pin numbers when no explicit pinLabels are defined - this broke the original behavior where chips without pinLabels got no extra width, and (3) increasing DEFAULT_SCHEMATIC_BOX_PADDING_MM from 0.4mm to 0.6mm - this broke 45 tests. The final trade-off is: the bug is fixed (schPinSpacing no longer affects dimensions), but chips with large schPinSpacing values like repro3 will appear narrower than before since they no longer get proportionally larger padding. What do you think - should we accept this trade-off, or do you have any suggestions on how to provide adequate width for repro3 without breaking the other tests? |
3684dff to
7b1179e
Compare
e298466 to
7b1179e
Compare
- Check if left/right pins actually have labels (not just if pinLabels prop exists) - Apply 0.5mm minimum width when labels are present on left/right sides - Fixes repro3 narrow box issue while keeping schPinSpacing independent


/Fixes #1576
/claim #1576
Reproduction
Before Fix:


Small spacing - width: 0.4 height: 1
Large spacing - width: 1.6 height: 4
Test FAILED
Fix
Changed width/height calculation in getAllDimensionsForSchematicBox.ts to use fixed MIN_PADDING (0.4) instead of schPinSpacing * 2.

After Fix:
Result
After fix, box width stays constant at 0.4 regardless of schPinSpacing value.