Allow point-and-click Offset Plane to work from sweep faces (#7882)

* WIP: Offset Plane point-and-click for sweep faces
Edit flows not working yet
Will eventually close #7555

* Typo

* Cont fixing of tests

* Rm px coordinate from test

* Clean up faces and unit tests for default planes

* Add offset plane on offset plane unit test

* Add offset plane on face unit tests

* Add face offset tests

* WIP: Add PlaneOfFace client-side artifact

* Fix rust errors

* Working unit tests and edit flows

* Remove log, thanks bot

* Allow face selection after command starts

* Update snapshots

* Move artifact creation logic to big match on commands

* fmt

* Update Mermaid snapshot in plane_of test

* Update snapshots

---------

Co-authored-by: Jonathan Tran <[email protected]>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Adam Chalmers <[email protected]>

Author: 4 people

e2e/playwright/feature-tree-pane.spec.ts

@@ -394,26 +394,23 @@ test.describe('Feature Tree pane', () => {
       await cmdBar.expectState({
         commandName: 'Offset plane',
         stage: 'arguments',
-        currentArgKey: 'distance',
+        currentArgKey: 'offset',
         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()
       await cmdBar.expectState({
         stage: 'review',
         headerArguments: {
-          Plane: '1 plane',
-          // We show the calculated value in the argument summary
-          Distance: '15',
+          Offset: '15',
         },
         commandName: 'Offset plane',
       })

e2e/playwright/point-click.spec.ts

@@ -912,18 +912,10 @@ openSketch = startSketchOn(XY)
     toolbar,
     cmdBar,
   }) => {
-    // 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
@@ -933,30 +925,30 @@ openSketch = startSketchOn(XY)
         stage: 'arguments',
         currentArgKey: 'plane',
         currentArgValue: '',
-        headerArguments: { Plane: '', Distance: '' },
+        headerArguments: { Plane: '', Offset: '' },
         highlightedHeaderArg: 'plane',
         commandName: 'Offset plane',
       })
-      await clickOnXzPlane()
+      const xz = await toolbar.getFeatureTreeOperation('Front plane', 0)
+      await xz.click()
       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()
     })
 
-    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 () => {
@@ -967,7 +959,6 @@ openSketch = startSketchOn(XY)
       )
       await operationButton.click({ button: 'left' })
       await page.keyboard.press('Delete')
-      //await scene.expectPixelColor([50, 51, 96], testPoint, 15) // FIXME
     })
   })
 

e2e/playwright/snapshot-tests.spec.ts-snapshots/Draft-circle-should-look-right-1-Google-Chrome-linux.png

@@ -1,7 +1,7 @@
 use fnv::FnvHashMap;
 use indexmap::IndexMap;
 use kittycad_modeling_cmds::{
-    self as kcmc, EnableSketchMode, ModelingCmd,
+    self as kcmc, EnableSketchMode, FaceIsPlanar, ModelingCmd,
     ok_response::OkModelingCmdResponse,
     shared::ExtrusionFaceCapType,
     websocket::{BatchResponse, OkWebSocketResponseData, WebSocketResponse},
@@ -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),
@@ -346,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,
@@ -367,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,
@@ -387,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),
@@ -406,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),
@@ -760,6 +774,13 @@ fn artifacts_to_update(
                 code_ref,
             })]);
         }
