diff --git a/docs/design/layout-quality.md b/docs/design/layout-quality.md index 44be2ad5b..1fa4961b9 100644 --- a/docs/design/layout-quality.md +++ b/docs/design/layout-quality.md @@ -157,6 +157,53 @@ values beside the current ones. The committed baseline (`examples/layout_eval_baseline.json`, see its README) is diffed the same way on every run. +## Evaluating edits + +A diagram an agent or a notebook user edits is synced after every patch by +`incremental_layout`. Its quality is not one static score: a sync must leave +alone what the edit did not touch, keep the view consistent with the model, and +put what it creates somewhere sensible. `layout::edit_audit` states that +contract from an edit's inputs and outputs alone, in three layers: + +- **Scope.** An untouched element comes back exactly as it was. Touched is + derived from the two models and the patch: a deleted variable, a renamed one + (only its name changes), one whose kind changed (rebuilt, keeping its center + unless it became a flow or its new shape there would cover another shape), a flow whose attachment changed. A link whose + dependency survives keeps its uid, endpoints, polarity, and shape, and a link + drawing no dependency the model has survives unless the patch names its + reader. A connector the view did not draw is drawn only into a variable the + patch names or between elements drawn for the first time, and a variable the + view did not draw is drawn only when the patch names it: what an author left + out elsewhere stays out. +- **Consistency.** Every variable drawn once with its kind, references resolve, + links and drawn dependencies agree, flows attach where the stock lists say, + and every flow the sync created or changed holds the strict flow invariants + (`editing::invariants`). Only findings the edit introduced count, so an + imported view's own inconsistencies are not charged to an edit. +- **Placement.** What the sync created or changed does not cover another shape + it did not already cover before the edit, and a pipe it routed does not pass + through a stock that is not one of its ends. + +It also records what it does not charge: how far rebuilt elements moved, and +the metric's cost before and after. + +`layout::edit_scenarios` generates the edits for any model -- restate a +variable, add or delete a parameter, insert an intermediate, delete a flow or a +middle stock, detach a flow, turn an aux into a stock, rename (with the rename +operation, and the way an agent without one does it), add a flow between two +stocks, close a loop, extend a chain, add a side flow, add a sector, add then +undo -- picking targets deterministically, and runs each through `apply_patch` +and the production sync rule, auditing every step and checking that two syncs +of one edit agree and that an edit expected to return the original view does. + +The unit battery (`layout/edit_scenarios_tests.rs`) drives every scenario over +hand-drawn and imported views and pins every finding in `KNOWN_DEFECTS`, one row +per (fixture, scenario, finding) naming the defect: a finding no row expects +fails, and so does a row that no longer reproduces. The harness runs the same +scenarios over the whole corpus (`LAYOUT_EVAL_EDITS=0` skips them) and writes +`edits.json` and `edits.html`, with each run's last step rendered before (removed +elements marked) and after (created, changed, and every located finding marked). + ## The improvement loop 1. Run the harness on the current code into one directory, and on the changed diff --git a/src/libsimlin/CLAUDE.md b/src/libsimlin/CLAUDE.md index 7e1378c66..d72b004e8 100644 --- a/src/libsimlin/CLAUDE.md +++ b/src/libsimlin/CLAUDE.md @@ -71,7 +71,7 @@ Error formatting has no module here: `src/patch.rs` imports `simlin_engine::erro ### Layout - **`src/layout.rs`** - Automatic diagram layout: - - `simlin_project_diagram_sync(project, model_name, patch_json, out_error)` - Generate layout for a model, replacing its views in-place. When `patch_json` is non-null and the model already has a non-empty view, uses incremental layout (preserving existing element positions); otherwise generates a full layout from scratch. Preserves existing zoom. Works on all targets including WASM. Requires the project to be synced to the salsa db first (returns an error otherwise). + - `simlin_project_diagram_sync(project, model_name, patch_json, out_error)` - Generate layout for a model, replacing its first view in place (added when the model has none); any other view the project carries is the author's and comes back unchanged. When `patch_json` is non-null and the model already has a non-empty view, uses incremental layout (preserving existing element positions); otherwise generates a full layout from scratch. Preserves existing zoom. Works on all targets including WASM. Requires the project to be synced to the salsa db first (returns an error otherwise). ### Diagram editing diff --git a/src/libsimlin/src/layout.rs b/src/libsimlin/src/layout.rs index 57bbe8712..9d75525f2 100644 --- a/src/libsimlin/src/layout.rs +++ b/src/libsimlin/src/layout.rs @@ -214,7 +214,13 @@ pub unsafe extern "C" fn simlin_project_diagram_sync( layout.zoom = zoom; } - // Model existence was verified above, so this should always succeed. + // Model existence was verified above, so this should always succeed. The + // layout syncs the first view only; any other view the project carries is + // the author's and stays as it was. let model = datamodel_locked.get_model_mut(model_name_str).unwrap(); - model.views = vec![engine::datamodel::View::StockFlow(layout)]; + let view = engine::datamodel::View::StockFlow(layout); + match model.views.first_mut() { + Some(first) => *first = view, + None => model.views.push(view), + } } diff --git a/src/libsimlin/tests/integration/diagram.rs b/src/libsimlin/tests/integration/diagram.rs index b53360a6f..762b9d9c3 100644 --- a/src/libsimlin/tests/integration/diagram.rs +++ b/src/libsimlin/tests/integration/diagram.rs @@ -47,6 +47,50 @@ fn test_diagram_sync_sir_model() { } } +#[test] +fn test_diagram_sync_keeps_every_view_but_the_first() { + // The sync lays out a model's first view; a project can carry more, and + // those are the author's, so they come back exactly as they were. + let mut datamodel = TestProject::new("two_views") + .with_sim_time(0.0, 10.0, 1.0) + .stock("population", "100", &["births"], &[], None) + .flow("births", "population * 0.02", None) + .build_datamodel(); + let laid_out = engine::layout::generate_best_layout(&datamodel, "main", None).expect("layout"); + let overview = datamodel::View::StockFlow(datamodel::StockFlow { + zoom: 0.5, + ..laid_out + }); + let empty_first = datamodel::View::StockFlow(datamodel::StockFlow { + elements: Vec::new(), + ..match &overview { + datamodel::View::StockFlow(sf) => sf.clone(), + } + }); + datamodel.models[0].views = vec![empty_first, overview.clone()]; + let proj = open_project_from_datamodel(&datamodel); + + unsafe { + let model_name = CString::new("main").unwrap(); + let mut err: *mut SimlinError = ptr::null_mut(); + simlin_project_diagram_sync(proj, model_name.as_ptr(), ptr::null(), &mut err); + assert!(err.is_null(), "diagram_sync should succeed"); + + let datamodel_locked = (*proj).datamodel.lock().unwrap(); + let model = datamodel_locked.get_model("main").unwrap(); + assert_eq!(model.views.len(), 2, "no view is dropped"); + let datamodel::View::StockFlow(first) = &model.views[0]; + assert!(!first.elements.is_empty(), "the first view is laid out"); + assert!( + model.views[1] == overview, + "the second view comes back as it was" + ); + drop(datamodel_locked); + + simlin_project_unref(proj); + } +} + #[test] fn test_diagram_sync_test_project() { let test_project = TestProject::new("layout_test") diff --git a/src/pysimlin/e2e/notebook-editor.spec.ts b/src/pysimlin/e2e/notebook-editor.spec.ts index a0772df98..87db1390e 100644 --- a/src/pysimlin/e2e/notebook-editor.spec.ts +++ b/src/pysimlin/e2e/notebook-editor.spec.ts @@ -285,11 +285,14 @@ test('pysimlin-widget.AC4.2: JupyterLab notebook edits a model file through the // Put the creation tool away, then a click (no drag) on the variable's // circle opens its details (a click on the label would start renaming - // it). Clicking the rendered-equation preview swaps in the raw editor. - // The details panel's classes are CSS-module names, `-`. + // it). The new aux's empty equation is an equation error, and a variable + // with an equation error opens its details on the raw equation editor rather + // than the rendered preview (VariableDetails' showPreview), so the editor is + // already there to type into. The details panel's classes are CSS-module + // names, `-`. await widget.getByRole('button', { name: 'Variable', exact: true }).click(); await canvas.locator('g.simlin-aux', { hasText: 'New Variable' }).locator('circle').first().click(); - await widget.locator('[class*="eqnPreview"]').click(); + await expect(widget.getByText('error: Variable has empty equation')).toBeVisible(); const equationEditor = widget.locator('[data-slate-editor="true"][class*="eqnEditor"]'); await expect(equationEditor).toBeVisible(); await equationEditor.click(); diff --git a/src/simlin-engine/CLAUDE.md b/src/simlin-engine/CLAUDE.md index 4a82abf3e..5df4dbd42 100644 --- a/src/simlin-engine/CLAUDE.md +++ b/src/simlin-engine/CLAUDE.md @@ -188,8 +188,9 @@ Opt-in: a model that declares units on no variable gets no unit diagnostics. `un ## Analysis, layout, diagrams - `analysis.rs::analyze_model` bundles compilation, LTM discovery, and dominant-period selection into `ModelAnalysis`; a model that cannot compile returns `Ok` with `analysis_error` set so "could not analyze" is distinct from "no loops". -- `layout/` generates and incrementally updates diagram layouts (force-directed placement, crossing reduction, a calibrated quality metric; deterministic per seed). The metric and the eval harness that measures layouts against it are described in [layout quality](/docs/design/layout-quality.md). The incremental path lives in `layout/incremental.rs`: chains an edit adds whole are laid out as chains beside the diagram, a new stock hung off a drawn chain continues its row, and what the edit added is decluttered around the fixed diagram (`declutter::declutter_part`). Incremental layout never rewrites an element the patch did not touch: position and `label_side` come back byte for byte (`layout_label_tests.rs` enumerates the arms), a label side is chosen only for elements created in that pass, and a new connector that runs through an existing label is accepted rather than re-optimizing its neighbours -- hand placement wins, and re-optimizing is exactly the churn that snaps a notebook user's dragged label somewhere else on the next edit. A label the layout wraps carries the stored two-character `\n` escape (`text::LABEL_LINE_BREAK`, the form the TypeScript editor's `encodeNameNewlines` produces), never a raw newline. -- The flow arm of that rule (`layout_flow_tests.rs` enumerates it): incremental layout rebuilds a flow only when the patch creates it or changes the flow's own attachment -- moves it to another stock, drops it from a stock's list (that end becomes a cloud), lists it on a stock at its cloud end (that end becomes the stock), deletes an attached stock, or changes an attached stock's kind -- because its stored endpoints then name the wrong element. A flow the patch names keeps its geometry (a rename changes only its name, a delete removes it with its clouds), and every other flow, a sibling of a flow added to or removed from the same stock included, comes back byte for byte even where a fresh layout would draw it differently. A created side flow takes a face its stock's drawn side flows leave free where there is one. The endpoint snap (`resnap_flow_endpoints`), the slot placement (`face_slots`: a created flow's stock end keeps the design plan's `PIPE_SPACING` from the ends already on its face where the face has room, else the farthest slot, always within the corner clearance; a two-point flow between parallel faces takes one line only where that line keeps the spacing on both faces; a created cloud end keeps off other clouds) and the finishing pass (`finish_flow_geometry`) run only on the flows the pass creates. The one repair made to an untouched flow is wiring an endpoint the view left unattached to the flow's own cloud (`diff_clouds`), which moves nothing. +- `layout/` generates and incrementally updates diagram layouts (force-directed placement, crossing reduction, a calibrated quality metric; deterministic per seed). The metric and the eval harness that measures layouts against it are described in [layout quality](/docs/design/layout-quality.md). The incremental path lives in `layout/incremental.rs`: chains an edit adds whole are laid out as chains beside the diagram, a new stock hung off a drawn chain continues its row, and what the edit added is decluttered around the fixed diagram (`declutter::declutter_part`). Incremental layout never rewrites an element the patch did not touch: position and `label_side` come back byte for byte (`layout_label_tests.rs` enumerates the arms), a label side is chosen only for elements created in that pass, and a new connector that runs through an existing label is accepted rather than re-optimizing its neighbours -- hand placement wins, and re-optimizing is exactly the churn that snaps a notebook user's dragged label somewhere else on the next edit. A dependency the view does not draw gets a connector only where the edit is about it -- into a variable the patch names (upserts, renames, re-lists), or where an end is drawn for the first time -- so a connector an author left out stays out whatever unrelated edit follows; likewise a link drawing no dependency the model has goes only with an edit to its reader, and a variable the view does not draw is drawn only when the patch names it. A connector or cloud the pass creates references only what the view draws: a variable left out of the view can still have a uid (a project MCP opened gives every variable one), and a link into it or a cloud of it would name no element. An element the pass rebuilds -- a variable whose kind changed, a flow whose attachment changed -- keeps its uid (`LayoutState::remove_for_rebuild`), so the links and aliases touching it survive, and a surviving curved link whose endpoint moved turns its takeoff with its chord (`rebow_moved_links`); a variable that became a stock, aux or module is redrawn at its old center and no polish pass moves it, save that one whose new, larger shape covers another shape there (a parameter turned into a stock) moves to the nearest clear spot. A label the layout wraps carries the stored two-character `\n` escape (`text::LABEL_LINE_BREAK`, the form the TypeScript editor's `encodeNameNewlines` produces), never a raw newline. +- The flow arm of that rule (`layout_flow_tests.rs` enumerates it): incremental layout rebuilds a flow only when the patch creates it or changes the flow's own attachment -- moves it to another stock, drops it from a stock's list (that end becomes a cloud), lists it on a stock at its cloud end (that end becomes the stock), deletes an attached stock, or changes an attached stock's kind -- because its stored endpoints then name the wrong element. A drawn flow whose attachment changed is re-attached in place through the editing core (`retarget_flow`, over `editing::{heal, route, route_end}`): an end that becomes a cloud stays where it was, moved off a stock it no longer attaches to (`clear_of_stocks`), an end that attaches to another stock is routed to it keeping as much of the pipe as stays valid, and the flow keeps its uid, name and valve; only a flow that must attach to a stock this pass creates is rebuilt as new. A flow the patch names keeps its geometry (a rename changes only its name, a delete removes it with its clouds), and every other flow, a sibling of a flow added to or removed from the same stock included, comes back byte for byte even where a fresh layout would draw it differently. A created side flow takes a face its stock's drawn side flows leave free where there is one. The endpoint snap (`resnap_flow_endpoints`), the slot placement (`face_slots`: a created flow's stock end keeps the design plan's `PIPE_SPACING` from the ends already on its face where the face has room, else the farthest slot, always within the corner clearance; a two-point flow between parallel faces takes one line only where that line keeps the spacing on both faces; a created cloud end keeps off other clouds) and the finishing pass (`finish_flow_geometry`) run only on the flows the pass creates. The one repair made to an untouched flow is wiring an endpoint the view left unattached to the flow's own cloud (`diff_clouds`), which moves nothing. +- What a sync after an edit must do is stated independently of `incremental.rs` by `layout/edit_audit.rs` (scope: untouched elements and surviving links come back as they were; consistency: the view agrees with the model and routed flows hold the strict invariants, charging only what the edit introduced; placement: created elements land on no shape and routed pipes through no foreign stock), and `layout/edit_scenarios.rs` generates the edits an agent makes for any model and drives them through `apply_patch` and the production sync rule. The battery (`edit_scenarios_tests.rs`) pins every finding in `KNOWN_DEFECTS`, each row naming its defect: a new finding fails, and so does a row that stops reproducing, so a fix deletes its rows. Element order is the view's draw order and what a saved file lists: a sync keeps surviving links and clouds in place and appends what it creates in a deterministic order. - `diagram/` renders a model's first stock-and-flow view as SVG (`render_svg`, byte-identical to the TypeScript static renderer; `src/diagram/tests/svg-rendering.test.ts` is that parity test), as PNG (`render_png`, behind `png_render`), and as a scene display list (`build_scene`, whose contract is [the diagram scene](/docs/design/diagram-scene.md)), and exposes the exact geometry the layout metric scores. Every drawing decision has one owner, and both serializers read it: - `resolve::resolve_view` decides which elements are drawn (a link or flow whose endpoints are missing from the view is not), their layer and draw order, whether each is arrayed, an alias's target, and the fit-to-content bounds. - Each element's geometry is one function: `aux_geometry`, `stock_geometry`, `module_geometry`, `alias_geometry`, `group_geometry`, `cloud_transform` (over the shared `CLOUD_PATH`), `flow_geometry`, `connector_geometry`, `arrowhead_geometry`, and `label::label_lines` for per-line label anchors. `render_*` prints from it and `scene.rs` reads it. Never compute a drawn number a second time in either serializer: a hand-maintained twin drifts exactly where the geometry is non-trivial; extend the geometry function instead. diff --git a/src/simlin-engine/examples/layout_eval/edits.rs b/src/simlin-engine/examples/layout_eval/edits.rs new file mode 100644 index 000000000..5d2939cbe --- /dev/null +++ b/src/simlin-engine/examples/layout_eval/edits.rs @@ -0,0 +1,309 @@ +// Copyright 2026 The Simlin Authors. All rights reserved. +// Use of this source code is governed by the Apache License, +// Version 2.0, that can be found in the LICENSE file. + +//! Edit scenarios over the corpus (`layout::edit_scenarios`): every scenario +//! kind driven from a model's shipped diagram (its production layout when it +//! ships none) through the production patch and sync path, and audited step by +//! step (`layout::edit_audit`). Each run is rendered before and after its last +//! step, marking what the sync created (green), changed (orange) and removed +//! (red, on the before render), and every finding with a location (magenta, on +//! the after render). Writes `edits.json` and the `edits.html` contact sheet. + +use std::collections::BTreeMap; +use std::fmt::Write as _; + +use serde::Serialize; +use simlin_engine::datamodel::{self, StockFlow}; +use simlin_engine::layout::edit_audit::{ChangeKind, Displacement, Finding, view_changes}; +use simlin_engine::layout::edit_scenarios::{ScenarioKind, build_scenario, run_scenario}; +use simlin_engine::layout::generate_best_layout; + +use crate::corpus::{self, MAIN_MODEL, ModelSpec}; +use crate::render; +use crate::report::{html_escape, write_json}; + +/// One scenario run on one model. +#[derive(Serialize)] +pub struct EditReport { + pub model: String, + pub scenario: String, + pub description: String, + /// Steps that applied and synced, of `planned`. + pub steps: usize, + pub planned: usize, + pub findings: Vec, + /// For a rename spelled as a delete and a create: how far the variable + /// moved. + pub continuity: Vec, + /// Elements rebuilt by kind changes or re-attachment, and how far each moved. + pub displacements: Vec, + /// Layout-quality cost before the first step and after the last. + pub cost_before: Option, + pub cost_after: Option, + pub before_png: Option, + pub after_png: Option, +} + +fn rect(region: [f64; 4], stroke: &str, fill: &str, dash: &str) -> String { + let [l, t, r, b] = region; + format!( + "", + l - 3.0, + t - 3.0, + (r - l).max(0.0) + 6.0, + (b - t).max(0.0) + 6.0 + ) +} + +/// SVG marks for one render: what changed, and for the after render what the +/// audit found. +fn marks(before: &StockFlow, after: &StockFlow, findings: &[Finding], after_side: bool) -> String { + let mut svg = String::from(""); + for change in view_changes(before, after) { + let Some(region) = change.region else { + continue; + }; + match (change.kind, after_side) { + (ChangeKind::Created, true) => { + svg.push_str(&rect(region, "#2e7d32", "rgba(46,125,50,0.10)", "none")) + } + (ChangeKind::Changed, true) => { + svg.push_str(&rect(region, "#ef6c00", "rgba(239,108,0,0.10)", "4,2")) + } + (ChangeKind::Removed, false) => { + svg.push_str(&rect(region, "#c62828", "rgba(198,40,40,0.10)", "4,2")) + } + _ => {} + } + } + if after_side { + for f in findings { + if let Some(region) = f.region { + svg.push_str(&rect(region, "#ad1457", "rgba(173,20,87,0.20)", "none")); + } + } + } + svg.push_str(""); + svg +} + +fn with_view(project: &datamodel::Project, view: &StockFlow) -> datamodel::Project { + let mut p = project.clone(); + if let Some(m) = p.get_model_mut(MAIN_MODEL) { + m.views = vec![datamodel::View::StockFlow(view.clone())]; + } + p +} + +/// Run every applicable scenario on `spec`'s model, rendering each run into +/// `out`. A model that fails to load or lay out is reported and yields nothing. +pub fn run_model(spec: &ModelSpec, out: &str) -> Vec { + let project = match corpus::load_model(spec) { + Ok(p) => p, + Err(err) => { + eprintln!("WARN: {} edits: {err}", spec.key); + return Vec::new(); + } + }; + let view = match corpus::reference_view(&project) { + Some(sf) => sf.clone(), + None => match generate_best_layout(&project, MAIN_MODEL, None) { + Ok(v) => v, + Err(err) => { + eprintln!("WARN: {} edits: no starting diagram: {err}", spec.key); + return Vec::new(); + } + }, + }; + let start = with_view(&project, &view); + let mut reports = Vec::new(); + for kind in ScenarioKind::ALL { + let Some(scenario) = build_scenario(&start, MAIN_MODEL, kind) else { + continue; + }; + let outcome = run_scenario(&start, MAIN_MODEL, &view, &scenario); + let stem = format!("{}_edit_{}", spec.key, kind.name()); + let (before_png, after_png) = match outcome.steps.last() { + Some(step) => { + let before_project = match outcome.steps.len() { + 1 => &start, + n => &outcome.steps[n - 2].after, + }; + let step_findings: Vec = step.audit.findings.clone(); + let before_file = format!("{stem}_before.png"); + let after_file = format!("{stem}_after.png"); + let before_ok = render::render_marked( + before_project, + &step.before_view, + &before_file, + out, + &marks(&step.before_view, &step.after_view, &[], false), + ); + let after_ok = render::render_marked( + &step.after, + &step.after_view, + &after_file, + out, + &marks(&step.before_view, &step.after_view, &step_findings, true), + ); + ( + before_ok.then_some(before_file), + after_ok.then_some(after_file), + ) + } + None => (None, None), + }; + let audits = outcome.steps.iter().map(|s| &s.audit); + reports.push(EditReport { + model: spec.key.clone(), + scenario: kind.name().to_string(), + description: outcome.description.clone(), + steps: outcome.steps.len(), + planned: scenario.steps.len(), + findings: outcome.findings.clone(), + continuity: outcome.continuity.clone(), + displacements: audits.flat_map(|a| a.displacements.clone()).collect(), + cost_before: outcome.steps.first().map(|s| s.audit.cost_before), + cost_after: outcome.steps.last().map(|s| s.audit.cost_after), + before_png, + after_png, + }); + } + let with_findings = reports.iter().filter(|r| !r.findings.is_empty()).count(); + println!( + "{}: edits: {} scenario(s), {with_findings} with findings", + spec.key, + reports.len() + ); + reports +} + +/// Write `edits.json` and `edits.html` under `out`. +pub fn write(reports: &[EditReport], out: &str) { + write_json(&format!("{out}/edits.json"), &reports); + let path = format!("{out}/edits.html"); + match std::fs::write(&path, render_html(reports)) { + Ok(()) => println!("wrote {path}"), + Err(err) => eprintln!("WARN: failed to write {path}: {err}"), + } +} + +fn render_html(reports: &[EditReport]) -> String { + let mut html = String::new(); + html.push_str( + "\n\n\n\n\ + \n\ + Diagram edit eval\n\n\n\n

Diagram edit eval

\n", + ); + let dirty = reports.iter().filter(|r| !r.findings.is_empty()).count(); + let _ = writeln!( + html, + "

{} scenario run(s) over {} model(s); {dirty} with findings.

", + reports.len(), + reports + .iter() + .map(|r| &r.model) + .collect::>() + .len() + ); + html.push_str( + "

created\ + changed\ + removed (before)\ + finding

", + ); + + let mut counts: BTreeMap<&str, usize> = BTreeMap::new(); + for r in reports { + for f in &r.findings { + *counts.entry(f.kind.name()).or_default() += 1; + } + } + html.push_str(""); + for (kind, n) in &counts { + let _ = write!(html, ""); + } + html.push_str("
findings by kind
{kind}{n}
\n"); + + let mut model = ""; + for r in reports { + if r.model != model { + model = &r.model; + let _ = writeln!(html, "

{}

", html_escape(model)); + } + let class = if r.findings.is_empty() { + "clean" + } else { + "dirty" + }; + let _ = write!( + html, + "

{}

{}", + html_escape(&r.scenario), + html_escape(&r.description) + ); + if let (Some(before), Some(after)) = (r.cost_before, r.cost_after) { + let _ = write!(html, " · cost {before:.3} → {after:.3}"); + } + if r.steps < r.planned { + let _ = write!( + html, + " · stopped after {} of {} steps", + r.steps, r.planned + ); + } + html.push_str("

"); + if !r.findings.is_empty() { + html.push_str("
    "); + for f in &r.findings { + let _ = write!( + html, + "
  • {} {}: {}
    • ", + f.kind.name(), + html_escape(&f.subject), + html_escape(&f.detail) + ); + } + html.push_str("
"); + } + for d in r.displacements.iter().chain(&r.continuity) { + let _ = write!( + html, + "

{} moved {:.1}

