From 3128762014c2ce641c2e7985eb4421773ea4cc2e Mon Sep 17 00:00:00 2001 From: MateuszSychXpl <151755914+MateuszSychXpl@users.noreply.github.com> Date: Fri, 16 Oct 2026 13:24:06 +0200 Subject: [PATCH] Motion analysis review round 2: a number names a study by id first, the dialog analyses the study it made on the scratch copy, duration 0.001 s or more, volumes by the innermost member --- crates/engine/src/cmd/motion.rs | 11 ++++++---- crates/engine/src/cmd/motion_analysis.rs | 14 +++++++------ .../engine/src/cmd/motion_analysis_tests.rs | 13 ++++++++++++ crates/ui-egui/src/dialogs_motion.rs | 14 +++++++++---- crates/ui-egui/src/dialogs_motion_plot.rs | 20 ++++++++++--------- 5 files changed, 49 insertions(+), 23 deletions(-) diff --git a/crates/engine/src/cmd/motion.rs b/crates/engine/src/cmd/motion.rs index 63fa3a26..84708845 100644 --- a/crates/engine/src/cmd/motion.rs +++ b/crates/engine/src/cmd/motion.rs @@ -476,12 +476,15 @@ fn check(s: &mut Session, _p: &Value) -> Result { // ---- Motion studies ---- fn study_index(s: &Session, p: &Value, cmd: &str) -> Result { - let key = match p.get("study") { - Some(Value::Number(n)) => n.to_string(), - Some(Value::String(x)) => x.clone(), + // A number is an id first, text a name first: a study may be named like another's id. + let (key, id_first) = match p.get("study") { + Some(Value::Number(n)) => (n.to_string(), true), + Some(Value::String(x)) => (x.clone(), false), _ => return Err(bad(cmd, "`study` must be an id or a name")), }; - s.doc.assembly.studies.iter().position(|m| m.id.to_string() == key || m.name == key).ok_or_else(|| bad(cmd, format!("no motion study `{key}`"))) + let studies = &s.doc.assembly.studies; + let (by_id, by_name) = (studies.iter().position(|m| m.id.to_string() == key), studies.iter().position(|m| m.name == key)); + if id_first { by_id.or(by_name) } else { by_name.or(by_id) }.ok_or_else(|| bad(cmd, format!("no motion study `{key}`"))) } fn study(s: &mut Session, p: &Value) -> Result { diff --git a/crates/engine/src/cmd/motion_analysis.rs b/crates/engine/src/cmd/motion_analysis.rs index 7a56f3b8..ffe250f3 100644 --- a/crates/engine/src/cmd/motion_analysis.rs +++ b/crates/engine/src/cmd/motion_analysis.rs @@ -19,7 +19,7 @@ use crate::{EngineError, Result, Session}; pub const PLOT_DOC: &str = "study: id|name; joints?: [id|name] (default: every joint that moves); occurrence? and point? (a point \ of a part to follow, as motion.trace takes them); samples? (3…1000, default the study's steps + 1); duration?: seconds the \ - whole study takes (velocities and accelerations then per second, else per step) → step: [the study step of each sample], \ + whole study takes, 0.001 or more (velocities and accelerations then per second, else per step) → step: [the study step of each sample], \ time?: [s], joints: [{joint, name, index, dof, unit, value, velocity, acceleration, velocity_unit, acceleration_unit}] \ (degrees for angles, mm for distances; angles unwrapped), point?: {occurrence, position: [[x,y,z]…], velocity, \ acceleration (vectors), speed, acceleration_norm, unit, velocity_unit, acceleration_unit}, samples (taken), stopped_at, \ @@ -173,7 +173,8 @@ pub(super) fn plot(s: &mut Session, p: &Value) -> Result { let duration = match p.get("duration") { None | Some(Value::Null) => None, Some(v) => { - Some(v.as_f64().filter(|d| d.is_finite() && *d > 0.0 && *d <= 1e9).ok_or_else(|| bad(cmd, "`duration` must be seconds, more than 0"))?) + // At least a millisecond: shorter sample spacings square to nothing. + Some(v.as_f64().filter(|d| (1e-3..=1e9).contains(d)).ok_or_else(|| bad(cmd, "`duration` must be seconds, 0.001 or more"))?) } }; let series = plotted(s, p, cmd)?; @@ -261,10 +262,11 @@ fn shared_volume(s: &Session, doc: &Document, a: u64, b: u64, booleans: &mut usi // The bodies under each (and not under the other), with their world frames and boxes. let mut sides: [Vec<(Mat, Aabb3, &solidcraft_doc::ModelBody)>; 2] = Default::default(); for (comp, path, m) in doc.placements() { - let side = match (path.contains(&a), path.contains(&b)) { - (true, false) => 0, - (false, true) => 1, - _ => continue, + // A body belongs to the innermost of the two on its path, as contact sets pair them. + let side = match path.iter().rev().find(|o| **o == a || **o == b) { + Some(o) if *o == a => 0, + Some(_) => 1, + None => continue, }; for body in st.bodies.iter().filter(|x| doc.model_body_component(x) == comp && !x.body.is_mesh()) { let frame = doc.model_body_offset(body).map(|o| mat_mul(&m, o)).unwrap_or(m); diff --git a/crates/engine/src/cmd/motion_analysis_tests.rs b/crates/engine/src/cmd/motion_analysis_tests.rs index 872aceaf..163fa333 100644 --- a/crates/engine/src/cmd/motion_analysis_tests.rs +++ b/crates/engine/src/cmd/motion_analysis_tests.rs @@ -153,6 +153,18 @@ fn interference_names_the_step_two_parts_start_to_collide() { assert!(r["clear"] == true && r["collisions"] == json!([]), "{r}"); } +/// A number names a study by its id first, text by its name first: a study may be named like +/// another's id. +#[test] +fn studies_by_id_or_name() { + let mut s = rail(); + let keys = json!([{"joint": "Rail", "points": [[0, 10], [15, -5]]}]); + let first = run(&mut s, "motion.study", json!({"name": "Late", "steps": 15, "keys": keys}))["study"].as_u64().unwrap_or(0); + run(&mut s, "motion.study", json!({"name": first.to_string(), "steps": 15, "keys": keys})); + assert_eq!(run(&mut s, "motion.plot", json!({"study": first, "samples": 3}))["study"], "Late"); + assert_eq!(run(&mut s, "motion.plot", json!({"study": first.to_string(), "samples": 3}))["study"], first.to_string()); +} + /// Hostile parameters are refused, never a panic. #[test] fn motion_analysis_refuses_what_it_cannot_do() { @@ -165,6 +177,7 @@ fn motion_analysis_refuses_what_it_cannot_do() { ("motion.plot", json!({"study": "Push", "samples": "many"})), ("motion.plot", json!({"study": "Push", "duration": 0})), ("motion.plot", json!({"study": "Push", "duration": -3})), + ("motion.plot", json!({"study": "Push", "duration": 1e-200})), ("motion.plot", json!({"study": "Push", "joints": ["Nope"]})), ("motion.plot", json!({"study": "Push", "joints": vec![json!("Rail"); 300]})), ("motion.plot", json!({"study": "Push", "point": [1, 2]})), diff --git a/crates/ui-egui/src/dialogs_motion.rs b/crates/ui-egui/src/dialogs_motion.rs index e314aaf3..7dc65ac0 100644 --- a/crates/ui-egui/src/dialogs_motion.rs +++ b/crates/ui-egui/src/dialogs_motion.rs @@ -264,8 +264,9 @@ pub fn study_params(s: &Session, name: &str, study: Option<&str>, steps: &str, t return Err("add a key for a joint first".into()); } let mut p = json!({"name": name, "steps": steps, "keys": keys}); + // The id as a number: a study may be named like another's id. if let Some(st) = study { - p["study"] = json!(st); + p["study"] = st.parse::().map_or_else(|_| json!(st), |id| json!(id)); } Ok(p) } @@ -376,9 +377,8 @@ fn moved(before: &Session, after: &Session) -> BTreeMap { /// The study's pose at `step`, computed on a scratch copy. fn study_pose(app: &SolveApp, params: &Value, step: u32) -> Result, String> { let mut sc = app.session.scratch(); - let name = params["study"].as_str().or(params["name"].as_str()).unwrap_or_default().to_string(); - sc.execute("motion.study", params).map_err(|e| e.to_string())?; - sc.execute("motion.play", &json!({"study": name, "step": step})).map_err(|e| e.to_string())?; + let id = scratch_study(&mut sc, params)?; + sc.execute("motion.play", &json!({"study": id, "step": step})).map_err(|e| e.to_string())?; Ok(moved(&app.session, &sc)) } @@ -410,6 +410,12 @@ fn export_animation(app: &mut SolveApp, params: Value, name: &str) { }); } +/// Makes (or edits) the study on a scratch copy, as OK would: its id there (a name may be +/// another study's too). +pub fn scratch_study(sc: &mut Session, params: &Value) -> Result { + Ok(sc.execute("motion.study", params).map_err(|e| e.to_string())?["study"].clone()) +} + /// The explosion at `scale`, computed on a scratch copy. fn explode_pose(app: &SolveApp, scale: f64) -> Result, String> { let mut sc = app.session.scratch(); diff --git a/crates/ui-egui/src/dialogs_motion_plot.rs b/crates/ui-egui/src/dialogs_motion_plot.rs index 86e4ac2c..49aa0d0b 100644 --- a/crates/ui-egui/src/dialogs_motion_plot.rs +++ b/crates/ui-egui/src/dialogs_motion_plot.rs @@ -79,7 +79,7 @@ pub fn plot_params(s: &Session, what: Plotted, study: &Value, duration: &str, in } let d = duration.trim(); if !d.is_empty() { - let seconds = d.parse::().ok().filter(|x| x.is_finite() && *x > 0.0).ok_or("the duration must be a number of seconds, more than 0")?; + let seconds = d.parse::().ok().filter(|x| (1e-3..=1e9).contains(x)).ok_or("the duration must be a number of seconds, 0.001 or more")?; p["duration"] = json!(seconds); } Ok(p) @@ -276,8 +276,10 @@ fn collision_rows( && let Some((st, mut p)) = check { let mut sc = app.session.scratch(); - p["study"] = st.get("study").or_else(|| st.get("name")).cloned().unwrap_or(Value::Null); - let r = sc.execute("motion.study", &st).and_then(|_| sc.execute("motion.interference", &p)).map_err(|e| e.to_string()); + let r = super::scratch_study(&mut sc, &st).and_then(|id| { + p["study"] = id; + sc.execute("motion.interference", &p).map_err(|e| e.to_string()) + }); a.collisions = Some((key, r)); } }); @@ -369,9 +371,9 @@ fn plot_rows( let params = study.clone().and_then(|st| plot_params(s, what, &st, &a.duration, inputs).map(|p| (st, p))); let key = key_of(¶ms); if a.plot.as_ref().is_none_or(|(k, _)| *k != key) { - let r = params.and_then(|(st, p)| { + let r = params.and_then(|(st, mut p)| { let mut sc = s.scratch(); - sc.execute("motion.study", &st).map_err(|e| e.to_string())?; + p["study"] = super::scratch_study(&mut sc, &st)?; sc.execute("motion.plot", &p).map_err(|e| e.to_string()) }); a.plot = Some((key, r)); @@ -380,10 +382,10 @@ fn plot_rows( match a.plot.as_ref().map(|(_, r)| r) { Some(Ok(v)) => { ui.vertical(|ui| { - if let Some((xs, ys, unit)) = line(v, what, a.quantity) { - if let Some(to) = chart(ui, &xs, &ys, &unit, total, *step, &bands, t) { - (*step, *playing) = (to, false); - } + if let Some((xs, ys, unit)) = line(v, what, a.quantity) + && let Some(to) = chart(ui, &xs, &ys, &unit, total, *step, &bands, t) + { + (*step, *playing) = (to, false); } if let Some(why) = stop_text(v) { ui.add(egui::Label::new(RichText::new(why).color(t.warning)).wrap());