From f9c19ade16029a55e599f007427fb664f4684724 Mon Sep 17 00:00:00 2001 From: Jonas Wrede Date: Tue, 13 Oct 2026 10:04:44 +0200 Subject: [PATCH] fix: raising a pattern's count after a rename numbers the new copies after the others (#372) (#540) A copy added by raising a body pattern's count after a rename is numbered after the highest stored copy, so it no longer takes the number the rename freed. Un-ignores the #372 regression test and adds one for undo/redo and save/reopen. Fixes #372. Co-authored-by: jonaswre --- crates/doc/src/eval.rs | 21 ++++++++++++++++++++- crates/engine/src/tests.rs | 22 +++++++++++++++++++++- 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/crates/doc/src/eval.rs b/crates/doc/src/eval.rs index e3d56d48..4457d01c 100644 --- a/crates/doc/src/eval.rs +++ b/crates/doc/src/eval.rs @@ -1143,6 +1143,25 @@ fn new_name(state: &mut ModelState, f: &Feature, k: usize) -> String { } } +/// The k-th body of a body pattern: its stored name, else a copy of `source`. Once `body.rename` +/// has stored this feature's names, a copy beyond them (the count was raised) is numbered after +/// the highest stored `source (n)`, so it never takes a number freed by a rename (#372). +/// Features without stored names keep the first free `source (n)`. +fn pattern_copy_name(state: &ModelState, f: &Feature, k: usize, source: &str) -> String { + if f.body_names.get(k).is_some_and(|name| !name.trim().is_empty()) { + return copy_body_name(state, f, k, source); + } + let base = if source.trim().is_empty() { "Body" } else { source.trim() }; + let prefix = format!("{base} ("); + let highest = f.body_names.iter().filter_map(|n| n.trim().strip_prefix(prefix.as_str())?.strip_suffix(')')?.parse::().ok()).max(); + match highest { + Some(h) => { + (u64::from(h) + 1..).map(|i| format!("{base} ({i})")).find(|n| state.body(n).is_none()).unwrap_or_else(|| unique_body_name(state, base)) + } + None => unique_body_name(state, base), + } +} + fn copy_body_name(state: &ModelState, f: &Feature, k: usize, source: &str) -> String { let base = f.body_names.get(k).filter(|name| !name.trim().is_empty()).map(String::as_str).unwrap_or(source); unique_body_name(state, base) @@ -3570,7 +3589,7 @@ fn eval_feature(doc: &Document, vals: &BTreeMap, f: &Feature, st: let mb = st.body(n).cloned().ok_or_else(|| DocError::Unknown(format!("body `{n}`")))?; for (k, m) in mats.iter().enumerate() { let copy = moved_copy(&mb, kernel::transform_matrix(&mb.body, *m)?, k + 1); - let name = copy_body_name(st, f, output, &mb.name); + let name = pattern_copy_name(st, f, output, &mb.name); st.bodies.push(ModelBody::traced_new(name, copy, f.id)); output += 1; } diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 0c32675d..8ebfc667 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -2806,7 +2806,6 @@ fn a_pattern_repeats_a_fillet_on_the_edges_it_found() { /// A copy added by raising the count after a rename takes the next number, not the freed one (#372). #[test] -#[ignore = "fails until #372 is fixed"] fn raising_a_pattern_count_after_a_rename_numbers_the_new_copy_after_the_others() { let mut s = Session::default(); run(&mut s, "solid.box", json!({"length": 10, "width": 10, "height": 10, "operation": "new_body", "body_name": "X"})); @@ -2966,3 +2965,24 @@ fn draft_on_a_cylinder_side_makes_a_frustum() { let expect = PI * h / 3.0 * (big * big + big * small + small * small); assert!(rel(volume(&mut s), expect) < 1e-3, "{} vs {expect}", volume(&mut s)); } + +/// The copies added after a rename keep their numbers through undo/redo, save/reopen and a +/// further count change; lowering the count drops the last copies (#372). +#[test] +fn pattern_copies_added_after_a_rename_keep_their_numbers() { + let mut s = Session::default(); + let names = |s: &Session| -> Vec { s.model.state().bodies.iter().map(|b| b.name.clone()).collect() }; + run(&mut s, "solid.box", json!({"length": 10, "width": 10, "height": 10, "operation": "new_body", "body_name": "X"})); + run(&mut s, "solid.pattern.rectangular", json!({"bodies": ["X"], "dir1": [1, 0, 0], "count1": 3, "spacing1": 20, "name": "Pat"})); + run(&mut s, "body.rename", json!({"body": "X (2)", "name": "Second"})); + run(&mut s, "timeline.edit", json!({"feature": "Pat", "set": {"pattern": {"count1": "5"}}})); + assert_eq!(names(&s), ["X", "Second", "X (3)", "X (4)", "X (5)"]); + run(&mut s, "edit.undo", json!({})); + assert_eq!(names(&s), ["X", "Second", "X (3)"]); + run(&mut s, "edit.redo", json!({})); + assert_eq!(names(&s), ["X", "Second", "X (3)", "X (4)", "X (5)"]); + let reopened = Session::new(solidcraft_doc::Document::from_json(&s.doc.to_json()).unwrap()); + assert_eq!(names(&reopened), ["X", "Second", "X (3)", "X (4)", "X (5)"]); + run(&mut s, "timeline.edit", json!({"feature": "Pat", "set": {"pattern": {"count1": "2"}}})); + assert_eq!(names(&s), ["X", "Second"]); +}