Start end for sketch on face (#1406)

* updates

Signed-off-by: Jess Frazelle <github@jessfraz.com>

* add tests

Signed-off-by: Jess Frazelle <github@jessfraz.com>

* clippy

Signed-off-by: Jess Frazelle <github@jessfraz.com>

---------

Signed-off-by: Jess Frazelle <github@jessfraz.com>

docs/kcl/std.json

@@ -27979,9 +27979,33 @@
       },
       {
         "name": "tag",
-        "type": "String",
+        "type": "SketchOnFaceTag",
         "schema": {
-          "type": "string",
+          "description": "A tag for sketch on face.",
+          "anyOf": [
+            {
+              "oneOf": [
+                {
+                  "description": "The start face as in before you extruded. This could also be known as the bottom face. But we do not call it bottom because it would be the top face if you extruded it in the opposite direction or flipped the camera.",
+                  "type": "string",
+                  "enum": [
+                    "start"
+                  ]
+                },
+                {
+                  "description": "The end face after you extruded. This could also be known as the top face. But we do not call it top because it would be the bottom face if you extruded it in the opposite direction or flipped the camera.",
+                  "type": "string",
+                  "enum": [
+                    "end"
+                  ]
+                }
+              ]
+            },
+            {
+              "description": "A string tag for the face you want to sketch on.",
+              "type": "string"
+            }
+          ],
           "nullable": true
         },
         "required": true

docs/kcl/std.md

@@ -5149,7 +5149,7 @@ Start a sketch on a specific plane or face.
 
 
 ```
-startSketchOn(data: SketchData, tag: String) -> SketchSurface
+startSketchOn(data: SketchData, tag: SketchOnFaceTag) -> SketchSurface
 ```
 
 #### Arguments
@@ -5239,7 +5239,11 @@ startSketchOn(data: SketchData, tag: String) -> SketchSurface
 },
 }
 ```
-* `tag`: `String`
+* `tag`: `SketchOnFaceTag` - A tag for sketch on face.
+```
+"start" | "end" |
+string
+```
 
 #### Returns
 

src/wasm-lib/kcl/src/executor.rs

@@ -862,7 +862,7 @@ impl ExtrudeSurface {
 
     pub fn get_name(&self) -> String {
         match self {
-            ExtrudeSurface::ExtrudePlane(ep) => ep.name.clone(),
+            ExtrudeSurface::ExtrudePlane(ep) => ep.name.to_string(),
         }
     }
 

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

@@ -8,12 +8,11 @@ pub use local::FileManager;
 #[cfg(target_arch = "wasm32")]
 #[cfg(not(test))]
 pub mod wasm;
+use anyhow::Result;
 #[cfg(target_arch = "wasm32")]
 #[cfg(not(test))]
 pub use wasm::FileManager;
 
-use anyhow::Result;
-
 #[async_trait::async_trait(?Send)]
 pub trait FileSystem: Clone {
     /// Read a file from the local file system.

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

@@ -123,7 +123,10 @@ async fn inner_extrude(length: f64, sketch_group: Box<SketchGroup>, args: Args)
     }
 
     Ok(Box::new(ExtrudeGroup {
-        id,
+        // Ok so you would think that the id would be the id of the extrude group,
+        // that we passed in to the function, but it's actually the id of the
+        // sketch group.
+        id: sketch_group.id,
         value: new_value,
         height: length,
         position: sketch_group.position,

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

@@ -20,9 +20,9 @@ use parse_display::{Display, FromStr};
 use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
 
-use self::kcl_stdlib::KclStdLibFn;
+use self::{kcl_stdlib::KclStdLibFn, sketch::SketchOnFaceTag};
 use crate::{
-    ast::types::{parse_json_number_as_f64, parse_json_value_as_string},
+    ast::types::parse_json_number_as_f64,
     docs::StdLibFn,
     engine::EngineManager,
     errors::{KclError, KclErrorDetails},
@@ -406,7 +406,9 @@ impl Args {
         }
     }
 
-    fn get_data_and_optional_tag<T: serde::de::DeserializeOwned>(&self) -> Result<(T, Option<String>), KclError> {
+    fn get_data_and_optional_tag<T: serde::de::DeserializeOwned>(
+        &self,
+    ) -> Result<(T, Option<SketchOnFaceTag>), KclError> {
         let first_value = self
             .args
             .first()
@@ -426,8 +428,13 @@ impl Args {
         })?;
 
         if let Some(second_value) = self.args.get(1) {
-            let tag = parse_json_value_as_string(&second_value.get_json_value()?);
+            let tag: SketchOnFaceTag = serde_json::from_value(second_value.get_json_value()?).map_err(|e| {
-            Ok((data, tag))
+                KclError::Type(KclErrorDetails {
+                    message: format!("Failed to deserialize SketchOnFaceTag from JSON: {}", e),
+                    source_ranges: vec![self.source_range],
+                })
+            })?;
+            Ok((data, Some(tag)))
         } else {
             Ok((data, None))
         }

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

@@ -4,6 +4,7 @@ use anyhow::Result;
 use derive_docs::stdlib;
 use kittycad::types::{Angle, ModelingCmd, Point3D};
 use kittycad_execution_plan_macros::ExecutionPlanValue;
+use parse_display::{Display, FromStr};
 use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
 
@@ -813,7 +814,7 @@ impl SketchSurface {
 
 /// Start a sketch on a specific plane or face.
 pub async fn start_sketch_on(args: Args) -> Result<MemoryItem, KclError> {
-    let (data, tag): (SketchData, Option<String>) = args.get_data_and_optional_tag()?;
+    let (data, tag): (SketchData, Option<SketchOnFaceTag>) = args.get_data_and_optional_tag()?;
 
     match inner_start_sketch_on(data, tag, args).await? {
         SketchSurface::Plane(plane) => Ok(MemoryItem::Plane(plane)),
@@ -825,7 +826,11 @@ pub async fn start_sketch_on(args: Args) -> Result<MemoryItem, KclError> {
 #[stdlib {
     name = "startSketchOn",
 }]
-async fn inner_start_sketch_on(data: SketchData, tag: Option<String>, args: Args) -> Result<SketchSurface, KclError> {
+async fn inner_start_sketch_on(
+    data: SketchData,
+    tag: Option<SketchOnFaceTag>,
+    args: Args,
+) -> Result<SketchSurface, KclError> {
     match data {
         SketchData::Plane(plane_data) => {
             let plane = start_sketch_on_plane(plane_data, args).await?;
@@ -838,26 +843,72 @@ async fn inner_start_sketch_on(data: SketchData, tag: Option<String>, args: Args
                     source_ranges: vec![args.source_range],
                 }));
             };
-            let face = start_sketch_on_face(extrude_group, &tag, args).await?;
+            let face = start_sketch_on_face(extrude_group, tag, args).await?;
             Ok(SketchSurface::Face(face))
         }
     }
 }
 
-async fn start_sketch_on_face(extrude_group: Box<ExtrudeGroup>, tag: &str, args: Args) -> Result<Box<Face>, KclError> {
+/// A tag for sketch on face.
-    let extrude_plane = extrude_group
+#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, ts_rs::TS, JsonSchema, FromStr, Display)]
-        .value
+#[ts(export)]
-        .iter()
+#[serde(rename_all = "snake_case", untagged)]
-        .find_map(|extrude_surface| match extrude_surface {
+#[display("{0}")]
-            ExtrudeSurface::ExtrudePlane(extrude_plane) if extrude_plane.name == tag => Some(extrude_plane),
+pub enum SketchOnFaceTag {
-            ExtrudeSurface::ExtrudePlane(_) => None,
+    StartOrEnd(StartOrEnd),
-        })
+    /// A string tag for the face you want to sketch on.
-        .ok_or_else(|| {
+    String(String),
+}
+
+#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, ts_rs::TS, JsonSchema, FromStr, Display)]
+#[ts(export)]
+#[serde(rename_all = "snake_case")]
+#[display(style = "snake_case")]
+pub enum StartOrEnd {
+    /// The start face as in before you extruded. This could also be known as the bottom
+    /// face. But we do not call it bottom because it would be the top face if you
+    /// extruded it in the opposite direction or flipped the camera.
+    #[serde(rename = "start", alias = "START")]
+    Start,
+    /// The end face after you extruded. This could also be known as the top
+    /// face. But we do not call it top because it would be the bottom face if you
+    /// extruded it in the opposite direction or flipped the camera.
+    #[serde(rename = "end", alias = "END")]
+    End,
+}
+
+async fn start_sketch_on_face(
+    extrude_group: Box<ExtrudeGroup>,
+    tag: SketchOnFaceTag,
+    args: Args,
+) -> Result<Box<Face>, KclError> {
+    let extrude_plane_id = match tag {
+        SketchOnFaceTag::String(ref s) => extrude_group
+            .value
+            .iter()
+            .find_map(|extrude_surface| match extrude_surface {
+                ExtrudeSurface::ExtrudePlane(extrude_plane) if extrude_plane.name == *s => Some(extrude_plane.face_id),
+                ExtrudeSurface::ExtrudePlane(_) => None,
+            })
+            .ok_or_else(|| {
+                KclError::Type(KclErrorDetails {
+                    message: format!("Expected a face with the tag `{}`", tag),
+                    source_ranges: vec![args.source_range],
+                })
+            })?,
+        SketchOnFaceTag::StartOrEnd(StartOrEnd::Start) => extrude_group.start_cap_id.ok_or_else(|| {
             KclError::Type(KclErrorDetails {
-                message: format!("Expected a face with the tag `{}`", tag),
+                message: "Expected a start face to sketch on".to_string(),
                 source_ranges: vec![args.source_range],
             })
-        })?;
+        })?,
+        SketchOnFaceTag::StartOrEnd(StartOrEnd::End) => extrude_group.end_cap_id.ok_or_else(|| {
+            KclError::Type(KclErrorDetails {
+                message: "Expected an end face to sketch on".to_string(),
+                source_ranges: vec![args.source_range],
+            })
+        })?,
+    };
 
     // Enter sketch mode on the face.
     let id = uuid::Uuid::new_v4();
@@ -866,7 +917,7 @@ async fn start_sketch_on_face(extrude_group: Box<ExtrudeGroup>, tag: &str, args:
         ModelingCmd::EnableSketchMode {
             animated: false,
             ortho: false,
-            entity_id: extrude_plane.face_id,
+            entity_id: extrude_plane_id,
         },
     )
     .await?;
@@ -1645,4 +1696,43 @@ mod tests {
         let data: PlaneData = serde_json::from_str(&str_json).unwrap();
         assert_eq!(data, PlaneData::NegXZ);
     }
+
+    #[test]
+    fn test_deserialize_sketch_on_face_tag() {
+        let data = "start";
+        let mut str_json = serde_json::to_string(&data).unwrap();
+        assert_eq!(str_json, "\"start\"");
+
+        str_json = "\"end\"".to_string();
+        let data: crate::std::sketch::SketchOnFaceTag = serde_json::from_str(&str_json).unwrap();
+        assert_eq!(
+            data,
+            crate::std::sketch::SketchOnFaceTag::StartOrEnd(crate::std::sketch::StartOrEnd::End)
+        );
+
+        str_json = "\"thing\"".to_string();
+        let data: crate::std::sketch::SketchOnFaceTag = serde_json::from_str(&str_json).unwrap();
+        assert_eq!(data, crate::std::sketch::SketchOnFaceTag::String("thing".to_string()));
+
+        str_json = "\"END\"".to_string();
+        let data: crate::std::sketch::SketchOnFaceTag = serde_json::from_str(&str_json).unwrap();
+        assert_eq!(
+            data,
+            crate::std::sketch::SketchOnFaceTag::StartOrEnd(crate::std::sketch::StartOrEnd::End)
+        );
+
+        str_json = "\"start\"".to_string();
+        let data: crate::std::sketch::SketchOnFaceTag = serde_json::from_str(&str_json).unwrap();
+        assert_eq!(
+            data,
+            crate::std::sketch::SketchOnFaceTag::StartOrEnd(crate::std::sketch::StartOrEnd::Start)
+        );
+
+        str_json = "\"START\"".to_string();
+        let data: crate::std::sketch::SketchOnFaceTag = serde_json::from_str(&str_json).unwrap();
+        assert_eq!(
+            data,
+            crate::std::sketch::SketchOnFaceTag::StartOrEnd(crate::std::sketch::StartOrEnd::Start)
+        );
+    }
 }

src/wasm-lib/tests/executor/main.rs

@@ -91,6 +91,62 @@ const part002 = startSketchOn(part001, "here")
     twenty_twenty::assert_image("tests/executor/outputs/sketch_on_face.png", &result, 0.999);
 }
 
+#[tokio::test(flavor = "multi_thread")]
+async fn serial_test_sketch_on_face_start() {
+    let code = r#"fn cube = (pos, scale) => {
+  const sg = startSketchOn('XY')
+    |> startProfileAt(pos, %)
+    |> line([0, scale], %)
+    |> line([scale, 0], %)
+    |> line([0, -scale], %)
+
+  return sg
+}
+const part001 = cube([0,0], 20)
+    |> close(%)
+    |> extrude(20, %)
+
+const part002 = startSketchOn(part001, "start")
+  |> startProfileAt([0, 0], %)
+  |> line([0, 10], %)
+  |> line([10, 0], %)
+  |> line([0, -10], %)
+  |> close(%)
+  |> extrude(5, %)
+"#;
+
+    let result = execute_and_snapshot(code).await.unwrap();
+    twenty_twenty::assert_image("tests/executor/outputs/sketch_on_face_start.png", &result, 0.999);
+}
+
+#[tokio::test(flavor = "multi_thread")]
+async fn serial_test_sketch_on_face_end() {
+    let code = r#"fn cube = (pos, scale) => {
+  const sg = startSketchOn('XY')
+    |> startProfileAt(pos, %)
+    |> line([0, scale], %)
+    |> line([scale, 0], %)
+    |> line([0, -scale], %)
+
+  return sg
+}
+const part001 = cube([0,0], 20)
+    |> close(%)
+    |> extrude(20, %)
+
+const part002 = startSketchOn(part001, "END")
+  |> startProfileAt([0, 0], %)
+  |> line([0, 10], %)
+  |> line([10, 0], %)
+  |> line([0, -10], %)
+  |> close(%)
+  |> extrude(5, %)
+"#;
+
+    let result = execute_and_snapshot(code).await.unwrap();
+    twenty_twenty::assert_image("tests/executor/outputs/sketch_on_face_end.png", &result, 0.999);
+}
+
 #[tokio::test(flavor = "multi_thread")]
 async fn serial_test_execute_with_function_sketch() {
     let code = r#"fn box = (h, l, w) => {