Skip to content

Commit 595de3b

Browse files
adamchalmersadamchalmers
authored
KCL: Track profile open/close, better error on double-close (#7933)
Previously, trying to close an already-closed KCL sketch would give the error ``` engine: The provided object is of type Object, which is not a valid input for use in this operation. ``` That sucks! Terrible! Now it gives ``` This sketch is already closed. Remove this unnecessary `close()` call ``` Fixes #7931
1 parent 46e5cae commit 595de3b

File tree

33 files changed

+851
-26
lines changed

33 files changed

+851
-26
lines changed

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

202 Bytes
Loading

‎rust/kcl-lib/src/execution/geometry.rs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -648,6 +648,17 @@ pub struct Sketch {
648648
/// Metadata.
649649
#[serde(skip)]
650650
pub meta: Vec<Metadata>,
651+
/// If not given, defaults to true.
652+
#[serde(default = "very_true", skip_serializing_if = "is_true")]
653+
pub is_closed: bool,
654+
}
655+
656+
fn very_true() -> bool {
657+
true
658+
}
659+
660+
fn is_true(b: &bool) -> bool {
661+
*b
651662
}
652663

653664
impl Sketch {

‎rust/kcl-lib/src/simulation_tests.rs

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3838,6 +3838,29 @@ mod tag_inner_face {
38383838
super::execute(TEST_NAME, true).await
38393839
}
38403840
}
3841+
3842+
mod double_close {
3843+
const TEST_NAME: &str = "double_close";
3844+
3845+
/// Test parsing KCL.
3846+
#[test]
3847+
fn parse() {
3848+
super::parse(TEST_NAME)
3849+
}
3850+
3851+
/// Test that parsing and unparsing KCL produces the original KCL input.
3852+
#[tokio::test(flavor = "multi_thread")]
3853+
async fn unparse() {
3854+
super::unparse(TEST_NAME).await
3855+
}
3856+
3857+
/// Test that KCL is executed correctly.
3858+
#[tokio::test(flavor = "multi_thread")]
3859+
async fn kcl_test_execute() {
3860+
super::execute(TEST_NAME, true).await
3861+
}
3862+
}
3863+
38413864
mod revolve_on_face {
38423865
const TEST_NAME: &str = "revolve_on_face";
38433866

‎rust/kcl-lib/src/std/extrude.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -209,6 +209,7 @@ pub(crate) async fn do_post_extrude<'a>(
209209
};
210210

211211
let mut sketch = sketch.clone();
212+
sketch.is_closed = true;
212213

213214
// If we were sketching on a face, we need the original face id.
214215
if let SketchSurface::Face(ref face) = sketch.on {

‎rust/kcl-lib/src/std/shapes.rs

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,8 @@ async fn inner_rectangle(
125125
.await?;
126126

127127
// Update the sketch in KCL memory.
128-
let mut new_sketch = sketch.clone();
128+
let mut new_sketch = sketch;
129+
new_sketch.is_closed = true;
129130
fn add(a: [f64; 2], b: [f64; 2]) -> [f64; 2] {
130131
[a[0] + b[0], a[1] + b[1]]
131132
}
@@ -226,7 +227,8 @@ async fn inner_circle(
226227
ccw: angle_start < angle_end,
227228
};
228229

229-
let mut new_sketch = sketch.clone();
230+
let mut new_sketch = sketch;
231+
new_sketch.is_closed = true;
230232
if let Some(tag) = &tag {
231233
new_sketch.add_tag(tag, &current_path, exec_state);
232234
}
@@ -327,7 +329,8 @@ async fn inner_circle_three_point(
327329
p3,
328330
};
329331

330-
let mut new_sketch = sketch.clone();
332+
let mut new_sketch = sketch;
333+
new_sketch.is_closed = true;
331334
if let Some(tag) = &tag {
332335
new_sketch.add_tag(tag, &current_path, exec_state);
333336
}
@@ -611,7 +614,8 @@ async fn inner_ellipse(
611614
ccw: angle_start < angle_end,
612615
};
613616

614-
let mut new_sketch = sketch.clone();
617+
let mut new_sketch = sketch;
618+
new_sketch.is_closed = true;
615619
if let Some(tag) = &tag {
616620
new_sketch.add_tag(tag, &current_path, exec_state);
617621
}

‎rust/kcl-lib/src/std/sketch.rs

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1175,6 +1175,7 @@ pub(crate) async fn inner_start_profile(
11751175
Default::default()
11761176
},
11771177
start: current_path,
1178+
is_closed: false,
11781179
};
11791180
Ok(sketch)
11801181
}
@@ -1231,6 +1232,12 @@ pub(crate) async fn inner_close(
12311232
exec_state: &mut ExecState,
12321233
args: Args,
12331234
) -> Result<Sketch, KclError> {
1235+
if sketch.is_closed {
1236+
return Err(KclError::new_semantic(KclErrorDetails::new(
1237+
"This sketch is already closed. Remove this unnecessary `close()` call".to_owned(),
1238+
vec![args.source_range],
1239+
)));
1240+
}
12341241
let from = sketch.current_pen_position()?;
12351242
let to = point_to_len_unit(sketch.start.get_from(), from.units);
12361243

@@ -1260,8 +1267,8 @@ pub(crate) async fn inner_close(
12601267
if let Some(tag) = &tag {
12611268
new_sketch.add_tag(tag, &current_path, exec_state);
12621269
}
1263-
12641270
new_sketch.paths.push(current_path);
1271+
new_sketch.is_closed = true;
12651272

12661273
Ok(new_sketch)
12671274
}

‎rust/kcl-lib/tests/artifact_graph_example_code_no_3d/program_memory.snap

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -352,7 +352,8 @@ description: Variables in memory after executing artifact_graph_example_code_no_
352352
"originalId": "[uuid]",
353353
"units": {
354354
"type": "Mm"
355-
}
355+
},
356+
"isClosed": false
356357
}
357358
}
358359
}

