-
Notifications
You must be signed in to change notification settings - Fork 17
Initial completion of ViaGridBoard to best of ability w current tsc functionality #42
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?
Conversation
…unctionality Key points: -4 corner "pacman" are individually hardcoded while polygon rotation is implemented - Vias are all correctly placed but not named - copperpour is defined but does not appear to render? and outline/ clearance props are broken? - bottom text is silkscreen for now since I don't think copper text exists in tsc
|
@raykholo is attempting to deploy a commit to the tscircuit Team on Vercel. A member of the Team first needs to authorize it. |
| ...rest, | ||
| }) as { | ||
| boardProps: any | ||
| chipProps: Record<string, any> |
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.
is this type casting necessary? (CC @Abse2001 if so we should patch this)
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.
@seveibar lines 14 through 27 are a direct copy from the Arduino Shield in this repo. You said "there's an existing pattern in that tscircuit/common repo" which I interpreted as this so I copied it over directly. I don't use anything like this in the similar "userland" stuff I have been doing, but I defer to you to determine what is ultimately necessary or not for everything to work properly. However, it is important to deal with passing BoardProps and children into the that ViaGridBoard encompasses. So some variation of all this seems necessary.
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.
generally looking good! A couple thoughts
for claims (i'm not sure if you claimed) the full board is required (ready-to-be-used), this is in part how we incentive people to "dive deeper" into fixes
| //radius="2.5mm" | ||
|
|
||
| //width="5mm" | ||
| //height="5mm" |
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.
| //radius="2.5mm" | |
| //width="5mm" | |
| //height="5mm" |
| /> | ||
|
|
||
| {pacmanGridCells.map((cell) => ( | ||
| <chip |
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.
not as clear as it could be:
{["TL", "BL", "BR", "TR"].map(cornerPositionName => {
const cornerPosition = getCornerPosition(cornerPositionName)
return <SemiCircleCorner name={`${cornerPositionName}_CORNER`} pcbX={cornerPosition.x} pcbY={cornerPosition.y} cornerPositionName={cornerPositionName} />
)}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.
severely cleaned this up in latest commits
| /> | ||
| ))} | ||
|
|
||
| <ViaGridPlus pcbX={30} pcbY={25} /> |
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.
every via should have some kind of reference i think (is this in the standard?)
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.
In kicad, vias do not get designators like "via1". they just get assigned to a net they are connected to, typically automatically, as part of routing. In tsc, I don't see how to give vias a name in the docs, but I do see that they have a name option in the type definition. The larger question is how will the end user ultimately refer to these pre-placed vias? Should I scriptologically give them names and the user can then use name.pin1 type syntax to connect a trace to the target via?
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.
I looked into how we would do this in our autorouter and we would just need them to have a property like "netIsAssignable" I think, but I think for debugging purposes they should have deterministic names otherwise it'll be difficult to refer to them consistently in logs etc.
| // { x: 5, y: 60 }, | ||
| // { x: 95, y: 60 }, | ||
| // { x: 95, y: 5 } | ||
| // ] |
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.
CC @ShiboSoftwareDev we should get this working!
lib/ViaGridBoard/pacmanPolygon.ts
Outdated
| { x: -2.5, y: -0.2 }, | ||
|
|
||
| { x: 0.2, y: -0.2 }, | ||
| ] |
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'll want to define an arc function, that should be an example of doing this in this codebase e.g.
buildOutline()
.moveTo(0,0)
.arcTo(25,25, ...)
.lineTo(0, 50)this will allow you to define an arbitrary shape.
I don't know if semi-circles will make it into the spec, we need to see more usage
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.
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.
@seveibar @Abse2001 sorry I'm not clear on exactly what you want me to do here. I found the function in question and the arcTo declaration:
Line 179 in 0873a80
| arcTo( |
Are you asking me to implement this syntax into polygon? or are you simply suggesting I can use this code to generate my list of points as above programmatically?
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.
yep you can use this function to give you proper arcs!
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.
err for more clarity, the function can be used to get a list of points. But just for additional clarity we shouldn't see the XY positions of every point, the function is run at runtime (this is how the other board works)
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.
perfect, love it, implemented and working now!
lib/ViaGridBoard/pacmanPolygon.ts
Outdated
| { x: -0.2, y: -0.2 }, // Back to center/origin - closes the shape | ||
|
|
||
| ] | ||
| */ |
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.
some ghost code here
|
@raykholo use |
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.
this looks good just make sure to address all comments by Seve
Nice work
I got some fun console results when I ran that, possibly due to things we are in the process of fixing.
|
|
@raykholo that resvg error should be fixed in the latest versions of tscircuit, might require a dep update to tscircuit, but resvg-js is now included as a dep inside tscircuit |
…tation of chips now working, non-grid simple array for positioning
|
Ok, I think I got as much as I am able done and IMO nothing is a blocker from being "end user ready!"
Ready for review! |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Cc @techmmanih @Abse2001 we should figure out this 3d snapshot camera and why its wrong (maybe a cli issue i have no idea) |





/claim #40
Key points: