fix: reject net-label candidates whose connector crosses another net (40 → 19 crossings) - #719
fix: reject net-label candidates whose connector crosses another net (40 → 19 crossings)#719DPS0340 wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Follow-up on the remaining 19, since I offered to look at them — I investigated and decided not to send a fix. Posting the findings so the next person doesn't repeat the attempt. After this PR, the connector class is fully gone. Re-attributing all 19 remaining crossings by trace id: So every remaining crossing is between two ordinary routed traces, and they're present from the routing stage — not introduced later: Why they exist: What I tried and rejected. Then I measured whether it did anything:
The tiebreaker almost never fires, because candidates that differ in crossings essentially always differ in length first. So the change added a per-candidate O(n·m) scan for no measurable benefit. I reverted it rather than ship something that looks like an improvement and isn't. What would actually be needed: making crossings a real cost the router optimises against, which means accepting longer routes to avoid them — a direct reversal of the current design decision, and a much larger change than this PR. That's a judgement call about what the schematic should look like, so I'd rather not make it unilaterally. Happy to attempt it if you think it's the right direction. |
|
One more datapoint while probing what this PR does and doesn't cover. I measured a defect class nobody has been counting: different-net traces drawn collinearly on top of each other — visually a single wire where there are two nets. Across the corpus there is exactly 1, on Same This PR does not fix it — I checked rather than assuming, since side B is an The reason is that #719 rejects candidates whose connector crosses another net, and this connector doesn't cross Recording it here rather than opening another PR: I have a lot open in this repo already, and I'd rather land some of what's there before adding more. If a maintainer wants it, the natural fix is extending the connector validation in (Corrected an earlier read while investigating: I initially attributed the overlap to |
ad5f1db to
6bb1f82
Compare
|
@mohan-bee @MustafaMulla29 @ShiboSoftwareDev — I had 13 PRs open here, which is too many to ask anyone to sort through. I've cut that to 11 and verified the merge order so it doesn't cost you a decision. What I withdrew, after measuring:
Merge order. I merged all 11 onto I rebased #719 onto #718 and #723 onto #719 so the stack is now linear — previously all three added Three of the later merges conflict only in regenerated Result of merging all 11:
Those are exactly the "if a change legitimately improves a board, lower the number in the same PR" case the harness documents. I've left them un-lowered because which PR should own the reduction depends on what you merge — happy to fold the updates into whichever lands last. If you only have time for two: #718 then #719. That's the measurement plus the fix it justifies, 2 source files, and it removes 21 of the 40 crossings on its own. |
Adds a helper that counts places where traces on different nets properly cross, and a test that pins the current count for every imported bug report. These are current values, not targets — several of these boards are open bugs. The point is that a routing or cleanup change which makes a board worse now fails a named assertion instead of disappearing into a regenerated SVG snapshot. Verified the guard bites: changing the expected count for bug-report-20260706T213649Z from 11 to 10 fails with 'Expected: 10, Received: 11'.
tscircuit#727 (TraceOverlapShiftSolver) fixed the RP2040 USB-C overlap and moved bug-report-20260707T092615Z from 2 different-net crossings to 3. Measured either side of that merge: ca7e80c (before) TOTAL 40, this board 2 be20aa7 (tscircuit#727) TOTAL 41, this board 3 Pinned at the current value with a comment recording the change, and reported it on tscircuit#727 so the trade is visible rather than absorbed.
getCandidateStatus already checks the candidate's connector against chips and other net labels, but never against existing traces. A candidate could therefore be accepted while its connector cut straight across a different net, which reads as a short in the rendered schematic. Reuse the existing tracePathCrossesAnyTrace helper, filtered to traces on other nets, and reject those candidates so the search moves to the next one. Measured across every imported bug report: 40 -> 19 different-net crossings, 7 boards improved, none regressed. Worst board goes 11 -> 4, and two boards reach zero.
6bb1f82 to
f788a17
Compare
|
@seveibar — your #707 note ("synthetically constructs input, you have to use the FULL solver") was a useful bar, so I audited my other 10 PRs against it rather than wait to be told twice. One correction and a summary below. Audit result: none of the others construct input. No PR here adds a synthetic asset; every repro imports an unmodified bug report and drives One does not change any board, and says so. Sorted by review cost, if it helps:
#719 is the one with the largest measured effect — different-net crossings across all imported bug reports go from 41 on #717's file count is misleading: the fix is 19 lines in All 10 are rebased onto current |
|
Correction to my audit comment above: I listed #721 as "changes no board today — the easiest one to decline". That was based on the wrong measurement. The final overlap count is unchanged (43 → 43), which is what I measured. But the skipped-collision keys on The rest of that table stands. #718/#719 are still the pair with the largest measured effect (41 → 20 crossings). |
1 participant
Builds on #718 (the measurement) by fixing the largest source of what it measures. #718 should merge first — this PR updates the numbers that one pins.
Root cause
AvailableNetOrientationSolverplaces net labels and, for each one, appends a connector trace joining the label to its net.getCandidateStatusvalidates a candidate's connector against chips:…and against other net labels. It never checks the connector against existing traces. So a candidate is accepted whose connector cuts straight across a different net — which reads as a short in the rendered schematic.
I traced this by counting crossings after each pipeline stage on the worst board:
The jump is where
availableNetOrientationSolver's connectors enter. Attributing the 11 crossings by trace id: 7 involve a connector namedavailable-net-orientation-*, 4 are between pre-existing traces.Fix
Reject candidates whose connector properly crosses a trace on a different net, so the search continues to the next candidate.
This reuses
tracePathCrossesAnyTrace— already ingeometry.tsfor this exact purpose — filtered to other-net traces. Same-net traces are excluded because a connector legitimately meets the trace it attaches to. The rejection reuses the existingtrace-collisionstatus, so no new candidate state is introduced.Result
Measured across all 19 imported bug reports:
Total 40 → 19 (−52.5%). 7 boards improved, 0 regressed. Two boards reach zero.
Verification
tsc --noEmitclean, biome clean.repro51-overlap-junction-crossingis worth calling out. Its snapshot changed, but its behavioural assertion —tracesHavePositiveLengthOverlap(gndTrace, vccTrace)is false for every GND/VCC pair — still passes. The guarantee that test exists to protect is intact; only the rendering moved.Diff: 1 source file (+~20 lines, no new helper) plus the test/snapshot updates.
Remaining 19 crossings are mostly between pre-existing traces rather than connectors, so they'd need a different fix — happy to look at those next if this lands.