From ffb255978775ffff7af937100710fa1f82e2033f Mon Sep 17 00:00:00 2001 From: Pierre Jacquier Date: Fri, 17 Jan 2025 12:02:50 -0500 Subject: [PATCH 1/6] Add dry-run validation for Sweep Fixes #5095 --- e2e/playwright/point-click.spec.ts | 10 +-- .../modelingCommandConfig.ts | 8 +-- src/lib/commandBarConfigs/validators.ts | 61 +++++++++++++++++++ 3 files changed, 67 insertions(+), 12 deletions(-) diff --git a/e2e/playwright/point-click.spec.ts b/e2e/playwright/point-click.spec.ts index 3afce11121..002ca3cc9c 100644 --- a/e2e/playwright/point-click.spec.ts +++ b/e2e/playwright/point-click.spec.ts @@ -985,15 +985,9 @@ sketch002 = startSketchOn('XZ') stage: 'arguments', }) await clickOnSketch2() - await cmdBar.expectState({ - commandName: 'Sweep', - headerArguments: { - Path: '1 face', - Profile: '1 face', - }, - stage: 'review', - }) + await page.waitForTimeout(500) await cmdBar.progressCmdBar() + await page.waitForTimeout(500) }) await test.step(`Confirm code is added to the editor, scene has changed`, async () => { diff --git a/src/lib/commandBarConfigs/modelingCommandConfig.ts b/src/lib/commandBarConfigs/modelingCommandConfig.ts index 8b7c9a760f..d317674193 100644 --- a/src/lib/commandBarConfigs/modelingCommandConfig.ts +++ b/src/lib/commandBarConfigs/modelingCommandConfig.ts @@ -13,6 +13,7 @@ import { loftValidator, revolveAxisValidator, shellValidator, + sweepValidator, } from './validators' type OutputFormat = Models['OutputFormat_type'] @@ -308,7 +309,7 @@ export const modelingMachineCommandConfig: StateMachineCommandSetConfig< 'Create a 3D body by moving a sketch region along an arbitrary path.', icon: 'sweep', status: 'development', - needsReview: true, + needsReview: false, args: { profile: { inputType: 'selection', @@ -316,7 +317,6 @@ export const modelingMachineCommandConfig: StateMachineCommandSetConfig< required: true, skip: true, multiple: false, - // TODO: add dry-run validation warningMessage: 'The sweep workflow is new and under tested. Please break it and report issues.', }, @@ -324,9 +324,9 @@ export const modelingMachineCommandConfig: StateMachineCommandSetConfig< inputType: 'selection', selectionTypes: ['segment', 'path'], required: true, - skip: true, + skip: false, multiple: false, - // TODO: add dry-run validation + validation: sweepValidator, }, }, }, diff --git a/src/lib/commandBarConfigs/validators.ts b/src/lib/commandBarConfigs/validators.ts index 0032187719..8ea67e4e32 100644 --- a/src/lib/commandBarConfigs/validators.ts +++ b/src/lib/commandBarConfigs/validators.ts @@ -207,3 +207,64 @@ export const shellValidator = async ({ return 'Unable to shell with the provided selection' } + +export const sweepValidator = async ({ + context, + data, +}: { + context: CommandBarContext + data: { path: Selections } +}): Promise => { + if (!isSelections(data.path)) { + console.log('Unable to sweep, selections are missing') + return 'Unable to sweep, selections are missing' + } + + // Retrieve the parent path from the segment selection directly + const trajectoryArtifact = data.path.graphSelections[0].artifact + if (!trajectoryArtifact) { + return "Unable to sweep, couldn't find the trajectory artifact" + } + if (trajectoryArtifact.type !== 'segment') { + return "Unable to sweep, couldn't find the target from a non-segment selection" + } + const trajectory = trajectoryArtifact.pathId + + // Get the former arg in the command bar flow, and retrieve the path from the solid2d directly + const profileArg = context.argumentsToSubmit['profile'] as Selections + const profileArtifact = profileArg.graphSelections[0].artifact + if (!profileArtifact) { + return "Unable to sweep, couldn't find the profile artifact" + } + if (profileArtifact.type !== 'solid2D') { + return "Unable to sweep, couldn't find the target from a non-solid2d selection" + } + const target = profileArtifact.pathId + + const sweepCommand = async () => { + // TODO: second look on defaults here + const DEFAULT_TOLERANCE: Models['LengthUnit_type'] = 1e-7 + const DEFAULT_SECTIONAL = false + const cmdArgs = { + target, + trajectory, + sectional: DEFAULT_SECTIONAL, + tolerance: DEFAULT_TOLERANCE, + } + return await engineCommandManager.sendSceneCommand({ + type: 'modeling_cmd_req', + cmd_id: uuidv4(), + cmd: { + type: 'sweep', + ...cmdArgs, + }, + }) + } + + const attemptSweep = await dryRunWrapper(sweepCommand) + if (attemptSweep?.success) { + return true + } + + return 'Unable to sweep with the provided selection' +} From 7be40018390d0af2bd5444a9d218afec5f3b37c6 Mon Sep 17 00:00:00 2001 From: Pierre Jacquier Date: Fri, 17 Jan 2025 12:08:51 -0500 Subject: [PATCH 2/6] Add sweep test failing validation --- e2e/playwright/point-click.spec.ts | 69 ++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/e2e/playwright/point-click.spec.ts b/e2e/playwright/point-click.spec.ts index 002ca3cc9c..88bad61d15 100644 --- a/e2e/playwright/point-click.spec.ts +++ b/e2e/playwright/point-click.spec.ts @@ -1014,6 +1014,75 @@ sketch002 = startSketchOn('XZ') }) }) +test(`Sweep point-and-click failing validation`, async ({ + context, + page, + homePage, + scene, + toolbar, + cmdBar, +}) => { + const initialCode = `sketch001 = startSketchOn('YZ') + |> circle({ + center = [0, 0], + radius = 500 + }, %) +sketch002 = startSketchOn('XZ') + |> startProfileAt([0, 0], %) + |> xLine(-500, %) + |> lineTo([-2000, 500], %) +` + await context.addInitScript((initialCode) => { + localStorage.setItem('persistCode', initialCode) + }, initialCode) + await page.setBodyDimensions({ width: 1000, height: 500 }) + await homePage.goToModelingScene() + await scene.waitForExecutionDone() + + // One dumb hardcoded screen pixel value + const testPoint = { x: 700, y: 250 } + const [clickOnSketch1] = scene.makeMouseHelpers(testPoint.x, testPoint.y) + const [clickOnSketch2] = scene.makeMouseHelpers(testPoint.x - 50, testPoint.y) + + await test.step(`Look for sketch001`, async () => { + await toolbar.closePane('code') + await scene.expectPixelColor([53, 53, 53], testPoint, 15) + }) + + await test.step(`Go through the command bar flow and fail validation with a toast`, async () => { + await toolbar.sweepButton.click() + await cmdBar.expectState({ + commandName: 'Sweep', + currentArgKey: 'profile', + currentArgValue: '', + headerArguments: { + Path: '', + Profile: '', + }, + highlightedHeaderArg: 'profile', + stage: 'arguments', + }) + await clickOnSketch1() + await cmdBar.expectState({ + commandName: 'Sweep', + currentArgKey: 'path', + currentArgValue: '', + headerArguments: { + Path: '', + Profile: '1 face', + }, + highlightedHeaderArg: 'path', + stage: 'arguments', + }) + await clickOnSketch2() + await page.waitForTimeout(500) + await cmdBar.progressCmdBar() + await expect( + page.getByText('Unable to sweep with the provided selection') + ).toBeVisible() + }) +}) + test(`Fillet point-and-click`, async ({ context, page, From 16b5eeadb16dcbc9b4cbf1f07536c42547d71d68 Mon Sep 17 00:00:00 2001 From: Pierre Jacquier Date: Fri, 17 Jan 2025 12:25:07 -0500 Subject: [PATCH 3/6] Make naming more consistent with engine --- .../modelingCommandConfig.ts | 8 +++--- src/lib/commandBarConfigs/validators.ts | 16 +++++------ src/machines/modelingMachine.ts | 28 +++++++++---------- 3 files changed, 26 insertions(+), 26 deletions(-) diff --git a/src/lib/commandBarConfigs/modelingCommandConfig.ts b/src/lib/commandBarConfigs/modelingCommandConfig.ts index d317674193..fb8e70bf8f 100644 --- a/src/lib/commandBarConfigs/modelingCommandConfig.ts +++ b/src/lib/commandBarConfigs/modelingCommandConfig.ts @@ -43,8 +43,8 @@ export type ModelingCommandSchema = { distance: KclCommandValue } Sweep: { - path: Selections - profile: Selections + target: Selections + trajectory: Selections } Loft: { selection: Selections @@ -311,7 +311,7 @@ export const modelingMachineCommandConfig: StateMachineCommandSetConfig< status: 'development', needsReview: false, args: { - profile: { + target: { inputType: 'selection', selectionTypes: ['solid2D'], required: true, @@ -320,7 +320,7 @@ export const modelingMachineCommandConfig: StateMachineCommandSetConfig< warningMessage: 'The sweep workflow is new and under tested. Please break it and report issues.', }, - path: { + trajectory: { inputType: 'selection', selectionTypes: ['segment', 'path'], required: true, diff --git a/src/lib/commandBarConfigs/validators.ts b/src/lib/commandBarConfigs/validators.ts index 8ea67e4e32..1b752b4b45 100644 --- a/src/lib/commandBarConfigs/validators.ts +++ b/src/lib/commandBarConfigs/validators.ts @@ -213,15 +213,15 @@ export const sweepValidator = async ({ data, }: { context: CommandBarContext - data: { path: Selections } + data: { trajectory: Selections } }): Promise => { - if (!isSelections(data.path)) { + if (!isSelections(data.trajectory)) { console.log('Unable to sweep, selections are missing') return 'Unable to sweep, selections are missing' } // Retrieve the parent path from the segment selection directly - const trajectoryArtifact = data.path.graphSelections[0].artifact + const trajectoryArtifact = data.trajectory.graphSelections[0].artifact if (!trajectoryArtifact) { return "Unable to sweep, couldn't find the trajectory artifact" } @@ -231,15 +231,15 @@ export const sweepValidator = async ({ const trajectory = trajectoryArtifact.pathId // Get the former arg in the command bar flow, and retrieve the path from the solid2d directly - const profileArg = context.argumentsToSubmit['profile'] as Selections - const profileArtifact = profileArg.graphSelections[0].artifact - if (!profileArtifact) { + const targetArg = context.argumentsToSubmit['target'] as Selections + const targetArtifact = targetArg.graphSelections[0].artifact + if (!targetArtifact) { return "Unable to sweep, couldn't find the profile artifact" } - if (profileArtifact.type !== 'solid2D') { + if (targetArtifact.type !== 'solid2D') { return "Unable to sweep, couldn't find the target from a non-solid2d selection" } - const target = profileArtifact.pathId + const target = targetArtifact.pathId const sweepCommand = async () => { // TODO: second look on defaults here diff --git a/src/machines/modelingMachine.ts b/src/machines/modelingMachine.ts index a3e4426a61..6ea012a96e 100644 --- a/src/machines/modelingMachine.ts +++ b/src/machines/modelingMachine.ts @@ -1561,40 +1561,40 @@ export const modelingMachine = setup({ if (!input) return new Error('No input provided') // Extract inputs const ast = kclManager.ast - const { profile, path } = input + const { target, trajectory } = input // Find the profile declaration - const profileNodePath = getNodePathFromSourceRange( + const targetNodePath = getNodePathFromSourceRange( ast, - profile.graphSelections[0].codeRef.range + target.graphSelections[0].codeRef.range ) - const profileNode = getNodeFromPath( + const targetNode = getNodeFromPath( ast, - profileNodePath, + targetNodePath, 'VariableDeclarator' ) - if (err(profileNode)) { + if (err(targetNode)) { return new Error("Couldn't parse profile selection") } - const profileDeclarator = profileNode.node + const targetDeclarator = targetNode.node // Find the path declaration - const pathNodePath = getNodePathFromSourceRange( + const trajectoryNodePath = getNodePathFromSourceRange( ast, - path.graphSelections[0].codeRef.range + trajectory.graphSelections[0].codeRef.range ) - const pathNode = getNodeFromPath( + const trajectoryNode = getNodeFromPath( ast, - pathNodePath, + trajectoryNodePath, 'VariableDeclarator' ) - if (err(pathNode)) { + if (err(trajectoryNode)) { return new Error("Couldn't parse path selection") } - const pathDeclarator = pathNode.node + const trajectoryDeclarator = trajectoryNode.node // Perform the sweep - const sweepRes = addSweep(ast, profileDeclarator, pathDeclarator) + const sweepRes = addSweep(ast, targetDeclarator, trajectoryDeclarator) const updateAstResult = await kclManager.updateAst( sweepRes.modifiedAst, true, From 2fe5ef7034b5da5d3bc53db3d7e69d65350e40d1 Mon Sep 17 00:00:00 2001 From: Pierre Jacquier Date: Fri, 17 Jan 2025 13:22:58 -0500 Subject: [PATCH 4/6] Fix tests after big rename --- e2e/playwright/point-click.spec.ts | 32 +++++++++++++++--------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/e2e/playwright/point-click.spec.ts b/e2e/playwright/point-click.spec.ts index 88bad61d15..76fbc60c17 100644 --- a/e2e/playwright/point-click.spec.ts +++ b/e2e/playwright/point-click.spec.ts @@ -963,25 +963,25 @@ sketch002 = startSketchOn('XZ') await toolbar.sweepButton.click() await cmdBar.expectState({ commandName: 'Sweep', - currentArgKey: 'profile', + currentArgKey: 'target', currentArgValue: '', headerArguments: { - Path: '', - Profile: '', + Target: '', + Trajectory: '', }, - highlightedHeaderArg: 'profile', + highlightedHeaderArg: 'target', stage: 'arguments', }) await clickOnSketch1() await cmdBar.expectState({ commandName: 'Sweep', - currentArgKey: 'path', + currentArgKey: 'trajectory', currentArgValue: '', headerArguments: { - Path: '', - Profile: '1 face', + Target: '1 face', + Trajectory: '', }, - highlightedHeaderArg: 'path', + highlightedHeaderArg: 'trajectory', stage: 'arguments', }) await clickOnSketch2() @@ -1053,25 +1053,25 @@ sketch002 = startSketchOn('XZ') await toolbar.sweepButton.click() await cmdBar.expectState({ commandName: 'Sweep', - currentArgKey: 'profile', + currentArgKey: 'target', currentArgValue: '', headerArguments: { - Path: '', - Profile: '', + Target: '', + Trajectory: '', }, - highlightedHeaderArg: 'profile', + highlightedHeaderArg: 'target', stage: 'arguments', }) await clickOnSketch1() await cmdBar.expectState({ commandName: 'Sweep', - currentArgKey: 'path', + currentArgKey: 'trajectory', currentArgValue: '', headerArguments: { - Path: '', - Profile: '1 face', + Target: '1 face', + Trajectory: '', }, - highlightedHeaderArg: 'path', + highlightedHeaderArg: 'trajectory', stage: 'arguments', }) await clickOnSketch2() From 54da18d8ab59ee87045464476699f53d120072da Mon Sep 17 00:00:00 2001 From: Pierre Jacquier Date: Fri, 17 Jan 2025 14:03:51 -0500 Subject: [PATCH 5/6] WIP: Allow feature tree selection for point-and-click Sweep Relates to #5101 --- .../modelingCommandConfig.ts | 4 +-- src/lib/commandBarConfigs/validators.ts | 27 ++++++++++++------- 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/src/lib/commandBarConfigs/modelingCommandConfig.ts b/src/lib/commandBarConfigs/modelingCommandConfig.ts index fb8e70bf8f..89d17f81ba 100644 --- a/src/lib/commandBarConfigs/modelingCommandConfig.ts +++ b/src/lib/commandBarConfigs/modelingCommandConfig.ts @@ -313,7 +313,7 @@ export const modelingMachineCommandConfig: StateMachineCommandSetConfig< args: { target: { inputType: 'selection', - selectionTypes: ['solid2D'], + selectionTypes: ['solid2D', 'plane'], required: true, skip: true, multiple: false, @@ -322,7 +322,7 @@ export const modelingMachineCommandConfig: StateMachineCommandSetConfig< }, trajectory: { inputType: 'selection', - selectionTypes: ['segment', 'path'], + selectionTypes: ['segment', 'plane'], required: true, skip: false, multiple: false, diff --git a/src/lib/commandBarConfigs/validators.ts b/src/lib/commandBarConfigs/validators.ts index 1b752b4b45..5e7ce2cb65 100644 --- a/src/lib/commandBarConfigs/validators.ts +++ b/src/lib/commandBarConfigs/validators.ts @@ -222,24 +222,31 @@ export const sweepValidator = async ({ // Retrieve the parent path from the segment selection directly const trajectoryArtifact = data.trajectory.graphSelections[0].artifact - if (!trajectoryArtifact) { - return "Unable to sweep, couldn't find the trajectory artifact" + let trajectory: string | undefined = undefined + if (trajectoryArtifact && trajectoryArtifact.type === 'segment') { + trajectory = trajectoryArtifact.pathId + } else if (trajectoryArtifact && trajectoryArtifact.type === 'plane') { + // TODO: check again after multi profile + trajectory = trajectoryArtifact.pathIds[0] } - if (trajectoryArtifact.type !== 'segment') { - return "Unable to sweep, couldn't find the target from a non-segment selection" + + if (!trajectory) { + return "Unable to sweep, couldn't find the trajectory artifact" } - const trajectory = trajectoryArtifact.pathId // Get the former arg in the command bar flow, and retrieve the path from the solid2d directly const targetArg = context.argumentsToSubmit['target'] as Selections const targetArtifact = targetArg.graphSelections[0].artifact - if (!targetArtifact) { - return "Unable to sweep, couldn't find the profile artifact" + let target: string | undefined = undefined + if (targetArtifact && targetArtifact.type === 'solid2D') { + target = targetArtifact.pathId + } else if (targetArtifact && targetArtifact.type === 'plane') { + target = targetArtifact.pathIds[0] } - if (targetArtifact.type !== 'solid2D') { - return "Unable to sweep, couldn't find the target from a non-solid2d selection" + + if (!target) { + return "Unable to sweep, couldn't find the profile artifact" } - const target = targetArtifact.pathId const sweepCommand = async () => { // TODO: second look on defaults here From d53172867532405e00898c27058b47b79c0deb7b Mon Sep 17 00:00:00 2001 From: Pierre Jacquier Date: Fri, 17 Jan 2025 14:52:10 -0500 Subject: [PATCH 6/6] Fix merge issue --- src/lib/commandBarConfigs/modelingCommandConfig.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/lib/commandBarConfigs/modelingCommandConfig.ts b/src/lib/commandBarConfigs/modelingCommandConfig.ts index 00c0542f46..b9b791781a 100644 --- a/src/lib/commandBarConfigs/modelingCommandConfig.ts +++ b/src/lib/commandBarConfigs/modelingCommandConfig.ts @@ -313,11 +313,7 @@ export const modelingMachineCommandConfig: StateMachineCommandSetConfig< args: { target: { inputType: 'selection', - selectionTypes: [' - - - - ', 'plane'], + selectionTypes: ['solid2d', 'plane'], required: true, skip: true, multiple: false,