From 054bb5b500cc1a8c20b76cf4e22b745d6af40c78 Mon Sep 17 00:00:00 2001 From: Pierre Jacquier Date: Thu, 20 Mar 2025 20:42:41 -0400 Subject: [PATCH] Add sectional argument and edit flow for point-and-click Sweep (#5480) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * WIP: Expose the sectional argument in the Sweep command flow Fixes #5301 * A snapshot a day keeps the bugs away! 📷🐛 (OS: namespace-profile-ubuntu-8-cores) * A snapshot a day keeps the bugs away! 📷🐛 (OS: namespace-profile-ubuntu-8-cores) * A snapshot a day keeps the bugs away! 📷🐛 (OS: namespace-profile-ubuntu-8-cores) * Working edit flow * Lint * Allow in place editing, more consistent code * A snapshot a day keeps the bugs away! 📷🐛 (OS: namespace-profile-ubuntu-8-cores) * A snapshot a day keeps the bugs away! 📷🐛 (OS: namespace-profile-ubuntu-8-cores) * Remove validation on non-selection arg * A snapshot a day keeps the bugs away! 📷🐛 (OS: namespace-profile-ubuntu-8-cores) * Comment out bad test * Clean up for review * Hack sectional * Made selection args hidden * Fix edit issue in e2e * Clean up * Add face filtering filter for opposite and next adjacent faces * Lint * Fixme back * A snapshot a day keeps the bugs away! 📷🐛 * A snapshot a day keeps the bugs away! 📷🐛 * A snapshot a day keeps the bugs away! 📷🐛 * Use updateModelingState in codemod * A snapshot a day keeps the bugs away! 📷🐛 * A snapshot a day keeps the bugs away! 📷🐛 * A snapshot a day keeps the bugs away! 📷🐛 * A snapshot a day keeps the bugs away! 📷🐛 * A snapshot a day keeps the bugs away! 📷🐛 * A snapshot a day keeps the bugs away! 📷🐛 * A snapshot a day keeps the bugs away! 📷🐛 * A snapshot a day keeps the bugs away! 📷🐛 * A snapshot a day keeps the bugs away! 📷🐛 * A snapshot a day keeps the bugs away! 📷🐛 * A snapshot a day keeps the bugs away! 📷🐛 * A snapshot a day keeps the bugs away! 📷🐛 * A snapshot a day keeps the bugs away! 📷🐛 * A snapshot a day keeps the bugs away! 📷🐛 * A snapshot a day keeps the bugs away! 📷🐛 * Improve filtering readibility * Fix base test * I liked return but clippy didn't * Working tests, isolating the change to sectional sweep, don't like the api change * A snapshot a day keeps the bugs away! 📷🐛 * A snapshot a day keeps the bugs away! 📷🐛 * A snapshot a day keeps the bugs away! 📷🐛 * A snapshot a day keeps the bugs away! 📷🐛 * Clean up snapshots * A snapshot a day keeps the bugs away! 📷🐛 * A snapshot a day keeps the bugs away! 📷🐛 --------- Co-authored-by: github-actions[bot] --- e2e/playwright/point-click.spec.ts | 57 ++++++-- rust/kcl-lib/src/std/extrude.rs | 22 +++ rust/kcl-lib/src/std/loft.rs | 1 + rust/kcl-lib/src/std/revolve.rs | 1 + rust/kcl-lib/src/std/sweep.rs | 1 + src/lang/modifyAst.ts | 50 +++++-- .../modelingCommandConfig.ts | 28 +++- src/lib/operations.ts | 138 ++++++++++++++++++ src/machines/modelingMachine.ts | 85 +++++++---- 9 files changed, 332 insertions(+), 51 deletions(-) diff --git a/e2e/playwright/point-click.spec.ts b/e2e/playwright/point-click.spec.ts index 6b5f764cb..b8a5ee284 100644 --- a/e2e/playwright/point-click.spec.ts +++ b/e2e/playwright/point-click.spec.ts @@ -1336,7 +1336,7 @@ loft001 = loft([sketch001, sketch002]) }) }) - test(`Sweep point-and-click`, async ({ + test(`Sweep point-and-click base`, async ({ context, page, homePage, @@ -1369,7 +1369,10 @@ sketch002 = startSketchOn('XZ') testPoint.x - 50, testPoint.y ) - const sweepDeclaration = 'sweep001 = sweep(sketch001, path = sketch002)' + const sweepDeclaration = + 'sweep001 = sweep(sketch001, path = sketch002, sectional = false)' + const editedSweepDeclaration = + 'sweep001 = sweep(sketch001, path = sketch002, sectional = true)' await test.step(`Look for sketch001`, async () => { await toolbar.closePane('code') @@ -1383,6 +1386,7 @@ sketch002 = startSketchOn('XZ') currentArgKey: 'target', currentArgValue: '', headerArguments: { + Sectional: '', Target: '', Trajectory: '', }, @@ -1395,6 +1399,7 @@ sketch002 = startSketchOn('XZ') currentArgKey: 'trajectory', currentArgValue: '', headerArguments: { + Sectional: '', Target: '1 face', Trajectory: '', }, @@ -1404,18 +1409,50 @@ sketch002 = startSketchOn('XZ') await clickOnSketch2() await page.waitForTimeout(500) await cmdBar.progressCmdBar() - await toolbar.openPane('code') - await page.waitForTimeout(500) + await cmdBar.expectState({ + commandName: 'Sweep', + headerArguments: { + Target: '1 face', + Trajectory: '1 segment', + Sectional: '', + }, + stage: 'review', + }) + await cmdBar.progressCmdBar() }) await test.step(`Confirm code is added to the editor, scene has changed`, async () => { - // await scene.expectPixelColor([135, 64, 73], testPoint, 15) // FIXME + await toolbar.openPane('code') await editor.expectEditor.toContain(sweepDeclaration) - await editor.expectState({ - diagnostics: [], - activeLines: [sweepDeclaration], - highlightedCode: '', + await toolbar.closePane('code') + }) + + await test.step('Edit sweep via feature tree selection works', async () => { + await toolbar.openPane('feature-tree') + const operationButton = await toolbar.getFeatureTreeOperation('Sweep', 0) + await operationButton.dblclick({ button: 'left' }) + await cmdBar.expectState({ + commandName: 'Sweep', + currentArgKey: 'sectional', + currentArgValue: '', + headerArguments: { + Sectional: '', + }, + highlightedHeaderArg: 'sectional', + stage: 'arguments', }) + await cmdBar.selectOption({ name: 'True' }).click() + await cmdBar.expectState({ + commandName: 'Sweep', + headerArguments: { + Sectional: '', + }, + stage: 'review', + }) + await cmdBar.progressCmdBar() + await toolbar.closePane('feature-tree') + await toolbar.openPane('code') + await editor.expectEditor.toContain(editedSweepDeclaration) await toolbar.closePane('code') }) @@ -1476,6 +1513,7 @@ sketch002 = startSketchOn('XZ') currentArgKey: 'target', currentArgValue: '', headerArguments: { + Sectional: '', Target: '', Trajectory: '', }, @@ -1488,6 +1526,7 @@ sketch002 = startSketchOn('XZ') currentArgKey: 'trajectory', currentArgValue: '', headerArguments: { + Sectional: '', Target: '1 face', Trajectory: '', }, diff --git a/rust/kcl-lib/src/std/extrude.rs b/rust/kcl-lib/src/std/extrude.rs index 03a10e6bb..99279f6e3 100644 --- a/rust/kcl-lib/src/std/extrude.rs +++ b/rust/kcl-lib/src/std/extrude.rs @@ -130,6 +130,7 @@ async fn inner_extrude( sketch, id.into(), length, + false, &NamedCapTags { start: tag_start.as_ref(), end: tag_end.as_ref(), @@ -154,6 +155,7 @@ pub(crate) async fn do_post_extrude<'a>( sketch: &Sketch, solid_id: ArtifactId, length: f64, + sectional: bool, named_cap_tags: &'a NamedCapTags<'a>, exec_state: &mut ExecState, args: &Args, @@ -201,6 +203,25 @@ pub(crate) async fn do_post_extrude<'a>( vec![] }; + // Face filtering attempt in order to resolve https://github.com/KittyCAD/modeling-app/issues/5328 + // In case of a sectional sweep, empirically it looks that the first n faces that are yielded from the sweep + // are the ones that work with GetOppositeEdge and GetNextAdjacentEdge, aka the n sides in the sweep. + // So here we're figuring out that n number as yielded_sides_count here, + // making sure that circle() calls count but close() don't (no length) + let count_of_first_set_of_faces_if_sectional = if sectional { + sketch + .paths + .iter() + .filter(|p| { + let is_circle = matches!(p, Path::Circle { .. }); + let has_length = p.get_base().from != p.get_base().to; + is_circle || has_length + }) + .count() + } else { + usize::MAX + }; + for (curve_id, face_id) in face_infos .iter() .filter(|face_info| face_info.cap == ExtrusionFaceCapType::None) @@ -211,6 +232,7 @@ pub(crate) async fn do_post_extrude<'a>( None } }) + .take(count_of_first_set_of_faces_if_sectional) { // Batch these commands, because the Rust code doesn't actually care about the outcome. // So, there's no need to await them. diff --git a/rust/kcl-lib/src/std/loft.rs b/rust/kcl-lib/src/std/loft.rs index 41be882c0..dbe6b47c5 100644 --- a/rust/kcl-lib/src/std/loft.rs +++ b/rust/kcl-lib/src/std/loft.rs @@ -174,6 +174,7 @@ async fn inner_loft( &sketch, id.into(), 0.0, + false, &super::extrude::NamedCapTags { start: tag_start.as_ref(), end: tag_end.as_ref(), diff --git a/rust/kcl-lib/src/std/revolve.rs b/rust/kcl-lib/src/std/revolve.rs index 802dd5ce6..5a8b839d0 100644 --- a/rust/kcl-lib/src/std/revolve.rs +++ b/rust/kcl-lib/src/std/revolve.rs @@ -299,6 +299,7 @@ async fn inner_revolve( sketch, id.into(), 0.0, + false, &super::extrude::NamedCapTags { start: tag_start.as_ref(), end: tag_end.as_ref(), diff --git a/rust/kcl-lib/src/std/sweep.rs b/rust/kcl-lib/src/std/sweep.rs index 17abc15bb..6582df95b 100644 --- a/rust/kcl-lib/src/std/sweep.rs +++ b/rust/kcl-lib/src/std/sweep.rs @@ -183,6 +183,7 @@ async fn inner_sweep( sketch, id.into(), 0.0, + sectional.unwrap_or(false), &super::extrude::NamedCapTags { start: tag_start.as_ref(), end: tag_end.as_ref(), diff --git a/src/lang/modifyAst.ts b/src/lang/modifyAst.ts index b2bfe06b1..354f910fb 100644 --- a/src/lang/modifyAst.ts +++ b/src/lang/modifyAst.ts @@ -515,30 +515,54 @@ export function addShell({ } } -export function addSweep( - node: Node, - profileDeclarator: VariableDeclarator, - pathDeclarator: VariableDeclarator -): { +export function addSweep({ + node, + targetDeclarator, + trajectoryDeclarator, + sectional, + variableName, + insertIndex, +}: { + node: Node + targetDeclarator: VariableDeclarator + trajectoryDeclarator: VariableDeclarator + sectional: boolean + variableName?: string + insertIndex?: number +}): { modifiedAst: Node pathToNode: PathToNode } { const modifiedAst = structuredClone(node) - const name = findUniqueName(node, KCL_DEFAULT_CONSTANT_PREFIXES.SWEEP) - const sweep = createCallExpressionStdLibKw( + const name = + variableName ?? findUniqueName(node, KCL_DEFAULT_CONSTANT_PREFIXES.SWEEP) + const call = createCallExpressionStdLibKw( 'sweep', - createIdentifier(profileDeclarator.id.name), - [createLabeledArg('path', createIdentifier(pathDeclarator.id.name))] + createIdentifier(targetDeclarator.id.name), + [ + createLabeledArg('path', createIdentifier(trajectoryDeclarator.id.name)), + createLabeledArg('sectional', createLiteral(sectional)), + ] ) - const declaration = createVariableDeclaration(name, sweep) - modifiedAst.body.push(declaration) + const variable = createVariableDeclaration(name, call) + const insertAt = + insertIndex !== undefined + ? insertIndex + : modifiedAst.body.length + ? modifiedAst.body.length + : 0 + + modifiedAst.body.length + ? modifiedAst.body.splice(insertAt, 0, variable) + : modifiedAst.body.push(variable) + const argIndex = 0 const pathToNode: PathToNode = [ ['body', ''], - [modifiedAst.body.length - 1, 'index'], + [insertAt, 'index'], ['declaration', 'VariableDeclaration'], ['init', 'VariableDeclarator'], ['arguments', 'CallExpressionKw'], - [0, ARG_INDEX_FIELD], + [argIndex, ARG_INDEX_FIELD], ['arg', LABELED_ARG_FIELD], ] diff --git a/src/lib/commandBarConfigs/modelingCommandConfig.ts b/src/lib/commandBarConfigs/modelingCommandConfig.ts index 6252e9965..3afa8764a 100644 --- a/src/lib/commandBarConfigs/modelingCommandConfig.ts +++ b/src/lib/commandBarConfigs/modelingCommandConfig.ts @@ -55,8 +55,12 @@ export type ModelingCommandSchema = { distance: KclCommandValue } Sweep: { + // Enables editing workflow + nodeToEdit?: PathToNode + // Arguments target: Selections trajectory: Selections + sectional: boolean } Loft: { selection: Selections @@ -357,22 +361,40 @@ export const modelingMachineCommandConfig: StateMachineCommandSetConfig< description: 'Create a 3D body by moving a sketch region along an arbitrary path.', icon: 'sweep', - needsReview: false, + needsReview: true, args: { + nodeToEdit: { + description: + 'Path to the node in the AST to edit. Never shown to the user.', + skip: true, + inputType: 'text', + required: false, + }, target: { inputType: 'selection', selectionTypes: ['solid2d'], required: true, skip: true, multiple: false, + hidden: (context) => Boolean(context.argumentsToSubmit.nodeToEdit), }, trajectory: { inputType: 'selection', - selectionTypes: ['segment', 'path'], + selectionTypes: ['segment'], required: true, - skip: false, + skip: true, multiple: false, validation: sweepValidator, + hidden: (context) => Boolean(context.argumentsToSubmit.nodeToEdit), + }, + sectional: { + inputType: 'options', + required: true, + options: [ + { name: 'False', value: false }, + { name: 'True', value: true }, + ], + // No validation possible here until we have rollback }, }, }, diff --git a/src/lib/operations.ts b/src/lib/operations.ts index fec432fb0..9b9b3014d 100644 --- a/src/lib/operations.ts +++ b/src/lib/operations.ts @@ -311,6 +311,143 @@ const prepareToEditOffsetPlane: PrepareToEditCallback = async ({ } } +const prepareToEditSweep: PrepareToEditCallback = async ({ + artifact, + operation, +}) => { + const baseCommand = { + name: 'Sweep', + groupId: 'modeling', + } + if ( + operation.type !== 'StdLibCall' || + !operation.labeledArgs || + !operation.unlabeledArg || + !('sectional' in operation.labeledArgs) || + !operation.labeledArgs.sectional + ) { + return baseCommand + } + if (!artifact || !('pathId' in artifact) || operation.type !== 'StdLibCall') { + return baseCommand + } + + // We have to go a little roundabout to get from the original artifact + // to the solid2DId that we need to pass to the Sweep command, just like Extrude. + const pathArtifact = getArtifactOfTypes( + { + key: artifact.pathId, + types: ['path'], + }, + engineCommandManager.artifactGraph + ) + + if ( + err(pathArtifact) || + pathArtifact.type !== 'path' || + !pathArtifact.solid2dId + ) { + return baseCommand + } + + const targetArtifact = getArtifactOfTypes( + { + key: pathArtifact.solid2dId, + types: ['solid2d'], + }, + engineCommandManager.artifactGraph + ) + + if (err(targetArtifact) || targetArtifact.type !== 'solid2d') { + return baseCommand + } + + const target = { + graphSelections: [ + { + artifact: targetArtifact, + codeRef: pathArtifact.codeRef, + }, + ], + otherSelections: [], + } + + // Same roundabout but twice for 'path' aka trajectory: sketch -> path -> segment + if (!('path' in operation.labeledArgs) || !operation.labeledArgs.path) { + return baseCommand + } + + if (operation.labeledArgs.path.value.type !== 'Sketch') { + return baseCommand + } + + const trajectoryPathArtifact = getArtifactOfTypes( + { + key: operation.labeledArgs.path.value.value.artifactId, + types: ['path'], + }, + engineCommandManager.artifactGraph + ) + + if (err(trajectoryPathArtifact) || trajectoryPathArtifact.type !== 'path') { + return baseCommand + } + + const trajectoryArtifact = getArtifactOfTypes( + { + key: trajectoryPathArtifact.segIds[0], + types: ['segment'], + }, + engineCommandManager.artifactGraph + ) + + if (err(trajectoryArtifact) || trajectoryArtifact.type !== 'segment') { + return baseCommand + } + + const trajectory = { + graphSelections: [ + { + artifact: trajectoryArtifact, + codeRef: trajectoryArtifact.codeRef, + }, + ], + otherSelections: [], + } + + // sectional options boolean arg + if ( + !('sectional' in operation.labeledArgs) || + !operation.labeledArgs.sectional + ) { + return baseCommand + } + + const sectional = + codeManager.code.slice( + operation.labeledArgs.sectional.sourceRange[0], + operation.labeledArgs.sectional.sourceRange[1] + ) === 'true' + + // 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['Sweep'] = { + target: target, + trajectory, + sectional, + nodeToEdit: getNodePathFromSourceRange( + kclManager.ast, + sourceRangeFromRust(operation.sourceRange) + ), + } + + return { + ...baseCommand, + argDefaultValues, + } +} + const prepareToEditHelix: PrepareToEditCallback = async ({ operation }) => { const baseCommand = { name: 'Helix', @@ -511,6 +648,7 @@ export const stdLibMap: Record = { sweep: { label: 'Sweep', icon: 'sweep', + prepareToEdit: prepareToEditSweep, supportsAppearance: true, }, } diff --git a/src/machines/modelingMachine.ts b/src/machines/modelingMachine.ts index 7bc3d35e2..c2e1962c1 100644 --- a/src/machines/modelingMachine.ts +++ b/src/machines/modelingMachine.ts @@ -78,7 +78,7 @@ import { import { ModelingCommandSchema } from 'lib/commandBarConfigs/modelingCommandConfig' import { err, reportRejection, trap } from 'lib/trap' import { DefaultPlaneStr } from 'lib/planes' -import { uuidv4 } from 'lib/utils' +import { isArray, uuidv4 } from 'lib/utils' import { Coords2d } from 'lang/std/sketch' import { deleteSegment } from 'clientSideScene/ClientSideSceneComp' import toast from 'react-hot-toast' @@ -1994,55 +1994,88 @@ export const modelingMachine = setup({ if (!input) return new Error('No input provided') // Extract inputs const ast = kclManager.ast - const { target, trajectory } = input + const { target, trajectory, sectional, nodeToEdit } = input + let variableName: string | undefined = undefined + let insertIndex: number | undefined = undefined - // Find the profile declaration + // If this is an edit flow, first we're going to remove the old one + if (nodeToEdit !== undefined && typeof nodeToEdit[1][0] === 'number') { + // Extract the plane name from the node to edit + const variableNode = getNodeFromPath( + ast, + nodeToEdit, + 'VariableDeclaration' + ) + + if (err(variableNode)) { + console.error('Error extracting name') + } else { + variableName = variableNode.node.declaration.id.name + } + + // Removing the old statement + const newBody = [...ast.body] + newBody.splice(nodeToEdit[1][0], 1) + ast.body = newBody + insertIndex = nodeToEdit[1][0] + } + + // Find the target declaration const targetNodePath = getNodePathFromSourceRange( ast, target.graphSelections[0].codeRef.range ) - const targetNode = getNodeFromPath( - ast, - targetNodePath, - 'VariableDeclarator' - ) + // Gotchas, not sure why + // - it seems like in some cases we get a list on edit, especially the state that e2e hits + // - looking for a VariableDeclaration seems more robust than VariableDeclarator + const targetNode = getNodeFromPath< + VariableDeclaration | VariableDeclaration[] + >(ast, targetNodePath, 'VariableDeclaration') if (err(targetNode)) { return new Error("Couldn't parse profile selection") } - const targetDeclarator = targetNode.node - // Find the path declaration + const targetDeclarator = isArray(targetNode.node) + ? targetNode.node[0].declaration + : targetNode.node.declaration + + // Find the trajectory (or path) declaration const trajectoryNodePath = getNodePathFromSourceRange( ast, trajectory.graphSelections[0].codeRef.range ) - const trajectoryNode = getNodeFromPath( + // Also looking for VariableDeclaration for consistency here + const trajectoryNode = getNodeFromPath( ast, trajectoryNodePath, - 'VariableDeclarator' + 'VariableDeclaration' ) if (err(trajectoryNode)) { return new Error("Couldn't parse path selection") } - const trajectoryDeclarator = trajectoryNode.node + + const trajectoryDeclarator = trajectoryNode.node.declaration // Perform the sweep - const sweepRes = addSweep(ast, targetDeclarator, trajectoryDeclarator) - const updateAstResult = await kclManager.updateAst( - sweepRes.modifiedAst, - true, + const { modifiedAst, pathToNode } = addSweep({ + node: ast, + targetDeclarator, + trajectoryDeclarator, + sectional, + variableName, + insertIndex, + }) + await updateModelingState( + modifiedAst, { - focusPath: [sweepRes.pathToNode], + kclManager, + editorManager, + codeManager, + }, + { + focusPath: [pathToNode], } ) - - await codeManager.updateEditorWithAstAndWriteToFile( - updateAstResult.newAst - ) - - if (updateAstResult?.selections) { - editorManager.selectRange(updateAstResult?.selections) - } } ), loftAstMod: fromPromise(