+        ModelingCmd::FaceIsPlanar(FaceIsPlanar { object_id, .. }) => {
+            return Ok(vec![Artifact::PlaneOfFace(PlaneOfFace {
+                id,
+                face_id: object_id.into(),
+                code_ref,
+            })]);
+        }
         ModelingCmd::EnableSketchMode(EnableSketchMode { entity_id, .. }) => {
             let existing_plane = artifacts.get(&ArtifactId::new(*entity_id));
             match existing_plane {

rust/kcl-lib/src/execution/artifact.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<br>{:?}\"]",
+                    code_ref_display(code_ref)
+                )?;
+                node_path_display(output, prefix, None, code_ref)?;
+            }
             Artifact::Sweep(sweep) => {
                 writeln!(
                     output,

rust/kcl-lib/src/execution/artifact/mermaid_tests.rs

@@ -13,10 +13,8 @@ flowchart LR
       %% [ProgramBodyItem { index: 0 }, VariableDeclarationDeclaration, VariableDeclarationInit, PipeBodyItem { index: 1 }]
     7[Solid2d]
   end
-  subgraph path21 [Path]
-    21["Path<br>[311, 361, 0]"]
-      %% [ProgramBodyItem { index: 2 }, ExpressionStatementExpr, PipeBodyItem { index: 1 }]
-    22["Segment<br>[311, 361, 0]"]
+  subgraph path22 [Path]
+    22["Path<br>[311, 361, 0]"]
       %% [ProgramBodyItem { index: 2 }, ExpressionStatementExpr, PipeBodyItem { index: 1 }]
     23["Segment<br>[311, 361, 0]"]
       %% [ProgramBodyItem { index: 2 }, ExpressionStatementExpr, PipeBodyItem { index: 1 }]
@@ -26,7 +24,9 @@ flowchart LR
       %% [ProgramBodyItem { index: 2 }, ExpressionStatementExpr, PipeBodyItem { index: 1 }]
     26["Segment<br>[311, 361, 0]"]
       %% [ProgramBodyItem { index: 2 }, ExpressionStatementExpr, PipeBodyItem { index: 1 }]
-    27[Solid2d]
+    27["Segment<br>[311, 361, 0]"]
+      %% [ProgramBodyItem { index: 2 }, ExpressionStatementExpr, PipeBodyItem { index: 1 }]
+    28[Solid2d]
   end
   1["Plane<br>[41, 58, 0]"]
     %% [ProgramBodyItem { index: 0 }, VariableDeclarationDeclaration, VariableDeclarationInit, PipeBodyItem { index: 0 }]
@@ -48,9 +48,11 @@ flowchart LR
   17["SweepEdge Adjacent"]
   18["SweepEdge Opposite"]
   19["SweepEdge Adjacent"]
-  20["Plane<br>[277, 304, 0]"]
+  20["PlaneOfFace<br>[184, 208, 0]"]
+    %% [ProgramBodyItem { index: 1 }, VariableDeclarationDeclaration, VariableDeclarationInit]
+  21["Plane<br>[277, 304, 0]"]
     %% [ProgramBodyItem { index: 2 }, ExpressionStatementExpr, PipeBodyItem { index: 0 }, CallKwUnlabeledArg]
-  28["StartSketchOnPlane<br>[263, 305, 0]"]
+  29["StartSketchOnPlane<br>[263, 305, 0]"]
     %% [ProgramBodyItem { index: 2 }, ExpressionStatementExpr, PipeBodyItem { index: 0 }]
   1 --- 2
   2 --- 3
@@ -94,12 +96,13 @@ flowchart LR
   14 <--x 13
   16 <--x 13
   18 <--x 13
-  20 --- 21
-  20 <--x 28
+  13 <--x 20
   21 --- 22
-  21 --- 23
-  21 --- 24
-  21 --- 25
-  21 --- 26
-  21 --- 27
+  21 <--x 29
+  22 --- 23
+  22 --- 24
+  22 --- 25
+  22 --- 26
+  22 --- 27
+  22 --- 28
 ```

rust/kcl-lib/tests/plane_of/artifact_graph_flowchart.snap.md

@@ -53,10 +53,7 @@ function CommandBarSelectionInput({
   useEffect(() => {
     if (arg.selectionTypes.includes('plane') && !canSubmitSelection) {
       toSync(() => {
-        return Promise.all([
-          kclManager.showPlanes(),
-          kclManager.setSelectionFilter(['plane', 'object']),
-        ])
+        return kclManager.showPlanes()
       }, reportRejection)()
     }
 

src/components/CommandBar/CommandBarSelectionInput.tsx

@@ -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<Program>
-  plane: Node<Literal> | Node<Name> // Can be DefaultPlaneStr or string for offsetPlanes
-  insertIndex?: number
-  offset: Expr
-  planeName?: string
-}): { modifiedAst: Node<Program>; 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
  */