-
Notifications
You must be signed in to change notification settings - Fork 88
KCL: new 'hole' module for mech-eng #8530
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
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
CodSpeed Performance ReportMerging #8530 will not alter performanceComparing Summary
Footnotes
|
7f11d1f
to
4faa15d
Compare
f7cd818
to
df4831d
Compare
4faa15d
to
7fe91b4
Compare
527ee29
to
04d5f29
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.
I noticed that this isn't marked as experimental. Reminder that that's an option.
rust/kcl-lib/std/hole.kcl
Outdated
/// From the hole's parts (bottom, middle, top), cut the hole into the given solid, | ||
/// at each of the given 2D positions on the given face. | ||
/// Basically like function `hole` but it takes multiple 2D positions in `cutsAt`. | ||
export fn holes(@solid, face, holeBottom, holeBody, holeType, cutsAt) { |
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 having both the singular and plural to make KCL read nicely?
I almost don't want to remind anyone because I absolutely hate this feature, but if you add the parameter types and use an array, any single value will be automatically wrapped in an array. Part of the reason we made the automatic coercion to array was to address this exact use case where a function can take either a single thing or an array, without duplication.
That said, if you'd rather be explicit, and avoid all that terrible automatic coercion, I support it, but wonder if the singlular hole() flavored fn is necessary.
|
||
/// End the hole flat. | ||
export fn flat() { | ||
return { drillBitAngle = 179.99deg } |
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 leftover from the engine bug that didn't allow 180deg? I thought it was fixed.
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.
It's actually a logic error, as tan(180) is 0 and somewhere in the math I divide by tan(drillBitAngle), so I kept it as 179.99
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.
Seems strange to me to hardcode such a weird value. It's a form of a tolerance. It means that if anyone uses { drillBitAngle = 180deg }
directly, without going through flat()
, we'll needlessly fail.
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.
Maybe we should move the check into sketchHoleProfile()
.
rust/kcl-lib/std/hole.kcl
Outdated
/// Place the given holes in a line. | ||
/// Basically like function `hole` but cuts multiple holes in a line. | ||
/// Works like linear patterns. | ||
export fn holesLinear(@solid, face, holeBottom, holeBody, holeType, cutAt, instances, distance, axis) { |
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.
Should we add parameter types? They would show up in the docs, and I think they're generally helpful to users.
dc4fd1d
to
b1b8f6b
Compare
b1b8f6b
to
b6f0fbd
Compare
b43b21b
to
9c072c4
Compare
cc21dd8
to
2a69014
Compare
e9d4f89
to
26c0cd9
Compare
26c0cd9
to
24f1554
Compare
holeType, | ||
/// Where to place the cut on the given face of the solid. | ||
/// This controls how far up/down/left/right the hole is placed. | ||
cutAt: [number(Length); 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.
@adamchalmers I know this is still experimental so we don't have to handle that case today, IMO it'd be really convenient to be able to select a sketch point here instead of giving relative coords to the face. One we have sketch v2 this would all integrate nicely with constraints!
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.
Oh, the docs are actually wrong, it's actually absolute position. I'm opening up a PR to fix. #8560
} | ||
} | ||
|
||
// Bodies |
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.
Through holes aren't implemented now, right?
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.
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.
Correct
Add a new
hole
module with countersink, counterbore etc holes. This has been ready for two months or so, but I got distracted by constraint solvers. Clearly the ML team has found workarounds for converting holes in other CAD programs, but we should just add this as an experimental library and let people test it out in prod.