‎rust/kcl-lib/tests/artifact_graph_example_code_offset_planes/program_memory.snap

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -209,7 +209,8 @@ description: Variables in memory after executing artifact_graph_example_code_off
209209
"originalId": "[uuid]",
210210
"units": {
211211
"type": "Mm"
212-
}
212+
},
213+
"isClosed": false
213214
}
214215
}
215216
}

‎rust/kcl-lib/tests/crazy_multi_profile/program_memory.snap

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1311,7 +1311,8 @@ description: Variables in memory after executing crazy_multi_profile.kcl
13111311
"originalId": "[uuid]",
13121312
"units": {
13131313
"type": "Mm"
1314-
}
1314+
},
1315+
"isClosed": false
13151316
}
13161317
},
13171318
"profile003": {
@@ -2796,7 +2797,8 @@ description: Variables in memory after executing crazy_multi_profile.kcl
27962797
"originalId": "[uuid]",
27972798
"units": {
27982799
"type": "Mm"
2799-
}
2800+
},
2801+
"isClosed": false
28002802
}
28012803
},
28022804
"profile008": {

‎rust/kcl-lib/tests/double_close/artifact_commands.snap

Lines changed: 207 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,207 @@
1+
---
2+
source: kcl-lib/src/simulation_tests.rs
3+
description: Artifact commands double_close.kcl
4+
---
5+
{
6+
"rust/kcl-lib/tests/double_close/input.kcl": [
7+
{
8+
"cmdId": "[uuid]",
9+
"range": [],
10+
"command": {
11+
"type": "make_plane",
12+
"origin": {
13+
"x": 0.0,
14+
"y": 0.0,
15+
"z": 0.0
16+
},
17+
"x_axis": {
18+
"x": 1.0,
19+
"y": 0.0,
20+
"z": 0.0
21+
},
22+
"y_axis": {
23+
"x": 0.0,
24+
"y": 1.0,
25+
"z": 0.0
26+
},
27+
"size": 60.0,
28+
"clobber": false,
29+
"hide": true
30+
}
31+
},
32+
{
33+
"cmdId": "[uuid]",
34+
"range": [],
35+
"command": {
36+
"type": "enable_sketch_mode",
37+
"entity_id": "[uuid]",
38+
"ortho": false,
39+
"animated": false,
40+
"adjust_camera": false,
41+
"planar_normal": {
42+
"x": 0.0,
43+
"y": 0.0,
44+
"z": 1.0
45+
}
46+
}
47+
},
48+
{
49+
"cmdId": "[uuid]",
50+
"range": [],
51+
"command": {
52+
"type": "start_path"
53+
}
54+
},
55+
{
56+
"cmdId": "[uuid]",
57+
"range": [],
58+
"command": {
59+
"type": "move_path_pen",
60+
"path": "[uuid]",
61+
"to": {
62+
"x": 0.0,
63+
"y": 0.0,
64+
"z": 0.0
65+
}
66+
}
67+
},
68+
{
69+
"cmdId": "[uuid]",
70+
"range": [],
71+
"command": {
72+
"type": "sketch_mode_disable"
73+
}
74+
},
75+
{
76+
"cmdId": "[uuid]",
77+
"range": [],
78+
"command": {
79+
"type": "enable_sketch_mode",
80+
"entity_id": "[uuid]",
81+
"ortho": false,
82+
"animated": false,
83+
"adjust_camera": false,
84+
"planar_normal": {
85+
"x": 0.0,
86+
"y": 0.0,
87+
"z": 1.0
88+
}
89+
}
90+
},
91+
{
92+
"cmdId": "[uuid]",
93+
"range": [],
94+
"command": {
95+
"type": "start_path"
96+
}
97+
},
98+
{
99+
"cmdId": "[uuid]",
100+
"range": [],
101+
"command": {
102+
"type": "move_path_pen",
103+
"path": "[uuid]",
104+
"to": {
105+
"x": -5.0,
106+
"y": -5.0,
107+
"z": 0.0
108+
}
109+
}
110+
},
111+
{
112+
"cmdId": "[uuid]",
113+
"range": [],
114+
"command": {
115+
"type": "sketch_mode_disable"
116+
}
117+
},
118+
{
119+
"cmdId": "[uuid]",
120+
"range": [],
121+
"command": {
122+
"type": "extend_path",
123+
"path": "[uuid]",
124+
"segment": {
125+
"type": "line",
126+
"end": {
127+
"x": 10.0,
128+
"y": 0.0,
129+
"z": 0.0
130+
},
131+
"relative": true
132+
}
133+
}
134+
},
135+
{
136+
"cmdId": "[uuid]",
137+
"range": [],
138+
"command": {
139+
"type": "extend_path",
140+
"path": "[uuid]",
141+
"segment": {
142+
"type": "line",
143+
"end": {
144+
"x": 0.0,
145+
"y": 10.0,
146+
"z": 0.0
147+
},
148+
"relative": true
149+
}
150+
}
151+
},
152+
{
153+
"cmdId": "[uuid]",
154+
"range": [],
155+
"command": {
156+
"type": "extend_path",
157+
"path": "[uuid]",
158+
"segment": {
159+
"type": "line",
160+
"end": {
161+
"x": -10.0,
162+
"y": 0.0,
163+
"z": 0.0
164+
},
165+
"relative": true
166+
}
167+
}
168+
},
169+
{
170+
"cmdId": "[uuid]",
171+
"range": [],
172+
"command": {
173+
"type": "extend_path",
174+
"path": "[uuid]",
175+
"segment": {
176+
"type": "line",
177+
"end": {
178+
"x": 0.0,
179+
"y": -10.0,
180+
"z": 0.0
181+
},
182+
"relative": true
183+
}
184+
}
185+
},
186+
{
187+
"cmdId": "[uuid]",
188+
"range": [],
189+
"command": {
190+
"type": "close_path",
191+
"path_id": "[uuid]"
192+
}
193+
}
194+
],
195+
"std::appearance": [],
196+
"std::array": [],
197+
"std::math": [],
198+
"std::prelude": [],
199+
"std::sketch": [],
200+
"std::solid": [],
201+
"std::sweep": [],
202+
"std::transform": [],
203+
"std::turns": [],
204+
"std::types": [],
205+
"std::units": [],
206+
"std::vector": []
207+
}

0 commit comments

Comments
 (0)