Fix sketch-on-face sketches to be in the artifact graph (#5489)

* Add code ref to sketch-on-face artifacts

* Fix to add execution artifacts to the artifact graph

* Update output after including exec artifacts

* A snapshot a day keeps the bugs away! 📷🐛 (OS: namespace-profile-ubuntu-8-cores)

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

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

@@ -201,7 +201,7 @@ test.describe('Feature Tree pane', () => {
         await toolbar.exitSketchBtn.click()
       })
 
-      await test.step('On an offset plane should *not* work', async () => {
+      await test.step('On an offset plane should work', async () => {
         // Tooltip is getting in the way of clicking, so I'm first closing the pane
         await toolbar.closeFeatureTreePane()
         await (await toolbar.getFeatureTreeOperation('Sketch', 2)).dblclick()
@@ -212,13 +212,7 @@ test.describe('Feature Tree pane', () => {
         })
         await expect(
           toolbar.exitSketchBtn,
-          'We should not be in sketch mode now'
+          'We should be in sketch mode now'
-        ).not.toBeVisible()
-        await expect(
-          page.getByText(
-            'Editing sketches on faces or offset planes through the feature tree is not yet supported'
-          ),
-          'We should see a toast message about this'
         ).toBeVisible()
       })
     }

src/components/ModelingMachineProvider.tsx

@@ -666,6 +666,13 @@ export const ModelingMachineProvider = ({
           if (event.data?.forceNewSketch) return false
           if (artifactIsPlaneWithPaths(selectionRanges)) {
             return true
+          } else if (selectionRanges.graphSelections[0]?.artifact) {
+            // See if the selection is "close enough" to be coerced to the plane later
+            const maybePlane = getPlaneFromArtifact(
+              selectionRanges.graphSelections[0].artifact,
+              engineCommandManager.artifactGraph
+            )
+            return !err(maybePlane)
           }
           if (
             isCursorInFunctionDefinition(

src/lang/queryAst.ts

@@ -39,7 +39,7 @@ import {
 import { err, Reason } from 'lib/trap'
 import { Node } from 'wasm-lib/kcl/bindings/Node'
 import { findKwArg } from './util'
-import { codeRefFromRange } from './std/artifactGraph'
+import { codeRefFromRange, getPlaneFromArtifact } from './std/artifactGraph'
 import { FunctionExpression } from 'wasm-lib/kcl/bindings/FunctionExpression'
 import { ImportStatement } from 'wasm-lib/kcl/bindings/ImportStatement'
 import { KclSettingsAnnotation } from 'lib/settings/settingsTypes'

src/lang/std/artifactGraph.ts

@@ -20,7 +20,13 @@ import { Models } from '@kittycad/lib'
 import { getNodePathFromSourceRange } from 'lang/queryAstNodePathUtils'
 import { Selection } from 'lib/selections'
 import { err } from 'lib/trap'
-import { Cap, Plane, Wall } from 'wasm-lib/kcl/bindings/Artifact'
+import {
+  Cap,
+  Plane,
+  StartSketchOnFace,
+  StartSketchOnPlane,
+  Wall,
+} from 'wasm-lib/kcl/bindings/Artifact'
 import { CapSubType } from 'wasm-lib/kcl/bindings/Artifact'
 
 export type { Artifact, ArtifactId, SegmentArtifact } from 'lang/wasm'
@@ -610,6 +616,26 @@ function getPlaneFromSweepEdge(edge: SweepEdge, graph: ArtifactGraph) {
   if (err(path)) return path
   return getPlaneFromPath(path, graph)
 }
+function getPlaneFromStartSketchOnFace(
+  sketch: StartSketchOnFace,
+  graph: ArtifactGraph
+) {
+  const plane = getArtifactOfTypes(
+    { key: sketch.faceId, types: ['plane'] },
+    graph
+  )
+  return plane
+}
+function getPlaneFromStartSketchOnPlane(
+  sketch: StartSketchOnPlane,
+  graph: ArtifactGraph
+) {
+  const plane = getArtifactOfTypes(
+    { key: sketch.planeId, types: ['plane'] },
+    graph
+  )
+  return plane
+}
 
 export function getPlaneFromArtifact(
   artifact: Artifact | undefined,
@@ -631,6 +657,10 @@ export function getPlaneFromArtifact(
   if (artifact.type === 'wall') return getPlaneFromWall(artifact, graph)
   if (artifact.type === 'sweepEdge')
     return getPlaneFromSweepEdge(artifact, graph)
+  if (artifact.type === 'startSketchOnFace')
+    return getPlaneFromStartSketchOnFace(artifact, graph)
+  if (artifact.type === 'startSketchOnPlane')
+    return getPlaneFromStartSketchOnPlane(artifact, graph)
   return new Error(`Artifact type ${artifact.type} does not have a plane`)
 }
 

src/wasm-lib/kcl/src/execution/artifact.rs

@@ -106,6 +106,15 @@ pub struct CodeRef {
     pub path_to_node: DummyPathToNode,
 }
 
+impl CodeRef {
+    pub fn placeholder(range: SourceRange) -> Self {
+        Self {
+            range,
+            path_to_node: Vec::new(),
+        }
+    }
+}
+
 #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, ts_rs::TS)]
 #[ts(export_to = "Artifact.ts")]
 #[serde(rename_all = "camelCase")]
@@ -178,6 +187,24 @@ pub struct Solid2d {
     pub path_id: ArtifactId,
 }
 
+#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, ts_rs::TS)]
+#[ts(export_to = "Artifact.ts")]
+#[serde(rename_all = "camelCase")]
+pub struct StartSketchOnFace {
+    pub id: ArtifactId,
+    pub face_id: ArtifactId,
+    pub code_ref: CodeRef,
+}
+
+#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, ts_rs::TS)]
+#[ts(export_to = "Artifact.ts")]
+#[serde(rename_all = "camelCase")]
+pub struct StartSketchOnPlane {
+    pub id: ArtifactId,
+    pub plane_id: ArtifactId,
+    pub code_ref: CodeRef,
+}
+
 #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, ts_rs::TS)]
 #[ts(export_to = "Artifact.ts")]
 #[serde(rename_all = "camelCase")]
@@ -295,18 +322,8 @@ pub enum Artifact {
     Path(Path),
     Segment(Segment),
     Solid2d(Solid2d),
-    #[serde(rename_all = "camelCase")]
+    StartSketchOnFace(StartSketchOnFace),
-    StartSketchOnFace {
+    StartSketchOnPlane(StartSketchOnPlane),
-        id: ArtifactId,
-        face_id: Uuid,
-        source_range: SourceRange,
-    },
-    #[serde(rename_all = "camelCase")]
-    StartSketchOnPlane {
-        id: ArtifactId,
-        plane_id: Uuid,
-        source_range: SourceRange,
-    },
     Sweep(Sweep),
     Wall(Wall),
     Cap(Cap),
@@ -323,8 +340,8 @@ impl Artifact {
             Artifact::Path(a) => a.id,
             Artifact::Segment(a) => a.id,
             Artifact::Solid2d(a) => a.id,
-            Artifact::StartSketchOnFace { id, .. } => *id,
+            Artifact::StartSketchOnFace(a) => a.id,
-            Artifact::StartSketchOnPlane { id, .. } => *id,
+            Artifact::StartSketchOnPlane(a) => a.id,
             Artifact::Sweep(a) => a.id,
             Artifact::Wall(a) => a.id,
             Artifact::Cap(a) => a.id,
@@ -342,9 +359,8 @@ impl Artifact {
             Artifact::Path(a) => Some(&a.code_ref),
             Artifact::Segment(a) => Some(&a.code_ref),
             Artifact::Solid2d(_) => None,
-            // TODO: We should add code refs for these.
+            Artifact::StartSketchOnFace(a) => Some(&a.code_ref),
-            Artifact::StartSketchOnFace { .. } => None,
+            Artifact::StartSketchOnPlane(a) => Some(&a.code_ref),
-            Artifact::StartSketchOnPlane { .. } => None,
             Artifact::Sweep(a) => Some(&a.code_ref),
             Artifact::Wall(_) => None,
             Artifact::Cap(_) => None,
@@ -507,6 +523,10 @@ pub(super) fn build_artifact_graph(
         }
     }
 
+    for exec_artifact in exec_artifacts.values() {
+        merge_artifact_into_map(&mut map, exec_artifact.clone());
+    }
+
     Ok(ArtifactGraph { map })
 }
 
@@ -841,15 +861,15 @@ fn artifacts_to_update(
                     })
                 })?;
                 let extra_artifact = exec_artifacts.values().find(|a| {
-                    if let Artifact::StartSketchOnFace { face_id: id, .. } = a {
+                    if let Artifact::StartSketchOnFace(s) = a {
-                        *id == face_id.0
+                        s.face_id == face_id
                     } else {
                         false
                     }
                 });
                 let sketch_on_face_source_range = extra_artifact
                     .and_then(|a| match a {
-                        Artifact::StartSketchOnFace { source_range, .. } => Some(*source_range),
+                        Artifact::StartSketchOnFace(s) => Some(s.code_ref.range),
                         // TODO: If we didn't find it, it's probably a bug.
                         _ => None,
                     })
@@ -894,15 +914,15 @@ fn artifacts_to_update(
                         })
                     })?;
                     let extra_artifact = exec_artifacts.values().find(|a| {
-                        if let Artifact::StartSketchOnFace { face_id: id, .. } = a {
+                        if let Artifact::StartSketchOnFace(s) = a {
-                            *id == face_id.0
+                            s.face_id == face_id
                         } else {
                             false
                         }
                     });
                     let sketch_on_face_source_range = extra_artifact
                         .and_then(|a| match a {
-                            Artifact::StartSketchOnFace { source_range, .. } => Some(*source_range),
+                            Artifact::StartSketchOnFace(s) => Some(s.code_ref.range),
                             _ => None,
                         })
                         .unwrap_or_default();

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

@@ -71,8 +71,8 @@ impl Artifact {
             Artifact::Path(a) => vec![a.plane_id],
             Artifact::Segment(a) => vec![a.path_id],
             Artifact::Solid2d(a) => vec![a.path_id],
-            Artifact::StartSketchOnFace { face_id, .. } => vec![face_id.into()],
+            Artifact::StartSketchOnFace(a) => vec![a.face_id],
-            Artifact::StartSketchOnPlane { plane_id, .. } => vec![plane_id.into()],
+            Artifact::StartSketchOnPlane(a) => vec![a.plane_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],
@@ -115,8 +115,14 @@ impl Artifact {
                 // Note: Don't include these since they're parents: path_id.
                 Vec::new()
             }
-            Artifact::StartSketchOnFace { .. } => Vec::new(),
+            Artifact::StartSketchOnFace { .. } => {
-            Artifact::StartSketchOnPlane { .. } => Vec::new(),
+                // Note: Don't include these since they're parents: face_id.
+                Vec::new()
+            }
+            Artifact::StartSketchOnPlane { .. } => {
+                // Note: Don't include these since they're parents: plane_id.
+                Vec::new()
+            }
             Artifact::Sweep(a) => {
                 // Note: Don't include these since they're parents: path_id.
                 let mut ids = Vec::new();
@@ -267,9 +273,7 @@ impl ArtifactGraph {
     ) -> std::fmt::Result {
         // For now, only showing the source range.
         fn code_ref_display(code_ref: &CodeRef) -> [usize; 3] {
-            range_display(code_ref.range)
+            let range = code_ref.range;
-        }
-        fn range_display(range: SourceRange) -> [usize; 3] {
             [range.start(), range.end(), range.module_id().as_usize()]
         }
 
@@ -301,20 +305,20 @@ impl ArtifactGraph {
             Artifact::Solid2d(_solid2d) => {
                 writeln!(output, "{prefix}{}[Solid2d]", id)?;
             }
-            Artifact::StartSketchOnFace { source_range, .. } => {
+            Artifact::StartSketchOnFace(StartSketchOnFace { code_ref, .. }) => {
                 writeln!(
                     output,
                     "{prefix}{}[\"StartSketchOnFace<br>{:?}\"]",
                     id,
-                    range_display(*source_range)
+                    code_ref_display(code_ref)
                 )?;
             }
-            Artifact::StartSketchOnPlane { source_range, .. } => {
+            Artifact::StartSketchOnPlane(StartSketchOnPlane { code_ref, .. }) => {
                 writeln!(
                     output,
                     "{prefix}{}[\"StartSketchOnPlane<br>{:?}\"]",
                     id,
-                    range_display(*source_range)
+                    code_ref_display(code_ref)
                 )?;
             }
             Artifact::Sweep(sweep) => {

src/wasm-lib/kcl/src/execution/mod.rs

@@ -3,7 +3,9 @@
 use std::{path::PathBuf, sync::Arc};
 
 use anyhow::Result;
-pub use artifact::{Artifact, ArtifactCommand, ArtifactGraph, ArtifactId};
+pub use artifact::{
+    Artifact, ArtifactCommand, ArtifactGraph, ArtifactId, CodeRef, StartSketchOnFace, StartSketchOnPlane,
+};
 use cache::OldAstState;
 pub use cache::{bust_cache, clear_mem_cache};
 pub use cad_op::Operation;

src/wasm-lib/kcl/src/std/sketch.rs

@@ -11,6 +11,7 @@ use parse_display::{Display, FromStr};
 use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
 
+use crate::execution::{CodeRef, StartSketchOnFace, StartSketchOnPlane};
 use crate::{
     errors::{KclError, KclErrorDetails},
     execution::{
@@ -1150,11 +1151,11 @@ async fn inner_start_sketch_on(
             } else {
                 // Create artifact used only by the UI, not the engine.
                 let id = exec_state.next_uuid();
-                exec_state.add_artifact(Artifact::StartSketchOnPlane {
+                exec_state.add_artifact(Artifact::StartSketchOnPlane(StartSketchOnPlane {
                     id: ArtifactId::from(id),
-                    plane_id: plane.id,
+                    plane_id: plane.artifact_id,
-                    source_range: args.source_range,
+                    code_ref: CodeRef::placeholder(args.source_range),
-                });
+                }));
 
                 Ok(SketchSurface::Plane(plane))
             }
@@ -1170,11 +1171,11 @@ async fn inner_start_sketch_on(
 
             // Create artifact used only by the UI, not the engine.
             let id = exec_state.next_uuid();
-            exec_state.add_artifact(Artifact::StartSketchOnFace {
+            exec_state.add_artifact(Artifact::StartSketchOnFace(StartSketchOnFace {
                 id: ArtifactId::from(id),
-                face_id: face.id,
+                face_id: face.artifact_id,
-                source_range: args.source_range,
+                code_ref: CodeRef::placeholder(args.source_range),
-            });
+            }));
 
             Ok(SketchSurface::Face(face))
         }

src/wasm-lib/kcl/tests/artifact_graph_example_code1/artifact_graph_flowchart.snap.md

@@ -45,6 +45,7 @@ flowchart LR
   39["SweepEdge Adjacent"]
   40["SweepEdge Opposite"]
   41["SweepEdge Adjacent"]
+  42["StartSketchOnFace<br>[345, 377, 0]"]
   1 --- 2
   2 --- 3
   2 --- 4
@@ -106,4 +107,5 @@ flowchart LR
   31 --- 39
   31 --- 40
   31 --- 41
+  11 <--x 42
 ```

src/wasm-lib/kcl/tests/artifact_graph_example_code_offset_planes/artifact_graph_flowchart.snap.md

@@ -7,6 +7,8 @@ flowchart LR
   1["Plane<br>[17, 47, 0]"]
   2["Plane<br>[65, 96, 0]"]
   3["Plane<br>[114, 144, 0]"]
+  6["StartSketchOnPlane<br>[158, 187, 0]"]
   1 --- 4
   4 --- 5
+  1 <--x 6
 ```

src/wasm-lib/kcl/tests/artifact_graph_sketch_on_face_etc/artifact_graph_flowchart.snap.md

@@ -78,6 +78,9 @@ flowchart LR
   68["SweepEdge Adjacent"]
   69["SweepEdge Opposite"]
   70["SweepEdge Adjacent"]
+  71["StartSketchOnFace<br>[257, 289, 0]"]
+  72["StartSketchOnFace<br>[506, 538, 0]"]
+  73["StartSketchOnFace<br>[768, 800, 0]"]
   1 --- 2
   2 --- 3
   2 --- 4
@@ -183,4 +186,7 @@ flowchart LR
   60 --- 68
   60 --- 69
   60 --- 70
+  10 <--x 71
+  30 <--x 72
+  46 <--x 73
 ```

src/wasm-lib/kcl/tests/pentagon_fillet_sugar/artifact_graph_flowchart.snap.md

@@ -45,6 +45,8 @@ flowchart LR
   37["SweepEdge Adjacent"]
   38["EdgeCut Fillet<br>[846, 973, 0]"]
   39["EdgeCut Fillet<br>[846, 973, 0]"]
+  40["StartSketchOnFace<br>[442, 464, 0]"]
+  41["StartSketchOnFace<br>[442, 464, 0]"]
   1 --- 2
   2 --- 3
   2 --- 4
@@ -98,4 +100,6 @@ flowchart LR
   33 --- 36
   33 --- 37
   36 <--x 39
+  9 <--x 40
+  7 <--x 41
 ```

src/wasm-lib/kcl/tests/sketch-on-chamfer-two-times-different-order/artifact_graph_flowchart.snap.md

@@ -60,6 +60,8 @@ flowchart LR
   52["SweepEdge Adjacent"]
   53["SweepEdge Opposite"]
   54["SweepEdge Adjacent"]
+  55["StartSketchOnFace<br>[670, 702, 0]"]
+  56["StartSketchOnFace<br>[1118, 1150, 0]"]
   1 --- 2
   2 --- 3
   2 --- 4
@@ -136,4 +138,6 @@ flowchart LR
   41 --- 52
   41 --- 53
   41 --- 54
+  26 <--x 55
+  25 <--x 56
 ```

src/wasm-lib/kcl/tests/sketch-on-chamfer-two-times/artifact_graph_flowchart.snap.md

@@ -60,6 +60,8 @@ flowchart LR
   52["SweepEdge Adjacent"]
   53["SweepEdge Opposite"]
   54["SweepEdge Adjacent"]
+  55["StartSketchOnFace<br>[670, 702, 0]"]
+  56["StartSketchOnFace<br>[1118, 1150, 0]"]
   1 --- 2
   2 --- 3
   2 --- 4
@@ -136,4 +138,6 @@ flowchart LR
   41 --- 52
   41 --- 53
   41 --- 54
+  25 <--x 55
+  26 <--x 56
 ```

src/wasm-lib/kcl/tests/sketch_on_face/artifact_graph_flowchart.snap.md

@@ -47,6 +47,7 @@ flowchart LR
   41["SweepEdge Adjacent"]
   42["SweepEdge Opposite"]
   43["SweepEdge Adjacent"]
+  44["StartSketchOnFace<br>[231, 259, 0]"]
   1 --- 2
   2 --- 3
   2 --- 4
@@ -113,4 +114,5 @@ flowchart LR
   29 --- 41
   29 --- 42
   29 --- 43
+  12 <--x 44
 ```

src/wasm-lib/kcl/tests/sketch_on_face_after_fillets_referencing_face/artifact_graph_flowchart.snap.md

@@ -57,6 +57,7 @@ flowchart LR
   51["SweepEdge Adjacent"]
   52["SweepEdge Opposite"]
   53["SweepEdge Adjacent"]
+  54["StartSketchOnFace<br>[1496, 1525, 0]"]
   1 --- 2
   2 --- 3
   2 --- 4
@@ -139,4 +140,5 @@ flowchart LR
   40 --- 51
   40 --- 52
   40 --- 53
+  12 <--x 54
 ```

src/wasm-lib/kcl/tests/sketch_on_face_circle_tagged/artifact_graph_flowchart.snap.md

@@ -35,6 +35,7 @@ flowchart LR
   29["Cap End"]
   30["SweepEdge Opposite"]
   31["SweepEdge Adjacent"]
+  32["StartSketchOnFace<br>[263, 292, 0]"]
   1 --- 2
   2 --- 3
   2 --- 4
@@ -80,4 +81,5 @@ flowchart LR
   26 --- 29
   26 --- 30
   26 --- 31
+  14 <--x 32
 ```

src/wasm-lib/kcl/tests/sketch_on_face_end/artifact_graph_flowchart.snap.md

@@ -47,6 +47,7 @@ flowchart LR
   41["SweepEdge Adjacent"]
   42["SweepEdge Opposite"]
   43["SweepEdge Adjacent"]
+  44["StartSketchOnFace<br>[263, 292, 0]"]
   1 --- 2
   2 --- 3
   2 --- 4
@@ -113,4 +114,5 @@ flowchart LR
   29 --- 41
   29 --- 42
   29 --- 43
+  14 <--x 44
 ```

src/wasm-lib/kcl/tests/sketch_on_face_end_negative_extrude/artifact_graph_flowchart.snap.md

@@ -47,6 +47,7 @@ flowchart LR
   41["SweepEdge Adjacent"]
   42["SweepEdge Opposite"]
   43["SweepEdge Adjacent"]
+  44["StartSketchOnFace<br>[263, 292, 0]"]
   1 --- 2
   2 --- 3
   2 --- 4
@@ -113,4 +114,5 @@ flowchart LR
   29 --- 41
   29 --- 42
   29 --- 43
+  14 <--x 44
 ```

src/wasm-lib/kcl/tests/sketch_on_face_start/artifact_graph_flowchart.snap.md

@@ -47,6 +47,7 @@ flowchart LR
   41["SweepEdge Adjacent"]
   42["SweepEdge Opposite"]
   43["SweepEdge Adjacent"]
+  44["StartSketchOnFace<br>[270, 297, 0]"]
   1 --- 2
   2 --- 3
   2 --- 4
@@ -113,4 +114,5 @@ flowchart LR
   29 --- 41
   29 --- 42
   29 --- 43
+  13 <--x 44
 ```

src/wasm-lib/kcl/tests/ssi_pattern/artifact_graph_flowchart.snap.md

@@ -38,6 +38,7 @@ flowchart LR
   32[Wall]
   33["SweepEdge Opposite"]
   34["SweepEdge Adjacent"]
+  35["StartSketchOnFace<br>[332, 364, 0]"]
   1 --- 2
   2 --- 3
   2 --- 4
@@ -89,4 +90,5 @@ flowchart LR
   31 --- 32
   31 --- 33
   31 --- 34
+  11 <--x 35
 ```