", + html_escape(&d.subject), + d.distance + ); + } + html.push_str("
"); + for file in [&r.before_png, &r.after_png].into_iter().flatten() { + let src = html_escape(file); + let _ = write!( + html, + "\"{src}\"" + ); + } + html.push_str("
\n"); + } + html.push_str("\n\n"); + html +} diff --git a/src/simlin-engine/examples/layout_eval/knobs.rs b/src/simlin-engine/examples/layout_eval/knobs.rs index d8312ff40..ba67fa61f 100644 --- a/src/simlin-engine/examples/layout_eval/knobs.rs +++ b/src/simlin-engine/examples/layout_eval/knobs.rs @@ -36,6 +36,8 @@ pub struct Knobs { /// `LAYOUT_EVAL_REPLAY_STEPS`: edits in the incremental-build replay; 0 /// skips it. pub replay_steps: usize, + /// `LAYOUT_EVAL_EDITS=0` skips the edit scenarios. + pub edits: bool, } fn list(name: &str) -> Option> { @@ -110,6 +112,10 @@ impl Knobs { .ok() .and_then(|v| v.parse().ok()) .unwrap_or(DEFAULT_REPLAY_STEPS), + edits: !matches!( + env::var("LAYOUT_EVAL_EDITS").unwrap_or_default().trim(), + "0" | "false" + ), } } } diff --git a/src/simlin-engine/examples/layout_eval/main.rs b/src/simlin-engine/examples/layout_eval/main.rs index 7d196185e..fe2c377dc 100644 --- a/src/simlin-engine/examples/layout_eval/main.rs +++ b/src/simlin-engine/examples/layout_eval/main.rs @@ -11,11 +11,15 @@ //! incrementally after each (what an agent or notebook user GETS), and render //! the hand-authored reference, the production and incremental layouts, and //! the median and worst seeds to PNG. Writes `metrics.json`, `corpus.json`, and an -//! `index.html` contact sheet under a gitignored `target/` directory. +//! `index.html` contact sheet under a gitignored `target/` directory. Then it +//! drives every edit scenario (`layout::edit_scenarios`) from the model's +//! diagram, audits each synced step (`layout::edit_audit`), and writes +//! `edits.json` and an `edits.html` contact sheet of marked before and after +//! renders. //! //! This is a thin imperative shell over the metric core -//! (`layout::metrics::compute_layout_metrics`) and the statistics core -//! (`layout::eval_stats`). +//! (`layout::metrics::compute_layout_metrics`), the statistics core +//! (`layout::eval_stats`), and the edit audit. //! //! Usage: //! cargo run --release -p simlin-engine --features png_render,file_io --example layout_eval @@ -35,6 +39,7 @@ //! LAYOUT_EVAL_DECLUTTER 0 -> disable the declutter pass in the seed sweep //! LAYOUT_EVAL_REPLAY_STEPS edits in the incremental-build replay (default 4; //! 0 skips the replay) +//! LAYOUT_EVAL_EDITS 0 -> skip the edit scenarios //! //! Baseline diff: the committed `examples/layout_eval_baseline.json` (a //! serialized `CorpusReport`) records a reference run. A normal run re-scores @@ -45,6 +50,7 @@ //! and `file_io` so Vensim corpus models that reference external data load. mod corpus; +mod edits; mod knobs; mod render; mod replay; @@ -275,6 +281,7 @@ fn main() { let mut per_model = Vec::new(); let mut renders = Vec::new(); let mut facts = Vec::new(); + let mut edit_reports = Vec::new(); for spec in &specs { match process_model(spec, &seeds, &knobs) { Ok((stats, model_renders, model_facts)) => { @@ -284,6 +291,12 @@ fn main() { } Err(err) => eprintln!("WARN: skipping {}: {err}", spec.key), } + if knobs.edits { + edit_reports.extend(edits::run_model(spec, &knobs.out)); + } + } + if knobs.edits { + edits::write(&edit_reports, &knobs.out); } let weights = MetricWeights::default(); diff --git a/src/simlin-engine/examples/layout_eval/render.rs b/src/simlin-engine/examples/layout_eval/render.rs index 4c167eef2..c78be1110 100644 --- a/src/simlin-engine/examples/layout_eval/render.rs +++ b/src/simlin-engine/examples/layout_eval/render.rs @@ -108,6 +108,33 @@ fn rasterize(svg: &str, file: &str, out: &str) -> bool { true } +/// Render `view` (installed into a clone of `project`) to `{out}/{file}` with +/// the SVG `marks` drawn over it. On any failure WARN and return `false`. +pub fn render_marked( + project: &datamodel::Project, + view: &datamodel::StockFlow, + file: &str, + out: &str, + marks: &str, +) -> bool { + let mut p = project.clone(); + let Some(model) = p.get_model_mut(MAIN_MODEL) else { + return false; + }; + model.views = vec![datamodel::View::StockFlow(view.clone())]; + let svg = match render_svg(&p, MAIN_MODEL) { + Ok(svg) => svg, + Err(err) => { + eprintln!("WARN: failed to render {file}: {err}"); + return false; + } + }; + let Some(end) = svg.rfind("") else { + return false; + }; + rasterize(&format!("{}{marks}", &svg[..end]), file, out) +} + /// Render `view` (installed into a clone of `project`) to `{out}/{file}` and /// score it; with `overlay`, also write `{stem}_defects.png` with the metric's /// defects drawn over it. On any failure WARN and return `None` so the sweep diff --git a/src/simlin-engine/examples/layout_eval/report.rs b/src/simlin-engine/examples/layout_eval/report.rs index e3122db6a..6b68776b0 100644 --- a/src/simlin-engine/examples/layout_eval/report.rs +++ b/src/simlin-engine/examples/layout_eval/report.rs @@ -170,7 +170,7 @@ pub fn build_report( /// HTML-escape the five characters special in element text or attribute /// values. Model keys and filenames are static, so this is defense in depth. -fn html_escape(s: &str) -> String { +pub fn html_escape(s: &str) -> String { let mut out = String::with_capacity(s.len()); for ch in s.chars() { match ch { diff --git a/src/simlin-engine/src/ast/mod.rs b/src/simlin-engine/src/ast/mod.rs index 06afbb527..a22831423 100644 --- a/src/simlin-engine/src/ast/mod.rs +++ b/src/simlin-engine/src/ast/mod.rs @@ -554,8 +554,10 @@ pub(crate) fn needs_quoting(canonical: &str) -> bool { } /// Canonicalize an identifier for display, re-quoting if the canonical form -/// contains characters that can't appear in a bare identifier. -fn print_ident(raw: &str) -> String { +/// contains characters that can't appear in a bare identifier. The one spelling +/// of a name inside equation text: code composing an equation from names goes +/// through it rather than interpolating a name bare. +pub(crate) fn print_ident(raw: &str) -> String { let canonical = canonicalize(raw); if needs_quoting(&canonical) { format!("\"{}\"", canonical) diff --git a/src/simlin-engine/src/editing/mod.rs b/src/simlin-engine/src/editing/mod.rs index 013ce1044..038025f3b 100644 --- a/src/simlin-engine/src/editing/mod.rs +++ b/src/simlin-engine/src/editing/mod.rs @@ -21,7 +21,7 @@ mod geometry; mod gesture; mod heal; mod hit; -#[cfg(any(test, feature = "test-support"))] +#[cfg(any(test, feature = "test-support", feature = "layout_eval"))] pub mod invariants; mod links; mod offset; @@ -52,3 +52,12 @@ pub use hit::{Hit, HitPart, hit_test}; pub use preview::{Preview, preview}; pub(crate) use edit_view::{derived_operations, edited_view}; +// The flow geometry incremental layout routes through when an edit re-attaches +// a drawn flow, so a tool edit's pipes are drawn by the same core as a touch +// edit's. +pub(crate) use heal::heal; +pub(crate) use path::{arc_position, path_length, place_valve, point_at_arc}; +pub(crate) use route::{route, route_end}; +pub(crate) use terminal::{ + CloudRef, FlowGeometry, flow_terminals, free_terminal, target_stock_terminal, +}; diff --git a/src/simlin-engine/src/layout/edit_audit.rs b/src/simlin-engine/src/layout/edit_audit.rs new file mode 100644 index 000000000..c196e28ac --- /dev/null +++ b/src/simlin-engine/src/layout/edit_audit.rs @@ -0,0 +1,1465 @@ +// Copyright 2026 The Simlin Authors. All rights reserved. +// Use of this source code is governed by the Apache License, +// Version 2.0, that can be found in the LICENSE file. + +//! The edit audit: what syncing a diagram after a model edit must and must not +//! do, checked from the edit's inputs and outputs alone. +//! +//! A diagram sync (`incremental_layout`, which MCP `edit_model`, libsimlin's +//! patch sync and pysimlin run after every edit) takes the view before the +//! edit, the patch, and the model after it. The audit states the contract a +//! sync keeps without reading how it keeps it: +//! +//! - **Scope.** An element the edit did not touch comes back exactly as it +//! was. What counts as touched is derived from the two models and the patch: +//! a deleted variable (its element, clouds, aliases and links go), a renamed +//! one (only its name changes), one whose kind changed (it is rebuilt, and +//! anything but a flow keeps its center, unless its new shape there would +//! cover another shape), and a flow whose attachment changed +//! (its pipe, valve and clouds may be rebuilt). A link whose dependency +//! survives keeps its uid, endpoints and polarity, and its shape too unless +//! an endpoint moved, when it keeps at least its kind (straight or curved). +//! A link drawing no dependency the model has (an author's connector the +//! extraction does not explain) survives unless the patch names its reader. +//! A connector the view did not draw is drawn only where the edit is about +//! it: into a variable the patch names, or between elements drawn for the +//! first time, and a variable the view did not draw is drawn only when the +//! patch names it. What an author left out elsewhere stays out. +//! The one change allowed to an untouched element is wiring a flow endpoint +//! the view left unattached to the flow's own cloud, which moves nothing. +//! - **Consistency.** The view after the edit agrees with the model after it: +//! every variable drawn once with its kind, references resolve, links and +//! drawn dependencies agree, flows attach where the stock lists say, and a +//! flow the sync created or changed holds the strict flow invariants +//! (`editing::invariants`). Only findings the edit introduced count: an +//! imported view may carry inconsistencies of its own, and the edit is not +//! charged for them. +//! - **Placement.** What the sync created or changed does not land on another +//! element's shape, and a pipe it routed does not pass through a stock that +//! is not one of its ends. Two shapes that already overlapped before the edit +//! are the author's, and are not charged. +//! +//! The runner-level findings (a sync that failed, two syncs of one edit that +//! disagree, an edit that should have returned the original view) are raised +//! by `layout::edit_scenarios`, which drives edits through the production +//! patch and sync path and audits every step. + +use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; + +use crate::common::canonicalize; +use crate::datamodel::view_element::{Flow, FlowPoint, LinkShape}; +use crate::datamodel::{self, StockFlow, Variable, ViewElement}; +use crate::diagram::common::Rect; +use crate::editing::invariants::{Mode, check_flow_invariants}; +use crate::patch::{ModelOperation, ModelPatch}; + +use super::compute_dependency_metadata; +use super::config::LayoutConfig; +use super::metadata::ComputedMetadata; +use super::metrics::{MetricWeights, compute_layout_metrics, node_shape_box}; + +/// Coordinates within this distance are the same position. +const GEOMETRY_EPSILON: f64 = 1e-6; + +/// Two shapes overlap when their boxes share more than this area (px^2): a +/// shared edge or a float's sliver is not an overlap anyone sees. +const MIN_OVERLAP_AREA: f64 = 1.0; + +/// A pipe passes through a stock when it enters the stock's box shrunk by this +/// much on every side, so a pipe running along a face or ending on it does not. +const STOCK_INTERIOR_INSET: f64 = 0.5; + +/// Which part of the contract a finding concerns. +#[cfg_attr(feature = "debug-derive", derive(Debug))] +#[derive(Clone, Copy, PartialEq, Eq, serde::Serialize)] +#[serde(rename_all = "snake_case")] +pub enum Layer { + Scope, + Consistency, + Placement, + Runner, +} + +/// One kind of finding. +#[cfg_attr(feature = "debug-derive", derive(Debug))] +#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, serde::Serialize)] +#[serde(rename_all = "snake_case")] +pub enum FindingKind { + /// An element of a variable the edit deleted is still drawn: its own + /// element, a cloud of its flow, or an alias of it. + DeletedElementRemains, + /// An element the edit did not touch was moved, rebuilt, removed, or + /// otherwise changed. + UntouchedElementChanged, + /// A link whose dependency survived the edit was removed, re-created, or + /// changed its endpoints, polarity or shape. + UntouchedLinkChanged, + /// A link whose dependency the edit removed (or one of whose ends it + /// deleted), or a link drawing no dependency into a variable the patch + /// names, is still drawn. + StaleLinkRemains, + /// A link the view did not draw was added for a dependency the edit is not + /// about: the patch names neither its reader, nor was either end drawn for + /// the first time. An author's view that leaves a connector out keeps it + /// out. + UnrelatedLinkAdded, + /// A variable the view did not draw was drawn although the patch does not + /// name it. An author's view that leaves a variable out keeps it out. + UnrelatedElementAdded, + /// A variable whose kind changed to anything but a flow was rebuilt away + /// from where its old element was, although its new shape at the old center + /// would cover no other shape. + RebuiltElementMoved, + /// The view's own properties (viewport, zoom, name, font, polarity style) + /// changed. + ViewPropertiesChanged, + /// A uid is duplicated or not positive. + UidProblem, + /// A variable has no element. + VariableNotDrawn, + /// A variable has more than one element. + VariableDrawnTwice, + /// An element's kind is not its variable's kind. + ElementKindMismatch, + /// A stock, flow, aux or module element names no variable. + ElementNamesNoVariable, + /// A link, cloud, alias or flow endpoint references an element that does + /// not exist or cannot be referenced that way. + DanglingReference, + /// A link draws a dependency the model does not have. + LinkWithoutDependency, + /// A dependency between two drawn variables has no link. + DependencyWithoutLink, + /// A flow end is attached to something the stock lists do not say. + FlowAttachmentMismatch, + /// A flow the sync created or changed violates a strict flow invariant + /// (or any flow violates a tolerant one). + FlowInvariant, + /// A created or changed element's shape covers another element's shape + /// that it did not already cover before the edit. + ShapeOverlap, + /// A created or changed pipe passes through a stock that is not one of + /// its ends. + PipeThroughStock, + /// Two syncs of the same edit produced different views. + NotDeterministic, + /// An edit sequence expected to leave the view as it began did not. + ReturnToOriginal, + /// Applying the patch or syncing the view failed. + SyncFailed, +} + +impl FindingKind { + pub const ALL: [FindingKind; 23] = [ + FindingKind::DeletedElementRemains, + FindingKind::UntouchedElementChanged, + FindingKind::UntouchedLinkChanged, + FindingKind::StaleLinkRemains, + FindingKind::UnrelatedLinkAdded, + FindingKind::UnrelatedElementAdded, + FindingKind::RebuiltElementMoved, + FindingKind::ViewPropertiesChanged, + FindingKind::UidProblem, + FindingKind::VariableNotDrawn, + FindingKind::VariableDrawnTwice, + FindingKind::ElementKindMismatch, + FindingKind::ElementNamesNoVariable, + FindingKind::DanglingReference, + FindingKind::LinkWithoutDependency, + FindingKind::DependencyWithoutLink, + FindingKind::FlowAttachmentMismatch, + FindingKind::FlowInvariant, + FindingKind::ShapeOverlap, + FindingKind::PipeThroughStock, + FindingKind::NotDeterministic, + FindingKind::ReturnToOriginal, + FindingKind::SyncFailed, + ]; + + /// A short stable name for reports. + pub fn name(self) -> &'static str { + match self { + FindingKind::DeletedElementRemains => "deleted_element_remains", + FindingKind::UntouchedElementChanged => "untouched_element_changed", + FindingKind::UntouchedLinkChanged => "untouched_link_changed", + FindingKind::StaleLinkRemains => "stale_link_remains", + FindingKind::UnrelatedLinkAdded => "unrelated_link_added", + FindingKind::UnrelatedElementAdded => "unrelated_element_added", + FindingKind::RebuiltElementMoved => "rebuilt_element_moved", + FindingKind::ViewPropertiesChanged => "view_properties_changed", + FindingKind::UidProblem => "uid_problem", + FindingKind::VariableNotDrawn => "variable_not_drawn", + FindingKind::VariableDrawnTwice => "variable_drawn_twice", + FindingKind::ElementKindMismatch => "element_kind_mismatch", + FindingKind::ElementNamesNoVariable => "element_names_no_variable", + FindingKind::DanglingReference => "dangling_reference", + FindingKind::LinkWithoutDependency => "link_without_dependency", + FindingKind::DependencyWithoutLink => "dependency_without_link", + FindingKind::FlowAttachmentMismatch => "flow_attachment_mismatch", + FindingKind::FlowInvariant => "flow_invariant", + FindingKind::ShapeOverlap => "shape_overlap", + FindingKind::PipeThroughStock => "pipe_through_stock", + FindingKind::NotDeterministic => "not_deterministic", + FindingKind::ReturnToOriginal => "return_to_original", + FindingKind::SyncFailed => "sync_failed", + } + } + + pub fn layer(self) -> Layer { + match self { + FindingKind::DeletedElementRemains + | FindingKind::UntouchedElementChanged + | FindingKind::UntouchedLinkChanged + | FindingKind::StaleLinkRemains + | FindingKind::UnrelatedLinkAdded + | FindingKind::UnrelatedElementAdded + | FindingKind::RebuiltElementMoved + | FindingKind::ViewPropertiesChanged => Layer::Scope, + FindingKind::UidProblem + | FindingKind::VariableNotDrawn + | FindingKind::VariableDrawnTwice + | FindingKind::ElementKindMismatch + | FindingKind::ElementNamesNoVariable + | FindingKind::DanglingReference + | FindingKind::LinkWithoutDependency + | FindingKind::DependencyWithoutLink + | FindingKind::FlowAttachmentMismatch + | FindingKind::FlowInvariant => Layer::Consistency, + FindingKind::ShapeOverlap | FindingKind::PipeThroughStock => Layer::Placement, + FindingKind::NotDeterministic + | FindingKind::ReturnToOriginal + | FindingKind::SyncFailed => Layer::Runner, + } + } +} + +/// One thing a sync got wrong. +#[cfg_attr(feature = "debug-derive", derive(Debug))] +#[derive(Clone, PartialEq, serde::Serialize)] +pub struct Finding { + pub kind: FindingKind, + /// What the finding is about, in the model's idents after the edit + /// (`births`, `cloud of births`, `link birth_rate -> births`), so a finding + /// before the edit and the same finding after it compare equal across a + /// rename. + pub subject: String, + pub detail: String, + /// Where it is on the view after the edit (`[left, top, right, bottom]`), + /// when it is anywhere. + pub region: Option<[f64; 4]>, +} + +impl Finding { + pub fn new( + kind: FindingKind, + subject: impl Into, + detail: impl Into, + region: Option<[f64; 4]>, + ) -> Finding { + Finding { + kind, + subject: subject.into(), + detail: detail.into(), + region, + } + } +} + +/// How far an element the edit rebuilt moved. +#[cfg_attr(feature = "debug-derive", derive(Debug))] +#[derive(Clone, PartialEq, serde::Serialize)] +pub struct Displacement { + pub subject: String, + pub distance: f64, +} + +/// The audit of one edit. +#[cfg_attr(feature = "debug-derive", derive(Debug))] +#[derive(Clone, Default, serde::Serialize)] +pub struct EditAudit { + pub findings: Vec, + /// Elements the edit rebuilt (kind changes, re-attached flows), and how far + /// each one's center or valve moved. + pub displacements: Vec, + /// The layout-quality cost of the view before and after the edit. + pub cost_before: f64, + pub cost_after: f64, +} + +impl EditAudit { + pub fn kinds(&self) -> BTreeSet { + self.findings.iter().map(|f| f.kind).collect() + } +} + +/// One edit: the model and view before it, the patch, and the model and view +/// after it. +pub struct EditInput<'a> { + pub model_name: &'a str, + pub before: &'a datamodel::Project, + pub before_view: &'a StockFlow, + pub patch: &'a ModelPatch, + pub after: &'a datamodel::Project, + pub after_view: &'a StockFlow, +} + +#[derive(Clone, Copy, PartialEq, Eq)] +enum VarKind { + Stock, + Flow, + Aux, + Module, +} + +fn var_kind(v: &Variable) -> VarKind { + match v { + Variable::Stock(_) => VarKind::Stock, + Variable::Flow(_) => VarKind::Flow, + Variable::Aux(_) => VarKind::Aux, + Variable::Module(_) => VarKind::Module, + } +} + +fn element_kind(e: &ViewElement) -> Option { + match e { + ViewElement::Stock(_) => Some(VarKind::Stock), + ViewElement::Flow(_) => Some(VarKind::Flow), + ViewElement::Aux(_) => Some(VarKind::Aux), + ViewElement::Module(_) => Some(VarKind::Module), + _ => None, + } +} + +/// The canonical ident a stock, flow, aux or module element names. +fn named_ident(e: &ViewElement) -> Option { + element_kind(e)?; + e.get_name().map(|n| canonicalize(n).into_owned()) +} + +fn center(e: &ViewElement) -> Option<(f64, f64)> { + match e { + ViewElement::Aux(a) => Some((a.x, a.y)), + ViewElement::Stock(s) => Some((s.x, s.y)), + ViewElement::Flow(f) => Some((f.x, f.y)), + ViewElement::Module(m) => Some((m.x, m.y)), + ViewElement::Alias(a) => Some((a.x, a.y)), + ViewElement::Cloud(c) => Some((c.x, c.y)), + ViewElement::Link(_) | ViewElement::Group(_) => None, + } +} + +fn distance(a: (f64, f64), b: (f64, f64)) -> f64 { + (a.0 - b.0).hypot(a.1 - b.1) +} + +fn region_of(e: &ViewElement) -> Option<[f64; 4]> { + let r = node_shape_box(e)?; + Some([r.left, r.top, r.right, r.bottom]) +} + +/// The renames a patch applies, from each ident before the edit to its ident +/// after, in the order the patch applies them. +struct Renames(HashMap); + +impl Renames { + fn of(patch: &ModelPatch) -> Renames { + let mut map: HashMap = HashMap::new(); + for op in &patch.ops { + if let ModelOperation::RenameVariable { from, to } = op { + let from = canonicalize(from).into_owned(); + let to = canonicalize(to).into_owned(); + let mut chained = false; + for target in map.values_mut() { + if *target == from { + *target = to.clone(); + chained = true; + } + } + if !chained { + map.insert(from, to); + } + } + } + Renames(map) + } + + fn image(&self, ident: &str) -> String { + self.0 + .get(ident) + .cloned() + .unwrap_or_else(|| ident.to_string()) + } + + /// The ident before the edit that `ident` after it had. + fn preimage(&self, ident: &str) -> String { + self.0 + .iter() + .find(|(_, to)| to.as_str() == ident) + .map(|(from, _)| from.clone()) + .unwrap_or_else(|| ident.to_string()) + } +} + +/// One side of an edit: a model, the dependencies its diagram draws, and a +/// view. +struct Side<'a> { + kinds: BTreeMap, + meta: ComputedMetadata, + /// `(dependency, dependent)` for every dependency a diagram draws: a + /// variable's reads, less a stock's own inflows and outflows (a pipe draws + /// those). + edges: BTreeSet<(String, String)>, + view: &'a StockFlow, + by_uid: HashMap, +} + +impl<'a> Side<'a> { + fn new( + project: &datamodel::Project, + model_name: &str, + view: &'a StockFlow, + ) -> Option> { + let model = project.get_model(model_name)?; + let meta = compute_dependency_metadata(project, model_name, None)?; + let kinds = model + .variables + .iter() + .map(|v| (canonicalize(v.get_ident()).into_owned(), var_kind(v))) + .collect(); + let mut edges = BTreeSet::new(); + for (var, deps) in &meta.dep_graph { + let listed = |lists: &HashMap>, dep: &str| { + lists.get(var).is_some_and(|l| l.iter().any(|f| f == dep)) + }; + for dep in deps { + if dep == var + || listed(&meta.stock_to_inflows, dep) + || listed(&meta.stock_to_outflows, dep) + { + continue; + } + edges.insert((dep.clone(), var.clone())); + } + } + let by_uid = view.elements.iter().map(|e| (e.get_uid(), e)).collect(); + Some(Side { + kinds, + meta, + edges, + view, + by_uid, + }) + } + + /// The named element a link endpoint draws, through an alias, with its + /// ident. + fn endpoint(&self, uid: i32) -> Option<(String, &'a ViewElement)> { + let e = *self.by_uid.get(&uid)?; + let e = match e { + ViewElement::Alias(a) => *self.by_uid.get(&a.alias_of_uid)?, + other => other, + }; + named_ident(e).map(|i| (i, e)) + } + + fn first_named(&self, ident: &str) -> Option<&'a ViewElement> { + self.view + .elements + .iter() + .find(|e| named_ident(e).as_deref() == Some(ident)) + } + + /// The stock idents a flow element's two ends are attached to. + fn attachment(&self, flow: &Flow) -> (Option, Option) { + let end = |p: Option<&FlowPoint>| -> Option { + let uid = p?.attached_to_uid?; + match self.by_uid.get(&uid)? { + ViewElement::Stock(s) => Some(canonicalize(&s.name).into_owned()), + _ => None, + } + }; + (end(flow.points.first()), end(flow.points.last())) + } + + fn expected_attachment(&self, flow_ident: &str) -> (Option, Option) { + let (from, to) = self.meta.connected_stocks(flow_ident); + (from.map(str::to_string), to.map(str::to_string)) + } +} + +fn subject_of(side: &Side, e: &ViewElement, name: &dyn Fn(&str) -> String) -> String { + match e { + ViewElement::Link(l) => { + let end = |uid: i32| { + side.endpoint(uid) + .map(|(i, _)| name(&i)) + .unwrap_or_else(|| format!("#{uid}")) + }; + format!("link {} -> {}", end(l.from_uid), end(l.to_uid)) + } + ViewElement::Cloud(c) => match side.by_uid.get(&c.flow_uid).and_then(|f| named_ident(f)) { + Some(f) => format!("cloud of {}", name(&f)), + None => format!("cloud #{}", c.uid), + }, + ViewElement::Alias(a) => match side.endpoint(a.alias_of_uid) { + Some((i, _)) => format!("alias of {}", name(&i)), + None => format!("alias #{}", a.uid), + }, + ViewElement::Group(g) => format!("group {}", g.name), + named => named_ident(named) + .map(|i| name(&i)) + .unwrap_or_else(|| format!("#{}", named.get_uid())), + } +} + +fn set_name(e: &mut ViewElement, name: &str) { + match e { + ViewElement::Aux(a) => a.name = name.to_string(), + ViewElement::Stock(s) => s.name = name.to_string(), + ViewElement::Flow(f) => f.name = name.to_string(), + ViewElement::Module(m) => m.name = name.to_string(), + _ => {} + } +} + +/// `before` as an untouched element is allowed to come back, given what came +/// back: renamed when the new name is the rename's `image`, and with a flow +/// endpoint the view left unattached wired to the flow's own cloud. +fn allowed_form( + before: &ViewElement, + after: &ViewElement, + after_side: &Side, + image: &str, +) -> ViewElement { + let mut expect = before.clone(); + if let Some(n) = after.get_name() + && named_ident(after).as_deref() == Some(image) + { + set_name(&mut expect, n); + } + if let (ViewElement::Flow(f0), ViewElement::Flow(f1)) = (&mut expect, after) + && f0.points.len() == f1.points.len() + && !f0.points.is_empty() + { + let last = f0.points.len() - 1; + for i in [0, last] { + if f0.points[i].attached_to_uid.is_none() + && let Some(u) = f1.points[i].attached_to_uid + && matches!(after_side.by_uid.get(&u), Some(ViewElement::Cloud(c)) if c.flow_uid == f0.uid) + { + f0.points[i].attached_to_uid = Some(u); + } + } + } + expect +} + +/// What differs between an element and what came back, for a finding's +/// detail. +fn difference(expected: &ViewElement, got: &ViewElement) -> String { + match (expected, got) { + (ViewElement::Link(a), ViewElement::Link(b)) => { + if a.from_uid != b.from_uid || a.to_uid != b.to_uid { + "endpoints changed".to_string() + } else if a.polarity != b.polarity { + "polarity changed".to_string() + } else { + format!("shape {} -> {}", shape_name(&a.shape), shape_name(&b.shape)) + } + } + (ViewElement::Flow(a), ViewElement::Flow(b)) => { + if a.points != b.points { + format!( + "pipe changed ({} -> {} points)", + a.points.len(), + b.points.len() + ) + } else if (a.x, a.y) != (b.x, b.y) { + format!("valve moved {:.1}", distance((a.x, a.y), (b.x, b.y))) + } else { + "label or name changed".to_string() + } + } + _ => match (center(expected), center(got)) { + (Some(a), Some(b)) if distance(a, b) > GEOMETRY_EPSILON => { + format!("moved {:.1}", distance(a, b)) + } + _ => "changed".to_string(), + }, + } +} + +fn shape_name(s: &LinkShape) -> String { + match s { + LinkShape::Straight => "straight".to_string(), + LinkShape::Arc(a) => format!("arc({a:.1})"), + LinkShape::MultiPoint(_) => "multipoint".to_string(), + } +} + +fn same_shape_kind(a: &LinkShape, b: &LinkShape) -> bool { + std::mem::discriminant(a) == std::mem::discriminant(b) +} + +/// Audit one edit. +pub fn audit_edit(input: &EditInput) -> EditAudit { + let config = LayoutConfig::default(); + let weights = MetricWeights::default(); + let mut audit = EditAudit { + cost_before: compute_layout_metrics(input.before_view, &config).weighted_cost(&weights), + cost_after: compute_layout_metrics(input.after_view, &config).weighted_cost(&weights), + ..EditAudit::default() + }; + let (Some(before), Some(after)) = ( + Side::new(input.before, input.model_name, input.before_view), + Side::new(input.after, input.model_name, input.after_view), + ) else { + audit.findings.push(Finding::new( + FindingKind::SyncFailed, + input.model_name, + "the model is missing on one side of the edit", + None, + )); + return audit; + }; + let renames = Renames::of(input.patch); + + let changed = changed_uids(&before, &after, &renames); + scope_findings(&before, &after, &renames, input.patch, &mut audit); + + let image = |i: &str| renames.image(i); + let identity = |i: &str| i.to_string(); + let before_keys: HashSet<(FindingKind, String)> = + consistency_findings(&before, &HashSet::new(), &image) + .into_iter() + .map(|f| (f.kind, f.subject)) + .collect(); + for finding in consistency_findings(&after, &changed, &identity) { + if !before_keys.contains(&(finding.kind, finding.subject.clone())) { + audit.findings.push(finding); + } + } + placement_findings(&before, &after, &changed, &mut audit.findings); + audit +} + +/// The uids of elements on the after view that are new, or differ from the +/// element of the same uid before the edit in anything but an allowed rename +/// or repair: what the sync created or changed. +fn changed_uids(before: &Side, after: &Side, renames: &Renames) -> HashSet { + after + .view + .elements + .iter() + .filter(|e1| match before.by_uid.get(&e1.get_uid()) { + None => true, + Some(e0) => { + let image = named_ident(e0) + .map(|i| renames.image(&i)) + .unwrap_or_default(); + allowed_form(e0, e1, after, &image) != **e1 + } + }) + .map(|e| e.get_uid()) + .collect() +} + +/// A finding when `e0`, an element the edit did not touch, did not come back +/// as `allowed_form` allows. +fn unchanged_finding( + e0: &ViewElement, + e1: Option<&ViewElement>, + after: &Side, + image: &str, + subject: &str, +) -> Option { + match e1 { + None => Some(Finding::new( + FindingKind::UntouchedElementChanged, + subject, + "removed", + region_of(e0), + )), + Some(e1) => { + let expect = allowed_form(e0, e1, after, image); + (expect != *e1).then(|| { + Finding::new( + FindingKind::UntouchedElementChanged, + subject, + difference(&expect, e1), + region_of(e1), + ) + }) + } + } +} + +/// The scope layer: walk every element of the view before the edit and check +/// that what came back is what the edit allows. +fn scope_findings( + before: &Side, + after: &Side, + renames: &Renames, + patch: &ModelPatch, + audit: &mut EditAudit, +) { + let image = |i: &str| renames.image(i); + // The variables the patch names, by their idents after it: the readers + // whose connectors the edit is about. + let named: HashSet = patch.ops.iter().filter_map(named_by_op).collect(); + let deleted = |i0: &str| !after.kinds.contains_key(&renames.image(i0)); + let kind_changed = |i0: &str| { + matches!( + (before.kinds.get(i0), after.kinds.get(&renames.image(i0))), + (Some(a), Some(b)) if a != b + ) + }; + // A flow is re-attached when the stocks its drawn ends name (carried + // through renames) are not the stocks the model after the edit lists it + // on. + let reattached = |i0: &str, flow: &Flow| { + let (from, to) = before.attachment(flow); + let drawn = ( + from.map(|s| renames.image(&s)), + to.map(|s| renames.image(&s)), + ); + drawn != after.expected_attachment(&renames.image(i0)) + }; + // Elements whose center differs across the edit: a link touching one may + // re-bow. + let moved: HashSet = before + .view + .elements + .iter() + .filter_map(|e0| { + let c0 = center(e0)?; + let c1 = after.by_uid.get(&e0.get_uid()).and_then(|e1| center(e1))?; + (distance(c0, c1) > GEOMETRY_EPSILON).then_some(e0.get_uid()) + }) + .collect(); + + let mut findings: Vec = Vec::new(); + let mut displacements: Vec = Vec::new(); + for e0 in &before.view.elements { + let uid = e0.get_uid(); + let e1 = after.by_uid.get(&uid).copied(); + let subject = subject_of(before, e0, &image); + match e0 { + ViewElement::Link(l) => { + let (Some((from, _)), Some((to, _))) = + (before.endpoint(l.from_uid), before.endpoint(l.to_uid)) + else { + continue; + }; + let edge = (renames.image(&from), renames.image(&to)); + // A link drawing no dependency is an author's choice the + // extraction does not explain (a module port, an input the + // dependency walk does not see, a deliberate annotation), so + // only an edit to its reader may drop it. A dependency changes + // only through its reader or a deleted end, so a link whose + // dependency the edit removed always names a reader the patch + // names. + let survives = !deleted(&from) + && !deleted(&to) + && (after.edges.contains(&edge) || !named.contains(&edge.1)); + match (survives, e1) { + (false, Some(_)) => findings.push(Finding::new( + FindingKind::StaleLinkRemains, + subject, + "its dependency is gone", + None, + )), + (false, None) => {} + (true, None) => findings.push(Finding::new( + FindingKind::UntouchedLinkChanged, + subject, + "removed or re-created", + None, + )), + (true, Some(got @ ViewElement::Link(l1))) => { + let endpoint_moved = |uid: i32| { + moved.contains(&uid) + || before + .endpoint(uid) + .is_some_and(|(_, e)| moved.contains(&e.get_uid())) + }; + let bow_may_change = endpoint_moved(l.from_uid) || endpoint_moved(l.to_uid); + let shape_ok = if bow_may_change { + same_shape_kind(&l.shape, &l1.shape) + } else { + l.shape == l1.shape + }; + if l.from_uid != l1.from_uid + || l.to_uid != l1.to_uid + || l.polarity != l1.polarity + || !shape_ok + { + findings.push(Finding::new( + FindingKind::UntouchedLinkChanged, + subject, + difference(e0, got), + None, + )); + } + } + (true, Some(_)) => findings.push(Finding::new( + FindingKind::UntouchedLinkChanged, + subject, + "its uid now names another element", + None, + )), + } + } + ViewElement::Cloud(c) => { + let flow = before.by_uid.get(&c.flow_uid).copied(); + let Some((flow, f0)) = flow.and_then(|f| named_ident(f).map(|i| (f, i))) else { + findings.extend(unchanged_finding(e0, e1, after, "", &subject)); + continue; + }; + if deleted(&f0) { + if e1.is_some() { + findings.push(Finding::new( + FindingKind::DeletedElementRemains, + subject, + "its flow was deleted", + e1.and_then(region_of), + )); + } + continue; + } + let rebuilt = + kind_changed(&f0) || matches!(flow, ViewElement::Flow(f) if reattached(&f0, f)); + if !rebuilt { + findings.extend(unchanged_finding(e0, e1, after, "", &subject)); + } + } + ViewElement::Alias(a) => match before.endpoint(a.alias_of_uid) { + Some((target, _)) if deleted(&target) => { + if e1.is_some() { + findings.push(Finding::new( + FindingKind::DeletedElementRemains, + subject, + "the variable was deleted", + e1.and_then(region_of), + )); + } + } + _ => findings.extend(unchanged_finding(e0, e1, after, "", &subject)), + }, + ViewElement::Group(_) => { + findings.extend(unchanged_finding(e0, e1, after, "", &subject)); + } + named => { + let Some(i0) = named_ident(named) else { + continue; + }; + if !before.kinds.contains_key(&i0) { + // An element naming no variable: no edit touches it. + findings.extend(unchanged_finding(e0, e1, after, &i0, &subject)); + continue; + } + let i1 = renames.image(&i0); + if deleted(&i0) { + if e1.is_some_and(|e| named_ident(e).is_some()) { + findings.push(Finding::new( + FindingKind::DeletedElementRemains, + subject, + "the variable was deleted", + e1.and_then(region_of), + )); + } + continue; + } + if kind_changed(&i0) { + let rebuilt = after.first_named(&i1); + if let (Some(c0), Some(c1)) = (center(named), rebuilt.and_then(center)) { + let d = distance(c0, c1); + displacements.push(Displacement { + subject: i1.clone(), + distance: d, + }); + // A larger body at the old center (a parameter turned + // into a stock) may have to move off what it covers. + let rebuilt_uid = rebuilt.map(ViewElement::get_uid); + let blocked = rebuilt.and_then(node_shape_box).is_some_and(|b| { + let (dx, dy) = (c0.0 - c1.0, c0.1 - c1.1); + after + .view + .elements + .iter() + .filter(|e| Some(e.get_uid()) != rebuilt_uid) + .filter_map(node_shape_box) + .any(|r| { + let w = r.right.min(b.right + dx) - r.left.max(b.left + dx); + let h = r.bottom.min(b.bottom + dy) - r.top.max(b.top + dy); + w > 0.0 && h > 0.0 && w * h > MIN_OVERLAP_AREA + }) + }); + if after.kinds.get(&i1) != Some(&VarKind::Flow) + && d > GEOMETRY_EPSILON + && !blocked + { + findings.push(Finding::new( + FindingKind::RebuiltElementMoved, + subject, + format!("moved {d:.1}"), + rebuilt.and_then(region_of), + )); + } + } + continue; + } + if let ViewElement::Flow(f) = named + && reattached(&i0, f) + { + if let (Some(c0), Some(c1)) = + (center(named), after.first_named(&i1).and_then(center)) + { + displacements.push(Displacement { + subject: i1.clone(), + distance: distance(c0, c1), + }); + } + continue; + } + findings.extend(unchanged_finding(e0, e1, after, &i1, &subject)); + } + } + } + + let (v0, v1) = (before.view, after.view); + if v0.name != v1.name + || v0.view_box != v1.view_box + || v0.zoom != v1.zoom + || v0.use_lettered_polarity != v1.use_lettered_polarity + || v0.font != v1.font + { + findings.push(Finding::new( + FindingKind::ViewPropertiesChanged, + "view", + "name, viewport, zoom, font or polarity style changed", + None, + )); + } + audit.findings.extend(findings); + audit.displacements.extend(displacements); + + // A connector the view did not draw may be drawn into a variable the patch + // names, or between elements drawn for the first time, which carry no + // author's choice about their connectors; a variable the view did not draw + // may be drawn only when the patch names it. Anywhere else it is a change + // to a part of the diagram the edit is not about. + let untouched_var = |i1: &str| { + let i0 = renames.preimage(i1); + before.kinds.contains_key(&i0) && !kind_changed(&i0) + }; + let before_link_edges: HashSet<(String, String)> = before + .view + .elements + .iter() + .filter_map(|e| match e { + ViewElement::Link(l) => Some(( + renames.image(&before.endpoint(l.from_uid)?.0), + renames.image(&before.endpoint(l.to_uid)?.0), + )), + _ => None, + }) + .collect(); + let before_drawn: HashSet = before + .view + .elements + .iter() + .filter_map(|e| named_ident(e).map(|i| renames.image(&i))) + .collect(); + for e1 in &after.view.elements { + if before.by_uid.contains_key(&e1.get_uid()) { + continue; + } + if let ViewElement::Link(l) = e1 { + if let (Some((from, _)), Some((to, _))) = + (after.endpoint(l.from_uid), after.endpoint(l.to_uid)) + { + let related = named.contains(&to) + || !before_drawn.contains(&from) + || !before_drawn.contains(&to); + if !related && !before_link_edges.contains(&(from.clone(), to.clone())) { + audit.findings.push(Finding::new( + FindingKind::UnrelatedLinkAdded, + format!("link {from} -> {to}"), + "the patch names neither its reader nor a newly drawn end", + None, + )); + } + } + } else if let Some(i1) = named_ident(e1) + && untouched_var(&i1) + && !before_drawn.contains(&i1) + && !named.contains(&i1) + { + audit.findings.push(Finding::new( + FindingKind::UnrelatedElementAdded, + i1, + "the view did not draw it and the patch does not name it", + region_of(e1), + )); + } + } +} + +/// The variable an operation defines, by its ident after the patch: the reader +/// whose connectors the operation is about. +fn named_by_op(op: &ModelOperation) -> Option { + let ident = match op { + ModelOperation::UpsertStock(s) => &s.ident, + ModelOperation::UpsertFlow(f) => &f.ident, + ModelOperation::UpsertAux(a) => &a.ident, + ModelOperation::UpsertModule(m) => &m.ident, + ModelOperation::RenameVariable { to, .. } => to, + ModelOperation::UpdateStockFlows { ident, .. } => ident, + ModelOperation::DeleteVariable { .. } + | ModelOperation::UpsertView { .. } + | ModelOperation::DeleteView { .. } + | ModelOperation::SetLoopName { .. } + | ModelOperation::EditView { .. } => return None, + }; + Some(canonicalize(ident).into_owned()) +} + +/// The consistency layer over one side. `routed` holds the uids the sync +/// created or changed: their flows are held to the strict flow invariants. +/// `name` maps an ident on this side to the after-edit ident findings are +/// keyed by. +fn consistency_findings( + side: &Side, + routed: &HashSet, + name: &dyn Fn(&str) -> String, +) -> Vec { + let mut out = Vec::new(); + + let mut uid_counts: BTreeMap = BTreeMap::new(); + for e in &side.view.elements { + *uid_counts.entry(e.get_uid()).or_default() += 1; + } + for (uid, count) in &uid_counts { + if *count > 1 || *uid <= 0 { + out.push(Finding::new( + FindingKind::UidProblem, + format!("uid {uid}"), + if *count > 1 { + format!("{count} elements share it") + } else { + "not positive".to_string() + }, + None, + )); + } + } + + let mut drawn: BTreeMap> = BTreeMap::new(); + for e in &side.view.elements { + if let Some(i) = named_ident(e) { + drawn.entry(i).or_default().push(e); + } + } + for (ident, kind) in &side.kinds { + match drawn.get(ident) { + None => out.push(Finding::new( + FindingKind::VariableNotDrawn, + name(ident), + "", + None, + )), + Some(elements) => { + if elements.len() > 1 { + out.push(Finding::new( + FindingKind::VariableDrawnTwice, + name(ident), + format!("{} elements", elements.len()), + region_of(elements[1]), + )); + } + if elements.iter().any(|e| element_kind(e) != Some(*kind)) { + out.push(Finding::new( + FindingKind::ElementKindMismatch, + name(ident), + "", + region_of(elements[0]), + )); + } + } + } + } + for (ident, elements) in &drawn { + if !side.kinds.contains_key(ident) { + out.push(Finding::new( + FindingKind::ElementNamesNoVariable, + name(ident), + "", + region_of(elements[0]), + )); + } + } + + let mut link_edges: HashSet<(String, String)> = HashSet::new(); + for e in &side.view.elements { + match e { + ViewElement::Link(l) => match (side.endpoint(l.from_uid), side.endpoint(l.to_uid)) { + (Some((from, _)), Some((to, _))) => { + let edge = (from, to); + if !side.edges.contains(&edge) { + out.push(Finding::new( + FindingKind::LinkWithoutDependency, + format!("link {} -> {}", name(&edge.0), name(&edge.1)), + "", + None, + )); + } + link_edges.insert(edge); + } + _ => out.push(Finding::new( + FindingKind::DanglingReference, + format!("link #{}", l.uid), + "an end is not a named element or an alias of one", + None, + )), + }, + ViewElement::Alias(a) => { + if side.endpoint(a.alias_of_uid).is_none() { + out.push(Finding::new( + FindingKind::DanglingReference, + format!("alias #{}", a.uid), + "aliases no named element", + region_of(e), + )); + } + } + ViewElement::Cloud(c) => { + let owner = match side.by_uid.get(&c.flow_uid) { + Some(ViewElement::Flow(f)) => Some(f), + _ => None, + }; + let ends = owner.map_or(0, |f| { + let last = f.points.len().saturating_sub(1); + f.points + .iter() + .enumerate() + .filter(|(i, p)| { + (*i == 0 || *i == last) && p.attached_to_uid == Some(c.uid) + }) + .count() + }); + if ends != 1 { + out.push(Finding::new( + FindingKind::DanglingReference, + subject_of(side, e, name), + match owner { + None => "its flow does not exist".to_string(), + Some(_) => format!("an endpoint of its flow {ends} times"), + }, + region_of(e), + )); + } + } + ViewElement::Flow(f) => { + for p in &f.points { + if let Some(uid) = p.attached_to_uid + && !matches!( + side.by_uid.get(&uid), + Some(ViewElement::Stock(_)) | Some(ViewElement::Cloud(_)) + ) + { + out.push(Finding::new( + FindingKind::DanglingReference, + subject_of(side, e, name), + format!("attached to #{uid}, which is no stock or cloud"), + region_of(e), + )); + } + } + if let Some(ident) = named_ident(e) + && side.kinds.get(&ident) == Some(&VarKind::Flow) + { + let (from, to) = side.expected_attachment(&ident); + let end_ok = |p: Option<&FlowPoint>, expected: &Option| { + let attached = p + .and_then(|p| p.attached_to_uid) + .and_then(|u| side.by_uid.get(&u)); + match (expected, attached) { + (Some(stock), Some(ViewElement::Stock(s))) => { + canonicalize(&s.name) == stock.as_str() + } + (Some(_), _) => false, + (None, None) => true, + (None, Some(ViewElement::Cloud(c))) => c.flow_uid == f.uid, + (None, Some(_)) => false, + } + }; + if !end_ok(f.points.first(), &from) || !end_ok(f.points.last(), &to) { + out.push(Finding::new( + FindingKind::FlowAttachmentMismatch, + name(&ident), + format!( + "the model lists it from {} to {}", + from.as_deref().unwrap_or("a cloud"), + to.as_deref().unwrap_or("a cloud") + ), + region_of(e), + )); + } + } + } + _ => {} + } + } + for (dep, var) in &side.edges { + if drawn.contains_key(dep) + && drawn.contains_key(var) + && !link_edges.contains(&(dep.clone(), var.clone())) + { + out.push(Finding::new( + FindingKind::DependencyWithoutLink, + format!("link {} -> {}", name(dep), name(var)), + "", + None, + )); + } + } + + for v in check_flow_invariants( + &side.view.elements, + Mode::Strict { + routed: Some(routed), + }, + ) { + let flow = side.by_uid.get(&v.uid).copied(); + let subject = match flow.and_then(named_ident) { + Some(i) => format!("{} {}", v.arm.name(), name(&i)), + None => format!("{} #{}", v.arm.name(), v.uid), + }; + out.push(Finding::new( + FindingKind::FlowInvariant, + subject, + v.message.clone(), + flow.and_then(region_of), + )); + } + out +} + +/// What happened to one element across an edit. +#[cfg_attr(feature = "debug-derive", derive(Debug))] +#[derive(Clone, Copy, PartialEq, Eq, serde::Serialize)] +#[serde(rename_all = "snake_case")] +pub enum ChangeKind { + Created, + Changed, + Removed, +} + +/// One element that differs across an edit, with where it is drawn (on the +/// view after the edit, or before it for a removed element). +#[cfg_attr(feature = "debug-derive", derive(Debug))] +#[derive(Clone, PartialEq, serde::Serialize)] +pub struct ViewChange { + pub uid: i32, + pub kind: ChangeKind, + pub region: Option<[f64; 4]>, +} + +/// Where an element is drawn: its shape, and for a flow its whole pipe. +fn drawn_region(e: &ViewElement) -> Option<[f64; 4]> { + let mut region = region_of(e)?; + if let ViewElement::Flow(f) = e { + for p in &f.points { + region = [ + region[0].min(p.x - 4.0), + region[1].min(p.y - 4.0), + region[2].max(p.x + 4.0), + region[3].max(p.y + 4.0), + ]; + } + } + Some(region) +} + +/// Every element created, removed, or holding a different value across an +/// edit, by uid: the raw difference a reviewer draws over the renders, renames +/// and repairs included. Links have no region. +pub fn view_changes(before: &StockFlow, after: &StockFlow) -> Vec { + let old: HashMap = + before.elements.iter().map(|e| (e.get_uid(), e)).collect(); + let new: HashSet = after.elements.iter().map(ViewElement::get_uid).collect(); + let mut out: Vec = after + .elements + .iter() + .filter_map(|e| { + let kind = match old.get(&e.get_uid()) { + None => ChangeKind::Created, + Some(o) if *o != e => ChangeKind::Changed, + _ => return None, + }; + Some(ViewChange { + uid: e.get_uid(), + kind, + region: drawn_region(e), + }) + }) + .collect(); + out.extend( + before + .elements + .iter() + .filter(|e| !new.contains(&e.get_uid())) + .map(|e| ViewChange { + uid: e.get_uid(), + kind: ChangeKind::Removed, + region: drawn_region(e), + }), + ); + out +} + +fn overlap_area(a: &Rect, b: &Rect) -> f64 { + let w = a.right.min(b.right) - a.left.max(b.left); + let h = a.bottom.min(b.bottom) - a.top.max(b.top); + w.max(0.0) * h.max(0.0) +} + +/// Whether segment `a`-`b` enters the open box `r` (Liang-Barsky clipping): +/// a segment along an edge or ending on it does not. +fn segment_enters(a: (f64, f64), b: (f64, f64), r: &Rect) -> bool { + let (dx, dy) = (b.0 - a.0, b.1 - a.1); + let (mut t0, mut t1) = (0.0_f64, 1.0_f64); + for (p, q) in [ + (-dx, a.0 - r.left), + (dx, r.right - a.0), + (-dy, a.1 - r.top), + (dy, r.bottom - a.1), + ] { + if p.abs() < 1e-12 { + if q <= 0.0 { + return false; + } + continue; + } + let t = q / p; + if p < 0.0 { + if t >= t1 { + return false; + } + t0 = t0.max(t); + } else { + if t <= t0 { + return false; + } + t1 = t1.min(t); + } + } + t1 - t0 > 1e-9 +} + +/// The placement layer, over the elements the sync created or changed. +fn placement_findings(before: &Side, side: &Side, changed: &HashSet, out: &mut Vec) { + let name = |i: &str| i.to_string(); + let boxes = |s: &Side| -> Vec<(i32, Rect)> { + s.view + .elements + .iter() + .filter_map(|e| node_shape_box(e).map(|r| (e.get_uid(), r))) + .collect() + }; + // The pairs the author's view already overlapped: an imported view can + // draw an alias on a pipe or a valve, and a sync that re-attaches the flow + // may have nowhere clear to put it. + let before_boxes = boxes(before); + let overlapped_before: HashSet<(i32, i32)> = before_boxes + .iter() + .flat_map(|(a, ra)| { + before_boxes + .iter() + .filter(move |(b, rb)| a < b && overlap_area(ra, rb) > MIN_OVERLAP_AREA) + .map(move |(b, _)| (*a, *b)) + }) + .collect(); + let shapes: Vec<(&ViewElement, Rect)> = side + .view + .elements + .iter() + .filter_map(|e| node_shape_box(e).map(|r| (e, r))) + .collect(); + let mut reported: HashSet<(i32, i32)> = HashSet::new(); + for (e, r) in &shapes { + if !changed.contains(&e.get_uid()) { + continue; + } + for (o, ro) in &shapes { + if o.get_uid() == e.get_uid() { + continue; + } + let pair = (e.get_uid().min(o.get_uid()), e.get_uid().max(o.get_uid())); + let area = overlap_area(r, ro); + if area > MIN_OVERLAP_AREA + && !overlapped_before.contains(&pair) + && reported.insert(pair) + { + out.push(Finding::new( + FindingKind::ShapeOverlap, + format!( + "{} over {}", + subject_of(side, e, &name), + subject_of(side, o, &name) + ), + format!("{area:.0} px^2"), + Some([ + r.left.max(ro.left), + r.top.max(ro.top), + r.right.min(ro.right), + r.bottom.min(ro.bottom), + ]), + )); + } + } + } + + for e in &side.view.elements { + let ViewElement::Flow(f) = e else { continue }; + if !changed.contains(&f.uid) { + continue; + } + let terminals: HashSet = [f.points.first(), f.points.last()] + .into_iter() + .flatten() + .filter_map(|p| p.attached_to_uid) + .collect(); + for (o, ro) in &shapes { + if !matches!(o, ViewElement::Stock(_)) || terminals.contains(&o.get_uid()) { + continue; + } + let inner = Rect { + left: ro.left + STOCK_INTERIOR_INSET, + top: ro.top + STOCK_INTERIOR_INSET, + right: ro.right - STOCK_INTERIOR_INSET, + bottom: ro.bottom - STOCK_INTERIOR_INSET, + }; + if f.points + .windows(2) + .any(|w| segment_enters((w[0].x, w[0].y), (w[1].x, w[1].y), &inner)) + { + out.push(Finding::new( + FindingKind::PipeThroughStock, + format!( + "{} through {}", + subject_of(side, e, &name), + subject_of(side, o, &name) + ), + "", + Some([ro.left, ro.top, ro.right, ro.bottom]), + )); + } + } + } +} + +#[cfg(test)] +#[path = "edit_audit_tests.rs"] +mod tests; diff --git a/src/simlin-engine/src/layout/edit_audit_tests.rs b/src/simlin-engine/src/layout/edit_audit_tests.rs new file mode 100644 index 000000000..27ff4f2f8 --- /dev/null +++ b/src/simlin-engine/src/layout/edit_audit_tests.rs @@ -0,0 +1,670 @@ +// Copyright 2026 The Simlin Authors. All rights reserved. +// Use of this source code is governed by the Apache License, +// Version 2.0, that can be found in the LICENSE file. + +//! The audit's decision table: one row per `FindingKind`. +//! +//! Every row starts from an edit applied and synced through the production +//! path (`build_scenario`, `apply_patch`, `sync_view`) on a hand-drawn view, +//! then makes the one change to the synced view the finding exists for. A row +//! checks both arms: the view as a correct sync produces it raises no finding +//! of that kind, and the changed view does. Where the correct view is not what +//! today's sync produces (a rebuilt element that moved, a pipe through a +//! stock), the row builds the correct arm by hand, stated in the row, so the +//! row does not depend on the defect the finding reports. + +use super::*; +use crate::datamodel::view_element::{self, LabelSide}; +use crate::layout::edit_scenarios::{ + Scenario, ScenarioKind, build_scenario, first_difference, run_scenario, sync_view, +}; +use crate::patch::{ProjectPatch, apply_patch}; + +const MODEL: &str = "main"; +const POPULATION: &str = "default_projects/population/model.xmile"; +const SIR: &str = "test/test-models/samples/SIR/SIR.stmx"; + +fn load(rel: &str) -> datamodel::Project { + let path = format!("{}/../../{rel}", env!("CARGO_MANIFEST_DIR")); + let file = std::fs::File::open(&path).unwrap_or_else(|e| panic!("{path}: {e}")); + crate::compat::open_xmile(&mut std::io::BufReader::new(file)) + .unwrap_or_else(|e| panic!("{path}: {e:?}")) +} + +fn shipped_view(project: &datamodel::Project) -> StockFlow { + match project.get_model(MODEL).and_then(|m| m.views.first()) { + Some(datamodel::View::StockFlow(sf)) => sf.clone(), + None => panic!("no view"), + } +} + +/// One edit, applied and synced through the production path. +struct Edited { + before: datamodel::Project, + before_view: StockFlow, + patch: ModelPatch, + after: datamodel::Project, + after_view: StockFlow, +} + +fn edited(rel: &str, kind: ScenarioKind) -> Edited { + edited_from(rel, kind, |_| {}) +} + +/// `edited`, from the shipped view changed by `prepare` first. +fn edited_from(rel: &str, kind: ScenarioKind, prepare: impl FnOnce(&mut StockFlow)) -> Edited { + let mut before = load(rel); + let mut before_view = shipped_view(&before); + prepare(&mut before_view); + before.get_model_mut(MODEL).expect("model").views = + vec![datamodel::View::StockFlow(before_view.clone())]; + let scenario = build_scenario(&before, MODEL, kind).expect("the scenario applies"); + let patch = ModelPatch { + name: before.get_model(MODEL).expect("model").name.clone(), + ops: scenario.steps[0].clone(), + }; + let mut after = before.clone(); + apply_patch( + &mut after, + ProjectPatch { + project_ops: vec![], + models: vec![patch.clone()], + }, + ) + .expect("the patch applies"); + let after_view = sync_view(&after, MODEL, &patch, &before_view).expect("the sync succeeds"); + Edited { + before, + before_view, + patch, + after, + after_view, + } +} + +impl Edited { + /// The audit of this edit with the synced view changed by `change`. + fn audit(&self, change: impl FnOnce(&mut StockFlow)) -> EditAudit { + let mut view = self.after_view.clone(); + change(&mut view); + let mut after = self.after.clone(); + after.get_model_mut(MODEL).expect("model").views = + vec![datamodel::View::StockFlow(view.clone())]; + audit_edit(&EditInput { + model_name: MODEL, + before: &self.before, + before_view: &self.before_view, + patch: &self.patch, + after: &after, + after_view: &view, + }) + } + + /// Assert the finding is absent after `correct` and present after + /// `defective`. + fn row( + &self, + kind: FindingKind, + correct: impl FnOnce(&mut StockFlow), + defective: impl FnOnce(&mut StockFlow), + ) { + let clean = self.audit(correct); + assert!( + !clean.kinds().contains(&kind), + "{}: raised on the correct view: {:?}", + kind.name(), + clean + .findings + .iter() + .filter(|f| f.kind == kind) + .map(|f| &f.subject) + .collect::>() + ); + let dirty = self.audit(defective); + assert!( + dirty.kinds().contains(&kind), + "{}: not raised on the defective view; raised {:?}", + kind.name(), + dirty.kinds() + ); + } +} + +fn unchanged(_: &mut StockFlow) {} + +fn uid_named(view: &StockFlow, ident: &str) -> i32 { + view.elements + .iter() + .find(|e| named_ident(e).as_deref() == Some(ident)) + .map(ViewElement::get_uid) + .unwrap_or_else(|| panic!("{ident} is drawn")) +} + +fn element_named<'a>(view: &'a mut StockFlow, ident: &str) -> &'a mut ViewElement { + view.elements + .iter_mut() + .find(|e| named_ident(e).as_deref() == Some(ident)) + .unwrap_or_else(|| panic!("{ident} is drawn")) +} + +fn next_uid(view: &StockFlow) -> i32 { + view.elements + .iter() + .map(ViewElement::get_uid) + .max() + .unwrap_or(0) + + 1 +} + +fn link_between(view: &StockFlow, from: &str, to: &str) -> i32 { + let (f, t) = (uid_named(view, from), uid_named(view, to)); + view.elements + .iter() + .find_map(|e| match e { + ViewElement::Link(l) if l.from_uid == f && l.to_uid == t => Some(l.uid), + _ => None, + }) + .unwrap_or_else(|| panic!("a link {from} -> {to}")) +} + +fn set_center(e: &mut ViewElement, x: f64, y: f64) { + match e { + ViewElement::Aux(a) => (a.x, a.y) = (x, y), + ViewElement::Stock(s) => (s.x, s.y) = (x, y), + ViewElement::Module(m) => (m.x, m.y) = (x, y), + ViewElement::Flow(f) => (f.x, f.y) = (x, y), + _ => panic!("no center"), + } +} + +fn push_link(view: &mut StockFlow, from_uid: i32, to_uid: i32) { + let uid = next_uid(view); + view.elements.push(ViewElement::Link(view_element::Link { + uid, + from_uid, + to_uid, + shape: LinkShape::Straight, + polarity: None, + })); +} + +/// Remove average_lifespan and the links touching it from a population view. +fn undraw_average_lifespan(view: &mut StockFlow) { + let Some(uid) = view + .elements + .iter() + .find(|el| named_ident(el).as_deref() == Some("average_lifespan")) + .map(ViewElement::get_uid) + else { + return; + }; + view.elements.retain(|el| match el { + ViewElement::Link(l) => l.from_uid != uid && l.to_uid != uid, + other => other.get_uid() != uid, + }); +} + +fn row_for(kind: FindingKind) { + match kind { + FindingKind::DeletedElementRemains => { + // Delete a parameter; the defective sync still draws it. + let e = edited(POPULATION, ScenarioKind::DeleteParameter); + let old = e + .before_view + .elements + .iter() + .find(|el| named_ident(el).as_deref() == Some("average_lifespan")) + .cloned() + .expect("average_lifespan is drawn"); + e.row(kind, unchanged, |v| v.elements.push(old)); + } + FindingKind::UntouchedElementChanged => { + let e = edited(POPULATION, ScenarioKind::AddParameter); + e.row(kind, unchanged, |v| { + if let ViewElement::Aux(a) = element_named(v, "average_lifespan") { + a.x += 20.0; + } + }); + } + FindingKind::UntouchedLinkChanged => { + let e = edited(POPULATION, ScenarioKind::AddParameter); + let link = link_between(&e.after_view, "average_lifespan", "deaths"); + e.row(kind, unchanged, |v| { + for el in &mut v.elements { + if let ViewElement::Link(l) = el + && l.uid == link + { + l.shape = match l.shape { + LinkShape::Straight => LinkShape::Arc(30.0), + _ => LinkShape::Straight, + }; + } + } + }); + } + FindingKind::StaleLinkRemains => { + // Delete a parameter; the defective sync keeps the link it drew. + let e = edited(POPULATION, ScenarioKind::DeleteParameter); + let old = link_between(&e.before_view, "average_lifespan", "deaths"); + let link = e + .before_view + .elements + .iter() + .find(|el| el.get_uid() == old) + .cloned() + .expect("the link"); + e.row(kind, unchanged, |v| v.elements.push(link)); + } + FindingKind::UnrelatedLinkAdded => { + // The author's view leaves out average_lifespan -> deaths. An edit + // adding births_multiplier is not about that dependency, so the + // correct view still leaves it out. + let without = |v: &mut StockFlow| { + let (from, to) = (uid_named(v, "average_lifespan"), uid_named(v, "deaths")); + v.elements + .retain(|el| !matches!(el, ViewElement::Link(l) if l.from_uid == from && l.to_uid == to)); + }; + let e = edited_from(POPULATION, ScenarioKind::AddParameter, without); + let (from, to) = ( + uid_named(&e.after_view, "average_lifespan"), + uid_named(&e.after_view, "deaths"), + ); + e.row(kind, without, |v| { + without(v); + push_link(v, from, to); + }); + } + FindingKind::UnrelatedElementAdded => { + // The author's view leaves average_lifespan undrawn. An edit adding + // births_multiplier is not about it, so the correct view still + // leaves it out. + let element = shipped_view(&load(POPULATION)) + .elements + .iter() + .find(|el| named_ident(el).as_deref() == Some("average_lifespan")) + .cloned() + .expect("average_lifespan is drawn"); + let e = edited_from( + POPULATION, + ScenarioKind::AddParameter, + undraw_average_lifespan, + ); + e.row(kind, undraw_average_lifespan, |v| { + undraw_average_lifespan(v); + v.elements.push(element); + }); + } + FindingKind::RebuiltElementMoved => { + // Turn a parameter into a stock. The correct arm puts the rebuilt + // stock at the parameter's old center by hand. + let e = edited(POPULATION, ScenarioKind::AuxToStock); + let (x, y) = match e + .before_view + .elements + .iter() + .find(|el| named_ident(el).as_deref() == Some("average_lifespan")) + { + Some(ViewElement::Aux(a)) => (a.x, a.y), + _ => panic!("average_lifespan is an aux"), + }; + e.row( + kind, + |v| set_center(element_named(v, "average_lifespan"), x, y), + |v| set_center(element_named(v, "average_lifespan"), x + 30.0, y), + ); + // Where the stock's body at the old center would cover another + // shape (birth_rate parked there), moving off it is allowed. + let blocked = e.audit(|v| { + set_center(element_named(v, "average_lifespan"), x + 80.0, y); + set_center(element_named(v, "birth_rate"), x, y); + }); + assert!( + !blocked.kinds().contains(&kind), + "a rebuilt element whose old center is covered may move" + ); + } + FindingKind::ViewPropertiesChanged => { + let e = edited(POPULATION, ScenarioKind::AddParameter); + e.row(kind, unchanged, |v| v.zoom *= 2.0); + } + FindingKind::UidProblem => { + let e = edited(POPULATION, ScenarioKind::AddParameter); + let taken = uid_named(&e.after_view, "population"); + e.row(kind, unchanged, |v| { + if let ViewElement::Aux(a) = element_named(v, "births_multiplier") { + a.uid = taken; + } + }); + } + FindingKind::VariableNotDrawn => { + let e = edited(POPULATION, ScenarioKind::AddParameter); + let uid = uid_named(&e.after_view, "births_multiplier"); + e.row(kind, unchanged, |v| { + v.elements.retain(|el| match el { + ViewElement::Link(l) => l.from_uid != uid && l.to_uid != uid, + other => other.get_uid() != uid, + }) + }); + } + FindingKind::VariableDrawnTwice => { + let e = edited(POPULATION, ScenarioKind::AddParameter); + e.row(kind, unchanged, |v| { + let mut copy = element_named(v, "births_multiplier").clone(); + if let ViewElement::Aux(a) = &mut copy { + a.uid = next_uid(v); + a.y += 80.0; + } + v.elements.push(copy); + }); + } + FindingKind::ElementKindMismatch => { + let e = edited(POPULATION, ScenarioKind::AddParameter); + e.row(kind, unchanged, |v| { + let el = element_named(v, "births_multiplier"); + if let ViewElement::Aux(a) = el.clone() { + *el = ViewElement::Stock(view_element::Stock { + name: a.name, + uid: a.uid, + x: a.x, + y: a.y, + label_side: LabelSide::Bottom, + compat: None, + }); + } + }); + } + FindingKind::ElementNamesNoVariable => { + let e = edited(POPULATION, ScenarioKind::AddParameter); + e.row(kind, unchanged, |v| { + let uid = next_uid(v); + v.elements.push(ViewElement::Aux(view_element::Aux { + name: "phantom variable".to_string(), + uid, + x: 900.0, + y: 900.0, + label_side: LabelSide::Bottom, + compat: None, + })); + }); + } + FindingKind::DanglingReference => { + let e = edited(POPULATION, ScenarioKind::AddParameter); + let births = uid_named(&e.after_view, "births"); + e.row(kind, unchanged, |v| push_link(v, 99_999, births)); + } + FindingKind::LinkWithoutDependency => { + let e = edited(POPULATION, ScenarioKind::AddParameter); + let (from, to) = ( + uid_named(&e.after_view, "average_lifespan"), + uid_named(&e.after_view, "births"), + ); + e.row(kind, unchanged, |v| push_link(v, from, to)); + } + FindingKind::DependencyWithoutLink => { + let e = edited(POPULATION, ScenarioKind::AddParameter); + let link = link_between(&e.after_view, "births_multiplier", "births"); + e.row(kind, unchanged, |v| { + v.elements.retain(|el| el.get_uid() != link) + }); + } + FindingKind::FlowAttachmentMismatch => { + // deaths drains population into a cloud; attach its cloud end to + // population instead. + let e = edited(POPULATION, ScenarioKind::AddParameter); + let population = uid_named(&e.after_view, "population"); + e.row(kind, unchanged, |v| { + if let ViewElement::Flow(f) = element_named(v, "deaths") { + f.points.last_mut().expect("points").attached_to_uid = Some(population); + } + }); + } + FindingKind::FlowInvariant => { + // Pull a created side flow's valve off its pipe (G8). + let e = edited(POPULATION, ScenarioKind::AddSideFlow); + e.row(kind, unchanged, |v| { + if let ViewElement::Flow(f) = element_named(v, "population_loss") { + f.x += 40.0; + f.y += 40.0; + } + }); + } + FindingKind::ShapeOverlap => { + let e = edited(POPULATION, ScenarioKind::AddParameter); + let (x, y) = match e + .after_view + .elements + .iter() + .find(|el| named_ident(el).as_deref() == Some("population")) + { + Some(ViewElement::Stock(s)) => (s.x, s.y), + _ => panic!("population is a stock"), + }; + e.row(kind, unchanged, |v| { + set_center(element_named(v, "births_multiplier"), x, y) + }); + // A pair the author's view already overlapped is not charged, even + // where the sync changed one of them. + let parked = |v: &mut StockFlow| set_center(element_named(v, "birth_rate"), x, y); + let e = edited_from(POPULATION, ScenarioKind::AddParameter, parked); + let nudged = e.audit(|v| { + parked(v); + if let ViewElement::Aux(a) = element_named(v, "birth_rate") { + a.x += 1.0; + } + }); + assert!( + !nudged.kinds().contains(&kind), + "an overlap the author drew is not charged to the edit" + ); + } + FindingKind::PipeThroughStock => { + // A flow from recovered to susceptible, past infectious between + // them. The correct arm routes it by hand above every stock; the + // defective one runs it straight through infectious. + let e = edited(SIR, ScenarioKind::AddFlowBetweenStocks); + let stock = |ident: &str| match e + .after_view + .elements + .iter() + .find(|el| named_ident(el).as_deref() == Some(ident)) + { + Some(ViewElement::Stock(s)) => (s.uid, s.x, s.y), + _ => panic!("{ident} is a stock"), + }; + let (r_uid, rx, ry) = stock("recovered"); + let (s_uid, sx, sy) = stock("susceptible"); + let half_h = crate::diagram::constants::STOCK_HEIGHT / 2.0; + let half_w = crate::diagram::constants::STOCK_WIDTH / 2.0; + let top = ry.min(sy) - 100.0; + let point = |x: f64, y: f64, uid: Option| view_element::FlowPoint { + x, + y, + attached_to_uid: uid, + }; + let route = + |v: &mut StockFlow, points: Vec, valve: (f64, f64)| { + if let ViewElement::Flow(f) = element_named(v, "recovered_to_susceptible") { + f.points = points; + (f.x, f.y) = valve; + } + }; + e.row( + kind, + |v| { + route( + v, + vec![ + point(rx, ry - half_h, Some(r_uid)), + point(rx, top, None), + point(sx, top, None), + point(sx, sy - half_h, Some(s_uid)), + ], + ((rx + sx) / 2.0, top), + ) + }, + |v| { + route( + v, + vec![ + point(rx - half_w, ry, Some(r_uid)), + point(sx + half_w, sy, Some(s_uid)), + ], + ((rx + sx) / 2.0, (ry + sy) / 2.0), + ) + }, + ); + } + FindingKind::NotDeterministic => { + // The runner raises this exactly when `first_difference` between + // two syncs of one edit is `Some`; a production sync is + // deterministic, so the row pins the comparison's arms instead. + let project = load(POPULATION); + let view = shipped_view(&project); + assert_eq!(first_difference(&view, &view), None); + let mut moved = view.clone(); + if let ViewElement::Aux(a) = element_named(&mut moved, "birth_rate") { + a.x += 1.0; + } + assert!(first_difference(&view, &moved).is_some()); + let mut reordered = view.clone(); + reordered.elements.reverse(); + assert!(first_difference(&view, &reordered).is_some()); + let mut rezoomed = view.clone(); + rezoomed.zoom *= 2.0; + assert!(first_difference(&view, &rezoomed).is_some()); + } + FindingKind::ReturnToOriginal => { + let project = load(POPULATION); + let view = shipped_view(&project); + let restate = + build_scenario(&project, MODEL, ScenarioKind::RestateVariable).expect("applies"); + assert!( + !run_scenario(&project, MODEL, &view, &restate) + .kinds() + .contains(&kind) + ); + let adds = + build_scenario(&project, MODEL, ScenarioKind::AddParameter).expect("applies"); + let claims_identity = Scenario { + returns_to_original: true, + ..adds + }; + assert!( + run_scenario(&project, MODEL, &view, &claims_identity) + .kinds() + .contains(&kind) + ); + } + FindingKind::SyncFailed => { + let project = load(POPULATION); + let view = shipped_view(&project); + let restate = + build_scenario(&project, MODEL, ScenarioKind::RestateVariable).expect("applies"); + assert!( + !run_scenario(&project, MODEL, &view, &restate) + .kinds() + .contains(&kind) + ); + let broken = Scenario { + steps: vec![vec![ModelOperation::DeleteVariable { + ident: "no_such_variable".to_string(), + }]], + ..restate + }; + assert!( + run_scenario(&project, MODEL, &view, &broken) + .kinds() + .contains(&kind) + ); + } + } +} + +#[test] +fn every_finding_kind_is_raised_exactly_where_it_applies() { + for kind in FindingKind::ALL { + row_for(kind); + } +} + +#[test] +fn a_link_drawing_no_dependency_goes_only_with_an_edit_to_its_reader() { + // A link drawing no dependency is an author's choice the extraction does + // not explain. Adding births_multiplier names births, not deaths: a link + // birth_rate -> deaths must survive, and a link average_lifespan -> births + // must go. + const EXTRA: i32 = 90_000; + let with_link = |from: &'static str, to: &'static str| { + move |v: &mut StockFlow| { + if v.elements.iter().any(|el| el.get_uid() == EXTRA) { + return; + } + let (from_uid, to_uid) = (uid_named(v, from), uid_named(v, to)); + v.elements.push(ViewElement::Link(view_element::Link { + uid: EXTRA, + from_uid, + to_uid, + shape: LinkShape::Straight, + polarity: None, + })); + } + }; + let without_link = |v: &mut StockFlow| v.elements.retain(|el| el.get_uid() != EXTRA); + + let unnamed_reader = with_link("birth_rate", "deaths"); + let e = edited_from(POPULATION, ScenarioKind::AddParameter, unnamed_reader); + e.row( + FindingKind::UntouchedLinkChanged, + unnamed_reader, + without_link, + ); + + let named_reader = with_link("average_lifespan", "births"); + let e = edited_from(POPULATION, ScenarioKind::AddParameter, named_reader); + e.row(FindingKind::StaleLinkRemains, without_link, named_reader); +} + +#[test] +fn an_edit_that_changes_nothing_raises_nothing() { + let project = load(POPULATION); + let view = shipped_view(&project); + let patch = ModelPatch { + name: project.get_model(MODEL).expect("model").name.clone(), + ops: vec![], + }; + let audit = audit_edit(&EditInput { + model_name: MODEL, + before: &project, + before_view: &view, + patch: &patch, + after: &project, + after_view: &view, + }); + assert!( + audit.findings.is_empty(), + "{:?}", + audit + .findings + .iter() + .map(|f| (&f.kind, &f.subject)) + .collect::>() + ); +} + +#[test] +fn a_rename_is_charged_nothing_for_the_new_name() { + // Findings key on the ident after the edit, so an imported view's + // pre-existing inconsistency about a renamed variable is not charged to + // the rename, and the renamed element's name is its one allowed change. + let e = edited(POPULATION, ScenarioKind::RenameVariable); + let audit = e.audit(unchanged); + assert!( + audit.findings.is_empty(), + "{:?}", + audit + .findings + .iter() + .map(|f| (&f.kind, &f.subject)) + .collect::>() + ); +} diff --git a/src/simlin-engine/src/layout/edit_scenarios.rs b/src/simlin-engine/src/layout/edit_scenarios.rs new file mode 100644 index 000000000..b62933116 --- /dev/null +++ b/src/simlin-engine/src/layout/edit_scenarios.rs @@ -0,0 +1,882 @@ +// Copyright 2026 The Simlin Authors. All rights reserved. +// Use of this source code is governed by the Apache License, +// Version 2.0, that can be found in the LICENSE file. + +//! Edit scenarios: the edits an agent or a notebook user makes to a model, +//! generated for any model, driven through the production patch and sync path, +//! and audited step by step (`layout::edit_audit`). +//! +//! Every `ScenarioKind` picks its targets from the model deterministically +//! (the first candidate by ident) and is not applicable to a model that has +//! none. A scenario is a sequence of patches, each applied with `apply_patch` +//! to a project holding the current view and synced by `sync_view`, the rule +//! MCP `edit_model` and libsimlin's patch sync follow. + +use std::collections::{BTreeMap, BTreeSet, VecDeque}; + +use crate::common::canonicalize; +use crate::datamodel::{self, Equation, StockFlow, Variable, ViewElement}; +use crate::patch::{ModelOperation, ModelPatch, ProjectPatch, apply_patch}; + +use super::edit_audit::{Displacement, EditAudit, EditInput, Finding, FindingKind, audit_edit}; +use super::metadata::ComputedMetadata; +use super::{compute_dependency_metadata, generate_best_layout, incremental_layout}; + +/// One kind of edit. +#[cfg_attr(feature = "debug-derive", derive(Debug))] +#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, serde::Serialize)] +#[serde(rename_all = "snake_case")] +pub enum ScenarioKind { + /// Upsert a variable exactly as it is, as an agent restating a definition + /// does: the view must not change. + RestateVariable, + /// Add a parameter a flow's equation multiplies by. + AddParameter, + /// Put a new variable between a parameter and a variable that reads it. + InsertIntermediate, + /// Delete a parameter. + DeleteParameter, + /// Delete a flow. + DeleteFlow, + /// Delete a stock that has both inflows and outflows. + DeleteMiddleStock, + /// Restate a stock with one of its flows left out of its lists. + DetachFlow, + /// Turn a parameter into a stock. + AuxToStock, + /// Rename a parameter with the rename operation. + RenameVariable, + /// Rename a parameter the way an agent without a rename operation does: + /// delete it, create it under the new name, and rewrite its readers. + RenameByRemoveAndAdd, + /// Add a flow between two stocks no flow joins. + AddFlowBetweenStocks, + /// Make a parameter read a stock it feeds, closing a feedback loop. + CloseLoop, + /// Add a stock downstream of a stock, joined by a new flow. + ExtendChain, + /// Add an outflow from a stock to a cloud. + AddSideFlow, + /// Add a disconnected stock with an inflow, an outflow, and a parameter. + AddSector, + /// Add a parameter, then delete it and restore the flow: the view must + /// come back to where it started. + AddThenUndo, +} + +impl ScenarioKind { + pub const ALL: [ScenarioKind; 16] = [ + ScenarioKind::RestateVariable, + ScenarioKind::AddParameter, + ScenarioKind::InsertIntermediate, + ScenarioKind::DeleteParameter, + ScenarioKind::DeleteFlow, + ScenarioKind::DeleteMiddleStock, + ScenarioKind::DetachFlow, + ScenarioKind::AuxToStock, + ScenarioKind::RenameVariable, + ScenarioKind::RenameByRemoveAndAdd, + ScenarioKind::AddFlowBetweenStocks, + ScenarioKind::CloseLoop, + ScenarioKind::ExtendChain, + ScenarioKind::AddSideFlow, + ScenarioKind::AddSector, + ScenarioKind::AddThenUndo, + ]; + + pub fn name(self) -> &'static str { + match self { + ScenarioKind::RestateVariable => "restate_variable", + ScenarioKind::AddParameter => "add_parameter", + ScenarioKind::InsertIntermediate => "insert_intermediate", + ScenarioKind::DeleteParameter => "delete_parameter", + ScenarioKind::DeleteFlow => "delete_flow", + ScenarioKind::DeleteMiddleStock => "delete_middle_stock", + ScenarioKind::DetachFlow => "detach_flow", + ScenarioKind::AuxToStock => "aux_to_stock", + ScenarioKind::RenameVariable => "rename_variable", + ScenarioKind::RenameByRemoveAndAdd => "rename_by_remove_and_add", + ScenarioKind::AddFlowBetweenStocks => "add_flow_between_stocks", + ScenarioKind::CloseLoop => "close_loop", + ScenarioKind::ExtendChain => "extend_chain", + ScenarioKind::AddSideFlow => "add_side_flow", + ScenarioKind::AddSector => "add_sector", + ScenarioKind::AddThenUndo => "add_then_undo", + } + } +} + +/// A generated edit sequence for one model. +#[derive(Clone)] +pub struct Scenario { + pub kind: ScenarioKind, + /// What the scenario does to this model, naming its targets. + pub description: String, + /// The operations of each patch, in order. + pub steps: Vec>, + /// `(ident before, ident after)` for a variable an edit gives a new + /// identity (a rename spelled as a delete and a create): how far it moved + /// is reported, since no audit can tell they are one variable. + pub continuity: Vec<(String, String)>, + /// Whether the last step must leave the view exactly as it began. + pub returns_to_original: bool, +} + +/// One applied and synced patch. +pub struct StepOutcome { + pub before_view: StockFlow, + pub after: datamodel::Project, + pub after_view: StockFlow, + pub audit: EditAudit, +} + +/// A scenario run. +pub struct ScenarioOutcome { + pub kind: ScenarioKind, + pub description: String, + pub steps: Vec, + /// Every step's findings, then the runner's own. + pub findings: Vec, + pub continuity: Vec, +} + +impl ScenarioOutcome { + pub fn kinds(&self) -> BTreeSet { + self.findings.iter().map(|f| f.kind).collect() + } +} + +/// Sync `old_view` to `project`, which `patch` has already been applied to: +/// a full layout while the view is empty, the incremental layout after that, +/// keeping the view's zoom (the rule MCP `edit_model` and +/// `simlin_project_diagram_sync` follow). +pub fn sync_view( + project: &datamodel::Project, + model_name: &str, + patch: &ModelPatch, + old_view: &StockFlow, +) -> Result { + let mut view = if old_view.elements.is_empty() { + generate_best_layout(project, model_name, None)? + } else { + incremental_layout(old_view, project, model_name, patch, None)? + }; + if old_view.zoom > 0.0 { + view.zoom = old_view.zoom; + } + Ok(view) +} + +fn with_view( + project: &datamodel::Project, + model_name: &str, + view: &StockFlow, +) -> datamodel::Project { + let mut p = project.clone(); + if let Some(m) = p.get_model_mut(model_name) { + m.views = vec![datamodel::View::StockFlow(view.clone())]; + } + p +} + +/// The first way `b` differs from `a`, or `None` when they are equal. +pub fn first_difference(a: &StockFlow, b: &StockFlow) -> Option { + if a == b { + return None; + } + let props = |v: &StockFlow| { + ( + v.name.clone(), + v.view_box.clone(), + v.zoom, + v.use_lettered_polarity, + v.font.clone(), + ) + }; + if props(a) != props(b) { + return Some("view properties differ".to_string()); + } + let by_uid = |v: &StockFlow| -> BTreeMap { + v.elements + .iter() + .map(|e| (e.get_uid(), e.clone())) + .collect() + }; + let (ea, eb) = (by_uid(a), by_uid(b)); + for (uid, x) in &ea { + match eb.get(uid) { + None => return Some(format!("element #{uid} missing")), + Some(y) if y != x => return Some(format!("element #{uid} differs")), + _ => {} + } + } + if let Some(uid) = eb.keys().find(|u| !ea.contains_key(u)) { + return Some(format!("element #{uid} added")); + } + Some("element order differs".to_string()) +} + +/// Drive `scenario` from `view` on `project`, auditing every step. +pub fn run_scenario( + project: &datamodel::Project, + model_name: &str, + view: &StockFlow, + scenario: &Scenario, +) -> ScenarioOutcome { + let mut outcome = ScenarioOutcome { + kind: scenario.kind, + description: scenario.description.clone(), + steps: Vec::new(), + findings: Vec::new(), + continuity: Vec::new(), + }; + let Some(patch_name) = project.get_model(model_name).map(|m| m.name.clone()) else { + outcome.findings.push(Finding::new( + FindingKind::SyncFailed, + model_name, + "no such model", + None, + )); + return outcome; + }; + let mut current = with_view(project, model_name, view); + let mut current_view = view.clone(); + let mut runner: Vec = Vec::new(); + for ops in &scenario.steps { + let patch = ModelPatch { + name: patch_name.clone(), + ops: ops.clone(), + }; + let mut after = current.clone(); + if let Err(err) = apply_patch( + &mut after, + ProjectPatch { + project_ops: vec![], + models: vec![patch.clone()], + }, + ) { + runner.push(Finding::new( + FindingKind::SyncFailed, + model_name, + format!("patch failed: {err}"), + None, + )); + break; + } + let after_view = match sync_view(&after, model_name, &patch, ¤t_view) { + Ok(v) => v, + Err(err) => { + runner.push(Finding::new( + FindingKind::SyncFailed, + model_name, + format!("sync failed: {err}"), + None, + )); + break; + } + }; + match sync_view(&after, model_name, &patch, ¤t_view) { + Ok(again) => { + if let Some(diff) = first_difference(&after_view, &again) { + runner.push(Finding::new( + FindingKind::NotDeterministic, + model_name, + diff, + None, + )); + } + } + Err(err) => runner.push(Finding::new( + FindingKind::NotDeterministic, + model_name, + format!("a second sync failed: {err}"), + None, + )), + } + let after = with_view(&after, model_name, &after_view); + let audit = audit_edit(&EditInput { + model_name, + before: ¤t, + before_view: ¤t_view, + patch: &patch, + after: &after, + after_view: &after_view, + }); + outcome.findings.extend(audit.findings.iter().cloned()); + outcome.steps.push(StepOutcome { + before_view: current_view.clone(), + after: after.clone(), + after_view: after_view.clone(), + audit, + }); + current = after; + current_view = after_view; + } + let completed = outcome.steps.len() == scenario.steps.len(); + if scenario.returns_to_original + && completed + && let Some(diff) = first_difference(view, ¤t_view) + { + runner.push(Finding::new( + FindingKind::ReturnToOriginal, + model_name, + diff, + None, + )); + } + let center_named = |v: &StockFlow, ident: &str| { + v.elements.iter().find_map(|e| { + let name = e.get_name()?; + if canonicalize(name) != ident { + return None; + } + match e { + ViewElement::Aux(a) => Some((a.x, a.y)), + ViewElement::Stock(s) => Some((s.x, s.y)), + ViewElement::Flow(f) => Some((f.x, f.y)), + ViewElement::Module(m) => Some((m.x, m.y)), + _ => None, + } + }) + }; + if completed { + for (old, new) in &scenario.continuity { + if let (Some(a), Some(b)) = (center_named(view, old), center_named(¤t_view, new)) + { + outcome.continuity.push(Displacement { + subject: format!("{old} -> {new}"), + distance: (a.0 - b.0).hypot(a.1 - b.1), + }); + } + } + } + outcome.findings.extend(runner); + outcome +} + +fn scalar(v: &Variable) -> Option<&str> { + match v.get_equation()? { + Equation::Scalar(s) => Some(s.as_str()), + _ => None, + } +} + +fn has_table(v: &Variable) -> bool { + match v { + Variable::Aux(a) => a.gf.is_some(), + Variable::Flow(f) => f.gf.is_some(), + _ => false, + } +} + +fn aux(ident: &str, equation: &str) -> ModelOperation { + ModelOperation::UpsertAux(datamodel::Aux { + ident: ident.to_string(), + equation: Equation::Scalar(equation.to_string()), + documentation: String::new(), + units: None, + gf: None, + ai_state: None, + uid: None, + compat: datamodel::Compat::default(), + }) +} + +fn flow(ident: &str, equation: &str) -> ModelOperation { + ModelOperation::UpsertFlow(datamodel::Flow { + ident: ident.to_string(), + equation: Equation::Scalar(equation.to_string()), + documentation: String::new(), + units: None, + gf: None, + ai_state: None, + uid: None, + compat: datamodel::Compat::default(), + }) +} + +fn stock(ident: &str, equation: &str, inflows: &[String], outflows: &[String]) -> ModelOperation { + ModelOperation::UpsertStock(datamodel::Stock { + ident: ident.to_string(), + equation: Equation::Scalar(equation.to_string()), + documentation: String::new(), + units: None, + inflows: inflows.to_vec(), + outflows: outflows.to_vec(), + ai_state: None, + uid: None, + compat: datamodel::Compat::default(), + }) +} + +/// `ident` as equation text reads it: an agent's name for a variable (a +/// hyphen, a leading digit, a keyword) can need quotes, and interpolated bare +/// it parses as some other expression, so the scenario would audit a different +/// edit from the one it names. +fn eqn_text(ident: &str) -> String { + crate::ast::print_ident(ident) +} + +fn upsert(v: Variable) -> ModelOperation { + match v { + Variable::Stock(s) => ModelOperation::UpsertStock(s), + Variable::Flow(f) => ModelOperation::UpsertFlow(f), + Variable::Aux(a) => ModelOperation::UpsertAux(a), + Variable::Module(m) => ModelOperation::UpsertModule(m), + } +} + +/// The model's variables and the dependencies a diagram draws for them, with +/// the target choices every scenario picks from. +struct Targets<'a> { + project: &'a datamodel::Project, + model_name: &'a str, + patch_name: String, + vars: BTreeMap, + /// The variables the view draws, which are the ones scenarios target: a + /// variable the view does not draw is drawn by any edit that names it, so + /// a scenario about one exercises that rule instead of the edit, and an + /// edit expected to return the original view cannot. `None` when the model + /// has no view. + drawn: Option>, + meta: ComputedMetadata, +} + +impl<'a> Targets<'a> { + fn new(project: &'a datamodel::Project, model_name: &'a str) -> Option> { + let model = project.get_model(model_name)?; + let meta = compute_dependency_metadata(project, model_name, None)?; + let drawn = model + .views + .iter() + .map(|v| match v { + datamodel::View::StockFlow(sf) => sf + .elements + .iter() + .filter(|e| { + matches!( + e, + ViewElement::Stock(_) + | ViewElement::Flow(_) + | ViewElement::Aux(_) + | ViewElement::Module(_) + ) + }) + .filter_map(|e| e.get_name().map(|n| canonicalize(n).into_owned())) + .collect::>(), + }) + .next(); + Some(Targets { + project, + model_name, + patch_name: model.name.clone(), + drawn, + vars: model + .variables + .iter() + .map(|v| (canonicalize(v.get_ident()).into_owned(), v)) + .collect(), + meta, + }) + } + + fn var(&self, ident: &str) -> Option<&'a Variable> { + self.vars.get(ident).copied() + } + + fn is_drawn(&self, ident: &str) -> bool { + self.drawn.as_ref().is_none_or(|d| d.contains(ident)) + } + + /// The drawn variables that read `ident`. + fn dependents(&self, ident: &str) -> Vec { + self.meta + .reverse_dep_graph + .get(ident) + .map(|s| { + s.iter() + .filter(|d| *d != ident && self.is_drawn(d)) + .cloned() + .collect() + }) + .unwrap_or_default() + } + + /// Scalar auxes with no reads and at least one reader, by ident. + fn parameters(&self) -> Vec { + self.vars + .iter() + .filter(|(ident, v)| { + self.is_drawn(ident) + && matches!(v, Variable::Aux(_)) + && scalar(v).is_some() + && !has_table(v) + && self.meta.dep_graph.get(*ident).is_none_or(|d| d.is_empty()) + && !self.dependents(ident).is_empty() + }) + .map(|(ident, _)| ident.clone()) + .collect() + } + + fn parameter(&self) -> Option { + self.parameters().into_iter().next() + } + + fn flows(&self) -> Vec { + self.vars + .iter() + .filter(|(ident, v)| { + self.is_drawn(ident) + && matches!(v, Variable::Flow(_)) + && scalar(v).is_some() + && !has_table(v) + }) + .map(|(ident, _)| ident.clone()) + .collect() + } + + fn stocks(&self) -> Vec<(String, &'a datamodel::Stock)> { + self.vars + .iter() + .filter_map(|(ident, v)| match v { + Variable::Stock(s) if self.is_drawn(ident) && scalar(v).is_some() => { + Some((ident.clone(), s)) + } + _ => None, + }) + .collect() + } + + /// `base`, or `base` with a number appended, naming no variable. + fn fresh(&self, base: &str) -> String { + let base = canonicalize(base).into_owned(); + if !self.vars.contains_key(&base) { + return base; + } + (2..) + .map(|n| format!("{base}_{n}")) + .find(|name| !self.vars.contains_key(name)) + .expect("an unused name") + } + + fn with_equation(&self, ident: &str, equation: &str) -> Option { + let mut v = self.var(ident)?.clone(); + v.set_scalar_equation(equation); + Some(upsert(v)) + } + + fn with_flows( + &self, + ident: &str, + inflows: Vec, + outflows: Vec, + ) -> Option { + let Variable::Stock(mut s) = self.var(ident)?.clone() else { + return None; + }; + s.inflows = inflows; + s.outflows = outflows; + Some(ModelOperation::UpsertStock(s)) + } + + /// `reader`'s equation with every reference to `from` spelled `to`, by the + /// rename operation's own rewriting. + fn renamed_equation(&self, reader: &str, from: &str, to: &str) -> Option { + let mut p = self.project.clone(); + apply_patch( + &mut p, + ProjectPatch { + project_ops: vec![], + models: vec![ModelPatch { + name: self.patch_name.clone(), + ops: vec![ModelOperation::RenameVariable { + from: from.to_string(), + to: to.to_string(), + }], + }], + }, + ) + .ok()?; + let model = p.get_model(self.model_name)?; + let v = model + .variables + .iter() + .find(|v| canonicalize(v.get_ident()) == reader)?; + scalar(v).map(str::to_string) + } + + fn add_parameter(&self) -> Option<(String, String, Vec)> { + let f = self.flows().into_iter().next()?; + let eqn = scalar(self.var(&f)?)?; + let n = self.fresh(&format!("{f}_multiplier")); + let ops = vec![ + aux(&n, "1"), + self.with_equation(&f, &format!("({eqn}) * {}", eqn_text(&n)))?, + ]; + Some((f, n, ops)) + } +} + +/// The scenario of `kind` for `project`'s model, or `None` when the model has +/// nothing it applies to. +pub fn build_scenario( + project: &datamodel::Project, + model_name: &str, + kind: ScenarioKind, +) -> Option { + let t = Targets::new(project, model_name)?; + let single = |description: String, ops: Vec| Scenario { + kind, + description, + steps: vec![ops], + continuity: Vec::new(), + returns_to_original: false, + }; + match kind { + ScenarioKind::RestateVariable => { + let ident = t + .flows() + .into_iter() + .next() + .or_else(|| t.parameter()) + .or_else(|| t.stocks().into_iter().next().map(|(i, _)| i))?; + Some(Scenario { + returns_to_original: true, + ..single( + format!("restate {ident}"), + vec![upsert(t.var(&ident)?.clone())], + ) + }) + } + ScenarioKind::AddParameter => { + let (f, n, ops) = t.add_parameter()?; + Some(single(format!("add {n}, read by {f}"), ops)) + } + ScenarioKind::InsertIntermediate => { + let (p, reader) = t.parameters().into_iter().find_map(|p| { + let reader = t.dependents(&p).into_iter().find(|r| { + t.var(r).is_some_and(|v| { + matches!(v, Variable::Aux(_) | Variable::Flow(_)) && scalar(v).is_some() + }) + })?; + Some((p, reader)) + })?; + let x = t.fresh(&format!("{p}_effective")); + let eqn = t.renamed_equation(&reader, &p, &x)?; + Some(single( + format!("insert {x} between {p} and {reader}"), + vec![aux(&x, &eqn_text(&p)), t.with_equation(&reader, &eqn)?], + )) + } + ScenarioKind::DeleteParameter => { + let p = t.parameter()?; + Some(single( + format!("delete {p}"), + vec![ModelOperation::DeleteVariable { ident: p }], + )) + } + ScenarioKind::DeleteFlow => { + let f = t.flows().into_iter().next()?; + Some(single( + format!("delete {f}"), + vec![ModelOperation::DeleteVariable { ident: f }], + )) + } + ScenarioKind::DeleteMiddleStock => { + let (s, _) = t + .stocks() + .into_iter() + .find(|(_, s)| !s.inflows.is_empty() && !s.outflows.is_empty())?; + Some(single( + format!("delete {s}"), + vec![ModelOperation::DeleteVariable { ident: s }], + )) + } + ScenarioKind::DetachFlow => { + let (s, st) = t + .stocks() + .into_iter() + .find(|(_, s)| !s.outflows.is_empty() || !s.inflows.is_empty())?; + let (mut inflows, mut outflows) = (st.inflows.clone(), st.outflows.clone()); + let detached = if outflows.is_empty() { + inflows.remove(0) + } else { + outflows.remove(0) + }; + Some(single( + format!("restate {s} without {detached}"), + vec![t.with_flows(&s, inflows, outflows)?], + )) + } + ScenarioKind::AuxToStock => { + let p = t.parameter()?; + let Variable::Aux(a) = t.var(&p)? else { + return None; + }; + Some(single( + format!("turn {p} into a stock"), + vec![ModelOperation::UpsertStock(datamodel::Stock { + ident: a.ident.clone(), + equation: a.equation.clone(), + documentation: a.documentation.clone(), + units: a.units.clone(), + inflows: Vec::new(), + outflows: Vec::new(), + ai_state: None, + uid: a.uid, + compat: datamodel::Compat::default(), + })], + )) + } + ScenarioKind::RenameVariable => { + let p = t.parameter()?; + let to = t.fresh(&format!("{p}_renamed")); + Some(single( + format!("rename {p} to {to}"), + vec![ModelOperation::RenameVariable { + from: t.var(&p)?.get_ident().to_string(), + to, + }], + )) + } + ScenarioKind::RenameByRemoveAndAdd => { + let p = t.parameter()?; + let eqn = scalar(t.var(&p)?)?.to_string(); + let to = t.fresh(&format!("{p}_renamed")); + let mut ops = vec![ + ModelOperation::DeleteVariable { + ident: t.var(&p)?.get_ident().to_string(), + }, + aux(&to, &eqn), + ]; + for reader in t.dependents(&p) { + let renamed = t.renamed_equation(&reader, &p, &to)?; + ops.push(t.with_equation(&reader, &renamed)?); + } + Some(Scenario { + continuity: vec![(p.clone(), to.clone())], + ..single(format!("rename {p} to {to} by delete and create"), ops) + }) + } + ScenarioKind::AddFlowBetweenStocks => { + let stocks = t.stocks(); + let joined = |a: &str, b: &str| { + t.meta.flow_to_stocks.values().any(|(from, to)| { + let (from, to) = (from.as_deref(), to.as_deref()); + (from == Some(a) && to == Some(b)) || (from == Some(b) && to == Some(a)) + }) + }; + let (a, sa, b, sb) = stocks.iter().enumerate().find_map(|(i, (a, sa))| { + stocks[i + 1..] + .iter() + .find(|(b, _)| !joined(a, b)) + .map(|(b, sb)| (a.clone(), *sa, b.clone(), *sb)) + })?; + let n = t.fresh(&format!("{a}_to_{b}")); + let mut out_a = sa.outflows.clone(); + out_a.push(n.clone()); + let mut in_b = sb.inflows.clone(); + in_b.push(n.clone()); + Some(single( + format!("add {n} from {a} to {b}"), + vec![ + flow(&n, &format!("{} * 0.01", eqn_text(&a))), + t.with_flows(&a, sa.inflows.clone(), out_a)?, + t.with_flows(&b, in_b, sb.outflows.clone())?, + ], + )) + } + ScenarioKind::CloseLoop => { + let (p, s) = t.parameters().into_iter().find_map(|p| { + let mut seen: BTreeSet = BTreeSet::new(); + let mut queue: VecDeque = VecDeque::from([p.clone()]); + let mut reached: BTreeSet = BTreeSet::new(); + while let Some(ident) = queue.pop_front() { + for next in t.dependents(&ident) { + if seen.insert(next.clone()) { + if matches!(t.var(&next), Some(Variable::Stock(_))) { + reached.insert(next.clone()); + } + queue.push_back(next); + } + } + } + reached.into_iter().next().map(|s| (p, s)) + })?; + let eqn = scalar(t.var(&p)?)?; + Some(single( + format!("make {p} read {s}"), + vec![t.with_equation(&p, &format!("({eqn}) * (1 + {} / 1000)", eqn_text(&s)))?], + )) + } + ScenarioKind::ExtendChain => { + let (s, st) = t.stocks().into_iter().next()?; + let downstream = t.fresh(&format!("{s}_downstream")); + let transfer = t.fresh(&format!("{s}_transfer")); + let mut outflows = st.outflows.clone(); + outflows.push(transfer.clone()); + Some(single( + format!("add {downstream}, fed from {s} by {transfer}"), + vec![ + flow(&transfer, &format!("{} * 0.1", eqn_text(&s))), + stock(&downstream, "0", std::slice::from_ref(&transfer), &[]), + t.with_flows(&s, st.inflows.clone(), outflows)?, + ], + )) + } + ScenarioKind::AddSideFlow => { + let (s, st) = t.stocks().into_iter().next()?; + let loss = t.fresh(&format!("{s}_loss")); + let mut outflows = st.outflows.clone(); + outflows.push(loss.clone()); + Some(single( + format!("add {loss} out of {s}"), + vec![ + flow(&loss, &format!("{} * 0.01", eqn_text(&s))), + t.with_flows(&s, st.inflows.clone(), outflows)?, + ], + )) + } + ScenarioKind::AddSector => { + let level = t.fresh("edit_sector_stock"); + let inflow = t.fresh("edit_sector_inflow"); + let outflow = t.fresh("edit_sector_outflow"); + let rate = t.fresh("edit_sector_rate"); + Some(single( + format!("add the sector {level}"), + vec![ + aux(&rate, "0.1"), + flow(&inflow, "1"), + flow( + &outflow, + &format!("{} * {}", eqn_text(&level), eqn_text(&rate)), + ), + stock( + &level, + "10", + std::slice::from_ref(&inflow), + std::slice::from_ref(&outflow), + ), + ], + )) + } + ScenarioKind::AddThenUndo => { + let (f, n, ops) = t.add_parameter()?; + let undo = vec![ + ModelOperation::DeleteVariable { ident: n.clone() }, + upsert(t.var(&f)?.clone()), + ]; + Some(Scenario { + kind, + description: format!("add {n} to {f}, then take it back"), + steps: vec![ops, undo], + continuity: Vec::new(), + returns_to_original: true, + }) + } + } +} + +#[cfg(test)] +#[path = "edit_scenarios_tests.rs"] +mod tests; diff --git a/src/simlin-engine/src/layout/edit_scenarios_tests.rs b/src/simlin-engine/src/layout/edit_scenarios_tests.rs new file mode 100644 index 000000000..4e57af02e --- /dev/null +++ b/src/simlin-engine/src/layout/edit_scenarios_tests.rs @@ -0,0 +1,447 @@ +// Copyright 2026 The Simlin Authors. All rights reserved. +// Use of this source code is governed by the Apache License, +// Version 2.0, that can be found in the LICENSE file. + +//! The scenario battery: every `ScenarioKind` driven over hand-drawn and +//! imported views through the production patch and sync path, with every +//! finding the audit raises pinned. The imported fixtures are Vensim views +//! with aliases, links the dependency extraction does not explain, variables +//! the author did not draw, and flows that meet at one stock face: the shapes +//! of view a sync meets when an agent edits a published model. +//! +//! `KNOWN_DEFECTS` lists what the sync still gets wrong, one row per (fixture, +//! scenario, finding kind), each naming the defect. The test fails on a finding +//! no row expects and on a row that no longer reproduces, so fixing a defect +//! means deleting its rows, and a regression cannot hide behind a row. + +use super::*; +use crate::layout::edit_audit::FindingKind; +use crate::layout::taste::{Degradation, degrade}; + +struct Fixture { + key: &'static str, + path: &'static str, + /// Start from the shipped view with every connector drawn straight, the way + /// a modeler who straightens links leaves a diagram, so an edit that + /// re-curves an untouched link shows up. + straighten_links: bool, +} + +const FIXTURES: [Fixture; 13] = [ + Fixture { + key: "population", + path: "default_projects/population/model.xmile", + straighten_links: false, + }, + Fixture { + key: "logistic_growth", + path: "default_projects/logistic-growth/model.xmile", + straighten_links: false, + }, + Fixture { + key: "logistic_growth_straight", + path: "default_projects/logistic-growth/model.xmile", + straighten_links: true, + }, + Fixture { + key: "fishbanks", + path: "default_projects/fishbanks/model.xmile", + straighten_links: false, + }, + Fixture { + key: "reliability", + path: "default_projects/reliability/model.xmile", + straighten_links: false, + }, + Fixture { + key: "sir", + path: "test/test-models/samples/SIR/SIR.stmx", + straighten_links: false, + }, + Fixture { + key: "hares_and_foxes", + path: "test/modules_hares_and_foxes/modules_hares_and_foxes.stmx", + straighten_links: false, + }, + Fixture { + key: "lotka_volterra", + path: "test/test-models/samples/Lotka_Volterra/Lotka_Volterra.mdl", + straighten_links: false, + }, + Fixture { + key: "groupon", + path: "test/metasd/social-network-valuation/groupon 1.mdl", + straighten_links: false, + }, + Fixture { + key: "catastrophe", + path: "test/metasd/early-warnings-catastrophe/catastropeWarning2.mdl", + straighten_links: false, + }, + Fixture { + key: "beer_game", + path: "test/metasd/beer-game/RealBeer4-Sterman13.mdl", + straighten_links: false, + }, + Fixture { + key: "bathtub", + path: "test/metasd/bathtub-statistics/integration3.mdl", + straighten_links: false, + }, + Fixture { + key: "alias1", + path: "test/alias1/alias1.stmx", + straighten_links: false, + }, +]; + +/// `(fixture, scenario, finding kind, the defect behind it)`. +const KNOWN_DEFECTS: &[(&str, &str, &str, &str)] = &[]; + +fn load(rel: &str) -> datamodel::Project { + let path = format!("{}/../../{rel}", env!("CARGO_MANIFEST_DIR")); + if rel.ends_with(".mdl") { + let text = std::fs::read_to_string(&path).unwrap_or_else(|e| panic!("{path}: {e}")); + crate::compat::open_vensim(&text).unwrap_or_else(|e| panic!("{path}: {e:?}")) + } else { + let file = std::fs::File::open(&path).unwrap_or_else(|e| panic!("{path}: {e}")); + crate::compat::open_xmile(&mut std::io::BufReader::new(file)) + .unwrap_or_else(|e| panic!("{path}: {e:?}")) + } +} + +fn fixture(key: &str) -> &'static Fixture { + FIXTURES + .iter() + .find(|f| f.key == key) + .unwrap_or_else(|| panic!("no fixture {key}")) +} + +/// The fixture's project, holding the view its scenarios start from. +fn starting_point(f: &Fixture) -> (datamodel::Project, StockFlow) { + let mut project = load(f.path); + let shipped = match project.get_model("main").and_then(|m| m.views.first()) { + Some(datamodel::View::StockFlow(sf)) => sf.clone(), + None => panic!("{} ships no view", f.key), + }; + let view = if f.straighten_links { + degrade(&shipped, Degradation::StraightenLinks).expect("the view curves some link") + } else { + shipped + }; + project.get_model_mut("main").expect("main").views = + vec![datamodel::View::StockFlow(view.clone())]; + (project, view) +} + +/// Every `(scenario, finding kind)` the battery raises on `f`. +fn battery(f: &Fixture) -> BTreeSet<(&'static str, &'static str)> { + let (project, view) = starting_point(f); + let mut found = BTreeSet::new(); + for kind in ScenarioKind::ALL { + let Some(scenario) = build_scenario(&project, "main", kind) else { + continue; + }; + let outcome = run_scenario(&project, "main", &view, &scenario); + for finding in &outcome.findings { + eprintln!( + "{} {} ({}): {} {}: {}", + f.key, + kind.name(), + scenario.description, + finding.kind.name(), + finding.subject, + finding.detail + ); + found.insert((kind.name(), finding.kind.name())); + } + } + found +} + +fn check(key: &str) { + let f = fixture(key); + let expected: BTreeSet<(&str, &str)> = KNOWN_DEFECTS + .iter() + .filter(|row| row.0 == key) + .map(|row| (row.1, row.2)) + .collect(); + let actual = battery(f); + let rows = |set: BTreeSet<&(&str, &str)>| { + set.into_iter() + .map(|(s, k)| format!(" (\"{key}\", \"{s}\", \"{k}\", \"\"),")) + .collect::>() + .join("\n") + }; + let unexpected = rows(actual.difference(&expected).collect()); + let fixed = rows(expected.difference(&actual).collect()); + assert!( + unexpected.is_empty() && fixed.is_empty(), + "{key}: findings no row expects:\n{unexpected}\nrows that no longer reproduce (delete them):\n{fixed}" + ); +} + +#[test] +fn population() { + check("population"); +} + +#[test] +fn logistic_growth() { + check("logistic_growth"); +} + +#[test] +fn logistic_growth_straight() { + check("logistic_growth_straight"); +} + +#[test] +fn fishbanks() { + check("fishbanks"); +} + +#[test] +fn reliability() { + check("reliability"); +} + +#[test] +fn sir() { + check("sir"); +} + +#[test] +fn hares_and_foxes() { + check("hares_and_foxes"); +} + +#[test] +fn lotka_volterra() { + check("lotka_volterra"); +} + +#[test] +fn groupon() { + check("groupon"); +} + +#[test] +fn catastrophe() { + check("catastrophe"); +} + +#[test] +fn beer_game() { + check("beer_game"); +} + +#[test] +fn bathtub() { + check("bathtub"); +} + +#[test] +fn alias1() { + check("alias1"); +} + +#[test] +fn every_fixture_has_a_test() { + // The per-fixture tests above are written out so they run in parallel; + // this names every fixture key they must cover. + let tested = [ + "population", + "logistic_growth", + "logistic_growth_straight", + "fishbanks", + "reliability", + "sir", + "hares_and_foxes", + "lotka_volterra", + "groupon", + "catastrophe", + "beer_game", + "bathtub", + "alias1", + ]; + let keys: Vec<&str> = FIXTURES.iter().map(|f| f.key).collect(); + assert_eq!(keys, tested); +} + +#[test] +fn scenarios_target_only_variables_the_view_draws() { + // A variable the view does not draw is drawn by any edit that names it, so + // a scenario about one exercises that rule (pinned by incremental layout's + // own tests) rather than the edit, and an edit expected to return the + // original view cannot. Population's first flow by ident is births; with + // births left out of the view, the restate names deaths instead. + let (mut project, mut view) = starting_point(fixture("population")); + let births = view + .elements + .iter() + .find(|e| e.get_name().is_some_and(|n| canonicalize(n) == "births")) + .map(ViewElement::get_uid) + .expect("births drawn"); + view.elements.retain(|e| match e { + ViewElement::Link(l) => l.from_uid != births && l.to_uid != births, + ViewElement::Cloud(c) => c.flow_uid != births, + other => other.get_uid() != births, + }); + project.get_model_mut("main").expect("main").views = vec![datamodel::View::StockFlow(view)]; + let restate = build_scenario(&project, "main", ScenarioKind::RestateVariable).expect("applies"); + assert!( + restate.description.contains("deaths"), + "{}", + restate.description + ); +} + +#[test] +fn every_scenario_kind_applies_to_some_fixture() { + let projects: Vec = FIXTURES.iter().map(|f| load(f.path)).collect(); + for kind in ScenarioKind::ALL { + assert!( + projects + .iter() + .any(|p| build_scenario(p, "main", kind).is_some()), + "{} applies to no fixture, so the battery never runs it", + kind.name() + ); + } +} + +#[test] +fn every_known_defect_names_a_fixture_scenario_and_finding() { + for (key, scenario, finding, defect) in KNOWN_DEFECTS { + assert!(FIXTURES.iter().any(|f| f.key == *key), "fixture {key}"); + assert!( + ScenarioKind::ALL.iter().any(|k| k.name() == *scenario), + "scenario {scenario}" + ); + assert!( + FindingKind::ALL.iter().any(|k| k.name() == *finding), + "finding {finding}" + ); + assert!( + !defect.is_empty(), + "{key} {scenario} {finding} names no defect" + ); + } +} + +#[test] +fn every_scenario_writes_equations_that_read_names_needing_quotes() { + // Every variable here has a name the lexer cannot read bare, so a + // scenario that interpolates one into equation text unquoted writes some + // other expression (`labor-force * 0.1` is a subtraction reading `labor` + // and `force`), and the battery would audit a different edit from the one + // it names. The oracle is the production compiler: a variable a step + // writes that comes out with an equation error was written an equation + // that does not read what it says. Only the written variables are charged: + // deleting a parameter leaves its readers with an unknown dependency, which + // is the edit, not a spelling. Every kind must apply, so every arm of + // `build_scenario` is covered. + use crate::db::{ + DiagnosticSeverity, LtmOverlay, SimlinDb, collect_all_diagnostics, sync_from_datamodel, + }; + let errors = |project: &datamodel::Project, only: Option<&BTreeSet>| -> Vec { + let db = SimlinDb::default(); + let sync = sync_from_datamodel(&db, project); + collect_all_diagnostics(&db, sync.project, LtmOverlay::Off) + .iter() + .filter(|d| d.severity == DiagnosticSeverity::Error) + .filter(|d| { + only.is_none_or(|written| { + d.variable + .as_deref() + .is_some_and(|v| written.contains(canonicalize(v).as_ref())) + }) + }) + .map(|d| format!("{:?}: {:?}", d.variable, d.error.code())) + .collect() + }; + let names = |idents: &[&str]| idents.iter().map(|s| s.to_string()).collect::>(); + let mut project = datamodel::Project { + name: "quoted".to_string(), + sim_specs: datamodel::SimSpecs::default(), + dimensions: Vec::new(), + units: Vec::new(), + models: vec![datamodel::Model { + name: "main".to_string(), + sim_specs: None, + variables: Vec::new(), + views: Vec::new(), + loop_metadata: Vec::new(), + groups: Vec::new(), + macro_spec: None, + }], + source: None, + ai_information: None, + }; + apply_patch( + &mut project, + ProjectPatch { + project_ops: vec![], + models: vec![ModelPatch { + name: "main".to_string(), + ops: vec![ + stock( + "labor-force", + "100", + &names(&["hiring-rate"]), + &names(&["quit-rate"]), + ), + stock("retirees", "0", &names(&["quit-rate"]), &[]), + stock("open-positions", "10", &[], &[]), + flow("hiring-rate", "\"labor-force\" * \"hire-fraction\""), + flow("quit-rate", "\"labor-force\" * 0.05"), + aux("hire-fraction", "0.1"), + ], + }], + }, + ) + .expect("the fixture applies"); + assert_eq!( + errors(&project, None), + Vec::::new(), + "the fixture compiles" + ); + + for kind in ScenarioKind::ALL { + let scenario = build_scenario(&project, "main", kind) + .unwrap_or_else(|| panic!("{} applies to the fixture", kind.name())); + let mut current = project.clone(); + for (step, ops) in scenario.steps.iter().enumerate() { + apply_patch( + &mut current, + ProjectPatch { + project_ops: vec![], + models: vec![ModelPatch { + name: "main".to_string(), + ops: ops.clone(), + }], + }, + ) + .unwrap_or_else(|e| panic!("{} step {step} applies: {e:?}", kind.name())); + let written: BTreeSet = ops + .iter() + .filter_map(|op| match op { + ModelOperation::UpsertStock(s) => Some(&s.ident), + ModelOperation::UpsertFlow(f) => Some(&f.ident), + ModelOperation::UpsertAux(a) => Some(&a.ident), + ModelOperation::UpsertModule(m) => Some(&m.ident), + _ => None, + }) + .map(|ident| canonicalize(ident).into_owned()) + .collect(); + assert_eq!( + errors(¤t, Some(&written)), + Vec::::new(), + "{} ({}) step {step} leaves the model with equation errors", + kind.name(), + scenario.description + ); + } + } +} diff --git a/src/simlin-engine/src/layout/face_slots.rs b/src/simlin-engine/src/layout/face_slots.rs index a4f5ef013..16e1e423e 100644 --- a/src/simlin-engine/src/layout/face_slots.rs +++ b/src/simlin-engine/src/layout/face_slots.rs @@ -380,8 +380,10 @@ pub(crate) fn place_created_flow_ends(elements: &mut [ViewElement], created: &Ha separate_created_clouds(elements, created, &stocks, &order); } /// Push each created flow's free end out along its end segment until the cloud -/// on it overlaps no other cloud: every other flow's free end, a created one -/// once it is settled. +/// on it overlaps no other cloud -- every other flow's free end, a created one +/// once it is settled -- and covers no other shape: a stock, parameter, module, +/// alias or another flow's valve, which a person may have parked where the +/// cloud would go. fn separate_created_clouds( elements: &mut [ViewElement], created: &HashSet, @@ -389,6 +391,13 @@ fn separate_created_clouds( order: &[(i32, usize)], ) { let is_free = |p: &FlowPoint| !p.attached_to_uid.is_some_and(|u| stocks.contains_key(&u)); + // Clouds are measured by the free ends, since the finishing pass recenters + // them there. + let shapes: Vec<(i32, crate::diagram::common::Rect)> = elements + .iter() + .filter(|e| !matches!(e, ViewElement::Cloud(_))) + .filter_map(|e| crate::layout::metrics::node_shape_box(e).map(|r| (e.get_uid(), r))) + .collect(); let mut settled: HashSet = HashSet::new(); for &(uid, idx) in order { let ViewElement::Flow(f) = &elements[idx] else { @@ -435,9 +444,15 @@ fn separate_created_clouds( let step = 2.0 * CLOUD_RADIUS; for _ in 0..MAX_CLOUD_PUSHES { let p = &f.points[end]; - let overlaps = others - .iter() - .any(|&(x, y)| (x - p.x).hypot(y - p.y) < step - EPS); + let covers_shape = shapes.iter().filter(|(u, _)| *u != uid).any(|(_, r)| { + let w = r.right.min(p.x + CLOUD_RADIUS) - r.left.max(p.x - CLOUD_RADIUS); + let h = r.bottom.min(p.y + CLOUD_RADIUS) - r.top.max(p.y - CLOUD_RADIUS); + w > EPS && h > EPS + }); + let overlaps = covers_shape + || others + .iter() + .any(|&(x, y)| (x - p.x).hypot(y - p.y) < step - EPS); if !overlaps { break; } diff --git a/src/simlin-engine/src/layout/incremental.rs b/src/simlin-engine/src/layout/incremental.rs index fb6589b69..511d1e117 100644 --- a/src/simlin-engine/src/layout/incremental.rs +++ b/src/simlin-engine/src/layout/incremental.rs @@ -703,15 +703,27 @@ pub fn resnap_flow_endpoints( /// against edges derived from the current dep_graph, then preserve /// unchanged links, remove stale ones, and create new links with /// default shapes. -pub fn diff_connectors(state: &mut LayoutState, metadata: &ComputedMetadata) { - // Build HashMap<(from_uid, to_uid), ViewElement> for existing links - let mut old_links: HashMap<(i32, i32), ViewElement> = HashMap::new(); - for elem in &state.elements { - if let ViewElement::Link(l) = elem { - old_links.insert((l.from_uid, l.to_uid), elem.clone()); - } - } - +/// +/// A preserved link stays where it is in the view's element list and a +/// created one is appended, in `(from_uid, to_uid)` order: the element list is +/// the view's draw order and what a saved file lists, so an edit that leaves a +/// link alone must not move it, and two syncs of one edit must produce one +/// list. +/// +/// A dependency with no link gets one only when both its ends are drawn and +/// `draws(from, to)` accepts its idents: incremental layout draws a connector +/// only where the edit is about it, so a connector an author left out of the +/// view stays out. A link drawing no dependency the model has survives when +/// `keeps(reader)` accepts the ident of the variable it points into (through +/// an alias; empty when it points at no variable): an imported view's +/// connector the extraction does not explain is an author's choice that only +/// an edit to its reader may undo. +pub fn diff_connectors( + state: &mut LayoutState, + metadata: &ComputedMetadata, + draws: impl Fn(&str, &str) -> bool, + keeps: impl Fn(&str) -> bool, +) { // Compute new dependency edges from dep_graph, skipping structural flow-stock edges let stock_inflows: HashMap> = metadata .stock_to_inflows @@ -724,8 +736,10 @@ pub fn diff_connectors(state: &mut LayoutState, metadata: &ComputedMetadata) { .map(|(k, v)| (k.clone(), v.iter().cloned().collect())) .collect(); - let mut new_edges: HashSet<(i32, i32)> = HashSet::new(); - let mut new_edge_idents: HashMap<(i32, i32), (String, String)> = HashMap::new(); + // Ordered by uid pair: each created link allocates the next uid and is + // appended, so the edges must come out in one order however the maps hash + // (the incremental analogue of #633). + let mut new_edges: BTreeMap<(i32, i32), (String, String)> = BTreeMap::new(); for (var, deps) in &metadata.dep_graph { for dep in deps { @@ -746,8 +760,7 @@ pub fn diff_connectors(state: &mut LayoutState, metadata: &ComputedMetadata) { }; if from_uid != 0 && to_uid != 0 { - new_edges.insert((from_uid, to_uid)); - new_edge_idents.insert( + new_edges.insert( (from_uid, to_uid), (from_ident.to_string(), to_ident.to_string()), ); @@ -768,56 +781,59 @@ pub fn diff_connectors(state: &mut LayoutState, metadata: &ComputedMetadata) { }) .collect(); - // Remove all old links from elements - state + let ident_of: HashMap = state .elements - .retain(|elem| !matches!(elem, ViewElement::Link(_))); - - // Track which old links have been consumed so each is used at most once. - let mut consumed_old_links: HashSet<(i32, i32)> = HashSet::new(); - - // Iterate edges in a deterministic order. `new_edges` is a HashSet, so its - // iteration order is per-process random; since each newly-created link both - // allocates a sequential `uid` and is appended to `state.elements` in this - // loop, hash order would otherwise assign different uids / element ordering - // to the same logical link run-to-run (the incremental analogue of #633). - let mut sorted_new_edges: Vec<(i32, i32)> = new_edges.iter().copied().collect(); - sorted_new_edges.sort_unstable(); - - // Add back preserved links (unchanged) and create new links - for (from_uid, to_uid) in sorted_new_edges { - if let Some(old_link) = old_links.get(&(from_uid, to_uid)) { - // Preserved: keep the old link exactly as-is - state.elements.push(old_link.clone()); - consumed_old_links.insert((from_uid, to_uid)); - } else if let Some(key) = old_links - .keys() - .copied() - .filter(|&(of, ot)| { - if consumed_old_links.contains(&(of, ot)) { - return false; - } - let rf = alias_to_primary.get(&of).copied().unwrap_or(of); - let rt = alias_to_primary.get(&ot).copied().unwrap_or(ot); - rf == from_uid && rt == to_uid - }) - // Pick the lowest matching key so the alias-match selection is - // deterministic; HashMap iteration order would otherwise vary. - .min() + .iter() + .filter(|e| { + matches!( + e, + ViewElement::Stock(_) + | ViewElement::Flow(_) + | ViewElement::Aux(_) + | ViewElement::Module(_) + ) + }) + .filter_map(|e| Some((e.get_uid(), canonicalize(e.get_name()?).into_owned()))) + .collect(); + + // A link survives when the dependency it draws, read through aliases, is + // still one the model has. Every such link survives: an imported view can + // draw one dependency several times, through different aliases of the same + // variable. A link drawing no dependency survives where `keeps` says so. + let mut drawn: HashSet<(i32, i32)> = HashSet::new(); + state.elements.retain(|elem| { + let ViewElement::Link(l) = elem else { + return true; + }; + let edge = ( + alias_to_primary + .get(&l.from_uid) + .copied() + .unwrap_or(l.from_uid), + alias_to_primary.get(&l.to_uid).copied().unwrap_or(l.to_uid), + ); + if new_edges.contains_key(&edge) { + drawn.insert(edge); + return true; + } + keeps(ident_of.get(&edge.1).map(String::as_str).unwrap_or("")) + }); + + for (&(from_uid, to_uid), (from_ident, to_ident)) in &new_edges { + // A link is drawn between two drawn variables: a variable with a uid + // may still be one the view leaves out (a project MCP opened gives + // every variable a uid), and a link into it would reference no element. + if drawn.contains(&(from_uid, to_uid)) + || !ident_of.contains_key(&from_uid) + || !ident_of.contains_key(&to_uid) + || !draws(from_ident, to_ident) { - // Preserved via alias: the old link targets an alias whose primary - // variable matches this dependency edge. Keep the alias link as-is. - state.elements.push(old_links[&key].clone()); - consumed_old_links.insert(key); - } else if let Some((from_ident, to_ident)) = new_edge_idents.get(&(from_uid, to_uid)) { - // Added: create new link with default shape - let link_uid = state.uid_manager.alloc(""); - let shape = if is_structural_stock_flow( - from_ident, - to_ident, - &stock_inflows, - &stock_outflows, - ) { + continue; + } + // Added: create new link with default shape + let link_uid = state.uid_manager.alloc(""); + let shape = + if is_structural_stock_flow(from_ident, to_ident, &stock_inflows, &stock_outflows) { let arc_angle = if let (Some(&s_pos), Some(&f_pos)) = (state.positions.get(&from_uid), state.positions.get(&to_uid)) { @@ -843,34 +859,13 @@ pub fn diff_connectors(state: &mut LayoutState, metadata: &ComputedMetadata) { LinkShape::Straight }; - state.elements.push(ViewElement::Link(view_element::Link { - uid: link_uid, - from_uid, - to_uid, - shape, - polarity: None, - })); - } - } - - // Preserve remaining alias-backed links whose alias-resolved endpoints - // match a valid dependency. Imported views may have multiple rendered - // connectors for the same dependency (e.g., links to two different - // aliases of the same variable). - // Iterate in a deterministic order for the same reason as the new-edge loop: - // the preserved links are appended to `state.elements`, so HashMap iteration - // order would otherwise perturb element ordering run-to-run. - let mut sorted_old_links: Vec<&(i32, i32)> = old_links.keys().collect(); - sorted_old_links.sort_unstable(); - for &(of, ot) in sorted_old_links { - if consumed_old_links.contains(&(of, ot)) { - continue; - } - let rf = alias_to_primary.get(&of).copied().unwrap_or(of); - let rt = alias_to_primary.get(&ot).copied().unwrap_or(ot); - if new_edges.contains(&(rf, rt)) { - state.elements.push(old_links[&(of, ot)].clone()); - } + state.elements.push(ViewElement::Link(view_element::Link { + uid: link_uid, + from_uid, + to_uid, + shape, + polarity: None, + })); } } @@ -928,17 +923,18 @@ pub fn diff_clouds(state: &mut LayoutState, metadata: &ComputedMetadata) { }) .collect(); - // Remove all old clouds from elements - state - .elements - .retain(|elem| !matches!(elem, ViewElement::Cloud(_))); - - // For each flow, determine what to keep vs create - let all_flow_uids: HashSet = needed_flow_uids + // For each flow, determine what to keep vs create. A preserved cloud keeps + // its place in the element list and a created one is appended, flows in + // uid order: the element list is the view's draw order and what a saved + // file lists, so a cloud the edit leaves alone must not move in it, and two + // syncs of one edit must produce one list. + let all_flow_uids: BTreeSet = needed_flow_uids .iter() .chain(old_clouds_by_flow.keys()) .copied() .collect(); + let mut kept: HashSet = HashSet::new(); + let mut created: Vec = Vec::new(); for flow_uid in all_flow_uids { let old_clouds = old_clouds_by_flow @@ -951,11 +947,6 @@ pub fn diff_clouds(state: &mut LayoutState, metadata: &ComputedMetadata) { let needed_count = wants_source as usize + wants_sink as usize; if needed_count == 0 { - for c in &old_clouds { - if let ViewElement::Cloud(cloud) = c { - state.positions.remove(&cloud.uid); - } - } continue; } @@ -1005,45 +996,45 @@ pub fn diff_clouds(state: &mut LayoutState, metadata: &ComputedMetadata) { } } - // Push preserved clouds and remove positions of discarded ones - for cloud in &old_clouds { - if let ViewElement::Cloud(c) = cloud { - if used_uids.contains(&c.uid) { - state.elements.push(cloud.clone()); - } else { - state.positions.remove(&c.uid); - } - } - } + kept.extend(used_uids); - // Create new clouds for roles that couldn't be filled from old clouds - if wants_source && !preserved_source { - let pos = endpoints.map(|(src, _)| *src); - let (cx, cy) = pos.map_or((0.0, 0.0), |p| (p.x, p.y)); + // Create new clouds for roles that couldn't be filled from old clouds, + // at the drawn pipe's ends. A flow with no drawn pipe -- one the view + // leaves out, which may still have a uid -- has nowhere to put a cloud, + // and a cloud of it would belong to no element. + let Some(&(src_pos, snk_pos)) = endpoints else { + continue; + }; + for (wanted, preserved, at) in [ + (wants_source, preserved_source, src_pos), + (wants_sink, preserved_sink, snk_pos), + ] { + if !wanted || preserved { + continue; + } let cloud_uid = state.uid_manager.alloc(""); - state.elements.push(ViewElement::Cloud(view_element::Cloud { + created.push(ViewElement::Cloud(view_element::Cloud { uid: cloud_uid, flow_uid, - x: cx, - y: cy, + x: at.x, + y: at.y, compat: None, })); - state.positions.insert(cloud_uid, Position::new(cx, cy)); + state.positions.insert(cloud_uid, at); } - if wants_sink && !preserved_sink { - let pos = endpoints.map(|(_, sink)| *sink); - let (cx, cy) = pos.map_or((0.0, 0.0), |p| (p.x, p.y)); - let cloud_uid = state.uid_manager.alloc(""); - state.elements.push(ViewElement::Cloud(view_element::Cloud { - uid: cloud_uid, - flow_uid, - x: cx, - y: cy, - compat: None, - })); - state.positions.insert(cloud_uid, Position::new(cx, cy)); + } + + for elem in &state.elements { + if let ViewElement::Cloud(c) = elem + && !kept.contains(&c.uid) + { + state.positions.remove(&c.uid); } } + state + .elements + .retain(|elem| !matches!(elem, ViewElement::Cloud(c) if !kept.contains(&c.uid))); + state.elements.extend(created); // Repair pass: for XMILE-imported views a cloud element may exist but the // corresponding flow point's attached_to_uid may be None. Wire up any @@ -1332,7 +1323,8 @@ fn place_new_chains( /// Set each new stock that hangs off a drawn chain -- joined by a flow to a /// stock already drawn -- one chain step past that neighbor, in its row: right /// of the stock it drains, left of the stock it feeds, fanning vertically only -/// past a stock or parameter already drawn there +/// past a shape already drawn there -- a stock, a parameter, or a valve or +/// cloud of a flow /// (`chain::find_free_stock_position`). A new stock reached only through other /// new stocks is placed from them in turn. Returns the idents of the stocks /// placed; a flow between two stocks that are now drawn takes the stock-pair @@ -1363,6 +1355,9 @@ fn place_chain_extensions( (None, Some(anchor)) if pending.contains(from) => (anchor, from, -step), _ => continue, }; + // Every drawn shape blocks the spot, a side flow's valve and + // cloud included: a person often draws a side flow's pipe off + // the very face a chain continues from. let occupied: Vec = state .elements .iter() @@ -1370,7 +1365,10 @@ fn place_chain_extensions( ViewElement::Stock(s) => Some(Position::new(s.x, s.y)), ViewElement::Aux(a) => Some(Position::new(a.x, a.y)), ViewElement::Module(m) => Some(Position::new(m.x, m.y)), - _ => None, + ViewElement::Flow(f) => Some(Position::new(f.x, f.y)), + ViewElement::Cloud(c) => Some(Position::new(c.x, c.y)), + ViewElement::Alias(a) => Some(Position::new(a.x, a.y)), + ViewElement::Link(_) | ViewElement::Group(_) => None, }) .collect(); let pos = chain::find_free_stock_position( @@ -1475,6 +1473,739 @@ fn translate_view_element(elem: &mut ViewElement, dx: f64, dy: f64) { } } +/// How far past a stock's face a cloud end is placed beyond the cloud's own +/// radius, so the cloud reads as off the stock rather than on it. +const DETACHED_CLOUD_GAP: f64 = 2.0; + +fn point_of(p: &FlowPoint) -> crate::editing::Point { + crate::editing::Point::new(p.x, p.y) +} + +/// Where a flow end that becomes a cloud goes: `p` itself when no stock body +/// holds it (a deleted stock's face, open space), else along its pipe toward +/// `adjacent`, past the stock's face by the cloud's radius and a gap, so the +/// pipe keeps its line and the cloud sits off the stock it left. +fn clear_of_stocks( + p: crate::editing::Point, + adjacent: crate::editing::Point, + stocks: &[crate::editing::Point], +) -> crate::editing::Point { + use crate::diagram::constants::{CLOUD_RADIUS, STOCK_HEIGHT, STOCK_WIDTH}; + const EPS: f64 = 1e-6; + let (half_w, half_h) = (STOCK_WIDTH / 2.0, STOCK_HEIGHT / 2.0); + let Some(center) = stocks + .iter() + .find(|c| (p.x - c.x).abs() <= half_w + EPS && (p.y - c.y).abs() <= half_h + EPS) + else { + return p; + }; + let (dx, dy) = (adjacent.x - p.x, adjacent.y - p.y); + let len = dx.hypot(dy); + if len <= EPS { + return p; + } + let (ux, uy) = (dx / len, dy / len); + // The distance along the pipe at which the end leaves the stock's body. + let exit = |v: f64, c: f64, half: f64, u: f64| { + if u > EPS { + (c + half - v) / u + } else if u < -EPS { + (c - half - v) / u + } else { + f64::INFINITY + } + }; + let leave = exit(p.x, center.x, half_w, ux) + .min(exit(p.y, center.y, half_h, uy)) + .max(0.0); + let step = leave + CLOUD_RADIUS + DETACHED_CLOUD_GAP; + crate::editing::Point::new(p.x + ux * step, p.y + uy * step) +} + +/// How far past a flow end, along its end segment, a cloud may be set when no +/// position along the pipe clears: the space a deleted stock took, or open +/// space off a stock the flow left. +const MAX_CLOUD_EXTENSION: f64 = 4.0 * crate::diagram::constants::CLOUD_RADIUS; + +/// Where a flow end that becomes a cloud goes when its cloud would cover +/// another shape, and how many of the pipe's points it passes: the nearest +/// position, a pixel at a time, whose cloud covers none of `shapes`, either +/// along the pipe from the end toward the valve (stopping a cloud's and a +/// valve's radius short of it, the pipe dropping the points the end passes) +/// or past the end along its segment (extending the pipe, at most +/// `MAX_CLOUD_EXTENSION`), the position along the pipe on a tie; the end +/// itself when it is clear or nothing is. `path` runs from the end inward. +/// Flows that met a deleted stock keep their ends on its faces, where clouds +/// on perpendicular faces cover each other; an imported view can draw two +/// flows leaving one face point along one line, so the end segment alone may +/// not separate them; and one can draw a parameter on the short pipe between +/// a stock and a valve, where only the space the stock took is clear. +fn clear_along_pipe( + path: &[crate::editing::Point], + valve: crate::editing::Point, + shapes: &[crate::diagram::common::Rect], +) -> (crate::editing::Point, usize) { + use crate::diagram::constants::{AUX_RADIUS, CLOUD_RADIUS}; + use crate::editing::{Point, arc_position, point_at_arc}; + /// The step, in px, at which candidate positions are tried. + const STEP: f64 = 1.0; + const EPS: f64 = 1e-9; + let covers = |q: Point| { + shapes.iter().any(|r| { + let w = r.right.min(q.x + CLOUD_RADIUS) - r.left.max(q.x - CLOUD_RADIUS); + let h = r.bottom.min(q.y + CLOUD_RADIUS) - r.top.max(q.y - CLOUD_RADIUS); + w > 0.0 && h > 0.0 + }) + }; + let Some(&end) = path.first() else { + return (Point::new(f64::NAN, f64::NAN), 0); + }; + if path.len() < 2 || !covers(end) { + return (end, 0); + } + let arcs: Vec = std::iter::once(0.0) + .chain(path.windows(2).scan(0.0, |total, w| { + *total += (w[1].x - w[0].x).hypot(w[1].y - w[0].y); + Some(*total) + })) + .collect(); + let inward_limit = arc_position(path, valve) - CLOUD_RADIUS - AUX_RADIUS; + let (dx, dy) = (end.x - path[1].x, end.y - path[1].y); + let segment = dx.hypot(dy); + let outward = (segment > EPS).then(|| (dx / segment, dy / segment)); + let reach = inward_limit.max(MAX_CLOUD_EXTENSION); + let steps = (reach / STEP).floor() as usize; + for k in 1..=steps { + let d = k as f64 * STEP; + if d <= inward_limit { + let q = point_at_arc(path, d); + if !covers(q) { + let passed = arcs[1..path.len() - 1] + .iter() + .filter(|&&a| a <= d + EPS) + .count(); + return (q, passed); + } + } + if let Some((ux, uy)) = outward + && d <= MAX_CLOUD_EXTENSION + { + let q = Point::new(end.x + ux * d, end.y + uy * d); + if !covers(q) { + return (q, 0); + } + } + } + (end, 0) +} + +/// What one end of a re-attached flow becomes. +enum RetargetedEnd { + /// Still attached where the model says. + Kept, + /// Attaches to a different stock, drawn at a center. + Stock(i32, crate::editing::Point), + /// Becomes a cloud at a point, past this many of the pipe's points, which + /// the pipe drops. + Cloud(crate::editing::Point, usize), +} + +/// Re-attach a drawn flow whose stocks changed by moving only the ends that +/// changed, through the editing core that routes a touch edit's pipes: an end +/// that becomes a cloud stays where it was (moved off a stock it no longer +/// attaches to, and along its pipe off any shape its cloud would cover) and the +/// pipe is healed; an end that attaches to another stock +/// is routed to it with the rest of the pipe kept where it stays valid; when +/// both do, the pipe is routed afresh, around every stock. The flow keeps its +/// uid, name and valve (unless a new cloud would cover the valve, which then +/// takes the pipe's middle); its label side is reset to the default for its +/// orientation, for the declutter to choose again. `expected` names the stock +/// uid each end must attach to (`None`: a cloud of its own). +/// +/// Returns false, touching nothing, when an end must attach to a stock with no +/// drawn position yet (one this pass creates): that flow is rebuilt as a new +/// flow instead. +fn retarget_flow( + state: &mut LayoutState, + flow_uid: i32, + expected: [Option; 2], + stock_centers: &BTreeMap, +) -> bool { + use crate::diagram::constants::{AUX_RADIUS, CLOUD_RADIUS}; + use crate::editing::{ + CloudRef, FlowEnd, FlowGeometry, flow_terminals, free_terminal, heal, place_valve, route, + route_end, target_stock_terminal, + }; + let Some(index) = state + .elements + .iter() + .position(|e| matches!(e, ViewElement::Flow(f) if f.uid == flow_uid)) + else { + return false; + }; + let ViewElement::Flow(flow) = state.elements[index].clone() else { + return false; + }; + let n = flow.points.len(); + if n < 2 { + return false; + } + let stocks: Vec = stock_centers.values().copied().collect(); + // Every shape but this flow's own valve and clouds, which a cloud end this + // pass creates must not cover: another flow's cloud (one an earlier + // re-attachment made included), a valve, a parameter. + let shapes: Vec = state + .elements + .iter() + .filter(|e| match e { + ViewElement::Flow(f) => f.uid != flow_uid, + ViewElement::Cloud(c) => c.flow_uid != flow_uid, + _ => true, + }) + .filter_map(crate::layout::metrics::node_shape_box) + .collect(); + + let (ends, current) = { + let by_uid: HashMap = + state.elements.iter().map(|e| (e.get_uid(), e)).collect(); + let mut ends: Vec = Vec::with_capacity(2); + for (i, point) in [0, n - 1].into_iter().enumerate() { + let attached = flow.points[point].attached_to_uid; + let own_cloud = attached.is_some_and( + |u| matches!(by_uid.get(&u), Some(ViewElement::Cloud(c)) if c.flow_uid == flow_uid), + ); + ends.push(match expected[i] { + Some(stock) if attached == Some(stock) => RetargetedEnd::Kept, + Some(stock) => match stock_centers.get(&stock) { + Some(¢er) => RetargetedEnd::Stock(stock, center), + None => return false, + }, + None if own_cloud => RetargetedEnd::Kept, + None => { + // The pipe from this end inward, its end moved off any + // stock body first. + let mut path: Vec = + flow.points.iter().map(point_of).collect(); + if i == 1 { + path.reverse(); + } + path[0] = clear_of_stocks(path[0], path[1], &stocks); + let (at, passed) = clear_along_pipe( + &path, + crate::editing::Point::new(flow.x, flow.y), + &shapes, + ); + RetargetedEnd::Cloud(at, passed) + } + }); + } + (ends, flow_terminals(&flow, |uid| by_uid.get(&uid).copied())) + }; + + let mut terminals = current; + let mut base = flow.clone(); + let mut created_clouds: Vec = Vec::new(); + for (i, end) in ends.iter().enumerate() { + let terminal = match *end { + RetargetedEnd::Kept => continue, + RetargetedEnd::Stock(uid, center) => target_stock_terminal(uid, center), + RetargetedEnd::Cloud(at, passed) => { + let uid = state.uid_manager.alloc(""); + created_clouds.push(uid); + // Both ends stop short of the valve, so the points one end + // passes are never the other's. + if i == 0 { + base.points.drain(..passed); + } else { + let len = base.points.len(); + base.points.truncate(len - passed); + } + let point = if i == 0 { 0 } else { base.points.len() - 1 }; + base.points[point].x = at.x; + base.points[point].y = at.y; + base.points[point].attached_to_uid = Some(uid); + free_terminal(at, Some(CloudRef { uid, at })) + } + }; + if i == 0 { + terminals.source = terminal; + } else { + terminals.sink = terminal; + } + } + let terminal_stocks: HashSet = [&terminals.source, &terminals.sink] + .into_iter() + .filter_map(|t| t.stock_center().and(t.uid())) + .collect(); + let occupied: Vec = state + .elements + .iter() + .filter_map(|e| match e { + ViewElement::Flow(f) if f.uid != flow_uid => Some(f), + _ => None, + }) + .flat_map(|f| [f.points.first(), f.points.last()]) + .flatten() + .filter(|p| { + p.attached_to_uid + .is_some_and(|u| terminal_stocks.contains(&u)) + }) + .map(point_of) + .collect(); + let to_stock = |end: &RetargetedEnd| matches!(end, RetargetedEnd::Stock(..)); + let FlowGeometry { + flow: mut next, + clouds: moved_clouds, + } = match (to_stock(&ends[0]), to_stock(&ends[1])) { + (false, false) => heal(&base, &terminals, stocks.as_slice()), + (true, false) => route_end( + &base, + FlowEnd::Source, + terminals.source, + terminals.sink, + &occupied, + stocks.as_slice(), + ), + (false, true) => route_end( + &base, + FlowEnd::Sink, + terminals.sink, + terminals.source, + &occupied, + stocks.as_slice(), + ), + (true, true) => route( + terminals.source, + terminals.sink, + &base, + FlowEnd::Source, + &occupied, + stocks.as_slice(), + ), + }; + let Some(last) = next.points.len().checked_sub(1).filter(|&l| l > 0) else { + return false; + }; + let end_at = |uid: i32| { + [&next.points[0], &next.points[last]] + .into_iter() + .find(|p| p.attached_to_uid == Some(uid)) + .map(point_of) + }; + let valve = crate::editing::Point::new(next.x, next.y); + let covered = created_clouds.iter().any(|&uid| { + end_at(uid) + .is_some_and(|at| (at.x - valve.x).hypot(at.y - valve.y) < CLOUD_RADIUS + AUX_RADIUS) + }); + if covered { + let path: Vec = next.points.iter().map(point_of).collect(); + let middle = place_valve(&path, FlowEnd::Source, None); + (next.x, next.y) = (middle.x, middle.y); + } + let created: Vec<(i32, crate::editing::Point)> = created_clouds + .iter() + .filter_map(|&uid| end_at(uid).map(|at| (uid, at))) + .collect(); + next.label_side = match compute_flow_orientation(&next.points) { + FlowOrientation::Horizontal => LabelSide::Top, + FlowOrientation::Vertical => LabelSide::Left, + }; + + let ends_now: HashSet = [ + next.points[0].attached_to_uid, + next.points[last].attached_to_uid, + ] + .into_iter() + .flatten() + .collect(); + let removed: HashSet = state + .elements + .iter() + .filter_map(|e| match e { + ViewElement::Cloud(c) if c.flow_uid == flow_uid && !ends_now.contains(&c.uid) => { + Some(c.uid) + } + _ => None, + }) + .collect(); + state + .elements + .retain(|e| !matches!(e, ViewElement::Cloud(c) if removed.contains(&c.uid))); + for uid in &removed { + state.positions.remove(uid); + } + for mv in &moved_clouds { + for e in &mut state.elements { + if let ViewElement::Cloud(c) = e + && c.uid == mv.uid + { + (c.x, c.y) = (mv.at.x, mv.at.y); + } + } + state + .positions + .insert(mv.uid, Position::new(mv.at.x, mv.at.y)); + } + let ident = canonicalize(&next.name).into_owned(); + if let Some(clouds) = state.flow_ident_to_clouds.get_mut(&ident) { + clouds.retain(|ci| { + state + .cloud_ident_to_uid + .get(ci) + .is_none_or(|u| !removed.contains(u)) + }); + } + for (uid, at) in &created { + state.elements.push(ViewElement::Cloud(view_element::Cloud { + uid: *uid, + flow_uid, + x: at.x, + y: at.y, + compat: None, + })); + state.positions.insert(*uid, Position::new(at.x, at.y)); + let cloud_ident = make_cloud_node_ident(*uid); + state.cloud_ident_to_uid.insert(cloud_ident.clone(), *uid); + state + .cloud_ident_to_flow_ident + .insert(cloud_ident.clone(), ident.clone()); + state + .flow_ident_to_clouds + .entry(ident.clone()) + .or_default() + .push(cloud_ident); + } + state + .positions + .insert(flow_uid, Position::new(next.x, next.y)); + record_flow_template(state, &ident, &next); + if let Some(slot) = state + .elements + .iter_mut() + .find(|e| matches!(e, ViewElement::Flow(f) if f.uid == flow_uid)) + { + *slot = ViewElement::Flow(next); + } + true +} + +/// Whether `flow`'s pipe passes through the interior of a stock that is not +/// one of its ends. +fn crosses_foreign_stock( + flow: &view_element::Flow, + stocks: &[(i32, crate::editing::Point)], +) -> bool { + use crate::diagram::constants::{STOCK_HEIGHT, STOCK_WIDTH}; + const EPS: f64 = 1e-6; + let ends: [Option; 2] = [ + flow.points.first().and_then(|p| p.attached_to_uid), + flow.points.last().and_then(|p| p.attached_to_uid), + ]; + let (hw, hh) = (STOCK_WIDTH / 2.0, STOCK_HEIGHT / 2.0); + stocks + .iter() + .filter(|(uid, _)| !ends.contains(&Some(*uid))) + .any(|(_, c)| { + flow.points.windows(2).any(|w| { + let (a, b) = (&w[0], &w[1]); + if (a.y - b.y).abs() <= EPS { + (a.y - c.y).abs() < hh - EPS + && a.x.min(b.x) < c.x + hw - EPS + && a.x.max(b.x) > c.x - hw + EPS + } else if (a.x - b.x).abs() <= EPS { + (a.x - c.x).abs() < hw - EPS + && a.y.min(b.y) < c.y + hh - EPS + && a.y.max(b.y) > c.y - hh + EPS + } else { + false + } + }) + }) +} + +/// Re-route every flow this pass created whose pipe passes through a stock +/// that is not one of its ends, through the editing core's router, which ranks +/// a path through a stock as crossing and generates detours around it. The +/// finishing pass orthogonalizes between the ends it is given and sees no +/// other stock, so a flow between two stocks with a third drawn between them +/// would otherwise run straight through the third, reading as attached to it. +/// A pipe that already clears every stock keeps what the finishing pass gave +/// it. +fn route_created_flows_around_stocks(elements: &mut [ViewElement], created: &HashSet) { + use crate::editing::{FlowEnd, FlowGeometry, Point, flow_terminals, route}; + let stocks: Vec<(i32, Point)> = elements + .iter() + .filter_map(|e| match e { + ViewElement::Stock(s) => Some((s.uid, Point::new(s.x, s.y))), + _ => None, + }) + .collect(); + let centers: Vec = stocks.iter().map(|(_, c)| *c).collect(); + let mut updates: Vec<(usize, FlowGeometry)> = Vec::new(); + { + let by_uid: HashMap = + elements.iter().map(|e| (e.get_uid(), e)).collect(); + for (index, elem) in elements.iter().enumerate() { + let ViewElement::Flow(f) = elem else { continue }; + if !created.contains(&f.uid) || f.points.len() < 2 || !crosses_foreign_stock(f, &stocks) + { + continue; + } + let terminals = flow_terminals(f, |uid| by_uid.get(&uid).copied()); + let terminal_stocks: Vec = [&terminals.source, &terminals.sink] + .into_iter() + .filter_map(|t| t.stock_center().and(t.uid())) + .collect(); + let occupied: Vec = elements + .iter() + .filter_map(|e| match e { + ViewElement::Flow(g) if g.uid != f.uid => Some(g), + _ => None, + }) + .flat_map(|g| [g.points.first(), g.points.last()]) + .flatten() + .filter(|p| { + p.attached_to_uid + .is_some_and(|u| terminal_stocks.contains(&u)) + }) + .map(point_of) + .collect(); + updates.push(( + index, + route( + terminals.source, + terminals.sink, + f, + FlowEnd::Source, + &occupied, + centers.as_slice(), + ), + )); + } + } + for (index, geometry) in updates { + for moved in &geometry.clouds { + for e in elements.iter_mut() { + if let ViewElement::Cloud(c) = e + && c.uid == moved.uid + { + (c.x, c.y) = (moved.at.x, moved.at.y); + } + } + } + elements[index] = ViewElement::Flow(geometry.flow); + } +} + +/// Slide the valve of every flow in `created` (the flows this pass created, or +/// the ones it re-attached) along its pipe to the position nearest where it +/// sits whose valve covers no other shape, when where it sits covers one: a +/// person may have parked a parameter, or drawn another valve or cloud, exactly +/// where placement puts a valve. The valve keeps +/// `VALVE_CLAMP_MARGIN` from the pipe's ends; a pipe with no clear position +/// keeps its valve. +fn keep_created_valves_clear(elements: &mut [ViewElement], created: &HashSet) { + use crate::diagram::common::Rect; + use crate::diagram::constants::AUX_RADIUS; + use crate::editing::{VALVE_CLAMP_MARGIN, arc_position, path_length, point_at_arc}; + use crate::layout::metrics::node_shape_box; + /// The step, in px of arc length, at which candidate valve positions are + /// tried. + const STEP: f64 = 1.0; + let shapes: Vec<(i32, Rect)> = elements + .iter() + .filter_map(|e| node_shape_box(e).map(|r| (e.get_uid(), r))) + .collect(); + for elem in elements.iter_mut() { + let ViewElement::Flow(f) = elem else { continue }; + if !created.contains(&f.uid) || f.points.len() < 2 { + continue; + } + let clear = |p: crate::editing::Point| { + shapes + .iter() + .filter(|(uid, _)| *uid != f.uid) + .all(|(_, r)| { + let w = r.right.min(p.x + AUX_RADIUS) - r.left.max(p.x - AUX_RADIUS); + let h = r.bottom.min(p.y + AUX_RADIUS) - r.top.max(p.y - AUX_RADIUS); + w <= 0.0 || h <= 0.0 + }) + }; + let valve = crate::editing::Point::new(f.x, f.y); + if clear(valve) { + continue; + } + let length = path_length(&f.points); + if length < 2.0 * VALVE_CLAMP_MARGIN { + continue; + } + let at = arc_position(&f.points, valve); + let steps = ((length - 2.0 * VALVE_CLAMP_MARGIN) / STEP).floor() as usize; + let best = (0..=steps) + .map(|i| VALVE_CLAMP_MARGIN + i as f64 * STEP) + .filter(|&s| clear(point_at_arc(&f.points, s))) + .min_by(|a, b| (a - at).abs().total_cmp(&(b - at).abs())); + if let Some(s) = best { + let p = point_at_arc(&f.points, s); + (f.x, f.y) = (p.x, p.y); + } + } +} + +/// Record in `state`'s caches where each flow in `flows` is now drawn -- its +/// valve position and pipe template, and its clouds' positions -- after a +/// geometry pass moved the elements themselves. Later steps read the caches: +/// the settled-coordinate copy translates every flow by the offset from its +/// cached valve position, so a flow whose valve slid with a stale cache comes +/// back to the old spot with its whole pipe, pulling the ends off their stocks +/// and clouds. +fn record_flow_geometry(state: &mut LayoutState, flows: &HashSet) { + let mut moved: Vec = Vec::new(); + for elem in &state.elements { + match elem { + ViewElement::Flow(f) if flows.contains(&f.uid) => { + state.positions.insert(f.uid, Position::new(f.x, f.y)); + moved.push(f.clone()); + } + ViewElement::Cloud(c) if flows.contains(&c.flow_uid) => { + state.positions.insert(c.uid, Position::new(c.x, c.y)); + } + _ => {} + } + } + for f in &moved { + record_flow_template(state, &canonicalize(&f.name), f); + } +} + +/// Move each created or rebuilt parameter, module or stock whose shape still +/// covers another shape to the nearest clear spot: rings a parameter's radius +/// apart around where it sits, the first clear position by ring and then by +/// angle. The declutter's relaxation pushes footprints apart but can jam in a +/// crowded region and leave a created element where it landed, and a variable +/// redrawn in place as a stock takes a larger body at its old center; nothing +/// drawn before the sync may move, so the created or rebuilt element takes the +/// clearance. `moves` accepts the uids this pass created or rebuilt; a stock a +/// flow attaches to stays, since moving it would tear the pipe. +fn keep_created_nodes_clear(elements: &mut [ViewElement], moves: impl Fn(i32) -> bool) { + use crate::diagram::common::Rect; + use crate::diagram::constants::AUX_RADIUS; + use crate::layout::metrics::node_shape_box; + const MAX_RINGS: usize = 24; + let overlaps = |a: &Rect, b: &Rect| { + a.right.min(b.right) - a.left.max(b.left) > 0.0 + && a.bottom.min(b.bottom) - a.top.max(b.top) > 0.0 + }; + for i in 0..elements.len() { + if !matches!( + elements[i], + ViewElement::Aux(_) | ViewElement::Module(_) | ViewElement::Stock(_) + ) || !moves(elements[i].get_uid()) + { + continue; + } + let uid = elements[i].get_uid(); + let attached = elements.iter().any(|e| { + matches!(e, ViewElement::Flow(f) + if f.points.iter().any(|p| p.attached_to_uid == Some(uid))) + }); + if attached { + continue; + } + let Some(shape) = node_shape_box(&elements[i]) else { + continue; + }; + let others: Vec = elements + .iter() + .enumerate() + .filter(|(j, _)| *j != i) + .filter_map(|(_, e)| node_shape_box(e)) + .collect(); + let clear = |dx: f64, dy: f64| { + let moved = Rect { + left: shape.left + dx, + right: shape.right + dx, + top: shape.top + dy, + bottom: shape.bottom + dy, + }; + !others.iter().any(|r| overlaps(&moved, r)) + }; + if clear(0.0, 0.0) { + continue; + } + let found = (1..=MAX_RINGS).find_map(|k| { + let radius = k as f64 * AUX_RADIUS; + let n = 8 * k; + (0..n) + .map(|m| { + let angle = std::f64::consts::TAU * m as f64 / n as f64; + (radius * angle.cos(), radius * angle.sin()) + }) + .find(|&(dx, dy)| clear(dx, dy)) + }); + if let Some((dx, dy)) = found { + translate_view_element(&mut elements[i], dx, dy); + } + } +} + +/// The center of a parameter, module or stock. +fn element_center(elem: &ViewElement) -> Option<(f64, f64)> { + match elem { + ViewElement::Aux(a) => Some((a.x, a.y)), + ViewElement::Module(m) => Some((m.x, m.y)), + ViewElement::Stock(s) => Some((s.x, s.y)), + _ => None, + } +} + +/// Keep the bow of every curved link the view already drew whose endpoint this +/// pass moved (a re-attached or rebuilt flow's valve, a variable redrawn in +/// place that moved off a shape): its takeoff angle turns with the chord +/// between its ends, so it curves as it did relative to that line. +fn rebow_moved_links(elements: &mut [ViewElement], old_view: &datamodel::StockFlow) { + use crate::diagram::connector::get_visual_center; + let not_arrayed = |_: &str| false; + let centers = |els: &[ViewElement]| -> HashMap { + els.iter() + .filter(|e| !matches!(e, ViewElement::Link(_) | ViewElement::Group(_))) + .map(|e| (e.get_uid(), get_visual_center(e, ¬_arrayed))) + .collect() + }; + let before = centers(&old_view.elements); + let after = centers(elements); + let drawn_before: HashSet = old_view + .elements + .iter() + .filter(|e| matches!(e, ViewElement::Link(_))) + .map(ViewElement::get_uid) + .collect(); + let chord = |a: (f64, f64), b: (f64, f64)| (b.1 - a.1).atan2(b.0 - a.0).to_degrees(); + for elem in elements.iter_mut() { + let ViewElement::Link(link) = elem else { + continue; + }; + let LinkShape::Arc(takeoff) = link.shape else { + continue; + }; + if !drawn_before.contains(&link.uid) { + continue; + } + let (Some(&f0), Some(&t0), Some(&f1), Some(&t1)) = ( + before.get(&link.from_uid), + before.get(&link.to_uid), + after.get(&link.from_uid), + after.get(&link.to_uid), + ) else { + continue; + }; + if (f0, t0) == (f1, t1) { + continue; + } + link.shape = LinkShape::Arc(takeoff + chord(f1, t1) - chord(f0, t0)); + } +} + /// Apply a model patch incrementally to an existing diagram view, /// preserving existing element positions and only placing new or /// modified elements. @@ -1484,7 +2215,9 @@ fn translate_view_element(elem: &mut ViewElement, dx: f64, dy: f64) { /// reference so callers can inspect the operations. /// /// Contract for elements the patch did not touch: position AND -/// `label_side` are returned byte-for-byte. A label side is chosen only +/// `label_side` are returned byte-for-byte. A variable the view does not +/// draw stays undrawn unless the patch names it, and no connector or cloud the +/// pass creates references it. A label side is chosen only /// for elements created in this pass -- new variables, kind-changed /// rebuilds, and flows rebuilt because their attachment changed. The /// optimizer never revisits an existing side, even when a connector added @@ -1576,12 +2309,64 @@ pub fn incremental_layout( } } + // What the edit is about, for the connectors it may draw: a dependency the + // view does not draw gets a link only into a variable the patch names (it + // upserted, renamed, or re-listed that variable), or where either end is an + // element drawn for the first time, which carries no author's choice about + // its connectors. A connector an author left out anywhere else stays out. + let named: HashSet = patch + .ops + .iter() + .filter_map(|op| { + let ident = match op { + crate::patch::ModelOperation::UpsertStock(s) => &s.ident, + crate::patch::ModelOperation::UpsertFlow(f) => &f.ident, + crate::patch::ModelOperation::UpsertAux(a) => &a.ident, + crate::patch::ModelOperation::UpsertModule(m) => &m.ident, + crate::patch::ModelOperation::RenameVariable { to, .. } => to, + crate::patch::ModelOperation::UpdateStockFlows { ident, .. } => ident, + crate::patch::ModelOperation::DeleteVariable { .. } + | crate::patch::ModelOperation::UpsertView { .. } + | crate::patch::ModelOperation::DeleteView { .. } + | crate::patch::ModelOperation::SetLoopName { .. } + | crate::patch::ModelOperation::EditView { .. } => return None, + }; + Some(canonicalize(ident).into_owned()) + }) + .collect(); + let drawn_before: HashSet = state + .elements + .iter() + .filter(|e| { + matches!( + e, + ViewElement::Stock(_) + | ViewElement::Flow(_) + | ViewElement::Aux(_) + | ViewElement::Module(_) + ) + }) + .filter_map(|e| e.get_name().map(|n| canonicalize(n).into_owned())) + .collect(); + let draws_connector = |from: &str, to: &str| { + named.contains(to) || !drawn_before.contains(from) || !drawn_before.contains(to) + }; + // Likewise a link the view draws for no dependency the model has goes only + // with an edit to its reader, and a variable the view does not draw is + // drawn only when the patch names it: an author who left a variable out of + // a view keeps it out whatever unrelated edit follows. Whatever the view + // drew stays drawn, rebuilt where its kind or attachment changed. + let keeps_connector = |to: &str| !named.contains(to); + let draws_element = |ident: &str| named.contains(ident) || drawn_before.contains(ident); + // Between steps 3 and 4a: detect variables whose type changed (e.g., Aux -> Stock). // When a caller issues UpsertStock for a variable that was previously an Aux, there // is no DeleteVariable in the patch and the old Aux element is still in state. // identify_new_elements only checks for UID presence, not element type, so the // stale element would survive. We detect type mismatches here and remove the - // old element so it is rebuilt with the correct type. + // old element so it is rebuilt with the correct type, keeping its uid (so its + // links and aliases survive) and recording its center for the rebuild. + let mut kind_changed_centers: Vec<(String, Position)> = Vec::new(); { let kind_changed: Vec = model .variables @@ -1603,27 +2388,27 @@ pub fn incremental_layout( }) .collect(); for ident in kind_changed { - // Save the display name before apply_deletion removes it from display_names, - // so the rebuilt element can recover the original casing (e.g. "Growth Rate" - // instead of "growth_rate"). - let saved_display = state.display_names.get(&ident).cloned(); - state.apply_deletion(&ident); - // Restore: use the saved original display name when available, otherwise - // fall back to the canonical ident so the entry is always present. - let display = saved_display.unwrap_or_else(|| ident.clone()); - state.display_names.insert(ident, display); + if let Some(uid) = state.uid_manager.get_uid(&ident) + && let Some(&pos) = state.positions.get(&uid) + { + kind_changed_centers.push((ident.clone(), pos)); + } + state.remove_for_rebuild(&ident); } } // Between steps 3 and 4: detect flows whose stock connections changed. // A flow element keeps its old attached_to_uid values when preserved in state, // so a flow that moved from one stock to another would keep stale endpoints. - // Remove such flows (and their clouds) so identify_new_elements picks them - // up as new and they get rebuilt with correct endpoints. + // Such a flow is re-attached in place (`retarget_flow`): only its changed + // ends move. When an end must attach to a stock that has no position yet, the + // flow is removed instead, so identify_new_elements picks it up as new and it + // is rebuilt with correct endpoints. // // This also handles transitions between stock and cloud endpoints: if the // model now expects a cloud source (from_stock == None) but the preserved // flow's source point is still attached to a stock UID, the flow is stale. + let mut retargeted: HashSet = HashSet::new(); { let uid_to_ident: HashMap = model .variables @@ -1699,23 +2484,59 @@ pub fn incremental_layout( }) .collect(); + // The stocks a re-attached pipe routes around and whose bodies a cloud + // end stays out of: every drawn stock, plus the stocks a kind change + // redraws in place below. + let stock_centers: BTreeMap = state + .elements + .iter() + .filter_map(|e| match e { + ViewElement::Stock(s) => Some((s.uid, crate::editing::Point::new(s.x, s.y))), + _ => None, + }) + .chain(kind_changed_centers.iter().filter_map(|(ident, pos)| { + matches!( + model.get_variable(ident), + Some(datamodel::Variable::Stock(_)) + ) + .then_some(())?; + let uid = state.uid_manager.get_uid(ident)?; + Some((uid, crate::editing::Point::new(pos.x, pos.y))) + })) + .collect(); for flow_ident in flows_to_reset { - // apply_deletion removes the element from state.elements but leaves - // the UID in uid_manager. identify_new_elements will see a UID with - // no corresponding element and classify the flow as new, causing - // create_flow_view_element to rebuild it with correct endpoints. - let canonical = canonicalize(&flow_ident).into_owned(); - // Save the display name before apply_deletion removes it so the - // rebuilt element recovers the original casing. - let saved_display = state.display_names.get(&canonical).cloned(); - state.apply_deletion(&flow_ident); - let display = saved_display.unwrap_or_else(|| flow_ident.clone()); - state.display_names.insert(canonical, display); + let uid = state.uid_manager.get_uid(&flow_ident); + let expected = metadata + .flow_to_stocks + .get(&flow_ident) + .and_then(|(from, to)| { + let stock_uid = |s: &Option| match s { + None => Some(None), + Some(ident) => state.uid_manager.get_uid(ident).map(Some), + }; + Some([stock_uid(from)?, stock_uid(to)?]) + }); + if let (Some(uid), Some(expected)) = (uid, expected) + && retarget_flow(&mut state, uid, expected, &stock_centers) + { + retargeted.insert(uid); + continue; + } + // identify_new_elements sees the uid with no element and classifies + // the flow as new, so create_flow_view_element rebuilds it with + // correct endpoints under the same uid, and the links into it + // survive. + state.remove_for_rebuild(&flow_ident); } } + // A re-attached flow keeps its valve unless a new cloud covered it, when + // the valve took the pipe's middle, which may be where a person parked + // something. + keep_created_valves_clear(&mut state.elements, &retargeted); + record_flow_geometry(&mut state, &retargeted); // Step 4: Identify new elements and compute initial positions - let new_elements = state.identify_new_elements(model); + let new_elements = state.identify_new_elements(model).filtered(draws_element); // The face each new flow attaches on, from its stocks' current flow lists // and the faces their drawn side flows already take. Only new flows read @@ -1735,11 +2556,12 @@ pub fn incremental_layout( } // Every element still standing at this point survived the patch untouched - // (or was merely renamed): it keeps its position, and a named one its label - // side, for the rest of the pass. Whatever gets created from here on -- new - // variables, kind-changed rebuilds, flows rebuilt because their attachment - // changed -- is absent from this snapshot, so `declutter_part` below chooses - // its side and may move it. + // (or was merely renamed): it keeps its position, and a labelled one its + // label side, for the rest of the pass. Whatever gets created from here on + // -- new variables, kind-changed rebuilds, flows rebuilt because their + // attachment changed -- is absent from this snapshot, so `declutter_part` + // below chooses its side and may move it. An alias is never created or + // rebuilt here, so every alias keeps its side. let standing_uids: HashSet = state.elements.iter().map(ViewElement::get_uid).collect(); let pinned_labels: HashSet = state .elements @@ -1751,20 +2573,92 @@ pub fn incremental_layout( | ViewElement::Flow(_) | ViewElement::Aux(_) | ViewElement::Module(_) - ) + | ViewElement::Alias(_) + ) && !retargeted.contains(&elem.get_uid()) }) .map(ViewElement::get_uid) .collect(); let needs_label_placement = |uid: i32| !pinned_labels.contains(&uid); + // A link this pass creates has a uid the view before it did not use. + let old_uids: HashSet = old_view.elements.iter().map(ViewElement::get_uid).collect(); + let created_link = |uid: i32| !old_uids.contains(&uid); + + // A variable whose kind changed to a stock, a parameter or a module is + // redrawn at its old element's center: an agent turning a parameter into a + // stock changed what it is, not where a person put it. Its label side is + // chosen afresh (it is absent from `pinned_labels`), and no pass below moves + // it, unless its new shape covers another shape at that center + // (`keep_created_nodes_clear`). A variable that became a flow is placed as a new flow, since its valve + // belongs on the pipe between its stocks. + let mut rebuilt_in_place: HashSet = HashSet::new(); + let mut in_place_idents: HashSet = HashSet::new(); + for (ident, pos) in &kind_changed_centers { + let Some(var) = model.get_variable(ident) else { + continue; + }; + let uid = state.get_or_alloc_uid(ident); + let name = format_label_with_line_breaks(&state.display_name(ident)); + let (x, y) = (pos.x, pos.y); + let element = match var { + datamodel::Variable::Stock(_) => ViewElement::Stock(view_element::Stock { + name, + uid, + x, + y, + label_side: LabelSide::Bottom, + compat: None, + }), + datamodel::Variable::Aux(_) => ViewElement::Aux(view_element::Aux { + name, + uid, + x, + y, + label_side: LabelSide::Bottom, + compat: None, + }), + datamodel::Variable::Module(_) => ViewElement::Module(view_element::Module { + name, + uid, + x, + y, + label_side: LabelSide::Bottom, + }), + datamodel::Variable::Flow(_) => continue, + }; + state.elements.push(element); + state.positions.insert(uid, *pos); + rebuilt_in_place.insert(uid); + in_place_idents.insert(ident.clone()); + } + let new_elements = NewElements { + new_stocks: without(new_elements.new_stocks, &in_place_idents), + new_flows: new_elements.new_flows, + new_auxes: without(new_elements.new_auxes, &in_place_idents), + new_modules: without(new_elements.new_modules, &in_place_idents), + }; + // What the polish passes may move: what this pass created, less what it + // redrew in place. + let moves = |uid: i32| !standing_uids.contains(&uid) && !rebuilt_in_place.contains(&uid); if new_elements.is_empty() { - // No new element, so no flow is created or rebuilt: every flow in the - // view is untouched, and none of the flow geometry passes runs. - diff_connectors(&mut state, &metadata); + // No new element, so no flow is created: every flow in the view is + // untouched or was re-attached above, and none of the created-flow + // geometry passes runs. A re-attached valve or a variable redrawn in + // place may still have moved, so the curved links into them turn. + diff_connectors(&mut state, &metadata, draws_connector, keeps_connector); diff_clouds(&mut state, &metadata); declutter::declutter_part(&mut state.elements, needs_label_placement, |_| false); - apply_loop_curvature(&mut state, &config, model, &metadata); - validate_view_completeness(&state, model)?; + keep_created_nodes_clear(&mut state.elements, |uid| rebuilt_in_place.contains(&uid)); + for elem in &state.elements { + if rebuilt_in_place.contains(&elem.get_uid()) + && let Some((x, y)) = element_center(elem) + { + state.positions.insert(elem.get_uid(), Position::new(x, y)); + } + } + rebow_moved_links(&mut state.elements, old_view); + apply_loop_curvature(&mut state, &config, model, &metadata, created_link); + validate_view_completeness(&state, model, draws_element)?; return Ok(build_stock_flow_from_state(state, old_view)); } @@ -1956,9 +2850,12 @@ pub fn incremental_layout( resnap_flow_endpoints(&mut state, &config, is_created); face_slots::place_created_flow_ends(&mut state.elements, &created_flows); finish_flow_geometry(&mut state.elements, is_created); + route_created_flows_around_stocks(&mut state.elements, &created_flows); + keep_created_valves_clear(&mut state.elements, &created_flows); + record_flow_geometry(&mut state, &created_flows); // Step 7: Diff connectors and clouds - diff_connectors(&mut state, &metadata); + diff_connectors(&mut state, &metadata, draws_connector, keeps_connector); diff_clouds(&mut state, &metadata); // Step 8: Polish. The new free-floating elements step off crossings and @@ -1966,9 +2863,10 @@ pub fn incremental_layout( // label sides chosen by what the metric charges. Pinned elements keep their positions and // sides even if a new connector now runs through a label (hand placement // wins; the human can move it). - polish::polish_crossings_for(&mut state.elements, |uid| !standing_uids.contains(&uid)); - declutter::declutter_part(&mut state.elements, needs_label_placement, |uid| { - !standing_uids.contains(&uid) + polish::polish_crossings_for(&mut state.elements, moves); + declutter::declutter_part(&mut state.elements, needs_label_placement, moves); + keep_created_nodes_clear(&mut state.elements, |uid| { + moves(uid) || rebuilt_in_place.contains(&uid) }); // The decluttered free-floating elements' positions, for the loop arcs. for elem in &state.elements { @@ -1976,15 +2874,17 @@ pub fn incremental_layout( ViewElement::Aux(a) => (a.uid, a.x, a.y), ViewElement::Module(m) => (m.uid, m.x, m.y), ViewElement::Alias(a) => (a.uid, a.x, a.y), + ViewElement::Stock(s) if rebuilt_in_place.contains(&s.uid) => (s.uid, s.x, s.y), _ => continue, }; if let Some(pos) = state.positions.get_mut(&uid) { *pos = Position::new(x, y); } } - apply_loop_curvature(&mut state, &config, model, &metadata); + rebow_moved_links(&mut state.elements, old_view); + apply_loop_curvature(&mut state, &config, model, &metadata, created_link); - validate_view_completeness(&state, model)?; + validate_view_completeness(&state, model, draws_element)?; // Step 9: Build StockFlow Ok(build_stock_flow_from_state(state, old_view)) diff --git a/src/simlin-engine/src/layout/incremental_tests.rs b/src/simlin-engine/src/layout/incremental_tests.rs index 4f76d8c09..1120aa253 100644 --- a/src/simlin-engine/src/layout/incremental_tests.rs +++ b/src/simlin-engine/src/layout/incremental_tests.rs @@ -112,6 +112,648 @@ fn geometry(view: &datamodel::StockFlow) -> HashMap .collect() } +fn default_project(name: &str) -> datamodel::Project { + let path = format!( + "{}/../../default_projects/{name}/model.xmile", + env!("CARGO_MANIFEST_DIR") + ); + let file = std::fs::File::open(&path).unwrap_or_else(|e| panic!("{path}: {e}")); + crate::compat::open_xmile(&mut std::io::BufReader::new(file)).expect("model imports") +} + +fn shipped_view(project: &datamodel::Project) -> datamodel::StockFlow { + match project.get_model(TEST_MODEL).and_then(|m| m.views.first()) { + Some(datamodel::View::StockFlow(sf)) => sf.clone(), + None => panic!("the project ships no view"), + } +} + +#[test] +fn an_edit_that_changes_no_structure_returns_the_view_byte_for_byte() { + // An agent restates a flow exactly as it is. Nothing about the diagram + // changed, so the view comes back as it was -- element order included, + // since the order is the draw order and what the saved file lists. + // Fishbanks' hand-drawn view has several links and clouds, so a sync that + // re-lists connectors or clouds shows up. + let project = default_project("fishbanks"); + let view = shipped_view(&project); + let harvest = project + .get_model(TEST_MODEL) + .and_then(|m| m.get_variable("harvest_rate")) + .cloned() + .expect("harvest_rate"); + let datamodel::Variable::Flow(harvest) = harvest else { + panic!("harvest_rate is a flow"); + }; + let (_, synced) = sync(&project, &view, vec![ModelOperation::UpsertFlow(harvest)]); + let order = + |v: &datamodel::StockFlow| v.elements.iter().map(|e| e.get_uid()).collect::>(); + assert_eq!(order(&synced), order(&view), "element order"); + assert!(synced == view, "the restated view must equal the original"); +} + +#[test] +fn syncing_one_edit_twice_produces_one_view() { + // One edit creates three side flows, each ending at a cloud, plus the + // links their rates read. Every sync of it must list the created clouds + // and links in the same order, however the sync's maps hash. + let project = project_with(vec![ + datamodel::Variable::Stock(stock("population", &["births"], &[])), + datamodel::Variable::Flow(flow("births", "population * 0.03")), + ]); + let base = generate_layout(&project, TEST_MODEL, None).expect("base layout"); + let ops = vec![ + ModelOperation::UpsertStock(stock( + "population", + &["births"], + &["deaths", "emigration", "retirement"], + )), + ModelOperation::UpsertFlow(flow("deaths", "population * 0.01")), + ModelOperation::UpsertFlow(flow("emigration", "population * 0.02")), + ModelOperation::UpsertFlow(flow("retirement", "population * 0.005")), + ]; + let (_, first) = sync(&project, &base, ops.clone()); + for _ in 0..12 { + let (_, again) = sync(&project, &base, ops.clone()); + assert!( + again == first, + "a second sync of one edit must equal the first" + ); + } +} + +/// The shape of the link drawn from the element named `from` to the one named +/// `to`, if one is drawn. +fn link_shape(view: &datamodel::StockFlow, from: &str, to: &str) -> Option { + let uid = |name: &str| { + view.elements.iter().find_map(|e| { + let n = e.get_name()?; + (canonicalize(n) == name).then(|| e.get_uid()) + }) + }; + let (f, t) = (uid(from)?, uid(to)?); + view.elements.iter().find_map(|e| match e { + ViewElement::Link(l) if l.from_uid == f && l.to_uid == t => Some(l.shape.clone()), + _ => None, + }) +} + +#[test] +fn only_the_links_a_sync_creates_are_curved_for_a_loop() { + // An agent makes birth_rate read population, closing the loop population + // -> birth_rate -> births -> population. The diagram's links were drawn + // straight by hand. The link the edit creates is curved as a loop link, + // and the two links it did not create keep the shapes a person chose. + let project = project_with(vec![ + datamodel::Variable::Stock(stock("population", &["births"], &[])), + datamodel::Variable::Flow(flow("births", "population * birth_rate")), + datamodel::Variable::Aux(aux("birth_rate", "0.1")), + ]); + let mut base = generate_layout(&project, TEST_MODEL, None).expect("base layout"); + for e in &mut base.elements { + if let ViewElement::Link(l) = e { + l.shape = LinkShape::Straight; + } + } + let (_, view) = sync( + &project, + &base, + vec![ModelOperation::UpsertAux(aux( + "birth_rate", + "0.1 * (1 - population / 1000)", + ))], + ); + assert!( + matches!( + link_shape(&view, "population", "birth_rate"), + Some(LinkShape::Arc(_)) + ), + "the link the edit created on the loop is curved" + ); + for (from, to) in [("birth_rate", "births"), ("population", "births")] { + assert!( + matches!(link_shape(&view, from, to), Some(LinkShape::Straight)), + "the link {from} -> {to} the edit did not create keeps its straight shape" + ); + } +} + +/// The link drawn from the element named `from` to the one named `to`. +fn link_between(view: &datamodel::StockFlow, from: &str, to: &str) -> Option { + let uid = |name: &str| { + view.elements.iter().find_map(|e| { + let n = e.get_name()?; + (canonicalize(n) == name).then(|| e.get_uid()) + }) + }; + let (f, t) = (uid(from)?, uid(to)?); + view.elements.iter().find_map(|e| match e { + ViewElement::Link(l) if l.from_uid == f && l.to_uid == t => Some(l.clone()), + _ => None, + }) +} + +fn center_named(view: &datamodel::StockFlow, name: &str) -> Option<(f64, f64)> { + view.elements.iter().find_map(|e| match e { + ViewElement::Aux(a) if canonicalize(&a.name) == name => Some((a.x, a.y)), + ViewElement::Stock(s) if canonicalize(&s.name) == name => Some((s.x, s.y)), + ViewElement::Module(m) if canonicalize(&m.name) == name => Some((m.x, m.y)), + _ => None, + }) +} + +#[test] +fn a_variable_whose_kind_changes_is_redrawn_where_it_was() { + // An agent turns the parameter birth_rate into a stock with an upsert. The + // stock is drawn where the parameter was, and the link from it into + // births -- still a dependency -- is the link a person drew. + let project = project_with(vec![ + datamodel::Variable::Stock(stock("population", &["births"], &[])), + datamodel::Variable::Flow(flow("births", "population * birth_rate")), + datamodel::Variable::Aux(aux("birth_rate", "0.1")), + ]); + let base = generate_layout(&project, TEST_MODEL, None).expect("base layout"); + let (_, view) = sync( + &project, + &base, + vec![ModelOperation::UpsertStock(stock("birth_rate", &[], &[]))], + ); + assert!( + view.elements + .iter() + .any(|e| matches!(e, ViewElement::Stock(s) if canonicalize(&s.name) == "birth_rate")), + "birth_rate is drawn as a stock" + ); + assert_eq!( + center_named(&view, "birth_rate"), + center_named(&base, "birth_rate"), + "the stock is drawn where the parameter was" + ); + assert_eq!( + link_between(&view, "birth_rate", "births"), + link_between(&base, "birth_rate", "births"), + "the link keeps its uid and shape" + ); +} + +#[test] +fn a_flow_rebuilt_for_a_new_attachment_keeps_its_links() { + // transfer drains source into sink, at a rate read from source and rate. + // Deleting sink rebuilds transfer with a cloud end; the links into it are + // still dependencies, so they keep their uids and their kinds of shape. + let project = project_with(vec![ + datamodel::Variable::Stock(stock("source", &[], &["transfer"])), + datamodel::Variable::Stock(stock("sink", &["transfer"], &[])), + datamodel::Variable::Flow(flow("transfer", "source * rate")), + datamodel::Variable::Aux(aux("rate", "0.1")), + ]); + let base = generate_layout(&project, TEST_MODEL, None).expect("base layout"); + let (_, view) = sync( + &project, + &base, + vec![ModelOperation::DeleteVariable { + ident: "sink".to_string(), + }], + ); + for from in ["rate", "source"] { + let before = link_between(&base, from, "transfer").expect("drawn before"); + let after = link_between(&view, from, "transfer").expect("drawn after"); + assert_eq!(after.uid, before.uid, "{from} -> transfer keeps its uid"); + assert_eq!( + std::mem::discriminant(&after.shape), + std::mem::discriminant(&before.shape), + "{from} -> transfer keeps its kind of shape" + ); + } +} + +fn flow_named(view: &datamodel::StockFlow, name: &str) -> view_element::Flow { + view.elements + .iter() + .find_map(|e| match e { + ViewElement::Flow(f) if canonicalize(&f.name) == name => Some(f.clone()), + _ => None, + }) + .unwrap_or_else(|| panic!("{name} drawn")) +} + +/// Every pair of shapes that overlap, where one of them is in `uids`. +fn overlaps_involving(view: &datamodel::StockFlow, uids: &HashSet) -> Vec<(i32, i32)> { + use crate::layout::metrics::node_shape_box; + let shapes: Vec<(i32, crate::diagram::common::Rect)> = view + .elements + .iter() + .filter_map(|e| node_shape_box(e).map(|r| (e.get_uid(), r))) + .collect(); + let mut out = Vec::new(); + for (i, (a, ra)) in shapes.iter().enumerate() { + for (b, rb) in &shapes[i + 1..] { + if !uids.contains(a) && !uids.contains(b) { + continue; + } + let w = ra.right.min(rb.right) - ra.left.max(rb.left); + let h = ra.bottom.min(rb.bottom) - ra.top.max(rb.top); + if w > 0.5 && h > 0.5 { + out.push((*a, *b)); + } + } + } + out +} + +/// The strict flow invariant violations of the flows in `uids`. +fn strict_violations(view: &datamodel::StockFlow, uids: &HashSet) -> String { + use crate::editing::invariants::{Mode, check_flow_invariants, format_violations}; + format_violations(&check_flow_invariants( + &view.elements, + Mode::Strict { routed: Some(uids) }, + )) +} + +#[test] +fn a_detached_flow_end_becomes_a_cloud_clear_of_the_stock() { + // source drains into sink through transfer. An agent restates source + // without transfer in its outflows, so transfer's source end is now a + // cloud. The pipe keeps its line, the cloud sits outside source, and what + // the edit changed covers no shape. + let project = project_with(vec![ + datamodel::Variable::Stock(stock("source", &[], &["transfer"])), + datamodel::Variable::Stock(stock("sink", &["transfer"], &[])), + datamodel::Variable::Flow(flow("transfer", "10")), + ]); + let base = generate_layout(&project, TEST_MODEL, None).expect("base layout"); + let (_, view) = sync( + &project, + &base, + vec![ModelOperation::UpsertStock(stock("source", &[], &[]))], + ); + let before = flow_named(&base, "transfer"); + let after = flow_named(&view, "transfer"); + let source_end = after.points[0].attached_to_uid.expect("attached"); + assert!( + view.elements.iter().any( + |e| matches!(e, ViewElement::Cloud(c) if c.uid == source_end && c.flow_uid == after.uid) + ), + "the source end is a cloud of transfer's own" + ); + assert_eq!( + after.points.last().map(|p| p.attached_to_uid), + before.points.last().map(|p| p.attached_to_uid), + "the sink end still attaches to sink" + ); + let line = before.points[0].y; + assert!( + after.points.iter().all(|p| (p.y - line).abs() < 1e-9), + "the pipe keeps its line: {:?}", + after.points + ); + let changed: HashSet = [after.uid, source_end].into_iter().collect(); + assert_eq!( + overlaps_involving(&view, &changed), + Vec::<(i32, i32)>::new() + ); + assert_eq!( + strict_violations(&view, &[after.uid].into_iter().collect()), + "" + ); +} + +#[test] +fn deleting_a_middle_stock_leaves_the_flows_through_it_in_place() { + // upstream -> inflow -> middle -> outflow -> downstream. Deleting middle + // turns the ends of inflow and outflow that touched it into clouds. The + // two pipes stay where they were drawn, and nothing the edit changed covers + // a shape. + let project = project_with(vec![ + datamodel::Variable::Stock(stock("upstream", &[], &["inflow"])), + datamodel::Variable::Stock(stock("middle", &["inflow"], &["outflow"])), + datamodel::Variable::Stock(stock("downstream", &["outflow"], &[])), + datamodel::Variable::Flow(flow("inflow", "10")), + datamodel::Variable::Flow(flow("outflow", "10")), + ]); + let base = generate_layout(&project, TEST_MODEL, None).expect("base layout"); + let (_, view) = sync( + &project, + &base, + vec![ModelOperation::DeleteVariable { + ident: "middle".to_string(), + }], + ); + let points = |f: &view_element::Flow| f.points.iter().map(|p| (p.x, p.y)).collect::>(); + let mut changed: HashSet = HashSet::new(); + for name in ["inflow", "outflow"] { + let (before, after) = (flow_named(&base, name), flow_named(&view, name)); + assert_eq!(points(&after), points(&before), "{name}'s pipe stays put"); + changed.insert(after.uid); + changed.extend(after.points.iter().filter_map(|p| p.attached_to_uid)); + } + assert_eq!( + overlaps_involving(&view, &changed), + Vec::<(i32, i32)>::new() + ); + let flows: HashSet = ["inflow", "outflow"] + .iter() + .map(|n| flow_named(&view, n).uid) + .collect(); + assert_eq!(strict_violations(&view, &flows), ""); +} + +/// The stocks whose interior a flow's pipe passes through, other than its own +/// ends. +fn stocks_crossed(view: &datamodel::StockFlow, flow: &view_element::Flow) -> Vec { + use crate::diagram::constants::{STOCK_HEIGHT, STOCK_WIDTH}; + let ends: HashSet = [flow.points.first(), flow.points.last()] + .into_iter() + .flatten() + .filter_map(|p| p.attached_to_uid) + .collect(); + let mut out = Vec::new(); + for e in &view.elements { + let ViewElement::Stock(s) = e else { continue }; + if ends.contains(&s.uid) { + continue; + } + let (hw, hh) = (STOCK_WIDTH / 2.0 - 0.5, STOCK_HEIGHT / 2.0 - 0.5); + let crosses = flow.points.windows(2).any(|w| { + // Pipes are orthogonal: a segment enters the interior when it runs + // within the body's span on its own axis and overlaps it on the + // other. + let (a, b) = (&w[0], &w[1]); + if (a.y - b.y).abs() < 1e-9 { + (a.y - s.y).abs() < hh && a.x.min(b.x) < s.x + hw && a.x.max(b.x) > s.x - hw + } else { + (a.x - s.x).abs() < hw && a.y.min(b.y) < s.y + hh && a.y.max(b.y) > s.y - hh + } + }); + if crosses { + out.push(canonicalize(&s.name).into_owned()); + } + } + out +} + +#[test] +fn a_flow_created_between_two_stocks_routes_around_the_stocks_between() { + // upstream -> inflow -> middle -> outflow -> downstream, drawn in a row. An + // agent adds a bypass from upstream straight to downstream. Its pipe goes + // around middle rather than through it, holds the flow invariants, and + // covers no shape. + let project = project_with(vec![ + datamodel::Variable::Stock(stock("upstream", &[], &["inflow"])), + datamodel::Variable::Stock(stock("middle", &["inflow"], &["outflow"])), + datamodel::Variable::Stock(stock("downstream", &["outflow"], &[])), + datamodel::Variable::Flow(flow("inflow", "10")), + datamodel::Variable::Flow(flow("outflow", "10")), + ]); + let base = generate_layout(&project, TEST_MODEL, None).expect("base layout"); + let (_, view) = sync( + &project, + &base, + vec![ + ModelOperation::UpsertFlow(flow("bypass", "1")), + ModelOperation::UpsertStock(stock("upstream", &[], &["inflow", "bypass"])), + ModelOperation::UpsertStock(stock("downstream", &["outflow", "bypass"], &[])), + ], + ); + let bypass = flow_named(&view, "bypass"); + assert_eq!(stocks_crossed(&view, &bypass), Vec::::new()); + let uids: HashSet = [bypass.uid].into_iter().collect(); + assert_eq!(strict_violations(&view, &uids), ""); + assert_eq!(overlaps_involving(&view, &uids), Vec::<(i32, i32)>::new()); +} + +#[test] +fn a_created_valve_lands_clear_of_a_parameter() { + // tank has a parameter, leak_rate, that a person parked just right of it, + // where a side flow leaving tank's right face puts its valve. An agent adds + // drain, a flow out of tank at leak_rate: its valve must not land on the + // parameter. + let project = project_with(vec![ + datamodel::Variable::Stock(stock("tank", &[], &[])), + datamodel::Variable::Aux(aux("leak_rate", "0.1")), + ]); + let mut base = generate_layout(&project, TEST_MODEL, None).expect("base layout"); + let config = LayoutConfig::default(); + let (tx, ty) = center_named(&base, "tank").expect("tank drawn"); + for e in &mut base.elements { + if let ViewElement::Aux(a) = e { + (a.x, a.y) = ( + tx + config.stock_width / 2.0 + config.horizontal_spacing / 2.0, + ty, + ); + } + } + let (_, view) = sync( + &project, + &base, + vec![ + ModelOperation::UpsertFlow(flow("drain", "tank * leak_rate")), + ModelOperation::UpsertStock(stock("tank", &[], &["drain"])), + ], + ); + let drain = flow_named(&view, "drain"); + let uids: HashSet = [drain.uid].into_iter().collect(); + assert_eq!(overlaps_involving(&view, &uids), Vec::<(i32, i32)>::new()); + assert_eq!(strict_violations(&view, &uids), ""); +} + +#[test] +fn a_sync_draws_a_missing_connector_only_where_the_edit_is_about_it() { + // population grows by births at birth_rate; doubled reads birth_rate and + // quad reads doubled. The author's view draws neither births' connectors + // nor doubled at all. Every arm of what an edit may draw: + // - an unrelated edit (a new note): births' connectors stay out, and so + // does doubled, a variable the author left undrawn; + // - an edit naming births (restating it): its connectors are drawn; + // - an edit naming doubled (restating it): doubled is drawn with the + // connector into it, and the connector from it to quad too, although + // quad is not named, since an element drawn for the first time carries + // no author's choice about its connectors. + let project = project_with(vec![ + datamodel::Variable::Stock(stock("population", &["births"], &[])), + datamodel::Variable::Flow(flow("births", "population * birth_rate")), + datamodel::Variable::Aux(aux("birth_rate", "0.1")), + datamodel::Variable::Aux(aux("doubled", "birth_rate * 2")), + datamodel::Variable::Aux(aux("quad", "doubled * 2")), + ]); + let mut base = generate_layout(&project, TEST_MODEL, None).expect("base layout"); + let doubled = base + .elements + .iter() + .find(|e| e.get_name().is_some_and(|n| canonicalize(n) == "doubled")) + .map(ViewElement::get_uid) + .expect("doubled drawn"); + let births = flow_named(&base, "births").uid; + base.elements.retain(|e| match e { + ViewElement::Link(l) => l.to_uid != births && l.to_uid != doubled && l.from_uid != doubled, + other => other.get_uid() != doubled, + }); + let drawn = |view: &datamodel::StockFlow, name: &str| { + view.elements + .iter() + .any(|e| e.get_name().is_some_and(|n| canonicalize(n) == name)) + }; + + let (_, unrelated) = sync( + &project, + &base, + vec![ModelOperation::UpsertAux(aux("note", "1"))], + ); + assert!(link_between(&unrelated, "birth_rate", "births").is_none()); + assert!(link_between(&unrelated, "population", "births").is_none()); + assert!( + !drawn(&unrelated, "doubled"), + "a variable the author left undrawn stays undrawn" + ); + + let (_, restated) = sync( + &project, + &base, + vec![ModelOperation::UpsertFlow(flow( + "births", + "population * birth_rate", + ))], + ); + assert!(link_between(&restated, "birth_rate", "births").is_some()); + assert!(link_between(&restated, "population", "births").is_some()); + assert!(!drawn(&restated, "doubled")); + + let (_, named) = sync( + &project, + &base, + vec![ModelOperation::UpsertAux(aux("doubled", "birth_rate * 2"))], + ); + assert!( + drawn(&named, "doubled"), + "a variable the patch names is drawn" + ); + assert!(link_between(&named, "birth_rate", "doubled").is_some()); + assert!( + link_between(&named, "doubled", "quad").is_some(), + "doubled is drawn for the first time, with its connector to quad" + ); +} + +#[test] +fn a_link_drawing_no_dependency_goes_only_with_an_edit_to_its_reader() { + // An imported view draws note_rate -> births, which no equation explains + // (a module port, an input the extraction does not see, an annotation). + // Every arm of whether a sync keeps it: + // - an unrelated edit (a new note): kept, byte for byte; + // - an edit to the link's source (restating note_rate): kept; + // - an edit to its reader (restating births): dropped, since the reader's + // equation is what says which connectors into it are drawn. + const EXTRA: i32 = 90_000; + let project = project_with(vec![ + datamodel::Variable::Stock(stock("population", &["births"], &[])), + datamodel::Variable::Flow(flow("births", "population * birth_rate")), + datamodel::Variable::Aux(aux("birth_rate", "0.1")), + datamodel::Variable::Aux(aux("note_rate", "0.5")), + ]); + let mut base = generate_layout(&project, TEST_MODEL, None).expect("base layout"); + let note_rate = base + .elements + .iter() + .find(|e| e.get_name().is_some_and(|n| canonicalize(n) == "note_rate")) + .map(ViewElement::get_uid) + .expect("note_rate drawn"); + let births = flow_named(&base, "births").uid; + let link = ViewElement::Link(view_element::Link { + uid: EXTRA, + from_uid: note_rate, + to_uid: births, + shape: LinkShape::Arc(30.0), + polarity: None, + }); + base.elements.push(link.clone()); + let kept = + |view: &datamodel::StockFlow| view.elements.iter().find(|e| e.get_uid() == EXTRA).cloned(); + + let (_, unrelated) = sync( + &project, + &base, + vec![ModelOperation::UpsertAux(aux("note", "1"))], + ); + assert_eq!( + kept(&unrelated), + Some(link.clone()), + "an unrelated edit keeps it" + ); + + let (_, source) = sync( + &project, + &base, + vec![ModelOperation::UpsertAux(aux("note_rate", "0.6"))], + ); + assert_eq!(kept(&source), Some(link), "an edit to its source keeps it"); + + let (_, reader) = sync( + &project, + &base, + vec![ModelOperation::UpsertFlow(flow( + "births", + "population * birth_rate", + ))], + ); + assert_eq!(kept(&reader), None, "an edit to its reader drops it"); +} + +#[test] +fn a_stock_added_to_a_drawn_chain_lands_clear_of_side_flows() { + // tank drains to a cloud off its right face, and a person drew the drain + // pipe long enough that its cloud sits where a downstream stock would + // naturally go. An agent adds a reservoir fed from tank: it must not land + // on the drain's cloud. + use crate::layout::metrics::node_shape_box; + let project = project_with(vec![ + datamodel::Variable::Stock(stock("tank", &[], &["drain"])), + datamodel::Variable::Flow(flow("drain", "tank * 0.1")), + ]); + let mut base = generate_layout(&project, TEST_MODEL, None).expect("base layout"); + let drain_cloud = base.elements.iter().find_map(|e| match e { + ViewElement::Flow(f) if canonicalize(&f.name) == "drain" => { + f.points.last()?.attached_to_uid + } + _ => None, + }); + for e in &mut base.elements { + match e { + ViewElement::Flow(f) if canonicalize(&f.name) == "drain" => { + f.points.last_mut().expect("points").x += 40.0; + } + ViewElement::Cloud(c) if Some(c.uid) == drain_cloud => c.x += 40.0, + _ => {} + } + } + let ops = vec![ + ModelOperation::UpsertStock(stock("tank", &[], &["drain", "transfer"])), + ModelOperation::UpsertFlow(flow("transfer", "tank * 0.2")), + ModelOperation::UpsertStock(stock("reservoir", &["transfer"], &[])), + ]; + let (_, view) = sync(&project, &base, ops); + let reservoir = view + .elements + .iter() + .find(|e| e.get_name().is_some_and(|n| canonicalize(n) == "reservoir")) + .expect("reservoir drawn"); + let r = node_shape_box(reservoir).expect("a stock has a shape"); + let base_uids: HashSet = base.elements.iter().map(ViewElement::get_uid).collect(); + for e in view + .elements + .iter() + .filter(|e| base_uids.contains(&e.get_uid())) + { + let Some(o) = node_shape_box(e) else { continue }; + let w = r.right.min(o.right) - r.left.max(o.left); + let h = r.bottom.min(o.bottom) - r.top.max(o.top); + assert!( + w <= 0.0 || h <= 0.0, + "reservoir covers #{} ({w:.1} x {h:.1})", + e.get_uid() + ); + } +} + #[test] fn new_parameters_are_decluttered_around_the_fixed_diagram() { // Six new parameters that all feed one existing flow are seeded in a tight @@ -252,3 +894,657 @@ fn a_stock_added_to_a_drawn_chain_continues_its_row() { assert_eq!(after.get(uid), Some(g), "element {uid} must stay put"); } } + +#[test] +fn deleting_a_variable_removes_the_links_of_its_aliases() { + // An imported view draws birth_rate a second time, as an alias beside + // births, with the connector from the alias. Deleting birth_rate (and + // nothing else, so births is not an edit the link's reader is named by) + // removes the alias and every link touching it, so nothing references a + // uid the view no longer draws. + let project = project_with(vec![ + datamodel::Variable::Stock(stock("population", &["births"], &[])), + datamodel::Variable::Flow(flow("births", "population * birth_rate")), + datamodel::Variable::Aux(aux("birth_rate", "0.1")), + ]); + let mut base = generate_layout(&project, TEST_MODEL, None).expect("base layout"); + let birth_rate = base + .elements + .iter() + .find(|e| { + e.get_name() + .is_some_and(|n| canonicalize(n) == "birth_rate") + }) + .map(ViewElement::get_uid) + .expect("birth_rate drawn"); + let births = flow_named(&base, "births").uid; + let (alias, link) = (90_000, 90_001); + base.elements.push(ViewElement::Alias(view_element::Alias { + uid: alias, + alias_of_uid: birth_rate, + x: 400.0, + y: 400.0, + label_side: LabelSide::Bottom, + compat: None, + })); + base.elements.push(ViewElement::Link(view_element::Link { + uid: link, + from_uid: alias, + to_uid: births, + shape: LinkShape::Straight, + polarity: None, + })); + let (_, view) = sync( + &project, + &base, + vec![ModelOperation::DeleteVariable { + ident: "birth_rate".to_string(), + }], + ); + let left: Vec = view + .elements + .iter() + .map(ViewElement::get_uid) + .filter(|uid| [alias, link].contains(uid)) + .collect(); + assert_eq!(left, Vec::::new(), "the alias and its link are gone"); +} + +#[test] +fn clouds_of_flows_through_a_deleted_stock_land_clear_of_each_other() { + // middle takes arrivals in on one face and sends departures and losses + // out of two others. Deleting middle turns the three ends that touched it + // into clouds; left at the old faces' points, the clouds on perpendicular + // faces cover each other. Each slides back along its own pipe instead. + let project = project_with(vec![ + datamodel::Variable::Stock(stock("middle", &["arrivals"], &["departures", "losses"])), + datamodel::Variable::Flow(flow("arrivals", "1")), + datamodel::Variable::Flow(flow("departures", "1")), + datamodel::Variable::Flow(flow("losses", "1")), + ]); + let base = generate_layout(&project, TEST_MODEL, None).expect("base layout"); + let (_, view) = sync( + &project, + &base, + vec![ModelOperation::DeleteVariable { + ident: "middle".to_string(), + }], + ); + let mut changed: HashSet = HashSet::new(); + let mut flows: HashSet = HashSet::new(); + for name in ["arrivals", "departures", "losses"] { + let f = flow_named(&view, name); + flows.insert(f.uid); + changed.insert(f.uid); + changed.extend(f.points.iter().filter_map(|p| p.attached_to_uid)); + } + assert_eq!( + overlaps_involving(&view, &changed), + Vec::<(i32, i32)>::new() + ); + assert_eq!(strict_violations(&view, &flows), ""); +} + +#[test] +fn a_created_side_flow_cloud_lands_clear_of_a_stock() { + // A person parked reservoir where a side flow out of tank puts its cloud. + // An agent adds drain out of tank: its cloud must not land on reservoir. + let project = project_with(vec![ + datamodel::Variable::Stock(stock("tank", &[], &[])), + datamodel::Variable::Stock(stock("reservoir", &[], &[])), + ]); + let base = generate_layout(&project, TEST_MODEL, None).expect("base layout"); + let ops = || { + vec![ + ModelOperation::UpsertFlow(flow("drain", "1")), + ModelOperation::UpsertStock(stock("tank", &[], &["drain"])), + ] + }; + let (_, probe) = sync(&project, &base, ops()); + let sink = flow_named(&probe, "drain") + .points + .last() + .map(|p| (p.x, p.y)) + .expect("drain drawn"); + let mut parked = base.clone(); + let reservoir = parked + .elements + .iter_mut() + .find_map(|e| match e { + ViewElement::Stock(s) if canonicalize(&s.name) == "reservoir" => Some(s), + _ => None, + }) + .expect("reservoir drawn"); + (reservoir.x, reservoir.y) = sink; + let reservoir = reservoir.uid; + assert_eq!( + overlaps_involving(&parked, &[reservoir].into_iter().collect()), + Vec::<(i32, i32)>::new(), + "fixture: reservoir is parked clear of tank" + ); + + let (_, view) = sync(&project, &parked, ops()); + let drain = flow_named(&view, "drain"); + let mut changed: HashSet = [drain.uid].into_iter().collect(); + changed.extend(drain.points.iter().filter_map(|p| p.attached_to_uid)); + changed.remove(&uid_named(&view, "tank")); + assert_eq!( + overlaps_involving(&view, &changed), + Vec::<(i32, i32)>::new() + ); + assert_eq!( + strict_violations(&view, &[drain.uid].into_iter().collect()), + "" + ); +} + +fn uid_named(view: &datamodel::StockFlow, name: &str) -> i32 { + view.elements + .iter() + .find(|e| e.get_name().is_some_and(|n| canonicalize(n) == name)) + .map(ViewElement::get_uid) + .unwrap_or_else(|| panic!("{name} drawn")) +} + +#[test] +fn a_created_parameter_left_on_a_shape_moves_to_the_nearest_clear_spot() { + // Rows over the pass's arms, on elements in the shape incremental layout + // hands it after the declutter: a created parameter still on a shape moves + // to the nearest clear position; one clear of every shape stays; an + // element the pass did not create stays where a person put it, overlap + // included. The composition through production is pinned by the battery's + // catastrophe fixture, where an inserted intermediate landed on an alias + // the jammed relaxation could not clear. + use crate::diagram::constants::AUX_RADIUS; + let aux = |uid: i32, x: f64| { + ViewElement::Aux(view_element::Aux { + name: format!("a{uid}"), + uid, + x, + y: 100.0, + label_side: LabelSide::Bottom, + compat: None, + }) + }; + let alias = ViewElement::Alias(view_element::Alias { + uid: 1, + alias_of_uid: 99, + x: 200.0, + y: 100.0, + label_side: LabelSide::Bottom, + compat: None, + }); + let center = |elements: &[ViewElement]| { + elements + .iter() + .find_map(|e| match e { + ViewElement::Aux(a) => Some((a.x, a.y)), + _ => None, + }) + .expect("the parameter") + }; + + let mut elements = vec![alias.clone(), aux(7, 205.0)]; + keep_created_nodes_clear(&mut elements, |uid| uid == 7); + assert_eq!( + center(&elements), + (205.0 + 2.0 * AUX_RADIUS, 100.0), + "a created parameter on a shape takes the nearest clear ring" + ); + + let mut elements = vec![alias.clone(), aux(7, 260.0)]; + keep_created_nodes_clear(&mut elements, |uid| uid == 7); + assert_eq!( + center(&elements), + (260.0, 100.0), + "clear of every shape: it stays" + ); + + let mut elements = vec![alias, aux(7, 205.0)]; + keep_created_nodes_clear(&mut elements, |_| false); + assert_eq!( + center(&elements), + (205.0, 100.0), + "not created by the pass: it stays" + ); +} + +#[test] +fn clouds_of_flows_leaving_a_deleted_stock_at_one_point_separate() { + // An imported view (thyroid's plasma T4) draws two flows leaving middle + // from the same point of its top face, rising together a short way before + // turning apart to a and b. Deleting middle turns both ends into clouds at + // that one point; each slides along its own pipe, past the shared rise, + // until its cloud covers nothing. + let project = project_with(vec![ + datamodel::Variable::Stock(stock("a", &["to_a"], &[])), + datamodel::Variable::Stock(stock("middle", &[], &["to_a", "to_b"])), + datamodel::Variable::Stock(stock("b", &["to_b"], &[])), + datamodel::Variable::Flow(flow("to_a", "1")), + datamodel::Variable::Flow(flow("to_b", "1")), + ]); + let mut base = generate_layout(&project, TEST_MODEL, None).expect("base layout"); + let uid = |view: &datamodel::StockFlow, name: &str| { + view.elements + .iter() + .find(|e| e.get_name().is_some_and(|n| canonicalize(n) == name)) + .map(ViewElement::get_uid) + .unwrap_or_else(|| panic!("{name} drawn")) + }; + let (a, middle, b) = (uid(&base, "a"), uid(&base, "middle"), uid(&base, "b")); + let point = |x: f64, y: f64, attached: Option| view_element::FlowPoint { + x, + y, + attached_to_uid: attached, + }; + for e in &mut base.elements { + match e { + ViewElement::Stock(s) if s.uid == a => (s.x, s.y) = (325.0, 595.0), + ViewElement::Stock(s) if s.uid == middle => (s.x, s.y) = (610.0, 595.0), + ViewElement::Stock(s) if s.uid == b => (s.x, s.y) = (920.0, 595.0), + ViewElement::Flow(f) if canonicalize(&f.name) == "to_a" => { + f.points = vec![ + point(610.0, 577.5, Some(middle)), + point(610.0, 540.0, None), + point(325.0, 540.0, None), + point(325.0, 577.5, Some(a)), + ]; + (f.x, f.y) = (465.0, 540.0); + } + ViewElement::Flow(f) if canonicalize(&f.name) == "to_b" => { + f.points = vec![ + point(610.0, 577.5, Some(middle)), + point(610.0, 539.0, None), + point(920.0, 539.0, None), + point(920.0, 577.5, Some(b)), + ]; + (f.x, f.y) = (770.0, 539.0); + } + _ => {} + } + } + let (_, view) = sync( + &project, + &base, + vec![ModelOperation::DeleteVariable { + ident: "middle".to_string(), + }], + ); + let mut changed: HashSet = HashSet::new(); + let mut flows: HashSet = HashSet::new(); + for name in ["to_a", "to_b"] { + let f = flow_named(&view, name); + flows.insert(f.uid); + changed.insert(f.uid); + changed.extend(f.points.iter().filter_map(|p| p.attached_to_uid)); + } + changed.remove(&a); + changed.remove(&b); + assert_eq!( + overlaps_involving(&view, &changed), + Vec::<(i32, i32)>::new() + ); + assert_eq!(strict_violations(&view, &flows), ""); +} + +/// transfer drains source into sink, its valve drawn just off source's face, +/// with note parked at the pipe's middle: the view `a_reattached_flow_*` tests +/// edit. +fn reattached_valve_fixture(transfer_equation: &str) -> (datamodel::Project, datamodel::StockFlow) { + let project = project_with(vec![ + datamodel::Variable::Stock(stock("source", &[], &["transfer"])), + datamodel::Variable::Stock(stock("sink", &["transfer"], &[])), + datamodel::Variable::Flow(flow("transfer", transfer_equation)), + datamodel::Variable::Aux(aux("note", "1")), + ]); + let mut base = generate_layout(&project, TEST_MODEL, None).expect("base layout"); + let (source, sink) = (uid_named(&base, "source"), uid_named(&base, "sink")); + let point = |x: f64, y: f64, attached: Option| view_element::FlowPoint { + x, + y, + attached_to_uid: attached, + }; + for e in &mut base.elements { + match e { + ViewElement::Stock(s) if s.uid == source => (s.x, s.y) = (100.0, 100.0), + ViewElement::Stock(s) if s.uid == sink => (s.x, s.y) = (400.0, 100.0), + ViewElement::Aux(a) if canonicalize(&a.name) == "note" => (a.x, a.y) = (250.0, 100.0), + ViewElement::Flow(f) if canonicalize(&f.name) == "transfer" => { + f.points = vec![ + point(122.5, 100.0, Some(source)), + point(377.5, 100.0, Some(sink)), + ]; + (f.x, f.y) = (135.0, 100.0); + } + _ => {} + } + } + (project, base) +} + +#[test] +fn a_reattached_flow_valve_lands_clear_of_a_parameter() { + // Restating source without transfer turns that end into a cloud, which + // covers the valve; the valve moves, but not onto note, and the pipe stays + // on sink's face and its new cloud. Rows over the two ways the pass runs: + // the edit alone creates no element, and with a parameter added the pass + // also places and settles a created element, which must not drag the + // re-attached flow back to where its valve sat before it slid. + let (project, base) = reattached_valve_fixture("10"); + let sink = uid_named(&base, "sink"); + for (row, extra) in [ + ("the re-attachment alone", vec![]), + ( + "with a created parameter", + vec![ModelOperation::UpsertAux(aux("extra", "1"))], + ), + ] { + let mut ops = vec![ModelOperation::UpsertStock(stock("source", &[], &[]))]; + ops.extend(extra); + let (_, view) = sync(&project, &base, ops); + let transfer = flow_named(&view, "transfer"); + let mut changed: HashSet = [transfer.uid].into_iter().collect(); + changed.extend(transfer.points.iter().filter_map(|p| p.attached_to_uid)); + changed.remove(&sink); + assert_eq!( + overlaps_involving(&view, &changed), + Vec::<(i32, i32)>::new(), + "{row}" + ); + assert_eq!( + strict_violations(&view, &[transfer.uid].into_iter().collect()), + "", + "{row}" + ); + } +} + +#[test] +fn a_variable_redrawn_as_a_larger_shape_moves_off_its_neighbour() { + // a and b are parameters a person drew 30 px apart. Turning a into a stock + // redraws it at its old center, where a stock's body covers b; it moves to + // the nearest spot clear of every shape, and b stays where it was. + let project = project_with(vec![ + datamodel::Variable::Aux(aux("a", "1")), + datamodel::Variable::Aux(aux("b", "2")), + ]); + let mut base = generate_layout(&project, TEST_MODEL, None).expect("base layout"); + for e in &mut base.elements { + match e { + ViewElement::Aux(x) if canonicalize(&x.name) == "a" => (x.x, x.y) = (100.0, 100.0), + ViewElement::Aux(x) if canonicalize(&x.name) == "b" => (x.x, x.y) = (130.0, 100.0), + _ => {} + } + } + let (_, view) = sync( + &project, + &base, + vec![ModelOperation::UpsertStock(stock("a", &[], &[]))], + ); + let a = uid_named(&view, "a"); + assert_eq!( + overlaps_involving(&view, &[a].into_iter().collect()), + Vec::<(i32, i32)>::new() + ); + assert_eq!( + center_named(&view, "b"), + Some((130.0, 100.0)), + "b stays put" + ); + let (x, y) = center_named(&view, "a").expect("a drawn"); + assert!( + (x - 100.0).hypot(y - 100.0) <= crate::diagram::constants::STOCK_WIDTH, + "a moves only as far as it must: ({x}, {y})" + ); +} + +#[test] +fn a_cloud_left_by_a_deleted_stock_steps_off_a_parameter_drawn_on_its_pipe() { + // Industrial dynamics draws an alias on the short pipe between a stock's + // bottom face and the valve of the flow draining it. Deleting the stock + // turns that end into a cloud, and no position short of the valve clears + // the alias; the space the stock took is free, so the end extends into it + // instead, as little as clears. + let project = project_with(vec![ + datamodel::Variable::Stock(stock("tank", &[], &["drain"])), + datamodel::Variable::Flow(flow("drain", "1")), + datamodel::Variable::Aux(aux("marker", "1")), + ]); + let mut base = generate_layout(&project, TEST_MODEL, None).expect("base layout"); + let tank = uid_named(&base, "tank"); + let point = |x: f64, y: f64, attached: Option| view_element::FlowPoint { + x, + y, + attached_to_uid: attached, + }; + let cloud = base + .elements + .iter() + .find_map(|e| match e { + ViewElement::Cloud(c) => Some(c.uid), + _ => None, + }) + .expect("drain's cloud"); + for e in &mut base.elements { + match e { + ViewElement::Stock(s) if s.uid == tank => (s.x, s.y) = (500.0, 1262.0), + ViewElement::Aux(a) if canonicalize(&a.name) == "marker" => { + (a.x, a.y) = (499.0, 1301.0) + } + ViewElement::Flow(f) if canonicalize(&f.name) == "drain" => { + f.points = vec![ + point(499.0, 1279.5, Some(tank)), + point(499.0, 1387.0, Some(cloud)), + ]; + (f.x, f.y) = (499.0, 1344.0); + } + ViewElement::Cloud(c) if c.uid == cloud => (c.x, c.y) = (499.0, 1387.0), + _ => {} + } + } + let (_, view) = sync( + &project, + &base, + vec![ModelOperation::DeleteVariable { + ident: "tank".to_string(), + }], + ); + let drain = flow_named(&view, "drain"); + let mut changed: HashSet = [drain.uid].into_iter().collect(); + changed.extend(drain.points.iter().filter_map(|p| p.attached_to_uid)); + assert_eq!( + overlaps_involving(&view, &changed), + Vec::<(i32, i32)>::new() + ); + assert_eq!( + strict_violations(&view, &[drain.uid].into_iter().collect()), + "" + ); +} + +#[test] +fn a_sync_draws_nothing_that_references_an_undrawn_variable() { + // population grows by births at birth_rate and drains by emigration; quad + // reads doubled, which reads birth_rate. The author's view leaves out + // doubled and emigration. Every variable has a uid, as in a project MCP + // opened (`simlin-mcp-core`'s `ensure_variable_uids` mints the missing + // ones), so the connector and cloud diffs can name the undrawn variables + // by uid. An edit naming neither must not draw a connector into or out of + // doubled, or a cloud of emigration: nothing is drawn at the other end, + // so the link or cloud would reference no element. Rows over the two ways + // the pass runs: an edit that creates an element, and one that creates + // none. + let mut project = project_with(vec![ + datamodel::Variable::Stock(stock("population", &["births"], &["emigration"])), + datamodel::Variable::Flow(flow("births", "population * birth_rate")), + datamodel::Variable::Flow(flow("emigration", "population * 0.01")), + datamodel::Variable::Aux(aux("birth_rate", "0.1")), + datamodel::Variable::Aux(aux("doubled", "birth_rate * 2")), + datamodel::Variable::Aux(aux("quad", "doubled * 2")), + ]); + let model = project.get_model_mut(TEST_MODEL).expect("model"); + for (uid, var) in (1..).zip(model.variables.iter_mut()) { + match var { + datamodel::Variable::Stock(s) => s.uid = Some(uid), + datamodel::Variable::Flow(f) => f.uid = Some(uid), + datamodel::Variable::Aux(a) => a.uid = Some(uid), + datamodel::Variable::Module(m) => m.uid = Some(uid), + } + } + let mut base = generate_layout(&project, TEST_MODEL, None).expect("base layout"); + let (doubled, emigration) = (uid_named(&base, "doubled"), uid_named(&base, "emigration")); + let undrawn = [doubled, emigration]; + base.elements.retain(|e| match e { + ViewElement::Link(l) => !undrawn.contains(&l.from_uid) && !undrawn.contains(&l.to_uid), + ViewElement::Cloud(c) => !undrawn.contains(&c.flow_uid), + other => !undrawn.contains(&other.get_uid()), + }); + + for (row, op) in [ + ( + "an edit that creates an element", + ModelOperation::UpsertAux(aux("note", "1")), + ), + ( + "an edit that creates none", + ModelOperation::UpsertAux(aux("birth_rate", "0.2")), + ), + ] { + let (_, view) = sync(&project, &base, vec![op]); + let by_uid: HashMap = + view.elements.iter().map(|e| (e.get_uid(), e)).collect(); + for e in &view.elements { + match e { + ViewElement::Link(l) => assert!( + [l.from_uid, l.to_uid].iter().all(|u| by_uid + .get(u) + .is_some_and(|x| !matches!(x, ViewElement::Link(_)))), + "{row}: link #{} {} -> {} references an element the view does not draw", + l.uid, + l.from_uid, + l.to_uid + ), + ViewElement::Cloud(c) => assert!( + matches!(by_uid.get(&c.flow_uid), Some(ViewElement::Flow(_))), + "{row}: cloud #{} belongs to #{}, which is no drawn flow", + c.uid, + c.flow_uid + ), + _ => {} + } + } + for name in ["doubled", "emigration"] { + assert!( + !view + .elements + .iter() + .any(|e| e.get_name().is_some_and(|n| canonicalize(n) == name)), + "{row}: {name} stays undrawn" + ); + } + } +} + +#[test] +fn a_curved_link_turns_with_an_endpoint_the_sync_moved() { + // A surviving curved link whose endpoint the sync moved keeps its bow: its + // takeoff angle turns by exactly as much as the chord between its ends. + // Rows over what moves an endpoint -- a re-attached flow's valve (restating + // source without transfer puts a cloud on the valve, which takes the pipe's + // middle and slides off note) and a variable redrawn as a stock (a, turned + // into a stock, covers b and moves off it) -- each alone, which creates no + // element, and with a created parameter, which makes the pass place one. + let mut valve_project_base = reattached_valve_fixture("note * 10"); + let rebuilt_project = project_with(vec![ + datamodel::Variable::Aux(aux("a", "1")), + datamodel::Variable::Aux(aux("b", "2")), + datamodel::Variable::Aux(aux("c", "a * 2")), + ]); + let mut rebuilt_base = + generate_layout(&rebuilt_project, TEST_MODEL, None).expect("base layout"); + for e in &mut rebuilt_base.elements { + match e { + ViewElement::Aux(x) if canonicalize(&x.name) == "a" => (x.x, x.y) = (100.0, 100.0), + ViewElement::Aux(x) if canonicalize(&x.name) == "b" => (x.x, x.y) = (130.0, 100.0), + ViewElement::Aux(x) if canonicalize(&x.name) == "c" => (x.x, x.y) = (100.0, 300.0), + _ => {} + } + } + let curve = |view: &mut datamodel::StockFlow| { + for e in &mut view.elements { + if let ViewElement::Link(l) = e { + l.shape = LinkShape::Arc(40.0); + } + } + }; + curve(&mut valve_project_base.1); + curve(&mut rebuilt_base); + let center = |view: &datamodel::StockFlow, name: &str| { + view.elements + .iter() + .find_map(|e| match e { + ViewElement::Aux(a) if canonicalize(&a.name) == name => Some((a.x, a.y)), + ViewElement::Stock(s) if canonicalize(&s.name) == name => Some((s.x, s.y)), + ViewElement::Flow(f) if canonicalize(&f.name) == name => Some((f.x, f.y)), + _ => None, + }) + .unwrap_or_else(|| panic!("{name} drawn")) + }; + let chord = |view: &datamodel::StockFlow, from: &str, to: &str| { + let (a, b) = (center(view, from), center(view, to)); + (b.1 - a.1).atan2(b.0 - a.0).to_degrees() + }; + let takeoff = + |view: &datamodel::StockFlow, from: &str, to: &str| match link_between(view, from, to) + .map(|l| l.shape) + { + Some(LinkShape::Arc(t)) => t, + other => panic!("{from} -> {to} is no curved link: {other:?}"), + }; + + let (valve_project, valve_base) = &valve_project_base; + let fixtures = [ + ( + "a re-attached flow's valve", + valve_project, + valve_base, + ModelOperation::UpsertStock(stock("source", &[], &[])), + ("note", "transfer"), + "transfer", + ), + ( + "a variable redrawn as a stock", + &rebuilt_project, + &rebuilt_base, + ModelOperation::UpsertStock(stock("a", &[], &[])), + ("a", "c"), + "a", + ), + ]; + for (what, project, base, op, (from, to), moved) in fixtures { + for (extra_row, extra) in [ + ("alone", vec![]), + ( + "with a created parameter", + vec![ModelOperation::UpsertAux(aux("extra", "1"))], + ), + ] { + let row = format!("{what}, {extra_row}"); + let mut ops = vec![op.clone()]; + ops.extend(extra); + let (_, view) = sync(project, base, ops); + assert_ne!( + center(&view, moved), + center(base, moved), + "{row}: the sync moves {moved}, or this row pins nothing" + ); + let turned = takeoff(&view, from, to) - takeoff(base, from, to); + let expected = chord(&view, from, to) - chord(base, from, to); + let off = (turned - expected).rem_euclid(360.0); + assert!( + off.min(360.0 - off) < 1e-9, + "{row}: the takeoff turned {turned} degrees, the chord {expected}" + ); + } + } +} diff --git a/src/simlin-engine/src/layout/layout_label_tests.rs b/src/simlin-engine/src/layout/layout_label_tests.rs index 18f8981cd..38b8906e4 100644 --- a/src/simlin-engine/src/layout/layout_label_tests.rs +++ b/src/simlin-engine/src/layout/layout_label_tests.rs @@ -13,6 +13,7 @@ //! - untouched element (aux, stock, flow, module): preserved, both when the //! patch adds an element (settle path) and when it adds only a connector or //! only deletes (the no-new-elements early-return path) +//! - untouched alias: preserved on both paths //! - renamed element: preserved (a rename keeps the element's geometry) //! - flow whose sibling on the same stock was added or removed: preserved //! - flow rebuilt because its attachment changed (an attached stock deleted): @@ -394,6 +395,67 @@ fn early_return_path_preserves_untouched_label_sides() { ); } +#[test] +fn an_untouched_alias_keeps_its_label_side() { + // An imported view draws birth_rate a second time, as an alias. Neither the + // settle path (a new aux) nor the early-return path (a new connector only) + // is about the alias, so it comes back byte for byte, label side included. + let project = project_with_module(); + let mut base_view = generate_layout(&project, TEST_MODEL, None).expect("initial layout"); + let (of, x, y) = base_view + .elements + .iter() + .find_map(|e| match e { + ViewElement::Aux(a) if canonicalize(&a.name) == "birth_rate" => Some((a.uid, a.x, a.y)), + _ => None, + }) + .expect("birth_rate drawn"); + let uid = base_view + .elements + .iter() + .map(ViewElement::get_uid) + .max() + .unwrap_or(0) + + 1; + base_view + .elements + .push(ViewElement::Alias(view_element::Alias { + uid, + alias_of_uid: of, + x: x + 150.0, + y: y + 150.0, + label_side: LabelSide::Bottom, + compat: None, + })); + let alias = |view: &datamodel::StockFlow| { + view.elements + .iter() + .find(|e| e.get_uid() == uid) + .cloned() + .expect("the alias is drawn") + }; + + let cases = [ + ("settle", add_dependent_aux(&project)), + ("connector-only", connector_only_patch(&project)), + ]; + for (label, (patched, patch)) in &cases { + for side in ALL_SIDES { + let mut old_view = base_view.clone(); + if let Some(ViewElement::Alias(a)) = old_view.elements.last_mut() { + a.label_side = side; + } + let new_view = incremental_layout(&old_view, patched, TEST_MODEL, patch, None) + .expect("incremental layout"); + assert_eq!( + alias(&new_view), + alias(&old_view), + "{label}: an untouched alias changed with old side {side:?}" + ); + } + } +} + #[test] fn incremental_layout_preserves_label_side_across_rename() { let project = test_project(simple_model()); diff --git a/src/simlin-engine/src/layout/layout_review_tests.rs b/src/simlin-engine/src/layout/layout_review_tests.rs index 9202b735c..d02d77322 100644 --- a/src/simlin-engine/src/layout/layout_review_tests.rs +++ b/src/simlin-engine/src/layout/layout_review_tests.rs @@ -1625,7 +1625,7 @@ fn test_diff_connectors_preserves_alias_links() { .dep_graph .insert("a".to_string(), ["b".to_string()].into_iter().collect()); - diff_connectors(&mut state, &metadata); + diff_connectors(&mut state, &metadata, |_, _| true, |_| false); // The link from alias_of_b to "a" should be preserved (not replaced by b->a) let alias_link = state diff --git a/src/simlin-engine/src/layout/layout_tests.rs b/src/simlin-engine/src/layout/layout_tests.rs index 7ef541148..05a737f86 100644 --- a/src/simlin-engine/src/layout/layout_tests.rs +++ b/src/simlin-engine/src/layout/layout_tests.rs @@ -3585,7 +3585,7 @@ fn make_connector_diff_state() -> (LayoutState, ComputedMetadata) { fn test_diff_connectors_preserves_existing_links() { let (mut state, metadata) = make_connector_diff_state(); - diff_connectors(&mut state, &metadata); + diff_connectors(&mut state, &metadata, |_, _| true, |_| false); // birth_rate(3)->births(2) should still exist with Arc(45.0) and Positive polarity let link_br = state @@ -3638,7 +3638,7 @@ fn test_diff_connectors_removes_stale_links() { deps.remove("death_rate"); } - diff_connectors(&mut state, &metadata); + diff_connectors(&mut state, &metadata, |_, _| true, |_| false); // death_rate->deaths link should no longer exist let link_dr = state @@ -3671,7 +3671,7 @@ fn test_diff_connectors_adds_new_links() { .count(); assert_eq!(link_count_before, 4, "precondition: four links"); - diff_connectors(&mut state, &metadata); + diff_connectors(&mut state, &metadata, |_, _| true, |_| false); // New link: death_rate(5)->births(2) let new_link = state @@ -3714,7 +3714,7 @@ fn test_diff_connectors_noop_same_dep_graph() { .collect(); let total_before = state.elements.len(); - diff_connectors(&mut state, &metadata); + diff_connectors(&mut state, &metadata, |_, _| true, |_| false); let links_after: Vec<(i32, i32, LinkShape)> = state .elements @@ -3745,7 +3745,7 @@ fn test_diff_connectors_noop_same_dep_graph() { fn test_diff_connectors_structural_flow_stock_skipped() { let (mut state, metadata) = make_connector_diff_state(); - diff_connectors(&mut state, &metadata); + diff_connectors(&mut state, &metadata, |_, _| true, |_| false); // births(2)->population(1) is structural flow->stock. The dep_graph // has this as population depending on births, which would be diff --git a/src/simlin-engine/src/layout/mod.rs b/src/simlin-engine/src/layout/mod.rs index 44198aecb..70a9497ce 100644 --- a/src/simlin-engine/src/layout/mod.rs +++ b/src/simlin-engine/src/layout/mod.rs @@ -11,6 +11,10 @@ pub mod connector; pub mod declutter; mod detect_ltm_loops; #[cfg(any(test, feature = "layout_eval"))] +pub mod edit_audit; +#[cfg(any(test, feature = "layout_eval"))] +pub mod edit_scenarios; +#[cfg(any(test, feature = "layout_eval"))] pub mod eval_stats; mod face_slots; pub mod graph; @@ -338,7 +342,16 @@ impl LayoutState { ViewElement::Stock(s) if s.uid == deleted_uid => false, ViewElement::Flow(f) if f.uid == deleted_uid => false, ViewElement::Module(m) if m.uid == deleted_uid => false, - ViewElement::Link(l) if l.from_uid == deleted_uid || l.to_uid == deleted_uid => false, + // A link touching one of the variable's aliases goes with the + // alias: the connector diff keeps a link it cannot explain, so + // nothing else removes it. + ViewElement::Link(l) + if [l.from_uid, l.to_uid] + .iter() + .any(|u| *u == deleted_uid || removed_alias_uids.contains(u)) => + { + false + } ViewElement::Cloud(c) if c.flow_uid == deleted_uid => false, ViewElement::Alias(a) if a.alias_of_uid == deleted_uid => false, _ => true, @@ -363,6 +376,45 @@ impl LayoutState { self.display_names.remove(&canonical_str); } + /// Remove a variable's element so this pass rebuilds it -- its kind + /// changed, or the stocks its flow attaches to did -- keeping everything + /// that refers to it by uid. The uid stays mapped, so the rebuilt element + /// takes it and the links and aliases touching it survive (the connector + /// diff still drops a link whose dependency is gone); its position and + /// display name stay for the rebuild to read. Its clouds go: they belong + /// to the old pipe. + pub fn remove_for_rebuild(&mut self, ident: &str) { + let canonical = canonicalize(ident).into_owned(); + let Some(uid) = self.uid_manager.get_uid(&canonical) else { + return; + }; + let cloud_uids: Vec = self + .elements + .iter() + .filter_map(|elem| match elem { + ViewElement::Cloud(c) if c.flow_uid == uid => Some(c.uid), + _ => None, + }) + .collect(); + self.elements.retain(|elem| match elem { + ViewElement::Aux(_) + | ViewElement::Stock(_) + | ViewElement::Flow(_) + | ViewElement::Module(_) => elem.get_uid() != uid, + ViewElement::Cloud(c) => c.flow_uid != uid, + ViewElement::Link(_) | ViewElement::Alias(_) | ViewElement::Group(_) => true, + }); + for cloud_uid in &cloud_uids { + self.positions.remove(cloud_uid); + } + if let Some(cloud_idents) = self.flow_ident_to_clouds.remove(&canonical) { + for ci in &cloud_idents { + self.cloud_ident_to_uid.remove(ci); + self.cloud_ident_to_flow_ident.remove(ci); + } + } + } + /// Update a variable's identity in-place while preserving its /// position and UID. Updates the element name, uid_manager /// mapping, and display_names entry. @@ -442,6 +494,17 @@ pub struct NewElements { } impl NewElements { + /// The new elements whose idents `keep` accepts. + pub fn filtered(self, keep: impl Fn(&str) -> bool) -> NewElements { + let only = |idents: Vec| idents.into_iter().filter(|i| keep(i)).collect(); + NewElements { + new_stocks: only(self.new_stocks), + new_flows: only(self.new_flows), + new_auxes: only(self.new_auxes), + new_modules: only(self.new_modules), + } + } + pub fn is_empty(&self) -> bool { self.new_stocks.is_empty() && self.new_flows.is_empty() @@ -2463,12 +2526,16 @@ fn optimize_labels(state: &mut LayoutState, model: &datamodel::Model, metadata: } } -/// Apply arc curvature to connectors involved in feedback loops. +/// Apply arc curvature to connectors involved in feedback loops, among the +/// links `curves` accepts (by uid): every link in a fresh layout, and only the +/// links an incremental pass creates, since a link a person drew straight is +/// theirs to keep, whichever loop an edit now puts it on. fn apply_loop_curvature( state: &mut LayoutState, config: &LayoutConfig, model: &datamodel::Model, metadata: &ComputedMetadata, + curves: impl Fn(i32) -> bool, ) { if metadata.feedback_loops.is_empty() { return; @@ -2528,7 +2595,7 @@ fn apply_loop_curvature( }; if let ViewElement::Link(link) = &state.elements[elem_idx] - && matches!(link.shape, LinkShape::Arc(_)) + && (matches!(link.shape, LinkShape::Arc(_)) || !curves(link.uid)) { continue; } @@ -2553,15 +2620,24 @@ fn apply_loop_curvature( } } -/// Ensure every stock/flow/aux/module variable in the model has a -/// corresponding rendered view element. -fn validate_view_completeness(state: &LayoutState, model: &datamodel::Model) -> Result<(), String> { +/// Ensure every stock/flow/aux/module variable in the model whose ident +/// `expected` accepts has a corresponding rendered view element: every +/// variable for a fresh layout, and for a sync the ones it must draw. +fn validate_view_completeness( + state: &LayoutState, + model: &datamodel::Model, + expected: impl Fn(&str) -> bool, +) -> Result<(), String> { let mut expected_stocks = BTreeSet::new(); let mut expected_flows = BTreeSet::new(); let mut expected_auxes = BTreeSet::new(); let mut expected_modules = BTreeSet::new(); - for var in &model.variables { + for var in model + .variables + .iter() + .filter(|v| expected(&canonicalize(v.get_ident()))) + { match var { datamodel::Variable::Stock(s) => { expected_stocks.insert(canonicalize(&s.ident).into_owned()); @@ -2896,9 +2972,9 @@ pub fn fresh_layout( finish_flow_geometry(&mut state.elements, |_| true); // Phase 6: Apply feedback loop curvature - apply_loop_curvature(&mut state, config, model, metadata); + apply_loop_curvature(&mut state, config, model, metadata, |_| true); - validate_view_completeness(&state, model)?; + validate_view_completeness(&state, model, |_| true)?; // Phase 7: Compute ViewBox from final element positions let (bmin_x, _bmin_y, bmax_x, bmax_y) = compute_bounds(&state.elements, config); @@ -3269,6 +3345,27 @@ pub fn compute_metadata( project: &datamodel::Project, model_name: &str, db_state: Option<(&crate::db::SimlinDb, crate::db::SourceProject)>, +) -> Option { + compute_metadata_parts(project, model_name, db_state, true) +} + +/// `compute_metadata` without the feedback loops and dominant periods: the +/// dependencies a diagram draws, the stock-flow chains and the stock lists. +/// Loop detection simulates the model, which a reader of the dependency +/// structure alone (the edit audit, the edit scenarios) does not need. +pub fn compute_dependency_metadata( + project: &datamodel::Project, + model_name: &str, + db_state: Option<(&crate::db::SimlinDb, crate::db::SourceProject)>, +) -> Option { + compute_metadata_parts(project, model_name, db_state, false) +} + +fn compute_metadata_parts( + project: &datamodel::Project, + model_name: &str, + db_state: Option<(&crate::db::SimlinDb, crate::db::SourceProject)>, + with_loops: bool, ) -> Option { let model = project.get_model(model_name)?; let mut dep_graph: BTreeMap> = BTreeMap::new(); @@ -3478,6 +3575,20 @@ pub fn compute_metadata( &all_flows, ); + if !with_loops { + return Some(ComputedMetadata { + chains, + feedback_loops: Vec::new(), + dominant_periods: Vec::new(), + dep_graph, + reverse_dep_graph, + constants, + stock_to_inflows, + stock_to_outflows, + flow_to_stocks, + }); + } + // Try LTM-based loop detection. Falls back to persisted loop_metadata // if LTM detection or simulation fails. The branch decides the // partition surface: detected loops carry partition metadata (a None diff --git a/src/simlin-engine/src/patch.rs b/src/simlin-engine/src/patch.rs index 2dcffac76..622587cf7 100644 --- a/src/simlin-engine/src/patch.rs +++ b/src/simlin-engine/src/patch.rs @@ -960,10 +960,13 @@ fn rename_canonical_ident( let prefix = &ident_str[..pos]; let suffix = &ident_str[pos + '·'.len_utf8()..]; - // Only rename self-qualified references (self·variable) - // Don't rename other module-qualified references as they refer to different variables - if suffix == old_ident.as_str() && prefix == "self" { - return Ident::from_unchecked(format!("self·{}", new_ident.as_str())); + // Only rename references to this model's own variable: self-qualified + // (`self·x`), or XMILE's parent-scope spelling (`.x`, canonicalized to + // `·x`), which every consumer reads as the bare name + // (`db::DepScope::resolve`). Any other qualifier names another + // module's variable. + if suffix == old_ident.as_str() && (prefix == "self" || prefix.is_empty()) { + return Ident::from_unchecked(format!("{prefix}·{}", new_ident.as_str())); } } @@ -1440,6 +1443,59 @@ mod tests { assert_eq!(module.references[0].dst, "self.target"); } + #[test] + fn rename_rewrites_a_parent_scope_module_source() { + // XMILE spells a module input's source in the enclosing model as `.x` + // (``), which the reader stores + // canonicalized, as `·x`, and which every consumer reads as the bare + // name. Renaming x must rewrite that source, or the module silently + // reads its input port's default. + let mut project = TestProject::new("test") + .aux("input", "1", None) + .build_datamodel(); + let parent_scope_source = canonicalize(".input").into_owned(); + let model = project.get_model_mut("main").expect("main model"); + model + .variables + .push(datamodel::Variable::Module(datamodel::Module { + ident: "child".to_string(), + model_name: "child".to_string(), + documentation: String::new(), + units: None, + references: vec![datamodel::ModuleReference { + src: parent_scope_source, + dst: canonicalize("child.target").into_owned(), + }], + compat: datamodel::Compat::default(), + ai_state: None, + uid: None, + })); + + let patch = ProjectPatch { + project_ops: vec![], + models: vec![ModelPatch { + name: "main".to_string(), + ops: vec![ModelOperation::RenameVariable { + from: "input".to_string(), + to: "new_input".to_string(), + }], + }], + }; + apply_patch(&mut project, patch).unwrap(); + + let Some(Variable::Module(module)) = + project.get_model("main").unwrap().get_variable("child") + else { + panic!("child is a module"); + }; + // A rename writes a reference's source spelling (`self.target`, `.x`); + // what it names is the canonical form every consumer reads. + assert_eq!( + canonicalize(&module.references[0].src), + canonicalize(".new_input") + ); + } + #[test] fn rename_does_not_affect_unrelated_module_variables() { let mut project = TestProject::new("test") diff --git a/src/simlin-engine/tests/integration/layout.rs b/src/simlin-engine/tests/integration/layout.rs index 737a09ae5..096890900 100644 --- a/src/simlin-engine/tests/integration/layout.rs +++ b/src/simlin-engine/tests/integration/layout.rs @@ -1499,7 +1499,7 @@ fn find_element_uid( fn test_incremental_combined_ops() { use simlin_engine::datamodel; use simlin_engine::layout::incremental_layout; - use simlin_engine::{ModelOperation, ModelPatch}; + use simlin_engine::{ModelOperation, ModelPatch, ProjectPatch, apply_patch}; // SIR model variables: // stocks: susceptible, infectious, recovered @@ -1516,7 +1516,8 @@ fn test_incremental_combined_ops() { // Patch: // 1. Delete contact_infectivity // 2. Rename total_population -> total_pop - // 3. Add immunity_rate = 1/duration, change recovering = infectious * immunity_rate + // 3. Restate succumbing without contact_infectivity + // 4. Add immunity_rate = 1/duration, change recovering = infectious * immunity_rate // This inserts immunity_rate between duration and recovering: // old: duration -> recovering // new: duration -> immunity_rate -> recovering @@ -1531,69 +1532,27 @@ fn test_incremental_combined_ops() { .get("total_population") .expect("total_population should exist"); - // Build post-patch model manually - let mut patched_project = project.clone(); - let model = patched_project.get_model_mut(MAIN_MODEL).unwrap(); - - // Delete contact_infectivity - model - .variables - .retain(|v| canonicalize(v.get_ident()).as_ref() != "contact_infectivity"); - - // Rename total_population -> total_pop - for var in &mut model.variables { - if canonicalize(var.get_ident()).as_ref() == "total_population" - && let datamodel::Variable::Aux(a) = var - { - a.ident = "total_pop".to_string(); - } - } - - // Update succumbing equation to remove contact_infectivity reference - for var in &mut model.variables { - if canonicalize(var.get_ident()).as_ref() == "succumbing" - && let datamodel::Variable::Flow(f) = var - { - f.equation = - datamodel::Equation::Scalar("susceptible*infectious/total_pop".to_string()); - } - } - - // Update susceptible init to reference total_pop - for var in &mut model.variables { - if canonicalize(var.get_ident()).as_ref() == "susceptible" - && let datamodel::Variable::Stock(s) = var - { - s.equation = datamodel::Equation::Scalar("total_pop".to_string()); - } - } - - // Change recovering equation to use immunity_rate instead of duration - for var in &mut model.variables { - if canonicalize(var.get_ident()).as_ref() == "recovering" - && let datamodel::Variable::Flow(f) = var - { - f.equation = datamodel::Equation::Scalar("infectious * immunity_rate".to_string()); - } - } - - // Add immunity_rate aux - model - .variables - .push(datamodel::Variable::Aux(datamodel::Aux { - ident: "immunity_rate".to_string(), - equation: datamodel::Equation::Scalar("1 / duration".to_string()), - documentation: String::new(), - units: None, - gf: None, - ai_state: None, - uid: None, - compat: Default::default(), - })); - - // Build the patch + // The patch an agent sends for this edit, and the model after it derived + // through the production patch path: a reader whose equation changes is + // one the patch restates, so the sync knows which connectors into it + // belong. + let flow_with = |ident: &str, equation: &str| { + project + .get_model(MAIN_MODEL) + .and_then(|m| { + m.variables.iter().find_map(|v| match v { + datamodel::Variable::Flow(f) if canonicalize(&f.ident).as_ref() == ident => { + let mut f = f.clone(); + f.equation = datamodel::Equation::Scalar(equation.to_string()); + Some(f) + } + _ => None, + }) + }) + .unwrap_or_else(|| panic!("{ident} is a flow")) + }; let patch = ModelPatch { - name: String::new(), + name: MAIN_MODEL.to_string(), ops: vec![ ModelOperation::DeleteVariable { ident: "contact_infectivity".to_string(), @@ -1612,8 +1571,24 @@ fn test_incremental_combined_ops() { uid: None, compat: Default::default(), }), + ModelOperation::UpsertFlow(flow_with("succumbing", "susceptible*infectious/total_pop")), + ModelOperation::UpsertFlow(flow_with("recovering", "infectious * immunity_rate")), ], }; + // As in production, the model carries the view being synced when the + // patch applies, so the uids it mints for new variables are past every + // uid the view uses. + let mut patched_project = project.clone(); + patched_project.get_model_mut(MAIN_MODEL).unwrap().views = + vec![datamodel::View::StockFlow(old_view.clone())]; + apply_patch( + &mut patched_project, + ProjectPatch { + project_ops: vec![], + models: vec![patch.clone()], + }, + ) + .expect("the patch applies"); let new_view = incremental_layout(&old_view, &patched_project, MAIN_MODEL, &patch, None) .expect("incremental layout with combined ops should succeed"); diff --git a/src/simlin-mcp-core/CLAUDE.md b/src/simlin-mcp-core/CLAUDE.md index fcf183951..983ade7f8 100644 --- a/src/simlin-mcp-core/CLAUDE.md +++ b/src/simlin-mcp-core/CLAUDE.md @@ -19,7 +19,7 @@ The library is generic over a concrete `A: ProjectAccess` (not `dyn`) so rmcp's - `src/types.rs` -- Wire-format types preserved verbatim from the pre-rmcp binary (`SourceFormat`, `LoopDominanceSummary`, `DominantPeriodOutput`, `ErrorOutput`) plus `build_empty_project` and `build_empty_project_with_specs` shared with the new-project HTTP route in `simlin-serve` for byte-identical create output, and the MDL export wire helpers: `mdl_export_warnings_to_outputs` / `mdl_export_error_to_output` are the one place the MDL writer's warnings and hard errors become wire `ErrorOutput`s (`generic` code, `model` kind, `MDL export:` message prefix, scoped to the single non-macro model) -- both `FileSystemAccess` and `simlin-serve`'s writer go through them so pysimlin, stdio MCP, and serve report the same thing -- and `preflight_export` dry-runs the writer for a `SourceFormat` (warnings a save would report, or the `Validation` error it would fail with). - `src/open.rs` -- Format-detection + parsing helpers (`format_for_extension`, `open_project`, `resolve_model_name`). I/O-free: callers pass already-loaded bytes. `format_for_extension` is the single extension dispatcher for the MCP surface: `open_project` picks its parser with it and `create_model` picks the writer for a fresh file with it, so a path's extension always names the format on disk (`.stmx`/`.xmile`/`.xml` XMILE, `.mdl` Vensim, everything else content-detected JSON). `ensure_variable_uids` is private: `open_project` is the only caller, and running it is part of what opening a project means rather than a step a caller may skip. - `src/fs_access.rs` -- `FileSystemAccess`, the stateless filesystem `ProjectAccess` impl. Used by the `simlin-mcp` binary (re-exported there as `simlin_mcp::access::FileSystemAccess`) AND by this crate's integration suites (`test_support::TestFileSystemAccess` is a type alias for it). It lives here rather than in the binary so the tests exercise the shipping impl -- a hand-maintained near-copy drifts at exactly the points where this file is non-trivial (the MDL lossiness-warning channel and the SD-AI `relationships` regeneration on save), so a test saving through a copy proves something about a simpler function than the one that ships. Every `SourceFormat` is written back in place in its own format; `Mdl` goes through `simlin_engine::to_mdl_with_warnings`, whose hard errors (more than one non-macro model, an ordinary Module variable) fail the save and whose lossiness warnings ride `SaveOutcome::warnings`. -- `src/tools/` -- The three reused tools (`read_model.rs`, `edit_model.rs`, `create_model.rs`) as async free functions taking `&impl ProjectAccess`. Exposed types use `#[serde(rename_all = "camelCase")]`; the curated *input* types deliberately exclude engine-internal fields (`uid`, `compat`, `aiState`), while both tool outputs embed the full engine `json::Model`, which serializes `uid`/`compat` when populated. `read_model`/`edit_model` both unconditionally run LTM loop analysis (`analysis::analyze_model`), so they (1) carry an `analysisError` field that surfaces the actionable compile error (naming the variable that failed to compile) when a model can't be compiled for LTM instead of returning a silent empty `loopDominance` (GH #660), and (2) collect their `collect_all_diagnostics` passes with the LTM overlay on (`simlin_engine::db::LtmOverlay::On`, the same harvest libsimlin's `simlin_project_get_errors` runs for a project that requested LTM, GH #466), surfacing the LTM auto-flip-to-discovery advisory and synthetic-fragment compile-failure warnings in a model-scoped `warnings` field that a collection under `Off` never carries (GH #662). `edit_model` enables LTM on both its pre- and post-edit diagnostic passes so the new-error gate compares like-with-like (the LTM advisories are Warnings, so they do not affect the Error-severity gate). Both outputs also carry the discovery-completeness triple `enumerationComplete` / `retainedLoops` / `universeLoops` from `analysis::ModelAnalysis`, plus `truncated` (elided when false, like `aggRecoveryTruncated`: candidate generation stopped early, which the fallback's candidate bound can cause even without a wall-clock budget). `enumerationComplete` is the one result-level flag that is ALWAYS serialized: `aggRecoveryTruncated` elides its `false` because the interesting value there is `true`, whereas here the interesting value IS `false` (a sampled analysis), and a client that cannot see the field reads a sample as exhaustive -- the exact mistake the flag exists to prevent. `retainedLoops` is likewise unconditional (`0` is a real statement, so no absence would mean anything a `0` does not); `universeLoops` is the only optional one, elided when `enumerationComplete` is false because a sample has no universe to report, which is a different claim from a universe of zero. `edit_model` pre-flights the writer (`preflight_export`) before touching the store -- a project the format cannot hold is a `Validation` error and never lands, so a registry-backed store is not left holding a merged doc it cannot persist -- and appends the writer's lossiness warnings to that same `warnings` field: the store's `SaveOutcome::warnings` after a real write, the pre-flight's on a `dryRun`, so an agent can preview what a save would degrade. +- `src/tools/` -- The three reused tools (`read_model.rs`, `edit_model.rs`, `create_model.rs`) as async free functions taking `&impl ProjectAccess`. Exposed types use `#[serde(rename_all = "camelCase")]`; the curated *input* types deliberately exclude engine-internal fields (`uid`, `compat`, `aiState`), while both tool outputs embed the full engine `json::Model`, which serializes `uid`/`compat` when populated. `read_model`/`edit_model` both unconditionally run LTM loop analysis (`analysis::analyze_model`), so they (1) carry an `analysisError` field that surfaces the actionable compile error (naming the variable that failed to compile) when a model can't be compiled for LTM instead of returning a silent empty `loopDominance` (GH #660), and (2) collect their `collect_all_diagnostics` passes with the LTM overlay on (`simlin_engine::db::LtmOverlay::On`, the same harvest libsimlin's `simlin_project_get_errors` runs for a project that requested LTM, GH #466), surfacing the LTM auto-flip-to-discovery advisory and synthetic-fragment compile-failure warnings in a model-scoped `warnings` field that a collection under `Off` never carries (GH #662). `edit_model` enables LTM on both its pre- and post-edit diagnostic passes so the new-error gate compares like-with-like (the LTM advisories are Warnings, so they do not affect the Error-severity gate). Both outputs also carry the discovery-completeness triple `enumerationComplete` / `retainedLoops` / `universeLoops` from `analysis::ModelAnalysis`, plus `truncated` (elided when false, like `aggRecoveryTruncated`: candidate generation stopped early, which the fallback's candidate bound can cause even without a wall-clock budget). `enumerationComplete` is the one result-level flag that is ALWAYS serialized: `aggRecoveryTruncated` elides its `false` because the interesting value there is `true`, whereas here the interesting value IS `false` (a sampled analysis), and a client that cannot see the field reads a sample as exhaustive -- the exact mistake the flag exists to prevent. `retainedLoops` is likewise unconditional (`0` is a real statement, so no absence would mean anything a `0` does not); `universeLoops` is the only optional one, elided when `enumerationComplete` is false because a sample has no universe to report, which is a different claim from a universe of zero. `edit_model` pre-flights the writer (`preflight_export`) before touching the store -- a project the format cannot hold is a `Validation` error and never lands, so a registry-backed store is not left holding a merged doc it cannot persist -- and appends the writer's lossiness warnings to that same `warnings` field: the store's `SaveOutcome::warnings` after a real write, the pre-flight's on a `dryRun`, so an agent can preview what a save would degrade. After a real (non-dry-run) edit with variable operations, `edit_model` syncs the model's first view (`incremental_layout` over a non-empty view, a full layout otherwise) and replaces that view only: a project can carry several views, and the others are the author's. A sync that fails never fails the edit; it leaves every view as it was and adds a `generic`-code, `model`-kind warning prefixed `diagram sync:`, since an agent that is not told reads the stale diagram as current. - `src/server.rs` -- `SimlinMcpServer` rmcp `ServerHandler` impl with the three `#[tool]` macros plus `list_resources` and `read_resource`. `version` is plumbed in by the binary so `serverInfo.version` reflects the binary's `CARGO_PKG_VERSION`, not the library's. - `src/test_support.rs` -- `#[doc(hidden)]` integration-test fixtures, gated behind the `test-support` feature so they are not compiled into shipped binaries (the crate takes a self dev-dependency enabling the feature so `tests/` still resolves them). `TestFileSystemAccess` is a type alias for the production `fs_access::FileSystemAccess`, never a second implementation (see its rustdoc for why); `chain_scc_project_json` builds the oversized-SCC model the LTM auto-flip warning tests need. diff --git a/src/simlin-mcp-core/src/server.rs b/src/simlin-mcp-core/src/server.rs index 431dd7e49..054e46c91 100644 --- a/src/simlin-mcp-core/src/server.rs +++ b/src/simlin-mcp-core/src/server.rs @@ -111,8 +111,8 @@ impl SimlinMcpServer { #[tool( name = "EditModel", description = "Edit a system dynamics model by applying operations. \ - Supports upserting stocks, flows, and auxiliaries, removing variables, \ - and updating simulation specs. Returns a refreshed model snapshot \ + Supports upserting stocks, flows, and auxiliaries, removing and renaming \ + variables, and updating simulation specs. Returns a refreshed model snapshot \ with loop dominance analysis after applying changes. \ Upsert replaces the full variable definition; omitted optional fields \ default to empty. Use ReadModel first to get current state, then \ diff --git a/src/simlin-mcp-core/src/tools/edit_model.rs b/src/simlin-mcp-core/src/tools/edit_model.rs index 97772792d..4d1052b42 100644 --- a/src/simlin-mcp-core/src/tools/edit_model.rs +++ b/src/simlin-mcp-core/src/tools/edit_model.rs @@ -138,6 +138,17 @@ pub struct RemoveVariableInput { pub name: String, } +/// Rename a variable. Every equation that reads it is rewritten to the new +/// name, and its diagram element keeps its place and connectors. +#[derive(Deserialize, JsonSchema)] +#[serde(rename_all = "camelCase")] +pub struct RenameVariableInput { + /// Current name of the variable. + pub from: String, + /// New name for the variable; no other variable may already have it. + pub to: String, +} + /// Assign a human-readable name to a feedback loop identified by its /// participating variables. #[derive(Deserialize, JsonSchema)] @@ -162,6 +173,7 @@ pub enum EditOperation { UpsertFlow(UpsertFlowInput), UpsertAuxiliary(UpsertAuxiliaryInput), RemoveVariable(RemoveVariableInput), + RenameVariable(RenameVariableInput), SetLoopName(SetLoopNameInput), } @@ -241,7 +253,9 @@ pub struct EditModelOutput { /// advisory and synthetic-fragment compile-failure warnings (GH #662), /// plus the on-disk writer's lossiness warnings (an MDL project holding /// a construct Vensim cannot express is saved in its closest - /// representable form; the message carries an `MDL export:` prefix). + /// representable form; the message carries an `MDL export:` prefix), and a + /// diagram sync that failed (the edit landed but the diagram still shows + /// the model as it was; the message carries a `diagram sync:` prefix). /// A `dryRun` computes the writer warnings without writing, so an agent /// can preview what a real save would degrade. Empty (and elided from /// JSON) when there are none. @@ -329,9 +343,11 @@ pub async fn edit_model( simlin_engine::apply_patch(&mut project, patch) .map_err(|e| AccessError::ParseError(anyhow::anyhow!("patch application failed: {e:?}")))?; - if !dry_run && has_variable_ops { - sync_diagram(&mut project, &model_name, model_patch.as_ref()); - } + let diagram_warning = if !dry_run && has_variable_ops { + sync_diagram(&mut project, &model_name, model_patch.as_ref()) + } else { + None + }; // One SimlinDb shared between the diagnostic gate and analyze_model // so salsa's caches are reused. @@ -371,6 +387,7 @@ pub async fn edit_model( .filter(|e| e.model_name.as_ref().is_none_or(|name| name == &model_name)) .map(ErrorOutput::from) .collect(); + warnings.extend(diagram_warning); let has_new_errors = post_edit_model_errors .iter() @@ -453,19 +470,19 @@ pub async fn edit_model( }) } -/// Regenerate the diagram layout for the named model, replacing its views -/// in-place. When a model patch is provided and the model already has a -/// non-empty view, uses incremental layout to preserve existing element -/// positions. Falls back to full layout generation otherwise. +/// Bring the named model's diagram in line with the patched model: its first +/// view is synced (incremental layout when a model patch is provided and the +/// view is non-empty, preserving existing element positions; a full layout +/// otherwise) and installed by [`install_synced_view`]. /// -/// Preserves the existing zoom level when the model already has a view. -/// Layout failures are silently ignored -- a missing diagram is non-fatal -/// and the model data is still correct. +/// Returns the warning to report when the sync failed. A failed sync never +/// fails the edit -- the model data is correct -- but the agent has to know +/// the diagram no longer matches it. fn sync_diagram( project: &mut simlin_engine::datamodel::Project, model_name: &str, model_patch: Option<&simlin_engine::ModelPatch>, -) { +) -> Option { let old_view = project .get_model(model_name) .and_then(|m| m.views.first()) @@ -473,9 +490,7 @@ fn sync_diagram( simlin_engine::datamodel::View::StockFlow(sf) => sf, }); - let existing_zoom = old_view.map(|sf| sf.zoom).filter(|&z| z > 0.0); - - let new_view = if let (Some(old_sf), Some(patch)) = (old_view, model_patch) { + let synced = if let (Some(old_sf), Some(patch)) = (old_view, model_patch) { if !old_sf.elements.is_empty() { let old_sf = old_sf.clone(); simlin_engine::layout::incremental_layout(&old_sf, project, model_name, patch, None) @@ -486,18 +501,52 @@ fn sync_diagram( simlin_engine::layout::generate_best_layout(project, model_name, None) }; - let mut layout = match new_view { - Ok(l) => l, - Err(_) => return, - }; - - if let Some(zoom) = existing_zoom { - layout.zoom = zoom; - } + install_synced_view(project, model_name, synced) +} - if let Some(model) = project.get_model_mut(model_name) { - model.views = vec![simlin_engine::datamodel::View::StockFlow(layout)]; +/// Install the result of syncing a model's first view. +/// +/// A synced view replaces the first view only, keeping its zoom, and every +/// other view the model holds stays as it was: the layout syncs one view, and +/// a project can carry more (JSON and protobuf hold a list), which are the +/// author's and must not vanish because an agent edited the model. +/// +/// A failed sync leaves every view as it was and returns a warning naming the +/// reason, so the agent learns the diagram still shows the model as it was +/// before this edit rather than reading a stale diagram as current. +fn install_synced_view( + project: &mut simlin_engine::datamodel::Project, + model_name: &str, + synced: Result, +) -> Option { + let mut layout = match synced { + Ok(layout) => layout, + Err(reason) => { + return Some(ErrorOutput { + code: simlin_engine::common::ErrorCode::Generic.to_string(), + message: format!( + "diagram sync: {reason}; the edit was applied, but the diagram was not \ + updated and still shows the model as it was before this edit" + ), + model_name: Some(model_name.to_string()), + variable_name: None, + kind: "model".to_string(), + }); + } + }; + let model = project.get_model_mut(model_name)?; + match model.views.first_mut() { + Some(simlin_engine::datamodel::View::StockFlow(first)) => { + if first.zoom > 0.0 { + layout.zoom = first.zoom; + } + *first = layout; + } + None => model + .views + .push(simlin_engine::datamodel::View::StockFlow(layout)), } + None } /// Build an engine `ProjectPatch` from the curated MCP inputs. @@ -623,6 +672,10 @@ fn convert_operation(op: EditOperation) -> simlin_engine::ModelOperation { EditOperation::RemoveVariable(r) => { simlin_engine::ModelOperation::DeleteVariable { ident: r.name } } + EditOperation::RenameVariable(r) => simlin_engine::ModelOperation::RenameVariable { + from: r.from, + to: r.to, + }, EditOperation::SetLoopName(input) => simlin_engine::ModelOperation::SetLoopName { variables: input.variables, name: input.name, @@ -635,6 +688,81 @@ fn convert_operation(op: EditOperation) -> simlin_engine::ModelOperation { mod tests { use super::*; + /// Every arm of installing a sync's result: a synced view replaces a + /// model's first view (keeping its zoom) and leaves the rest, is added to + /// a model with no view, and a failed sync leaves the views as they were + /// and reports why. How a sync fails is the layout's business; the warning + /// is what this layer owes the agent whatever the reason. + #[test] + fn install_synced_view_arms() { + use simlin_engine::datamodel::View; + let mut project = crate::types::build_empty_project(); + simlin_engine::apply_patch( + &mut project, + build_patch( + "main", + None, + Some(vec![EditOperation::UpsertAuxiliary(UpsertAuxiliaryInput { + name: "rate".into(), + equation: "0.1".into(), + units: None, + documentation: None, + graphical_function: None, + arrayed_equation: None, + })]), + ), + ) + .expect("the patch applies"); + let synced = + simlin_engine::layout::generate_best_layout(&project, "main", None).expect("layout"); + let first = simlin_engine::datamodel::StockFlow { + elements: Vec::new(), + zoom: 2.0, + ..synced.clone() + }; + let second = simlin_engine::datamodel::StockFlow { + zoom: 0.5, + ..synced.clone() + }; + + project.models[0].views = vec![ + View::StockFlow(first.clone()), + View::StockFlow(second.clone()), + ]; + assert!(install_synced_view(&mut project, "main", Ok(synced.clone())).is_none()); + let kept_zoom = simlin_engine::datamodel::StockFlow { + zoom: 2.0, + ..synced.clone() + }; + assert!( + project.models[0].views + == vec![View::StockFlow(kept_zoom), View::StockFlow(second.clone())], + "the first view is replaced at its zoom, the second is untouched" + ); + + project.models[0].views = Vec::new(); + assert!(install_synced_view(&mut project, "main", Ok(synced.clone())).is_none()); + assert!(project.models[0].views == vec![View::StockFlow(synced.clone())]); + + let before = vec![View::StockFlow(first), View::StockFlow(second)]; + project.models[0].views = before.clone(); + let warning = install_synced_view(&mut project, "main", Err("no room".into())) + .expect("a failed sync is reported"); + assert!( + project.models[0].views == before, + "a failed sync changes no view" + ); + assert_eq!(warning.code, "generic"); + assert_eq!(warning.kind, "model"); + assert_eq!(warning.model_name.as_deref(), Some("main")); + assert_eq!(warning.variable_name, None); + assert!( + warning.message.starts_with("diagram sync: no room;"), + "{}", + warning.message + ); + } + #[test] fn convert_arrayed_equation_infers_except_default() { let input = ArrayedEquationInput { @@ -671,11 +799,11 @@ mod tests { assert_eq!(convert_arrayed_equation(input).has_except_default, None); } - /// `convert_operation` is a five-way dispatch over `EditOperation`, and + /// `convert_operation` is a six-way dispatch over `EditOperation`, and /// each arm both selects a `ModelOperation` variant and carries the /// caller's fields across. The rows are derived from `EditOperation`'s - /// variant list, not sampled from it: a sixth variant added there needs a - /// sixth row here. `ModelOperation` is the wider enum (it also carries + /// variant list, not sampled from it: a seventh variant added there needs a + /// seventh row here. `ModelOperation` is the wider enum (it also carries /// operations the MCP surface does not expose), so the match keeps a /// catch-all -- reaching it means an arm mapped to the wrong family. #[test] @@ -711,6 +839,10 @@ mod tests { EditOperation::RemoveVariable(RemoveVariableInput { name: "deaths".into(), }), + EditOperation::RenameVariable(RenameVariableInput { + from: "rate".into(), + to: "growth rate".into(), + }), EditOperation::SetLoopName(SetLoopNameInput { variables: vec!["population".into(), "births".into()], name: "Growth Loop".into(), @@ -740,6 +872,10 @@ mod tests { ModelOperation::DeleteVariable { ident } => { assert_eq!(ident, "deaths"); } + ModelOperation::RenameVariable { from, to } => { + assert_eq!(from, "rate"); + assert_eq!(to, "growth rate"); + } ModelOperation::SetLoopName { variables, name, diff --git a/src/simlin-mcp-core/tests/integration/edit_model_e2e.rs b/src/simlin-mcp-core/tests/integration/edit_model_e2e.rs index 0a744bb0d..2daadf049 100644 --- a/src/simlin-mcp-core/tests/integration/edit_model_e2e.rs +++ b/src/simlin-mcp-core/tests/integration/edit_model_e2e.rs @@ -17,8 +17,8 @@ use simlin_mcp_core::access::ProjectAccess; use simlin_mcp_core::errors::AccessError; use simlin_mcp_core::test_support::{TestFileSystemAccess, chain_scc_project_json}; use simlin_mcp_core::tools::edit_model::{ - EditModelInput, EditOperation, RemoveVariableInput, SetLoopNameInput, UpsertAuxiliaryInput, - UpsertFlowInput, UpsertStockInput, edit_model, + EditModelInput, EditOperation, RemoveVariableInput, RenameVariableInput, SetLoopNameInput, + UpsertAuxiliaryInput, UpsertFlowInput, UpsertStockInput, edit_model, }; use simlin_mcp_core::types::SourceFormat; @@ -705,6 +705,176 @@ async fn edit_model_defaults_to_first_model_when_no_main() { ); } +fn edit_input(path: &Path, operations: Vec) -> EditModelInput { + EditModelInput { + project_path: path.to_str().unwrap().to_string(), + model_name: None, + dry_run: None, + sim_specs: None, + operations: Some(operations), + } +} + +/// `(uid, x, y)` of the aux element whose name is `name` canonically on the +/// saved diagram (the layout stores a display spelling). +fn aux_element(project: &datamodel::Project, name: &str) -> Option<(i32, f64, f64)> { + project.models[0].views.iter().find_map(|v| match v { + datamodel::View::StockFlow(sf) => sf.elements.iter().find_map(|e| match e { + datamodel::ViewElement::Aux(a) if simlin_engine::canonicalize(&a.name) == name => { + Some((a.uid, a.x, a.y)) + } + _ => None, + }), + }) +} + +/// The link from the element with uid `from` into the flow named `to`. +fn link_uid_into(project: &datamodel::Project, from: i32, to: &str) -> Option { + let datamodel::View::StockFlow(sf) = &project.models[0].views[0]; + let to_uid = sf.elements.iter().find_map(|e| match e { + datamodel::ViewElement::Flow(f) if simlin_engine::canonicalize(&f.name) == to => { + Some(f.uid) + } + _ => None, + })?; + sf.elements.iter().find_map(|e| match e { + datamodel::ViewElement::Link(l) if l.from_uid == from && l.to_uid == to_uid => Some(l.uid), + _ => None, + }) +} + +/// `renameVariable` renames a variable the way the engine does: every +/// equation that reads it is rewritten, and the diagram keeps the variable's +/// element -- its uid, where it was drawn, and the link from it -- rather than +/// deleting the element and drawing a new one somewhere else, which is what +/// spelling a rename as a remove and an upsert does. +#[tokio::test] +async fn rename_variable_rewrites_readers_and_keeps_the_diagram() { + let dir = tempfile::tempdir().unwrap(); + let path = write_model(dir.path(), "model.sd.json", &minimal_project_json()); + + edit_model( + &TestFileSystemAccess, + edit_input( + &path, + vec![ + EditOperation::UpsertStock(UpsertStockInput { + name: "population".into(), + initial_equation: "100".into(), + units: None, + documentation: None, + inflows: Some(vec!["births".into()]), + outflows: None, + arrayed_equation: None, + }), + EditOperation::UpsertFlow(UpsertFlowInput { + name: "births".into(), + equation: "population * birth_rate".into(), + units: None, + documentation: None, + graphical_function: None, + arrayed_equation: None, + }), + upsert_aux("birth_rate", "0.03"), + ], + ), + ) + .await + .expect("build the model"); + let before = TestFileSystemAccess.open(&path).await.expect("open"); + let (uid, x, y) = aux_element(&before.project, "birth_rate").expect("birth_rate is drawn"); + let link = link_uid_into(&before.project, uid, "births").expect("birth_rate -> births drawn"); + + edit_model( + &TestFileSystemAccess, + edit_input( + &path, + vec![EditOperation::RenameVariable(RenameVariableInput { + from: "birth_rate".into(), + to: "fertility".into(), + })], + ), + ) + .await + .expect("rename"); + + let after = TestFileSystemAccess.open(&path).await.expect("reopen"); + assert_eq!( + variable_names(&after.project), + vec!["births", "fertility", "population"] + ); + let births = after.project.models[0] + .get_variable("births") + .expect("births"); + assert_eq!( + births.get_equation(), + Some(&datamodel::Equation::Scalar( + "population * fertility".into() + )), + "the reader's equation follows the rename" + ); + assert_eq!( + aux_element(&after.project, "fertility"), + Some((uid, x, y)), + "the renamed variable keeps its element and where it was drawn" + ); + assert_eq!( + link_uid_into(&after.project, uid, "births"), + Some(link), + "the link from it survives" + ); +} + +/// The diagram sync redraws a model's first view; any other view the project +/// carries is the author's and survives an edit exactly as it was saved. +#[tokio::test] +async fn an_edit_keeps_every_view_but_the_first() { + let dir = tempfile::tempdir().unwrap(); + let path = write_model(dir.path(), "model.sd.json", &minimal_project_json()); + edit_model( + &TestFileSystemAccess, + edit_input(&path, vec![upsert_aux("birth_rate", "0.03")]), + ) + .await + .expect("build the model"); + + let opened = TestFileSystemAccess.open(&path).await.expect("open"); + let mut project = opened.project; + let datamodel::View::StockFlow(first) = project.models[0].views[0].clone(); + let overview = datamodel::StockFlow { zoom: 0.5, ..first }; + project.models[0] + .views + .push(datamodel::View::StockFlow(overview)); + TestFileSystemAccess + .save(&path, &project, opened.source_format, None) + .await + .expect("save the second view"); + let before = TestFileSystemAccess.open(&path).await.expect("reopen"); + assert_eq!( + before.project.models[0].views.len(), + 2, + "the file holds both views" + ); + + edit_model( + &TestFileSystemAccess, + edit_input(&path, vec![upsert_aux("death_rate", "0.01")]), + ) + .await + .expect("edit"); + + let after = TestFileSystemAccess.open(&path).await.expect("reopen"); + assert_eq!(after.project.models[0].views.len(), 2, "no view is dropped"); + assert!( + aux_element(&after.project, "death_rate").is_some(), + "the first view is synced" + ); + assert!( + after.project.models[0].views[1] == before.project.models[0].views[1], + "the second view comes back as it was saved" + ); +} + #[tokio::test] async fn upsert_stock_is_full_replacement() { let dir = tempfile::tempdir().unwrap(); diff --git a/src/simlin-mcp/src/instructions.md b/src/simlin-mcp/src/instructions.md index 43a15cbf5..56ca7b978 100644 --- a/src/simlin-mcp/src/instructions.md +++ b/src/simlin-mcp/src/instructions.md @@ -16,6 +16,7 @@ CRITICAL: this is new software -- `ReadModel` and `CreateModel` are safe, but ON - `upsertFlow` -- Create or replace a flow (rate). Requires `name` and `equation`. Optional: `units`, `documentation`, `graphicalFunction`. - `upsertAuxiliary` -- Create or replace an auxiliary variable. Requires `name` and `equation`. Optional: `units`, `documentation`, `graphicalFunction`. - `removeVariable` -- Remove a variable by `name`. +- `renameVariable` -- Rename a variable `from` its current name `to` a new one. Every equation that reads it is rewritten, and its diagram element keeps its place and connectors; prefer it to removing the variable and upserting it under the new name. - `setLoopName` -- Assign a human-readable name to a feedback loop. Requires `variables` (list of variable names forming the loop) and `name`. Optional: `description`. ### Typical workflow