-
Notifications
You must be signed in to change notification settings - Fork 90
Sketch 2.0 WIP, unknown branch, PR'd for commets #8935
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
This reverts commit 1cfb766321e5f81864ed20575715ab8ea70283e8.
This reverts commit 900fa49d53715f7106a017869160b9a6b43732dc.
See rust-lang/rust issue 37748. I'm avoiding using a real link to stop making backlinks on the Rust issue.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
CodSpeed Performance ReportMerging #8935 will not alter performanceComparing Summary
Footnotes
|
Use delete_objects() instead.
|
|
||
| // Call newSketch API | ||
| try { | ||
| const project = theProject.current | ||
| if (!project) { | ||
| console.warn('No project available for newSketch call') | ||
| } else { | ||
| // Construct SketchArgs based on the result | ||
| let sketchArgs: SketchArgs | ||
|
|
||
| // Determine the plane type from the result | ||
| if (result.type === 'defaultPlane') { | ||
| sketchArgs = { | ||
| on: { default: toPlaneName(result.plane) }, | ||
| } | ||
| } else { | ||
| sketchArgs = { on: { default: 'xy' } } | ||
| } | ||
|
|
||
| await rustContext.hackSetProgram( | ||
| kclManager.ast, | ||
| await jsAppSettings() | ||
| ) | ||
| const newSketchResult = await rustContext.newSketch( | ||
| 0, // projectId - using 0 as placeholder | ||
| 0, // fileId - using 0 as placeholder | ||
| 0, // version - using 0 as placeholder | ||
| sketchArgs, | ||
| { settings: { modeling: { base_unit: defaultUnit.current } } } | ||
| ) | ||
| codeManager.updateCodeEditor(newSketchResult.kclSource.text) | ||
| } | ||
| } catch (error) { | ||
| console.error('Error calling newSketch:', error) | ||
| } | ||
|
|
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.
Long term because we want rust to be in charge of code-mods, than the code state needs to live in rust, and as the UI fires code-mods in the rust side, we'll update the editor or what have you, so there needs to be a lifecycle api for updating files either way.
But since the sketch-revamp is first time that we're implementing code-mods in rust, there needs to be a handover to tell rust what the current AST is, for it to continue to modify from there, and that's what hackSetProgram is.
Also newSketch has a number of place holder params we're not using yet (i.e. all set to 0)
| export function forceSuffix(unitStr: string): NumericSuffix { | ||
| switch (unitStr) { | ||
| case 'None': | ||
| case 'Count': | ||
| case 'Length': | ||
| case 'Angle': | ||
| case 'Mm': | ||
| case 'Cm': | ||
| case 'M': | ||
| case 'Inch': | ||
| case 'Ft': | ||
| case 'Yd': | ||
| case 'Deg': | ||
| case 'Rad': | ||
| case 'Unknown': | ||
| return unitStr | ||
| default: | ||
| return 'Mm' // default to Mm if unknown | ||
| } | ||
| } | ||
|
|
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.
Added this to make TS happy.
| }), | ||
| }, | ||
| actors: { | ||
| modAndSolveFirstPoint: fromPromise( |
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 name is a little misleading. It is mentioning modAndsolveFirstPoint.
The input to this function is a pointData and it creates a segment with the same position for start and end? Is a point a segment with the same positions?
Also why is this called firstPoint? What does first mean?
| coincident: { | ||
| actions: async ({ self, context }) => { | ||
| // TODO this is not how coincident should operate long term, as it should be an equipable tool | ||
| const result = await context.rustContext.addConstraint( |
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 supposed to be fire and forget?
No description provided.