From adc34d9a966c0a519214e4e4b82aa3e282ee5eaf Mon Sep 17 00:00:00 2001 From: Pierre Jacquier Date: Tue, 22 Jul 2025 13:17:26 -0400 Subject: [PATCH 01/12] WIP: Offset Plane point-and-click for sweep faces Edit flows not working yet Will eventually close #7555 --- e2e/playwright/feature-tree-pane.spec.ts | 8 +- e2e/playwright/point-click.spec.ts | 8 +- src/lang/modifyAst/faces.ts | 106 ++++++++++++++++++ .../modelingCommandConfig.ts | 7 +- src/lib/operations.ts | 45 ++++---- src/machines/modelingMachine.ts | 78 ++----------- 6 files changed, 155 insertions(+), 97 deletions(-) diff --git a/e2e/playwright/feature-tree-pane.spec.ts b/e2e/playwright/feature-tree-pane.spec.ts index 2e1d75c90c4..9d62b1d1839 100644 --- a/e2e/playwright/feature-tree-pane.spec.ts +++ b/e2e/playwright/feature-tree-pane.spec.ts @@ -385,13 +385,13 @@ test.describe('Feature Tree pane', () => { currentArgValue: initialInput, headerArguments: { Plane: '1 plane', - Distance: initialInput, + Offset: initialInput, }, - highlightedHeaderArg: 'distance', + highlightedHeaderArg: 'offset', }) }) - await test.step('Edit the distance argument and submit', async () => { + await test.step('Edit the offset argument and submit', async () => { await expect(cmdBar.currentArgumentInput).toBeVisible() await cmdBar.currentArgumentInput.locator('.cm-content').fill(newInput) await cmdBar.progressCmdBar() @@ -400,7 +400,7 @@ test.describe('Feature Tree pane', () => { headerArguments: { Plane: '1 plane', // We show the calculated value in the argument summary - Distance: '15', + Offset: '15', }, commandName: 'Offset plane', }) diff --git a/e2e/playwright/point-click.spec.ts b/e2e/playwright/point-click.spec.ts index af42fc0751a..91b8e17884b 100644 --- a/e2e/playwright/point-click.spec.ts +++ b/e2e/playwright/point-click.spec.ts @@ -1104,17 +1104,17 @@ openSketch = startSketchOn(XY) stage: 'arguments', currentArgKey: 'plane', currentArgValue: '', - headerArguments: { Plane: '', Distance: '' }, + headerArguments: { Plane: '', Offset: '' }, highlightedHeaderArg: 'plane', commandName: 'Offset plane', }) await clickOnXzPlane() await cmdBar.expectState({ stage: 'arguments', - currentArgKey: 'distance', + currentArgKey: 'offset', currentArgValue: '5', - headerArguments: { Plane: '1 plane', Distance: '' }, - highlightedHeaderArg: 'distance', + headerArguments: { Plane: '1 plane', Offset: '' }, + highlightedHeaderArg: 'offset', commandName: 'Offset plane', }) await cmdBar.progressCmdBar() diff --git a/src/lang/modifyAst/faces.ts b/src/lang/modifyAst/faces.ts index f973287be11..13d2d76f54c 100644 --- a/src/lang/modifyAst/faces.ts +++ b/src/lang/modifyAst/faces.ts @@ -12,6 +12,7 @@ import { setCallInAst, } from '@src/lang/modifyAst' import { + getSelectedPlaneAsNode, getVariableExprsFromSelection, retrieveSelectionsFromOpArg, valueOrVariable, @@ -19,8 +20,10 @@ import { import type { Artifact, ArtifactGraph, + Expr, PathToNode, Program, + VariableMap, } from '@src/lang/wasm' import type { KclCommandValue } from '@src/lib/commandTypes' import type { Selection, Selections } from '@src/lib/selections' @@ -121,6 +124,109 @@ export function addShell({ } } +export function addOffsetPlane({ + ast, + artifactGraph, + variables, + plane, + offset, + nodeToEdit, +}: { + ast: Node + artifactGraph: ArtifactGraph + variables: VariableMap + plane: Selections + offset: KclCommandValue + nodeToEdit?: PathToNode +}): + | { + modifiedAst: Node + pathToNode: PathToNode + } + | Error { + // 1. Clone the ast so we can edit it + const modifiedAst = structuredClone(ast) + + // 2. Prepare unlabeled and labeled arguments + let planeExpr: Expr | undefined + const faceToOffset = plane.graphSelections.find( + (sel) => sel.artifact?.type === 'cap' || sel.artifact?.type === 'wall' + ) + if (faceToOffset) { + const sweep = getSweepArtifactFromSelection(faceToOffset, artifactGraph) + if (err(sweep) || !sweep) { + return new Error('No sweep artifact found corresponding to the selection') + } + const solid: Selections = { + graphSelections: [ + { + artifact: sweep as Artifact, + codeRef: sweep.codeRef, + }, + ], + otherSelections: [], + } + const lastChildLookup = true + const vars = getVariableExprsFromSelection( + solid, + modifiedAst, + nodeToEdit, + lastChildLookup, + artifactGraph + ) + if (err(vars)) { + return vars + } + const solidExpr = createVariableExpressionsArray(vars.exprs) + const facesExprs = getFacesExprsFromSelection( + modifiedAst, + plane, + artifactGraph + ) + const facesExpr = createVariableExpressionsArray(facesExprs) + if (!facesExpr) { + return new Error('No face found in the selection') + } + planeExpr = createCallExpressionStdLibKw('planeOf', solidExpr, [ + createLabeledArg('face', facesExpr), + ]) + } else { + planeExpr = getSelectedPlaneAsNode(plane, variables) + if (!planeExpr) { + return new Error('No plane found in the selection') + } + } + + const call = createCallExpressionStdLibKw('offsetPlane', planeExpr, [ + createLabeledArg('offset', valueOrVariable(offset)), + ]) + + // Insert variables for labeled arguments if provided + if ('variableName' in offset && offset.variableName) { + insertVariableAndOffsetPathToNode(offset, modifiedAst, nodeToEdit) + } + + // 3. If edit, we assign the new function call declaration to the existing node, + // otherwise just push to the end + const pathToNode = setCallInAst({ + ast: modifiedAst, + call, + pathToEdit: nodeToEdit, + pathIfNewPipe: undefined, + variableIfNewDecl: KCL_DEFAULT_CONSTANT_PREFIXES.PLANE, + }) + if (err(pathToNode)) { + return pathToNode + } + + return { + modifiedAst, + pathToNode, + } +} + +// Utilities + function getFacesExprsFromSelection( ast: Node, faces: Selections, diff --git a/src/lib/commandBarConfigs/modelingCommandConfig.ts b/src/lib/commandBarConfigs/modelingCommandConfig.ts index 18f25c1e752..33751e74e72 100644 --- a/src/lib/commandBarConfigs/modelingCommandConfig.ts +++ b/src/lib/commandBarConfigs/modelingCommandConfig.ts @@ -145,7 +145,7 @@ export type ModelingCommandSchema = { // Enables editing workflow nodeToEdit?: PathToNode plane: Selections - distance: KclCommandValue + offset: KclCommandValue } Helix: { // Enables editing workflow @@ -686,12 +686,13 @@ export const modelingMachineCommandConfig: StateMachineCommandSetConfig< }, plane: { inputType: 'selection', - selectionTypes: ['plane'], + selectionTypes: ['plane', 'cap', 'wall'], multiple: false, required: true, skip: true, + hidden: (context) => Boolean(context.argumentsToSubmit.nodeToEdit), }, - distance: { + offset: { inputType: 'kcl', defaultValue: KCL_DEFAULT_LENGTH, required: true, diff --git a/src/lib/operations.ts b/src/lib/operations.ts index 19e68a1184f..552a75f70d4 100644 --- a/src/lib/operations.ts +++ b/src/lib/operations.ts @@ -400,8 +400,8 @@ const prepareToEditShell: PrepareToEditCallback = async ({ operation }) => { // 2. Convert the thickness argument from a string to a KCL expression const thickness = await stringToKclExpression( codeManager.code.slice( - operation.labeledArgs?.['thickness']?.sourceRange[0], - operation.labeledArgs?.['thickness']?.sourceRange[1] + operation.labeledArgs?.thickness?.sourceRange[0], + operation.labeledArgs?.thickness?.sourceRange[1] ) ) if (err(thickness) || 'errors' in thickness) { @@ -429,22 +429,28 @@ const prepareToEditOffsetPlane: PrepareToEditCallback = async ({ name: 'Offset plane', groupId: 'modeling', } - if ( - operation.type !== 'StdLibCall' || - !operation.labeledArgs || - !operation.unlabeledArg || - !('offset' in operation.labeledArgs) || - !operation.labeledArgs.offset - ) { - return baseCommand + if (operation.type !== 'StdLibCall') { + return { reason: 'Wrong operation type' } } + + // 1. Map the plane and faces arguments to plane or face selections // TODO: Implement conversion to arbitrary plane selection // once the Offset Plane command supports it. - const stdPlane = operation.unlabeledArg + if (!operation.unlabeledArg) { + return { reason: `Couldn't retrieve operation arguments` } + } + + console.log('Offset plane operation:', operation) + + // TODO: replace with util function when available const planeName = codeManager.code - .slice(stdPlane.sourceRange[0], stdPlane.sourceRange[1]) + .slice( + operation.unlabeledArg.sourceRange[0], + operation.unlabeledArg.sourceRange[1] + ) .replaceAll(`'`, ``) + // TODO: figure this out for non-default planes and planeOf results if (!isDefaultPlaneStr(planeName)) { // TODO: error handling return baseCommand @@ -465,24 +471,23 @@ const prepareToEditOffsetPlane: PrepareToEditCallback = async ({ ], } - // Convert the distance argument from a string to a KCL expression - const distanceResult = await stringToKclExpression( + // 2. Convert the offset argument from a string to a KCL expression + const offset = await stringToKclExpression( codeManager.code.slice( - operation.labeledArgs.offset.sourceRange[0], - operation.labeledArgs.offset.sourceRange[1] + operation.labeledArgs?.offset?.sourceRange[0], + operation.labeledArgs?.offset?.sourceRange[1] ) ) - - if (err(distanceResult) || 'errors' in distanceResult) { - return baseCommand + if (err(offset) || 'errors' in offset) { + return { reason: "Couldn't retrieve thickness argument" } } // Assemble the default argument values for the Offset Plane command, // with `nodeToEdit` set, which will let the Offset Plane actor know // to edit the node that corresponds to the StdLibCall. const argDefaultValues: ModelingCommandSchema['Offset plane'] = { - distance: distanceResult, plane, + offset, nodeToEdit: pathToNodeFromRustNodePath(operation.nodePath), } diff --git a/src/machines/modelingMachine.ts b/src/machines/modelingMachine.ts index 53ef5ec76b4..4089faa9eee 100644 --- a/src/machines/modelingMachine.ts +++ b/src/machines/modelingMachine.ts @@ -45,7 +45,6 @@ import { import { angleLengthInfo } from '@src/components/Toolbar/angleLengthInfo' import { updateModelingState } from '@src/lang/modelingWorkflows' import { - addOffsetPlane, insertNamedConstant, replaceValueAtNodePath, } from '@src/lang/modifyAst' @@ -87,7 +86,6 @@ import { updatePathToNodesAfterEdit, artifactIsPlaneWithPaths, isCursorInFunctionDefinition, - getSelectedPlaneAsNode, } from '@src/lang/queryAst' import { getFaceCodeRef, @@ -138,7 +136,7 @@ import type { Plane } from '@rust/kcl-lib/bindings/Plane' import type { Point3d } from '@rust/kcl-lib/bindings/ModelingCmd' import { getNodePathFromSourceRange } from '@src/lang/queryAstNodePathUtils' import { letEngineAnimateAndSyncCamAfter } from '@src/clientSideScene/CameraControls' -import { addShell } from '@src/lang/modifyAst/faces' +import { addShell, addOffsetPlane } from '@src/lang/modifyAst/faces' import { intersectInfo } from '@src/components/Toolbar/Intersect' import { addHelix } from '@src/lang/modifyAst/geometry' @@ -2556,72 +2554,20 @@ export const modelingMachine = setup({ return Promise.reject(new Error(NO_INPUT_PROVIDED_MESSAGE)) } - // Extract inputs - const ast = kclManager.ast - const { plane: selection, distance, nodeToEdit } = input - - let insertIndex: number | undefined = undefined - let planeName: string | undefined = undefined - - // If this is an edit flow, first we're going to remove the old plane - if (nodeToEdit && typeof nodeToEdit[1][0] === 'number') { - // Extract the plane name from the node to edit - const planeNameNode = getNodeFromPath( - ast, - nodeToEdit, - 'VariableDeclaration' - ) - if (err(planeNameNode)) { - console.error('Error extracting plane name') - } else { - planeName = planeNameNode.node.declaration.id.name - } - - const newBody = [...ast.body] - newBody.splice(nodeToEdit[1][0], 1) - ast.body = newBody - insertIndex = nodeToEdit[1][0] - } - - const selectedPlane = getSelectedPlaneAsNode( - selection, - kclManager.variables - ) - if (!selectedPlane) { - return trap('No plane selected') - } - - // Get the default plane name from the selection - const offsetPlaneResult = addOffsetPlane({ - node: ast, - plane: selectedPlane, - offset: - 'variableName' in distance - ? distance.variableIdentifierAst - : distance.valueAst, - insertIndex, - planeName, + const { ast, artifactGraph, variables } = kclManager + const astResult = addOffsetPlane({ + ...input, + ast, + artifactGraph, + variables, }) - - // Insert the distance variable if the user has provided a variable name - if ( - 'variableName' in distance && - distance.variableName && - typeof offsetPlaneResult.pathToNode[1][0] === 'number' - ) { - const insertIndex = Math.min( - offsetPlaneResult.pathToNode[1][0], - distance.insertIndex - ) - const newBody = [...offsetPlaneResult.modifiedAst.body] - newBody.splice(insertIndex, 0, distance.variableDeclarationAst) - offsetPlaneResult.modifiedAst.body = newBody - // Since we inserted a new variable, we need to update the path to the extrude argument - offsetPlaneResult.pathToNode[1][0]++ + if (err(astResult)) { + return Promise.reject(astResult) } + const { modifiedAst, pathToNode } = astResult await updateModelingState( - offsetPlaneResult.modifiedAst, + modifiedAst, EXECUTION_TYPE_REAL, { kclManager, @@ -2629,7 +2575,7 @@ export const modelingMachine = setup({ codeManager, }, { - focusPath: [offsetPlaneResult.pathToNode], + focusPath: [pathToNode], } ) } From d82569a9640d245d517ccbe187ea8656e6e0ae23 Mon Sep 17 00:00:00 2001 From: Pierre Jacquier Date: Tue, 22 Jul 2025 13:18:41 -0400 Subject: [PATCH 02/12] Typo --- e2e/playwright/feature-tree-pane.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/e2e/playwright/feature-tree-pane.spec.ts b/e2e/playwright/feature-tree-pane.spec.ts index 9d62b1d1839..111844c193f 100644 --- a/e2e/playwright/feature-tree-pane.spec.ts +++ b/e2e/playwright/feature-tree-pane.spec.ts @@ -381,7 +381,7 @@ test.describe('Feature Tree pane', () => { await cmdBar.expectState({ commandName: 'Offset plane', stage: 'arguments', - currentArgKey: 'distance', + currentArgKey: 'offset', currentArgValue: initialInput, headerArguments: { Plane: '1 plane', From 7421d57a62bc7a34bb228fa6879bb4059172e576 Mon Sep 17 00:00:00 2001 From: Pierre Jacquier Date: Tue, 22 Jul 2025 13:42:18 -0400 Subject: [PATCH 03/12] Cont fixing of tests --- e2e/playwright/feature-tree-pane.spec.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/e2e/playwright/feature-tree-pane.spec.ts b/e2e/playwright/feature-tree-pane.spec.ts index 111844c193f..029cc6b0f72 100644 --- a/e2e/playwright/feature-tree-pane.spec.ts +++ b/e2e/playwright/feature-tree-pane.spec.ts @@ -384,7 +384,6 @@ test.describe('Feature Tree pane', () => { currentArgKey: 'offset', currentArgValue: initialInput, headerArguments: { - Plane: '1 plane', Offset: initialInput, }, highlightedHeaderArg: 'offset', @@ -398,8 +397,6 @@ test.describe('Feature Tree pane', () => { await cmdBar.expectState({ stage: 'review', headerArguments: { - Plane: '1 plane', - // We show the calculated value in the argument summary Offset: '15', }, commandName: 'Offset plane', From d90ac82f29ebb2effad1f9f54632777f66f4f9b7 Mon Sep 17 00:00:00 2001 From: Pierre Jacquier Date: Tue, 22 Jul 2025 13:54:59 -0400 Subject: [PATCH 04/12] Rm px coordinate from test --- e2e/playwright/point-click.spec.ts | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/e2e/playwright/point-click.spec.ts b/e2e/playwright/point-click.spec.ts index 91b8e17884b..3884d582e9b 100644 --- a/e2e/playwright/point-click.spec.ts +++ b/e2e/playwright/point-click.spec.ts @@ -1085,16 +1085,11 @@ openSketch = startSketchOn(XY) }) => { // One dumb hardcoded screen pixel value const testPoint = { x: 700, y: 200 } - // TODO: replace the testPoint selection with a feature tree click once that's supported #7544 - const [clickOnXzPlane] = scene.makeMouseHelpers(testPoint.x, testPoint.y) const expectedOutput = `plane001 = offsetPlane(XZ, offset = 5)` await homePage.goToModelingScene() await scene.settled(cmdBar) - await test.step(`Look for the blue of the XZ plane`, async () => { - //await scene.expectPixelColor([50, 51, 96], testPoint, 15) // FIXME - }) await test.step(`Go through the command bar flow`, async () => { await toolbar.offsetPlaneButton.click() await expect @@ -1108,7 +1103,8 @@ openSketch = startSketchOn(XY) highlightedHeaderArg: 'plane', commandName: 'Offset plane', }) - await clickOnXzPlane() + const xz = await toolbar.getFeatureTreeOperation('Front plane', 0) + await xz.click() await cmdBar.expectState({ stage: 'arguments', currentArgKey: 'offset', @@ -1120,14 +1116,13 @@ openSketch = startSketchOn(XY) await cmdBar.progressCmdBar() }) - await test.step(`Confirm code is added to the editor, scene has changed`, async () => { + await test.step(`Confirm code is added to the editor`, async () => { await editor.expectEditor.toContain(expectedOutput) await editor.expectState({ diagnostics: [], activeLines: [expectedOutput], highlightedCode: '', }) - await scene.expectPixelColor([74, 74, 74], testPoint, 15) }) await test.step('Delete offset plane via feature tree selection', async () => { @@ -1138,7 +1133,6 @@ openSketch = startSketchOn(XY) ) await operationButton.click({ button: 'left' }) await page.keyboard.press('Delete') - //await scene.expectPixelColor([50, 51, 96], testPoint, 15) // FIXME }) }) From 4526094fd2cc45c36f86d56fdfbe4c066fb156da Mon Sep 17 00:00:00 2001 From: Pierre Jacquier Date: Tue, 22 Jul 2025 14:18:52 -0400 Subject: [PATCH 05/12] Clean up faces and unit tests for default planes --- e2e/playwright/point-click.spec.ts | 3 - src/lang/modifyAst.ts | 57 +------------ src/lang/modifyAst/faces.test.ts | 61 ++++++++++++- src/lang/modifyAst/faces.ts | 133 ++++++++++++++--------------- 4 files changed, 125 insertions(+), 129 deletions(-) diff --git a/e2e/playwright/point-click.spec.ts b/e2e/playwright/point-click.spec.ts index 3884d582e9b..641eb6f9d8c 100644 --- a/e2e/playwright/point-click.spec.ts +++ b/e2e/playwright/point-click.spec.ts @@ -1083,10 +1083,7 @@ openSketch = startSketchOn(XY) toolbar, cmdBar, }) => { - // One dumb hardcoded screen pixel value - const testPoint = { x: 700, y: 200 } const expectedOutput = `plane001 = offsetPlane(XZ, offset = 5)` - await homePage.goToModelingScene() await scene.settled(cmdBar) diff --git a/src/lang/modifyAst.ts b/src/lang/modifyAst.ts index 9965618476e..612804f7d44 100644 --- a/src/lang/modifyAst.ts +++ b/src/lang/modifyAst.ts @@ -1,5 +1,4 @@ import type { BodyItem } from '@rust/kcl-lib/bindings/BodyItem' -import type { Name } from '@rust/kcl-lib/bindings/Name' import type { Node } from '@rust/kcl-lib/bindings/Node' import type { NonCodeMeta } from '@rust/kcl-lib/bindings/NonCodeMeta' @@ -27,11 +26,7 @@ import { isNodeSafeToReplace, isNodeSafeToReplacePath, } from '@src/lang/queryAst' -import { - ARG_INDEX_FIELD, - LABELED_ARG_FIELD, - UNLABELED_ARG, -} from '@src/lang/queryAstConstants' +import { ARG_INDEX_FIELD, LABELED_ARG_FIELD } from '@src/lang/queryAstConstants' import { getNodePathFromSourceRange } from '@src/lang/queryAstNodePathUtils' import { addTagForSketchOnFace, @@ -422,56 +417,6 @@ export function sketchOnExtrudedFace( } } -/** - * Append an offset plane to the AST - */ -export function addOffsetPlane({ - node, - plane, - insertIndex, - offset, - planeName, -}: { - node: Node - plane: Node | Node // Can be DefaultPlaneStr or string for offsetPlanes - insertIndex?: number - offset: Expr - planeName?: string -}): { modifiedAst: Node; pathToNode: PathToNode } { - const modifiedAst = structuredClone(node) - const newPlaneName = - planeName ?? findUniqueName(node, KCL_DEFAULT_CONSTANT_PREFIXES.PLANE) - - const newPlane = createVariableDeclaration( - newPlaneName, - createCallExpressionStdLibKw('offsetPlane', plane, [ - createLabeledArg('offset', offset), - ]) - ) - - const insertAt = - insertIndex !== undefined - ? insertIndex - : modifiedAst.body.length - ? modifiedAst.body.length - : 0 - - modifiedAst.body.length - ? modifiedAst.body.splice(insertAt, 0, newPlane) - : modifiedAst.body.push(newPlane) - const pathToNode: PathToNode = [ - ['body', ''], - [insertAt, 'index'], - ['declaration', 'VariableDeclaration'], - ['init', 'VariableDeclarator'], - ['unlabeled', UNLABELED_ARG], - ] - return { - modifiedAst, - pathToNode, - } -} - /** * Add an import call to load a part */ diff --git a/src/lang/modifyAst/faces.test.ts b/src/lang/modifyAst/faces.test.ts index 9adc0cd467b..b4444fe6ae3 100644 --- a/src/lang/modifyAst/faces.test.ts +++ b/src/lang/modifyAst/faces.test.ts @@ -8,16 +8,22 @@ import type { Selection, Selections } from '@src/lib/selections' import { enginelessExecutor } from '@src/lib/testHelpers' import { err } from '@src/lib/trap' import { + addOffsetPlane, addShell, retrieveFaceSelectionsFromOpArgs, } from '@src/lang/modifyAst/faces' import { initPromise } from '@src/lang/wasmUtils' -import { engineCommandManager, kclManager } from '@src/lib/singletons' +import { + engineCommandManager, + kclManager, + rustContext, +} from '@src/lib/singletons' import { getCodeRefsByArtifactId } from '@src/lang/std/artifactGraph' import { createPathToNodeForLastVariable } from '@src/lang/modifyAst' import { stringToKclExpression } from '@src/lib/kclHelpers' import type { KclCommandValue } from '@src/lib/commandTypes' import env from '@src/env' +import type { DefaultPlaneStr } from '@src/lib/planes' // Unfortunately, we need the real engine here it seems to get sweep faces populated beforeAll(async () => { @@ -46,9 +52,10 @@ async function getAstAndArtifactGraph(code: string) { const { artifactGraph, execState: { operations }, + variables, } = kclManager await new Promise((resolve) => setTimeout(resolve, 100)) - return { ast, artifactGraph, operations } + return { ast, artifactGraph, operations, variables } } function createSelectionFromArtifacts( @@ -339,3 +346,53 @@ shell001 = shell(extrude001, faces = END, thickness = 0.1) expect(selections.faces.graphSelections[1].artifact!.type).toEqual('cap') }) }) + +describe('Testing addOffsetPlane', () => { + it.each(['XY', 'XZ', 'YZ'])( + 'should add a basic offset plane call on default plane %s and then edit it', + async (name) => { + const { artifactGraph, ast, variables } = await getAstAndArtifactGraph('') + const offset = (await stringToKclExpression('1')) as KclCommandValue + const id = rustContext.getDefaultPlaneId(name) + if (err(id)) { + throw id + } + const plane: Selections = { + graphSelections: [], + otherSelections: [{ name, id }], + } + const result = addOffsetPlane({ + ast, + artifactGraph, + variables, + plane, + offset, + }) + if (err(result)) { + throw result + } + + const newCode = recast(result.modifiedAst) + expect(newCode).toContain(`plane001 = offsetPlane(${name}, offset = 1)`) + await enginelessExecutor(ast) + + const newOffset = (await stringToKclExpression('2')) as KclCommandValue + const nodeToEdit = createPathToNodeForLastVariable(result.modifiedAst) + const result2 = addOffsetPlane({ + ast: result.modifiedAst, + artifactGraph, + variables, + plane, + offset: newOffset, + nodeToEdit, + }) + if (err(result2)) { + throw result2 + } + const newCode2 = recast(result2.modifiedAst) + expect(newCode2).not.toContain(`offset = 1`) + expect(newCode2).toContain(`plane001 = offsetPlane(${name}, offset = 2)`) + await enginelessExecutor(result2.modifiedAst) + } + ) +}) diff --git a/src/lang/modifyAst/faces.ts b/src/lang/modifyAst/faces.ts index 13d2d76f54c..77b5649a2a8 100644 --- a/src/lang/modifyAst/faces.ts +++ b/src/lang/modifyAst/faces.ts @@ -59,43 +59,18 @@ export function addShell({ const modifiedAst = structuredClone(ast) // 2. Prepare unlabeled and labeled arguments - // Inferring solids from the faces selection, maybe someday we can expose this but no need for now - const solids: Selections = { - graphSelections: faces.graphSelections.flatMap((f) => { - const sweep = getSweepArtifactFromSelection(f, artifactGraph) - if (err(sweep) || !sweep) return [] - return { - artifact: sweep as Artifact, - codeRef: sweep.codeRef, - } - }), - otherSelections: [], - } - // Map the sketches selection into a list of kcl expressions to be passed as unlabeled argument - const lastChildLookup = true - const vars = getVariableExprsFromSelection( - solids, - modifiedAst, - nodeToEdit, - lastChildLookup, - artifactGraph - ) - if (err(vars)) { - return vars - } - - const sketchesExpr = createVariableExpressionsArray(vars.exprs) - const facesExprs = getFacesExprsFromSelection( - modifiedAst, + const result = buildSolidsAndFacesExprs( faces, - artifactGraph + artifactGraph, + modifiedAst, + nodeToEdit ) - const facesExpr = createVariableExpressionsArray(facesExprs) - if (!facesExpr) { - return new Error('No faces found in the selection') + if (err(result)) { + return result } - const call = createCallExpressionStdLibKw('shell', sketchesExpr, [ + const { solidsExpr, facesExpr, pathIfPipe } = result + const call = createCallExpressionStdLibKw('shell', solidsExpr, [ createLabeledArg('faces', facesExpr), createLabeledArg('thickness', valueOrVariable(thickness)), ]) @@ -111,7 +86,7 @@ export function addShell({ ast: modifiedAst, call, pathToEdit: nodeToEdit, - pathIfNewPipe: vars.pathIfPipe, + pathIfNewPipe: pathIfPipe, variableIfNewDecl: KCL_DEFAULT_CONSTANT_PREFIXES.SHELL, }) if (err(pathToNode)) { @@ -149,45 +124,22 @@ export function addOffsetPlane({ // 2. Prepare unlabeled and labeled arguments let planeExpr: Expr | undefined - const faceToOffset = plane.graphSelections.find( + const hasFaceToOffset = plane.graphSelections.some( (sel) => sel.artifact?.type === 'cap' || sel.artifact?.type === 'wall' ) - if (faceToOffset) { - const sweep = getSweepArtifactFromSelection(faceToOffset, artifactGraph) - if (err(sweep) || !sweep) { - return new Error('No sweep artifact found corresponding to the selection') - } - const solid: Selections = { - graphSelections: [ - { - artifact: sweep as Artifact, - codeRef: sweep.codeRef, - }, - ], - otherSelections: [], - } - const lastChildLookup = true - const vars = getVariableExprsFromSelection( - solid, - modifiedAst, - nodeToEdit, - lastChildLookup, - artifactGraph - ) - if (err(vars)) { - return vars - } - const solidExpr = createVariableExpressionsArray(vars.exprs) - const facesExprs = getFacesExprsFromSelection( - modifiedAst, + if (hasFaceToOffset) { + const result = buildSolidsAndFacesExprs( plane, - artifactGraph + artifactGraph, + modifiedAst, + nodeToEdit ) - const facesExpr = createVariableExpressionsArray(facesExprs) - if (!facesExpr) { - return new Error('No face found in the selection') + if (err(result)) { + return result } - planeExpr = createCallExpressionStdLibKw('planeOf', solidExpr, [ + + const { solidsExpr, facesExpr } = result + planeExpr = createCallExpressionStdLibKw('planeOf', solidsExpr, [ createLabeledArg('face', facesExpr), ]) } else { @@ -351,3 +303,48 @@ export function retrieveFaceSelectionsFromOpArgs( const faces = { graphSelections, otherSelections: [] } return { solids, faces } } + +function buildSolidsAndFacesExprs( + faces: Selections, + artifactGraph: ArtifactGraph, + modifiedAst: Node, + nodeToEdit?: PathToNode +) { + const solids: Selections = { + graphSelections: faces.graphSelections.flatMap((f) => { + const sweep = getSweepArtifactFromSelection(f, artifactGraph) + if (err(sweep) || !sweep) return [] + return { + artifact: sweep as Artifact, + codeRef: sweep.codeRef, + } + }), + otherSelections: [], + } + // Map the sketches selection into a list of kcl expressions to be passed as unlabeled argument + const lastChildLookup = true + const vars = getVariableExprsFromSelection( + solids, + modifiedAst, + nodeToEdit, + lastChildLookup, + artifactGraph + ) + if (err(vars)) { + return vars + } + + const pathIfPipe = vars.pathIfPipe + const solidsExpr = createVariableExpressionsArray(vars.exprs) + const facesExprs = getFacesExprsFromSelection( + modifiedAst, + faces, + artifactGraph + ) + const facesExpr = createVariableExpressionsArray(facesExprs) + if (!facesExpr) { + return new Error('No faces found in the selection') + } + + return { solidsExpr, facesExpr, pathIfPipe } +} From a618e4753809118fc49014f5b25002b4b5cb37ea Mon Sep 17 00:00:00 2001 From: Pierre Jacquier Date: Tue, 22 Jul 2025 14:33:34 -0400 Subject: [PATCH 06/12] Add offset plane on offset plane unit test --- src/lang/modifyAst/faces.test.ts | 52 ++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/src/lang/modifyAst/faces.test.ts b/src/lang/modifyAst/faces.test.ts index b4444fe6ae3..39572779f20 100644 --- a/src/lang/modifyAst/faces.test.ts +++ b/src/lang/modifyAst/faces.test.ts @@ -395,4 +395,56 @@ describe('Testing addOffsetPlane', () => { await enginelessExecutor(result2.modifiedAst) } ) + + it('should add an offset plane call on offset plane and then edit it', async () => { + const code = `plane001 = offsetPlane(XY, offset = 1)` + const { artifactGraph, ast, variables } = await getAstAndArtifactGraph(code) + const offset = (await stringToKclExpression('2')) as KclCommandValue + const artifact = [...artifactGraph.values()].find((a) => a.type === 'plane') + const plane: Selections = { + graphSelections: [ + { + artifact, + codeRef: artifact!.codeRef, + }, + ], + otherSelections: [], + } + const result = addOffsetPlane({ + ast, + artifactGraph, + variables, + plane, + offset, + }) + if (err(result)) { + throw result + } + + const newCode = recast(result.modifiedAst) + expect(newCode).toContain(`${code} +plane002 = offsetPlane(plane001, offset = 2)`) + await enginelessExecutor(ast) + + const newOffset = (await stringToKclExpression('3')) as KclCommandValue + const nodeToEdit = createPathToNodeForLastVariable(result.modifiedAst) + const result2 = addOffsetPlane({ + ast: result.modifiedAst, + artifactGraph, + variables, + plane, + offset: newOffset, + nodeToEdit, + }) + if (err(result2)) { + throw result2 + } + const newCode2 = recast(result2.modifiedAst) + expect(newCode2).not.toContain(`offset = 2`) + expect(newCode2).toContain(`${code} +plane002 = offsetPlane(plane001, offset = 3)`) + await enginelessExecutor(result2.modifiedAst) + }) + + // TODO: add test of offset plane on face selection }) From a8846b90d51410a34c75bb4ebdfc9ffd1ada19ba Mon Sep 17 00:00:00 2001 From: Pierre Jacquier Date: Tue, 22 Jul 2025 15:18:17 -0400 Subject: [PATCH 07/12] Add offset plane on face unit tests --- src/lang/modifyAst/faces.test.ts | 160 ++++++++++++++++++++++++------- 1 file changed, 124 insertions(+), 36 deletions(-) diff --git a/src/lang/modifyAst/faces.test.ts b/src/lang/modifyAst/faces.test.ts index 39572779f20..1d06d51ec04 100644 --- a/src/lang/modifyAst/faces.test.ts +++ b/src/lang/modifyAst/faces.test.ts @@ -100,18 +100,52 @@ thing2 = startSketchOn(case, face = END) const multiSolidsShell = `${multiSolids} shell001 = shell([thing1, thing2], faces = [END, END], thickness = 5)` -describe('Testing addShell', () => { - const cylinder = `sketch001 = startSketchOn(XY) +const cylinder = `sketch001 = startSketchOn(XY) profile001 = circle(sketch001, center = [0, 0], radius = 10) extrude001 = extrude(profile001, length = 10)` - function getCapFromCylinder(artifactGraph: ArtifactGraph) { - const endFace = [...artifactGraph.values()].find( - (a) => a.type === 'cap' && a.subType === 'end' - ) - return createSelectionFromArtifacts([endFace!], artifactGraph) - } +const box = `sketch001 = startSketchOn(XY) +profile001 = startProfile(sketch001, at = [0, 0]) + |> xLine(length = 10) + |> yLine(length = 10) + |> xLine(length = -10) + |> line(endAbsolute = [profileStartX(%), profileStartY(%)]) + |> close() +extrude001 = extrude(profile001, length = 10)` + +const boxWithOneTag = `sketch001 = startSketchOn(XY) +profile001 = startProfile(sketch001, at = [0, 0]) + |> xLine(length = 10, tag = $seg01) + |> yLine(length = 10) + |> xLine(length = -10) + |> line(endAbsolute = [profileStartX(%), profileStartY(%)]) + |> close() +extrude001 = extrude(profile001, length = 10)` + +const boxWithTwoTags = `sketch001 = startSketchOn(XY) +profile001 = startProfile(sketch001, at = [0, 0]) + |> xLine(length = 10, tag = $seg01) + |> yLine(length = 10, tag = $seg02) + |> xLine(length = -10) + |> line(endAbsolute = [profileStartX(%), profileStartY(%)]) + |> close() +extrude001 = extrude(profile001, length = 10)` + +function getCapFromCylinder(artifactGraph: ArtifactGraph) { + const endFace = [...artifactGraph.values()].find( + (a) => a.type === 'cap' && a.subType === 'end' + ) + return createSelectionFromArtifacts([endFace!], artifactGraph) +} + +function getFacesFromBox(artifactGraph: ArtifactGraph, count: number) { + const twoWalls = [...artifactGraph.values()] + .filter((a) => a.type === 'wall') + .slice(0, count) + return createSelectionFromArtifacts(twoWalls, artifactGraph) +} +describe('Testing addShell', () => { it('should add a basic shell call on cylinder end cap', async () => { const { artifactGraph, ast } = await getAstAndArtifactGraph(cylinder) const faces = getCapFromCylinder(artifactGraph) @@ -181,34 +215,9 @@ shell001 = shell(extrude001, faces = END, thickness = 1) await enginelessExecutor(ast) }) - const box = `sketch001 = startSketchOn(XY) -profile001 = startProfile(sketch001, at = [0, 0]) - |> xLine(length = 10) - |> yLine(length = 10) - |> xLine(length = -10) - |> line(endAbsolute = [profileStartX(%), profileStartY(%)]) - |> close() -extrude001 = extrude(profile001, length = 10) -` - const boxWithTwoTags = `sketch001 = startSketchOn(XY) -profile001 = startProfile(sketch001, at = [0, 0]) - |> xLine(length = 10, tag = $seg01) - |> yLine(length = 10, tag = $seg02) - |> xLine(length = -10) - |> line(endAbsolute = [profileStartX(%), profileStartY(%)]) - |> close() -extrude001 = extrude(profile001, length = 10)` - - function getTwoFacesFromBox(artifactGraph: ArtifactGraph) { - const twoWalls = [...artifactGraph.values()] - .filter((a) => a.type === 'wall') - .slice(0, 2) - return createSelectionFromArtifacts(twoWalls, artifactGraph) - } - it('should add a shell call on box for 2 walls', async () => { const { artifactGraph, ast } = await getAstAndArtifactGraph(box) - const faces = getTwoFacesFromBox(artifactGraph) + const faces = getFacesFromBox(artifactGraph, 2) const thickness = (await stringToKclExpression('1')) as KclCommandValue const result = addShell({ ast, artifactGraph, faces, thickness }) if (err(result)) { @@ -225,7 +234,7 @@ shell001 = shell(extrude001, faces = [seg01, seg02], thickness = 1)`) const { artifactGraph, ast } = await getAstAndArtifactGraph(`${boxWithTwoTags} shell001 = shell(extrude001, faces = [seg01, seg02], thickness = 1)`) - const faces = getTwoFacesFromBox(artifactGraph) + const faces = getFacesFromBox(artifactGraph, 2) const thickness = (await stringToKclExpression('2')) as KclCommandValue const nodeToEdit = createPathToNodeForLastVariable(ast) const result = addShell({ @@ -446,5 +455,84 @@ plane002 = offsetPlane(plane001, offset = 3)`) await enginelessExecutor(result2.modifiedAst) }) - // TODO: add test of offset plane on face selection + it('should add an offset plane call on cylinder end cap and allow edits', async () => { + const { artifactGraph, ast, variables } = + await getAstAndArtifactGraph(cylinder) + const plane = getCapFromCylinder(artifactGraph) + const offset = (await stringToKclExpression('2')) as KclCommandValue + const result = addOffsetPlane({ + ast, + artifactGraph, + variables, + plane, + offset, + }) + if (err(result)) { + throw result + } + + const newCode = recast(result.modifiedAst) + expect(newCode).toContain(`${cylinder} +plane001 = offsetPlane(planeOf(extrude001, face = END), offset = 2)`) + await enginelessExecutor(ast) + + const newOffset = (await stringToKclExpression('3')) as KclCommandValue + const nodeToEdit = createPathToNodeForLastVariable(result.modifiedAst) + const result2 = addOffsetPlane({ + ast: result.modifiedAst, + artifactGraph, + variables, + plane, + offset: newOffset, + nodeToEdit, + }) + if (err(result2)) { + throw result2 + } + const newCode2 = recast(result2.modifiedAst) + expect(newCode2).not.toContain(`offset = 2`) + expect(newCode2).toContain(`${cylinder} +plane001 = offsetPlane(planeOf(extrude001, face = END), offset = 3)`) + await enginelessExecutor(result2.modifiedAst) + }) + + it('should add an offset plane call on box wall and allow edits', async () => { + const { artifactGraph, ast, variables } = await getAstAndArtifactGraph(box) + const plane = getFacesFromBox(artifactGraph, 1) + const offset = (await stringToKclExpression('10')) as KclCommandValue + const result = addOffsetPlane({ + ast, + artifactGraph, + variables, + plane, + offset, + }) + if (err(result)) { + throw result + } + + const newCode = recast(result.modifiedAst) + expect(newCode).toContain(`${boxWithOneTag} +plane001 = offsetPlane(planeOf(extrude001, face = seg01), offset = 10)`) + await enginelessExecutor(ast) + + const newOffset = (await stringToKclExpression('20')) as KclCommandValue + const nodeToEdit = createPathToNodeForLastVariable(result.modifiedAst) + const result2 = addOffsetPlane({ + ast: result.modifiedAst, + artifactGraph, + variables, + plane, + offset: newOffset, + nodeToEdit, + }) + if (err(result2)) { + throw result2 + } + const newCode2 = recast(result2.modifiedAst) + expect(newCode2).not.toContain(`offset = 10`) + expect(newCode2).toContain(`${boxWithOneTag} +plane001 = offsetPlane(planeOf(extrude001, face = seg01), offset = 20)`) + await enginelessExecutor(result2.modifiedAst) + }) }) From 48ba38ae128efe23557af4c37f71a3f1af512dd0 Mon Sep 17 00:00:00 2001 From: Pierre Jacquier Date: Tue, 22 Jul 2025 16:20:23 -0400 Subject: [PATCH 08/12] Add face offset tests --- src/lang/modifyAst/faces.test.ts | 47 +++++++++++++++++++++++++++++ src/lang/modifyAst/faces.ts | 32 ++++++++++++++++++++ src/lib/operations.ts | 51 ++++++++++++++++---------------- 3 files changed, 104 insertions(+), 26 deletions(-) diff --git a/src/lang/modifyAst/faces.test.ts b/src/lang/modifyAst/faces.test.ts index 1d06d51ec04..3997c3bc337 100644 --- a/src/lang/modifyAst/faces.test.ts +++ b/src/lang/modifyAst/faces.test.ts @@ -1,6 +1,7 @@ import { type Artifact, type ArtifactGraph, + type PlaneArtifact, assertParse, recast, } from '@src/lang/wasm' @@ -11,6 +12,7 @@ import { addOffsetPlane, addShell, retrieveFaceSelectionsFromOpArgs, + retrieveNonDefaultPlaneSelectionFromOpArg, } from '@src/lang/modifyAst/faces' import { initPromise } from '@src/lang/wasmUtils' import { @@ -24,6 +26,7 @@ import { stringToKclExpression } from '@src/lib/kclHelpers' import type { KclCommandValue } from '@src/lib/commandTypes' import env from '@src/env' import type { DefaultPlaneStr } from '@src/lib/planes' +import type { StdLibCallOp } from '@src/lang/queryAst' // Unfortunately, we need the real engine here it seems to get sweep faces populated beforeAll(async () => { @@ -356,6 +359,50 @@ shell001 = shell(extrude001, faces = END, thickness = 0.1) }) }) +describe('Testing retrieveNonDefaultPlaneSelectionFromOpArg', () => { + it('should find an offset plane on an offset plane', async () => { + const code = `plane001 = offsetPlane(XY, offset = 1) +plane002 = offsetPlane(plane001, offset = 2)` + const { artifactGraph, operations } = await getAstAndArtifactGraph(code) + const op = operations.findLast( + (o) => o.type === 'StdLibCall' && o.name === 'offsetPlane' + ) as StdLibCallOp + const selections = retrieveNonDefaultPlaneSelectionFromOpArg( + op.unlabeledArg!, + artifactGraph + ) + if (err(selections)) throw selections + expect(selections.graphSelections).toHaveLength(1) + expect(selections.graphSelections[0].artifact!.type).toEqual('plane') + expect( + (selections.graphSelections[0].artifact as PlaneArtifact).codeRef + .pathToNode[1][0] + ).toEqual(0) + }) + + it('should find an offset plane on a sweep face', async () => { + const code = `${cylinder} +plane001 = offsetPlane(planeOf(extrude001, face = END), offset = 1)` + const { artifactGraph, operations } = await getAstAndArtifactGraph(code) + const op = operations.find( + (o) => o.type === 'StdLibCall' && o.name === 'offsetPlane' + ) as StdLibCallOp + const selections = retrieveNonDefaultPlaneSelectionFromOpArg( + op.unlabeledArg!, + artifactGraph + ) + // TODO: this is what we hit at the moment for sweep face offsets + // see https://github.com/KittyCAD/modeling-app/issues/7883 + if (err(selections)) throw selections + expect(selections.graphSelections).toHaveLength(1) + expect(selections.graphSelections[0].artifact!.type).toEqual('plane') + // expect( + // (selections.graphSelections[0].artifact as PlaneArtifact).codeRef + // .pathToNode[1][0] + // ).toEqual(0) + }) +}) + describe('Testing addOffsetPlane', () => { it.each(['XY', 'XZ', 'YZ'])( 'should add a basic offset plane call on default plane %s and then edit it', diff --git a/src/lang/modifyAst/faces.ts b/src/lang/modifyAst/faces.ts index 77b5649a2a8..0558758288e 100644 --- a/src/lang/modifyAst/faces.ts +++ b/src/lang/modifyAst/faces.ts @@ -304,6 +304,38 @@ export function retrieveFaceSelectionsFromOpArgs( return { solids, faces } } +export function retrieveNonDefaultPlaneSelectionFromOpArg( + planeArg: OpArg, + artifactGraph: ArtifactGraph +): Selections | Error { + if (planeArg.value.type !== 'Plane') { + return new Error( + 'Unsupported case for edit flows at the moment, check the KCL code' + ) + } + + const artifact = getArtifactOfTypes( + { + key: planeArg.value.artifact_id, + types: ['plane'], + }, + artifactGraph + ) + if (err(artifact)) { + return new Error("Couldn't retrieve plane artifact") + } + + return { + graphSelections: [ + { + artifact, + codeRef: artifact.codeRef, + }, + ], + otherSelections: [], + } +} + function buildSolidsAndFacesExprs( faces: Selections, artifactGraph: ArtifactGraph, diff --git a/src/lib/operations.ts b/src/lib/operations.ts index 552a75f70d4..ba6e2342bc1 100644 --- a/src/lib/operations.ts +++ b/src/lib/operations.ts @@ -1,7 +1,10 @@ import type { Operation } from '@rust/kcl-lib/bindings/Operation' import type { CustomIconName } from '@src/components/CustomIcon' -import { retrieveFaceSelectionsFromOpArgs } from '@src/lang/modifyAst/faces' +import { + retrieveFaceSelectionsFromOpArgs, + retrieveNonDefaultPlaneSelectionFromOpArg, +} from '@src/lang/modifyAst/faces' import { retrieveAxisOrEdgeSelectionsFromOpArg } from '@src/lang/modifyAst/sweeps' import { getNodeFromPath, @@ -434,41 +437,37 @@ const prepareToEditOffsetPlane: PrepareToEditCallback = async ({ } // 1. Map the plane and faces arguments to plane or face selections - // TODO: Implement conversion to arbitrary plane selection - // once the Offset Plane command supports it. if (!operation.unlabeledArg) { return { reason: `Couldn't retrieve operation arguments` } } - console.log('Offset plane operation:', operation) - - // TODO: replace with util function when available - const planeName = codeManager.code + console.log('operation', operation) + let plane: Selections | undefined + const maybeDefaultPlaneName = codeManager.code .slice( operation.unlabeledArg.sourceRange[0], operation.unlabeledArg.sourceRange[1] ) .replaceAll(`'`, ``) + if (isDefaultPlaneStr(maybeDefaultPlaneName)) { + const id = rustContext.getDefaultPlaneId(maybeDefaultPlaneName) + if (err(id)) { + return { reason: "Couldn't retrieve default plane ID" } + } - // TODO: figure this out for non-default planes and planeOf results - if (!isDefaultPlaneStr(planeName)) { - // TODO: error handling - return baseCommand - } - const planeId = rustContext.getDefaultPlaneId(planeName) - if (err(planeId)) { - // TODO: error handling - return baseCommand - } - - const plane: Selections = { - graphSelections: [], - otherSelections: [ - { - name: planeName, - id: planeId, - }, - ], + plane = { + graphSelections: [], + otherSelections: [{ id, name: maybeDefaultPlaneName }], + } + } else { + const result = retrieveNonDefaultPlaneSelectionFromOpArg( + operation.unlabeledArg, + kclManager.artifactGraph + ) + if (err(result)) { + return { reason: result.message } + } + plane = result } // 2. Convert the offset argument from a string to a KCL expression From 040e4d15827da7e78c656ee8b31c43b0b3a2a02f Mon Sep 17 00:00:00 2001 From: Jonathan Tran Date: Wed, 23 Jul 2025 14:00:36 -0400 Subject: [PATCH 09/12] WIP: Add PlaneOfFace client-side artifact --- rust/kcl-lib/src/execution/artifact.rs | 10 ++++++++++ rust/kcl-lib/src/execution/mod.rs | 4 +++- rust/kcl-lib/src/std/planes.rs | 11 +++++++++++ 3 files changed, 24 insertions(+), 1 deletion(-) diff --git a/rust/kcl-lib/src/execution/artifact.rs b/rust/kcl-lib/src/execution/artifact.rs index 36b22086dd0..2206dc9ef89 100644 --- a/rust/kcl-lib/src/execution/artifact.rs +++ b/rust/kcl-lib/src/execution/artifact.rs @@ -185,6 +185,15 @@ pub struct Solid2d { pub path_id: ArtifactId, } +#[derive(Debug, Clone, Serialize, PartialEq, ts_rs::TS)] +#[ts(export_to = "Artifact.ts")] +#[serde(rename_all = "camelCase")] +pub struct PlaneOfFace { + pub id: ArtifactId, + pub face_id: ArtifactId, + pub code_ref: CodeRef, +} + #[derive(Debug, Clone, Serialize, PartialEq, ts_rs::TS)] #[ts(export_to = "Artifact.ts")] #[serde(rename_all = "camelCase")] @@ -325,6 +334,7 @@ pub enum Artifact { Path(Path), Segment(Segment), Solid2d(Solid2d), + PlaneOfFace(PlaneOfFace), StartSketchOnFace(StartSketchOnFace), StartSketchOnPlane(StartSketchOnPlane), Sweep(Sweep), diff --git a/rust/kcl-lib/src/execution/mod.rs b/rust/kcl-lib/src/execution/mod.rs index e2c5ae86509..e120dc5027e 100644 --- a/rust/kcl-lib/src/execution/mod.rs +++ b/rust/kcl-lib/src/execution/mod.rs @@ -4,7 +4,9 @@ use std::sync::Arc; use anyhow::Result; #[cfg(feature = "artifact-graph")] -pub use artifact::{Artifact, ArtifactCommand, ArtifactGraph, CodeRef, StartSketchOnFace, StartSketchOnPlane}; +pub use artifact::{ + Artifact, ArtifactCommand, ArtifactGraph, CodeRef, PlaneOfFace, StartSketchOnFace, StartSketchOnPlane, +}; use cache::GlobalState; pub use cache::{bust_cache, clear_mem_cache}; #[cfg(feature = "artifact-graph")] diff --git a/rust/kcl-lib/src/std/planes.rs b/rust/kcl-lib/src/std/planes.rs index bcddaf4cd6b..d57f5a7d80d 100644 --- a/rust/kcl-lib/src/std/planes.rs +++ b/rust/kcl-lib/src/std/planes.rs @@ -125,6 +125,17 @@ async fn inner_plane_of( // Engine doesn't send back an ID, so let's just make a new plane ID. let plane_id = exec_state.id_generator().next_uuid(); + #[cfg(feature = "artifact-graph")] + { + use crate::execution::{Artifact, ArtifactId, CodeRef, PlaneOfFace}; + // Create artifact used only by the UI, not the engine. + exec_state.add_artifact(Artifact::PlaneOfFace(PlaneOfFace { + id: ArtifactId::from(plane_id), + face_id: face_id.into(), + code_ref: CodeRef::placeholder(args.source_range), + })); + } + Ok(Plane { artifact_id: plane_id.into(), id: plane_id, From 5c5245c95e865da2c9b20bf9c2540789b1a77e37 Mon Sep 17 00:00:00 2001 From: Pierre Jacquier Date: Wed, 23 Jul 2025 14:39:14 -0400 Subject: [PATCH 10/12] Fix rust errors --- rust/kcl-lib/src/execution/artifact.rs | 4 ++++ .../src/execution/artifact/mermaid_tests.rs | 14 ++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/rust/kcl-lib/src/execution/artifact.rs b/rust/kcl-lib/src/execution/artifact.rs index 2206dc9ef89..5b0c31c296a 100644 --- a/rust/kcl-lib/src/execution/artifact.rs +++ b/rust/kcl-lib/src/execution/artifact.rs @@ -356,6 +356,7 @@ impl Artifact { Artifact::Solid2d(a) => a.id, Artifact::StartSketchOnFace(a) => a.id, Artifact::StartSketchOnPlane(a) => a.id, + Artifact::PlaneOfFace(a) => a.id, Artifact::Sweep(a) => a.id, Artifact::Wall(a) => a.id, Artifact::Cap(a) => a.id, @@ -377,6 +378,7 @@ impl Artifact { Artifact::Solid2d(_) => None, Artifact::StartSketchOnFace(a) => Some(&a.code_ref), Artifact::StartSketchOnPlane(a) => Some(&a.code_ref), + Artifact::PlaneOfFace(a) => Some(&a.code_ref), Artifact::Sweep(a) => Some(&a.code_ref), Artifact::Wall(_) => None, Artifact::Cap(_) => None, @@ -397,6 +399,7 @@ impl Artifact { | Artifact::Segment(_) | Artifact::Solid2d(_) | Artifact::StartSketchOnFace(_) + | Artifact::PlaneOfFace(_) | Artifact::StartSketchOnPlane(_) | Artifact::Sweep(_) => None, Artifact::Wall(a) => Some(&a.face_code_ref), @@ -416,6 +419,7 @@ impl Artifact { Artifact::Solid2d(_) => Some(new), Artifact::StartSketchOnFace { .. } => Some(new), Artifact::StartSketchOnPlane { .. } => Some(new), + Artifact::PlaneOfFace { .. } => Some(new), Artifact::Sweep(a) => a.merge(new), Artifact::Wall(a) => a.merge(new), Artifact::Cap(a) => a.merge(new), diff --git a/rust/kcl-lib/src/execution/artifact/mermaid_tests.rs b/rust/kcl-lib/src/execution/artifact/mermaid_tests.rs index 537d3238494..da66a57d0fe 100644 --- a/rust/kcl-lib/src/execution/artifact/mermaid_tests.rs +++ b/rust/kcl-lib/src/execution/artifact/mermaid_tests.rs @@ -87,6 +87,7 @@ impl Artifact { Artifact::Solid2d(a) => vec![a.path_id], Artifact::StartSketchOnFace(a) => vec![a.face_id], Artifact::StartSketchOnPlane(a) => vec![a.plane_id], + Artifact::PlaneOfFace(a) => vec![a.face_id], Artifact::Sweep(a) => vec![a.path_id], Artifact::Wall(a) => vec![a.seg_id, a.sweep_id], Artifact::Cap(a) => vec![a.sweep_id], @@ -151,6 +152,10 @@ impl Artifact { // Note: Don't include these since they're parents: plane_id. Vec::new() } + Artifact::PlaneOfFace { .. } => { + // Note: Don't include these since they're parents: face_id. + Vec::new() + } Artifact::Sweep(a) => { // Note: Don't include these since they're parents: path_id. let mut ids = Vec::new(); @@ -262,6 +267,7 @@ impl ArtifactGraph { } Artifact::StartSketchOnFace { .. } | Artifact::StartSketchOnPlane { .. } + | Artifact::PlaneOfFace { .. } | Artifact::Sweep(_) | Artifact::Wall(_) | Artifact::Cap(_) @@ -381,6 +387,14 @@ impl ArtifactGraph { )?; node_path_display(output, prefix, None, code_ref)?; } + Artifact::PlaneOfFace(PlaneOfFace { code_ref, .. }) => { + writeln!( + output, + "{prefix}{id}[\"PlaneOfFace
{:?}\"]", + code_ref_display(code_ref) + )?; + node_path_display(output, prefix, None, code_ref)?; + } Artifact::Sweep(sweep) => { writeln!( output, From a3073b3f8e7ab00346bf524b926e08a64a34c5f8 Mon Sep 17 00:00:00 2001 From: Pierre Jacquier Date: Wed, 23 Jul 2025 14:54:43 -0400 Subject: [PATCH 11/12] Working unit tests and edit flows --- src/lang/modifyAst/faces.test.ts | 13 ++++---- src/lang/modifyAst/faces.ts | 52 ++++++++++++++++++++++++-------- 2 files changed, 46 insertions(+), 19 deletions(-) diff --git a/src/lang/modifyAst/faces.test.ts b/src/lang/modifyAst/faces.test.ts index 3997c3bc337..716831d65c4 100644 --- a/src/lang/modifyAst/faces.test.ts +++ b/src/lang/modifyAst/faces.test.ts @@ -391,15 +391,14 @@ plane001 = offsetPlane(planeOf(extrude001, face = END), offset = 1)` op.unlabeledArg!, artifactGraph ) - // TODO: this is what we hit at the moment for sweep face offsets - // see https://github.com/KittyCAD/modeling-app/issues/7883 if (err(selections)) throw selections + expect(selections.graphSelections).toHaveLength(1) - expect(selections.graphSelections[0].artifact!.type).toEqual('plane') - // expect( - // (selections.graphSelections[0].artifact as PlaneArtifact).codeRef - // .pathToNode[1][0] - // ).toEqual(0) + expect(selections.graphSelections[0].artifact!.type).toEqual('cap') + const cap = [...artifactGraph.values()].find( + (a) => a.type === 'cap' && a.subType === 'end' + ) + expect(selections.graphSelections[0].artifact!.id).toEqual(cap!.id) }) }) diff --git a/src/lang/modifyAst/faces.ts b/src/lang/modifyAst/faces.ts index 0558758288e..a30eddadde3 100644 --- a/src/lang/modifyAst/faces.ts +++ b/src/lang/modifyAst/faces.ts @@ -32,6 +32,7 @@ import { mutateAstWithTagForSketchSegment } from '@src/lang/modifyAst/addEdgeTre import { getArtifactOfTypes, getCapCodeRef, + getFaceCodeRef, getSweepArtifactFromSelection, } from '@src/lang/std/artifactGraph' import type { OpArg, OpKclValue } from '@rust/kcl-lib/bindings/Operation' @@ -314,26 +315,53 @@ export function retrieveNonDefaultPlaneSelectionFromOpArg( ) } - const artifact = getArtifactOfTypes( + const planeArtifact = getArtifactOfTypes( { key: planeArg.value.artifact_id, - types: ['plane'], + types: ['plane', 'planeOfFace'], }, artifactGraph ) - if (err(artifact)) { - return new Error("Couldn't retrieve plane artifact") + if (err(planeArtifact)) { + return new Error("Couldn't retrieve plane or planeOfFace artifact") } - return { - graphSelections: [ - { - artifact, - codeRef: artifact.codeRef, - }, - ], - otherSelections: [], + if (planeArtifact.type === 'plane') { + return { + graphSelections: [ + { + artifact: planeArtifact, + codeRef: planeArtifact.codeRef, + }, + ], + otherSelections: [], + } + } else if (planeArtifact.type === 'planeOfFace') { + const faceArtifact = getArtifactOfTypes( + { key: planeArtifact.faceId, types: ['cap', 'wall'] }, + artifactGraph + ) + if (err(faceArtifact)) { + return new Error("Couldn't retrieve face artifact for planeOfFace") + } + + const codeRef = getFaceCodeRef(faceArtifact) + if (!codeRef) { + return new Error("Couldn't retrieve code reference for face artifact") + } + + return { + graphSelections: [ + { + artifact: faceArtifact, + codeRef, + }, + ], + otherSelections: [], + } } + + return new Error('Unsupported plane artifact type') } function buildSolidsAndFacesExprs( From e13f1f40af1dedc52976f458a41f6089b6b3ce3e Mon Sep 17 00:00:00 2001 From: Pierre Jacquier Date: Wed, 23 Jul 2025 15:03:32 -0400 Subject: [PATCH 12/12] Remove log, thanks bot --- src/lib/operations.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/lib/operations.ts b/src/lib/operations.ts index 94b1737454e..fab4521ea62 100644 --- a/src/lib/operations.ts +++ b/src/lib/operations.ts @@ -441,7 +441,6 @@ const prepareToEditOffsetPlane: PrepareToEditCallback = async ({ return { reason: `Couldn't retrieve operation arguments` } } - console.log('operation', operation) let plane: Selections | undefined const maybeDefaultPlaneName = getStringValue( codeManager.code,