Fix Shell point-and-click picking the wrong face with piped extrudes (#…

…4981) * [BUG] Shell point and click references the wrong feature Fixes #4961 * Add test for sketch on face based on extrudes in pipe * Add no extrude in pipe case * Lint * Add scene.waitForExecutionDone() * A snapshot a day keeps the bugs away! 📷🐛 (OS: namespace-profile-ubuntu-8-cores) * Trigger CI * A snapshot a day keeps the bugs away! 📷🐛 (OS: namespace-profile-ubuntu-8-cores) * Trigger CI * Update src/lang/modifyAst/addShell.ts Co-authored-by: Frank Noirot <[email protected]> --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Frank Noirot <[email protected]>
KittyCAD · Jan 10, 2025 · c9860af · c9860af
1 parent 23a42f0
commit c9860af
Show file tree

Hide file tree

Showing 2 changed files with 106 additions and 6 deletions.
diff --git a/e2e/playwright/point-click.spec.ts b/e2e/playwright/point-click.spec.ts
@@ -1119,3 +1119,93 @@ extrude001 = extrude(40, sketch001)
     await scene.expectPixelColor([99, 99, 99], testPoint, 15)
   })
 })
+
+const shellSketchOnFacesCases = [
+  `sketch001 = startSketchOn('XZ')
+  |> circle({ center = [0, 0], radius = 100 }, %)
+  |> extrude(100, %)
+
+sketch002 = startSketchOn(sketch001, 'END')
+  |> circle({ center = [0, 0], radius = 50 }, %)
+  |> extrude(50, %)
+  `,
+  `sketch001 = startSketchOn('XZ')
+  |> circle({ center = [0, 0], radius = 100 }, %)
+extrude001 = extrude(100, sketch001)
+
+sketch002 = startSketchOn(extrude001, 'END')
+  |> circle({ center = [0, 0], radius = 50 }, %)
+extrude002 = extrude(50, sketch002)
+  `,
+]
+shellSketchOnFacesCases.forEach((initialCode, index) => {
+  const hasExtrudesInPipe = index === 0
+  test(`Shell point-and-click sketch on face (extrudes in pipes: ${hasExtrudesInPipe})`, async ({
+    context,
+    page,
+    homePage,
+    scene,
+    editor,
+    toolbar,
+    cmdBar,
+  }) => {
+    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: 550, y: 295 }
+    const [clickOnCap] = scene.makeMouseHelpers(testPoint.x, testPoint.y)
+    const shellDeclaration = `shell001 = shell({ faces = ['end'], thickness = 5 }, ${
+      hasExtrudesInPipe ? 'sketch002' : 'extrude002'
+    })`
+
+    await test.step(`Look for the grey of the shape`, async () => {
+      await toolbar.closePane('code')
+      await scene.expectPixelColor([128, 128, 128], testPoint, 15)
+    })
+
+    await test.step(`Go through the command bar flow, selecting a cap and keeping default thickness`, async () => {
+      await toolbar.shellButton.click()
+      await cmdBar.expectState({
+        stage: 'arguments',
+        currentArgKey: 'selection',
+        currentArgValue: '',
+        headerArguments: {
+          Selection: '',
+          Thickness: '',
+        },
+        highlightedHeaderArg: 'selection',
+        commandName: 'Shell',
+      })
+      await clickOnCap()
+      await cmdBar.progressCmdBar()
+      await page.waitForTimeout(500)
+      await cmdBar.progressCmdBar()
+      await cmdBar.expectState({
+        stage: 'review',
+        headerArguments: {
+          Selection: '1 cap',
+          Thickness: '5',
+        },
+        commandName: 'Shell',
+      })
+      await cmdBar.progressCmdBar()
+    })
+
+    await test.step(`Confirm code is added to the editor, scene has changed`, async () => {
+      await toolbar.openPane('code')
+      await editor.expectEditor.toContain(shellDeclaration)
+      await editor.expectState({
+        diagnostics: [],
+        activeLines: [shellDeclaration],
+        highlightedCode: '',
+      })
+      await toolbar.closePane('code')
+      await scene.expectPixelColor([73, 73, 73], testPoint, 15)
+    })
+  })
+})
diff --git a/src/lang/modifyAst/addShell.ts b/src/lang/modifyAst/addShell.ts
@@ -49,17 +49,27 @@ export function addShell({
       return new Error("Couldn't find extrude")
     }
 
-    pathToExtrudeNode = extrudeLookupResult.pathToExtrudeNode
-    // Get the sketch ref from the selection
     // TODO: this assumes the segment is piped directly from the sketch, with no intermediate `VariableDeclarator` between.
     // We must find a technique for these situations that is robust to intermediate declarations
-    const sketchNode = getNodeFromPath<VariableDeclarator>(
+    const extrudeNode = getNodeFromPath<VariableDeclarator>(
       modifiedAst,
-      graphSelection.codeRef.pathToNode,
+      extrudeLookupResult.pathToExtrudeNode,
       'VariableDeclarator'
     )
-    if (err(sketchNode)) {
-      return sketchNode
+    const segmentNode = getNodeFromPath<VariableDeclarator>(
+      modifiedAst,
+      extrudeLookupResult.pathToSegmentNode,
+      'VariableDeclarator'
+    )
+    if (err(extrudeNode) || err(segmentNode)) {
+      return new Error("Couldn't find extrude")
+    }
+    if (extrudeNode.node.init.type === 'CallExpression') {
+      pathToExtrudeNode = extrudeLookupResult.pathToExtrudeNode
+    } else if (segmentNode.node.init.type === 'PipeExpression') {
+      pathToExtrudeNode = extrudeLookupResult.pathToSegmentNode
+    } else {
+      return new Error("Couldn't find extrude")
     }
 
     const selectedArtifact = graphSelection.artifact