Fix horizontal fallback bounds for rail net labels - #1007
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
MustafaMulla29
left a comment
MustafaMulla29
left a comment
There was a problem hiding this comment.
Not sure what this fixes, can you show me before and after image from core if there is a repro.
|
@MustafaMulla29 repro #1005 Before 
After |
|
I don't think this is an improvement |
|
@MustafaMulla29 updated |
|
Gnd symbol is not solving, try to make it solve. |
| if (isSingleVerticalOrientation) { | ||
| const isGroundLabel = | ||
| this.inputProblem.netConnections.find((nc) => nc.netId === label.netId) | ||
| ?.isGround || label.netId === "GND" |
There was a problem hiding this comment.
isnt this a hack to check just for "GND"
| export class NetLabelPlacementSolver extends BaseSolver { | ||
| inputProblem: InputProblem | ||
| inputTraceMap: Record<MspConnectionPairId, SolvedTracePath> | ||
| normalizeHorizontalFallbackBoundsOnCompletion: boolean |
There was a problem hiding this comment.
Improve naming, there is no concept of fallbacks in this codebase i think
There was a problem hiding this comment.
Why are the netlabels suddenly becoming wider?
There was a problem hiding this comment.
I’ve removed that behavior—labels now retain their original dimensions
There was a problem hiding this comment.
@techmannih are these power labels? or normal netlabels?
There was a problem hiding this comment.
These are power net labels for AUDIO_VRA1 and AUDIO_VRA2 in this snapshot. They point upward because the input restricts both to y+; the PR moves them clear of GND.
There was a problem hiding this comment.
If there are not power/ground labels then they should be solved as inline labels.
There was a problem hiding this comment.
@MustafaMulla29 I rechecked core: its naming heuristic classifies AUDIO_VRA1 and AUDIO_VRA2 as power nets (is_power: true), so inline labels are disabled.
There was a problem hiding this comment.
And also can you figure out what is this label, why wasnt it solving to inline label before, seems like a regression(it might create some regressions in core).
There was a problem hiding this comment.
This is P1V8, which core already enables for inline labels. Previously, a VRA2 overlap at the U3 endpoint blocked inline conversion for both endpoints. This PR clears that overlap, so both now convert as intended.
|
Thank you for your contribution! 🎉 PR Rating: ⭐⭐⭐ Track your contributions and see the leaderboard at: tscircuit Contribution Tracker |
3 participants
Summary
Testing