Fix sketch on face

Author: jtran

rust/kcl-lib/src/execution/exec_ast.rs

@@ -158,15 +158,32 @@ impl ExecutorContext {
             exec_state.mod_local.sketch_mode,
             exec_state.mod_local.freedom_analysis,
         );
-        #[cfg(feature = "artifact-graph")]
-        {
-            local_state
-                .artifacts
-                .scene_objects
-                .clone_from(&exec_state.mod_local.artifacts.scene_objects);
-        }
-        if preserve_mem.normal() {
-            std::mem::swap(&mut exec_state.mod_local, &mut local_state);
+        match preserve_mem {
+            PreserveMem::Always => {
+                #[cfg(feature = "artifact-graph")]
+                {
+                    if !self.is_mock() {
+                        use crate::id::IncIdGenerator;
+                        exec_state
+                            .mod_local
+                            .artifacts
+                            .scene_objects
+                            .clone_from(&exec_state.global.root_module_artifacts.scene_objects);
+                        exec_state.mod_local.artifacts.object_id_generator =
+                            IncIdGenerator::new(exec_state.global.root_module_artifacts.scene_objects.len());
+                    }
+                }
+            }
+            PreserveMem::Normal => {
+                #[cfg(feature = "artifact-graph")]
+                {
+                    local_state
+                        .artifacts
+                        .scene_objects
+                        .clone_from(&exec_state.mod_local.artifacts.scene_objects);
+                }
+                std::mem::swap(&mut exec_state.mod_local, &mut local_state);
+            }
         }
 
         let no_prelude = self

rust/kcl-lib/src/execution/state.rs

@@ -361,7 +361,12 @@ impl ExecState {
     #[cfg(feature = "artifact-graph")]
     pub fn add_scene_object(&mut self, obj: Object, source_range: SourceRange) -> ObjectId {
         let id = obj.id;
-        debug_assert!(id.0 == self.mod_local.artifacts.scene_objects.len());
+        debug_assert!(
+            id.0 == self.mod_local.artifacts.scene_objects.len(),
+            "Adding scene object with ID {} but next ID is {}",
+            id.0,
+            self.mod_local.artifacts.scene_objects.len()
+        );
         self.mod_local.artifacts.scene_objects.push(obj);
         self.mod_local.artifacts.source_range_to_object.insert(source_range, id);
         id
@@ -711,7 +716,10 @@ impl ModuleArtifactState {
         self.unprocessed_commands.extend(other.unprocessed_commands);
         self.commands.extend(other.commands);
         self.operations.extend(other.operations);
-        self.scene_objects.extend(other.scene_objects);
+        if other.scene_objects.len() > self.scene_objects.len() {
+            self.scene_objects
+                .extend(other.scene_objects[self.scene_objects.len()..].iter().cloned());
+        }
         self.source_range_to_object.extend(other.source_range_to_object);
         self.var_solutions.extend(other.var_solutions);
     }

rust/kcl-lib/src/frontend.rs

@@ -154,7 +154,6 @@ impl SketchApi for FrontendState {
     async fn new_sketch(
         &mut self,
         ctx: &ExecutorContext,
-        mock_ctx: &ExecutorContext,
         _project: ProjectId,
         _file: FileId,
         _version: Version,
@@ -214,62 +213,27 @@ impl SketchApi for FrontendState {
             });
         };
 
-        let sketch_source_range = new_program
-            .ast
-            .body
-            .last()
-            .map(SourceRange::from)
-            .ok_or_else(|| Error {
-                msg: "No AST body items after adding sketch".to_owned(),
-            })?;
-
         // Make sure to only set this if there are no errors.
         self.program = new_program.clone();
 
         // We need to do an engine execute so that the plane object gets created
         // and is cached.
-        {
-            ctx.run_with_caching(new_program.clone()).await.map_err(|err| Error {
-                msg: err.error.message().to_owned(),
-            })?;
-        }
-
-        let mut truncated_program = new_program;
-        only_sketch_block(&mut truncated_program.ast, sketch_source_range, ChangeKind::None)?;
-
-        // Add one to reserve an ID for the plane.
-        let sketch_id = ObjectId(self.scene_graph.objects.len() + 1);
+        let outcome = ctx.run_with_caching(new_program.clone()).await.map_err(|err| Error {
+            msg: err.error.message().to_owned(),
+        })?;
+        let freedom_analysis_ran = true;
 
-        // Execute.
-        let outcome = mock_ctx
-            .run_mock(
-                &truncated_program,
-                &MockConfig::new_sketch_mode(sketch_id).no_freedom_analysis(),
-            )
-            .await
-            .map_err(|err| {
-                // TODO: sketch-api: Yeah, this needs to change. We need to
-                // return the full error.
-                Error {
-                    msg: err.error.message().to_owned(),
-                }
-            })?;
+        let outcome = self.update_state_after_exec(outcome, freedom_analysis_ran);
 
-        #[cfg(not(feature = "artifact-graph"))]
-        let sketch_id = ObjectId(0);
-        #[cfg(feature = "artifact-graph")]
-        let sketch_id = outcome
-            .source_range_to_object
-            .get(&sketch_source_range)
-            .copied()
-            .ok_or_else(|| Error {
-                msg: format!("Source range of sketch not found: {sketch_source_range:?}"),
-            })?;
-        let src_delta = SourceDelta { text: new_source };
+        let Some(sketch_id) = self.scene_graph.objects.last().map(|object| object.id) else {
+            return Err(Error {
+                msg: "No objects in scene graph after adding sketch".to_owned(),
+            });
+        };
         // Store the object in the scene.
         self.scene_graph.sketch_mode = Some(sketch_id);
-        // Uses .no_freedom_analysis() so freedom_analysis: false
-        let outcome = self.update_state_after_exec(outcome, false);
+
+        let src_delta = SourceDelta { text: new_source };
         let scene_graph_delta = SceneGraphDelta {
             new_graph: self.scene_graph.clone(),
             invalidates_ids: false,
@@ -1404,8 +1368,9 @@ impl FrontendState {
                 msg: err.error.message().to_owned(),
             }
         })?;
+        let freedom_analysis_ran = true;
 
-        let outcome = self.update_state_after_exec(outcome);
+        let outcome = self.update_state_after_exec(outcome, freedom_analysis_ran);
 
         let src_delta = SourceDelta { text: new_source };
         let scene_graph_delta = SceneGraphDelta {
@@ -2732,6 +2697,15 @@ mod tests {
         None
     }
 
+    fn find_first_face_object(scene_graph: &SceneGraph) -> Option<&Object> {
+        for object in &scene_graph.objects {
+            if let ObjectKind::Face(_) = &object.kind {
+                return Some(object);
+            }
+        }
+        None
+    }
+
     #[track_caller]
     fn expect_sketch(object: &Object) -> &Sketch {
         if let ObjectKind::Sketch(sketch) = &object.kind {
@@ -2756,7 +2730,7 @@ mod tests {
             on: PlaneName::Xy.to_string(),
         };
         let (_src_delta, scene_delta, sketch_id) = frontend
-            .new_sketch(&ctx, &mock_ctx, ProjectId(0), FileId(0), version, sketch_args)
+            .new_sketch(&ctx, ProjectId(0), FileId(0), version, sketch_args)
             .await
             .unwrap();
         assert_eq!(sketch_id, ObjectId(1));
@@ -2861,7 +2835,7 @@ sketch(on = XY) {
             on: PlaneName::Xy.to_string(),
         };
         let (_src_delta, scene_delta, sketch_id) = frontend
-            .new_sketch(&ctx, &mock_ctx, ProjectId(0), FileId(0), version, sketch_args)
+            .new_sketch(&ctx, ProjectId(0), FileId(0), version, sketch_args)
             .await
             .unwrap();
         assert_eq!(sketch_id, ObjectId(1));
@@ -2987,7 +2961,7 @@ sketch(on = XY) {
             on: PlaneName::Xy.to_string(),
         };
         let (_src_delta, scene_delta, sketch_id) = frontend
-            .new_sketch(&ctx, &mock_ctx, ProjectId(0), FileId(0), version, sketch_args)
+            .new_sketch(&ctx, ProjectId(0), FileId(0), version, sketch_args)
             .await
             .unwrap();
         assert_eq!(sketch_id, ObjectId(1));
@@ -3198,7 +3172,7 @@ s = sketch(on = XY) {
             on: PlaneName::Xy.to_string(),
         };
         let (_src_delta, scene_delta, sketch_id) = frontend
-            .new_sketch(&ctx, &mock_ctx, ProjectId(0), FileId(0), version, sketch_args)
+            .new_sketch(&ctx, ProjectId(0), FileId(0), version, sketch_args)
             .await
             .unwrap();
         assert_eq!(sketch_id, ObjectId(1));
@@ -4245,6 +4219,60 @@ sketch(on = XY) {
         mock_ctx.close().await;
     }
 
+    #[tokio::test(flavor = "multi_thread")]
+    async fn test_sketch_on_face_simple() {
+        let initial_source = "\
+@settings(experimentalFeatures = allow)
+
+len = 2mm
+cube = startSketchOn(XY)
+  |> startProfile(at = [0, 0])
+  |> line(end = [len, 0], tag = $side)
+  |> line(end = [0, len])
+  |> line(end = [-len, 0])
+  |> line(end = [0, -len])
+  |> close()
+  |> extrude(length = len)
+
+face = faceOf(cube, face = side)
+";
+
+        let program = Program::parse(initial_source).unwrap().0.unwrap();
+
+        let mut frontend = FrontendState::new();
+
+        let ctx = ExecutorContext::new_with_default_client().await.unwrap();
+        let mock_ctx = ExecutorContext::new_mock(None).await;
+        let version = Version(0);
+
+        frontend.hack_set_program(&ctx, program).await.unwrap();
+        let face_object = find_first_face_object(&frontend.scene_graph).unwrap();
+        let face_id = face_object.id;
+
+        let sketch_args = SketchCtor { on: "face".to_owned() };
+        let (_src_delta, scene_delta, sketch_id) = frontend
+            .new_sketch(&ctx, ProjectId(0), FileId(0), version, sketch_args)
+            .await
+            .unwrap();
+        assert_eq!(sketch_id, ObjectId(2));
+        assert_eq!(scene_delta.new_objects, vec![ObjectId(2)]);
+        let sketch_object = &scene_delta.new_graph.objects[2];
+        assert_eq!(sketch_object.id, ObjectId(2));
+        assert_eq!(
+            sketch_object.kind,
+            ObjectKind::Sketch(Sketch {
+                args: SketchCtor { on: "face".to_owned() },
+                plane: face_id,
+                segments: vec![],
+                constraints: vec![],
+            })
+        );
+        assert_eq!(scene_delta.new_graph.objects.len(), 3);
+
+        ctx.close().await;
+        mock_ctx.close().await;
+    }
+
     #[tokio::test(flavor = "multi_thread")]
     async fn test_multiple_sketch_blocks() {
         let initial_source = "\

rust/kcl-lib/src/frontend/sketch.rs

@@ -22,7 +22,6 @@ pub trait SketchApi {
     async fn new_sketch(
         &mut self,
         ctx: &ExecutorContext,
-        mock_ctx: &ExecutorContext,
         project: ProjectId,
         file: FileId,
         version: Version,

rust/kcl-wasm-lib/src/api.rs

@@ -157,14 +157,10 @@ impl Context {
             .create_executor_ctx(settings, None, false)
             .map_err(|e| format!("Could not create KCL executor context for new sketch. {TRUE_BUG} Details: {e}"))?;
 
-        let mock_ctx = self
-            .create_executor_ctx(settings, None, true)
-            .map_err(|e| format!("Could not create KCL executor context for new sketch. {TRUE_BUG} Details: {e}"))?;
-
         let frontend = Arc::clone(&self.frontend);
         let mut guard = frontend.write().await;
         let result = guard
-            .new_sketch(&ctx, &mock_ctx, project, file, version, args)
+            .new_sketch(&ctx, project, file, version, args)
             .await
             .map_err(|e| format!("Failed to create new sketch: {:?}", e))?;