Prevent MSP pairs from connecting across chip #28
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Hi @seveibar, This PR is ready for review. Would appreciate your feedback! |
There was a problem hiding this comment.
getPinSide etc. is unnecessary because there's pin._facingDirection. The function canPinsConnect is fine, but it should not need to compute the side. The code will be simpler just using the facing direction. You may need to compute all the facing directions in preprocessing
| if (dy >= halfH) return "top" | ||
| if (dy <= -halfH) return "bottom" | ||
| return null | ||
| } |
There was a problem hiding this comment.
this is also the wrong way to compute the pin side, there's already a better implementation in this project
|
You may need to create more examples to demonstrate that you've thought through the test cases
|
|
@seveibar I’ve simplified connection validation by rejecting MSP pairs whose pins face opposite edges of the same chip. I’ve also addressed all your comments — could you please review again? |
|
@baeoc bounties require videos in the PR demonstrating the fixed issue, ideally with a before/after |
|
hello @seveibar In the "before" video, you can see the tracing from pin 8 to pin 3 and pin 7, shown in two phases. In the "after" video, the tracing is more accurate — from pin 8 to pin 7 and pin 3, showing the actual connection from capacitor and resistor. |
lib/solvers/MspConnectionPairSolver/getMspConnectionPairsFromPins.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I really dont understand if this is correct or not. I should never have to paste input to test, download the page.tsx and insert so your code is testable, maybe in a separate pr so we can see the before/after. I dont understand the impact of this pr is it actually solves the problem because you’re not forbidding cross-chip connections, you’re just making them less direct
| { | ||
| maxDistance: this.maxMspPairDistance, | ||
| canConnect: (a, b) => | ||
| checkIfMspPairCanConnectDirectly(this.chipMap, a as any, b as any), |
There was a problem hiding this comment.
violates 2 parameter rule https://github.com/tscircuit/handbook/blob/main/guides/code.md#1-two-parameter-rule
| const [pair] = solver.mspConnectionPairs | ||
| const ids = pair.pins.map((p) => p.pinId).sort() | ||
| expect(ids).toEqual(["A", "B"]) | ||
| }) |
There was a problem hiding this comment.
useless, has no valid assertions for the problem we're replicating. Remove
There was a problem hiding this comment.
if anything, this test demonstrates that the issue isn't solved because you're creating a pair across a chip
Ref #25
/claim #25
In the "before" video, you can see the tracing from pin 8 to pin 3 and pin 7, shown in two phases.
In the "after" video, the tracing is more accurate — from pin 8 to pin 7 and pin 3, showing the actual connection from capacitor and resistor.
BEFORE
before_REC.mp4
AFTER
afterREC.mp4