Conversation
Variables with both an equation and a graphical function now emit WITH LOOKUP(input, (table_data)) instead of bare lookup data. Standalone lookup definitions (no input equation) use the correct name(\n\tbody) syntax instead of name=\n\t(body). Sketch fixes: valve type-11 field 3 set to 0, cloud type-12 field 3 set to 48 (ASCII '0'), flow pipe connectors include direction flag (4=stock, 100=cloud) and pipe type (22), influence connectors include field 9=64. Flow pipe direction flags are now based on endpoint type (stock vs cloud) rather than point order.
Vensim requires a specific element ordering within flow blocks: pipe connectors (type 1, flag 22) must precede the valve (type 11) and flow label (type 10). Cloud elements (type 12) must appear before the pipe connectors that reference them. The previous ordering (valve, label, pipes; clouds at end) caused Vensim to not display most sketch elements. Reorder write_flow_element to emit pipes first, then valve, then label. Move cloud emission from the main element loop to just before their associated flow, using the cloud's flow_uid field for grouping. Add mark2 sketch ordering test that validates pipe-before-valve ordering, cloud-before-pipe ordering, and correct direction/influence flags. Update mark2 test fixture with Vensim-reformatted version. Add water and lookup_ex models to the sketch roundtrip test suite.
Real newline characters in XMILE view element names (e.g. "Effect of fish density\non catch per ship") were emitted as actual line breaks in MDL sketch output, breaking Vensim's line-by-line parser. Add format_sketch_name() that escapes actual newlines to literal \n and use it for all sketch element name writers. Also emit the .Control group header before sim spec variables, and emit per-view sketch section separators for multi-view models.
Vensim requires CRLF (\r\n) line endings. Our writer was outputting LF (\n) which caused Vensim to report "syntax error" and fail to render most sketch elements. Convert all newlines to CRLF in the final output step.
The settings marker requires a DEL byte (0x7F) between :L and <%^E!@ to match what Vensim produces and expects: :L\x7F<%^E!@. Without this byte Vensim reports "syntax error" on the file. Also fix type 26 (display range end) to use FINAL TIME instead of TIME STEP, matching all reference MDL files.
Design for improving MDL writer output fidelity so roundtripped files preserve multi-view structure, element metadata, lookup syntax, variable name casing, and equation formatting. Six implementation phases covering datamodel extensions, parser metadata capture, writer multi-view split, lookup/casing fixes, equation formatting, and integration testing.
Infrastructure for preserving Vensim-specific view element metadata during MDL roundtripping. Adds ViewElementCompat (width, height, bits) to Aux, Stock, Flow, Cloud, and Alias view elements, and a font field to StockFlow. Flow gets both compat (valve dimensions) and label_compat (attached label dimensions). Updates protobuf schema, serde conversions, and all construction sites across the codebase (json, xmile, mdl, layout, diagram, stdlib).
…rsing Add raw bits field to VensimVariable, VensimValve, and VensimComment parsed types so the original MDL sketch metadata is preserved through the conversion pipeline. Populate ViewElementCompat on all element types during view conversion: stocks, auxes, aliases, flows (valve compat + label compat), and clouds. This is the parse-side of AC2.1/AC2.2 -- the writer side will consume these compat values to reproduce original sketch dimensions instead of hardcoded defaults.
Add font field to ViewHeader and capture the $-prefixed font line during MDL sketch parsing instead of discarding it. The font string (stored without the leading $) flows through convert_view into StockFlow.font for roundtrip fidelity. In multi-view merge, the font from the first view is preserved.
When MDL specifies explicit y_range via [(xmin,ymin)-(xmax,ymax)], preserve those bounds for roundtrip fidelity instead of always recomputing from data points. This mirrors the existing x_range handling. When no explicit bounds are present (e.g. WITH LOOKUP without range prefix), y_scale continues to be computed from actual data points.
When the MDL parser merges multiple named views into a single StockFlow, it inserts Group elements as view boundary markers. The writer now detects these Group boundaries and splits the merged elements back into separate MDL views, each with its own V300 header, font line, and view title. Shared maps (valve UIDs, element positions, names) are built from all elements before splitting to ensure cross-view link and alias references resolve correctly. The preserved font from the StockFlow is emitted when present, falling back to the default Times New Roman spec. Includes 11 unit tests covering: split logic with/without Groups, element partitioning, Module filtering, font preservation, multi-view MDL output headers, and V300 separator counts.
Each sketch element write function (stock, aux, flow, cloud, alias) now uses the preserved compat dimensions when present, falling back to hardcoded defaults for elements from non-MDL sources (XMILE imports). Flow elements use both compat (valve) and label_compat (attached label).
When the AST contains a lookup(table, input) call where the first argument is a bare variable, emit it as `table name ( input )` -- the native Vensim syntax -- instead of `LOOKUP(table name, input)`. Falls back to the generic LOOKUP(...) form when the first argument is not a simple variable reference (e.g. a subscripted expression).
Build a display name map from view elements (Aux, Stock, Flow) that preserves original casing from the MDL source. Variable ident names are canonical (lowercase, underscored), but the MDL format expects the original casing on equation LHS lines. The map is built at the start of write_equations_section and threaded through write_variable_entry and all downstream functions. When no view element matches a variable ident, the fallback format_mdl_ident produces the standard lowercase space-separated form.
Short equations (name + equation <= 80 chars, single line) now use Vensim's inline format with spaces around the operator: average repayment rate = 0.03 Longer or multiline equations still use the traditional format: name= equation Lookup and graphical function entries always use multiline format regardless of length.
Add tokenize_for_wrapping() to split MDL equation text at natural break points (after commas, before binary operators, around parens/brackets) and wrap_equation_with_continuations() to insert Vensim-style backslash continuation sequences (\<newline><tab><tab>) when lines exceed 80 characters. Applied in both the scalar multiline path and the arrayed element entry path.
Ungrouped variables in MDL output now appear in alphabetical order by canonical ident, matching Vensim's deterministic ordering convention. Grouped variables continue to follow their sector-based ordering from the model groups.
The comment claimed the operator and its surrounding spaces were collected into one token, which misrepresented what the code does. The spaces stay with the adjacent operand tokens; only the bare operator character is emitted as its own token.
Adds mdl_format_roundtrip test that roundtrips mark2.mdl through parse/write/re-parse and verifies: - AC1.1: 2 views with names "1 housing" and "2 investments" - AC1.2: Per-view named elements match; valve/cloud counts match; connector counts within tolerance of documented normalizations (Time shadow elements dropped, init-only links not preserved) - AC1.4: Font specification "Verdana|10" preserved per view - AC3.1: Lookup calls use table(input) syntax, not LOOKUP() - AC3.2: Explicit lookup range bounds preserved - AC4.1: Short equations use inline format - AC4.3: Variable name casing preserved on LHS
Document the new ViewElementCompat struct and font field added to datamodel view element types for Vensim MDL roundtrip fidelity. Expand the mdl/ module description to cover writer.rs and the view/ subdir's convert.rs module.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0de40e6961
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/simlin-engine/src/mdl/writer.rs
Outdated
| let has_groups = sf | ||
| .elements | ||
| .iter() | ||
| .any(|e| matches!(e, ViewElement::Group(_))); |
There was a problem hiding this comment.
split_view_on_groups now treats the presence of any ViewElement::Group as a signal that the sketch was a merged multi-view MDL and splits at each group boundary, but Group is also the normal visual container type (see datamodel.rs docs for view_element::Group). In a single-view model that legitimately uses diagram groups (for example XMILE-imported groups), this will emit multiple *<view> sections and repartition elements purely by ordering, changing layout structure on export. The split should be gated by explicit MDL-merge metadata instead of all group elements.
Useful? React with 👍 / 👎.
Code ReviewI've reviewed this PR thoroughly. The changes are well-structured, well-tested, and accomplish the stated goal of improving MDL roundtrip fidelity. ArchitectureThe approach of threading compat metadata through the parse → datamodel → write pipeline is clean and follows the existing Key changes verified
No blocking issues foundThe patch is correct. Tests are comprehensive and cover the key scenarios including edge cases (no groups, no font, short/long equations, standalone vs embedded lookups, compat fallbacks for non-MDL sources). The behavioral changes (type 26 → stop instead of dt, cloud field 3, link field 9, settings marker) are all intentional fidelity corrections documented with comments. Overall correctness: correct ✓ |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #397 +/- ##
==========================================
+ Coverage 76.72% 76.85% +0.12%
==========================================
Files 144 144
Lines 37162 37700 +538
==========================================
+ Hits 28513 28973 +460
- Misses 8649 8727 +78 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Code ReviewI've reviewed the full PR diff across all 34 changed files, focusing on correctness of the core implementation in FindingsNo bugs found. The implementation is clean and well-tested. Notable observations (not bugs)
Verdict: CorrectThe patch should not break existing code or tests. All new fields have proper fallback defaults, the writer handles both MDL-sourced (with compat metadata) and XMILE-sourced (without) models gracefully, and the comprehensive roundtrip test suite validates end-to-end correctness. 🤖 Generated with Claude Code |
Fix escape_mdl_quoted_ident to handle newline characters in variable names. Literal newlines now become the two-character \n escape, and existing \n sequences (backslash + n from XMILE name attributes) pass through without double-escaping. This fixes the "Missing matching quote" error when converting XMILE models with multi-line variable names (e.g. fishbanks). Fix split_view_on_groups to emit empty segments for consecutive Group markers, preventing silent loss of empty views in multi-view MDL files. Add clarifying comment on is_lookup_only_equation explaining the sentinel values and their MDL parser origin. Strengthen the mdl_format_roundtrip test with per-element field-level comparison (AC1.3): for each type-10 element matched by name, compare width, height, and bits fields between original and roundtripped output. Shape comparison is skipped because Vensim allows displaying any variable type with any shape, and cross-view duplicate shapes are excluded since they become aliases during view composition.
Code Review: engine: improve MDL roundtrip fidelityI reviewed the full diff (~4500 lines of changes across 34 files). This PR adds Vensim-specific metadata preservation through parse-write roundtrips: element dimensions, font specs, lookup syntax, variable casing, multi-view splitting, and equation formatting conventions. No blocking bugs foundThe code is well-structured with comprehensive unit and integration tests covering each new behavior. The changes to protobuf schema follow additive-only conventions (new optional fields). All the MDL format fixes (CRLF line endings, 0x7F settings marker, cloud field 3 = 48, valve field 3 = 0, link influence flag = 64, type 26 = stop time) are consistent with Vensim reference files and validated by the roundtrip tests. Minor observations (non-blocking)
Overall correctness verdict: CorrectThe patch is free of bugs and blocking issues. Existing code and tests will not break. The new functionality is thoroughly tested including a comprehensive mark2.mdl roundtrip integration test that validates per-view element matching, font preservation, lookup syntax, and equation formatting. |
Remove "0" from is_lookup_only_equation sentinels. The MDL parser produces "0+0" for standalone lookups and the actual input expression for WITH LOOKUP. A real equation like WITH LOOKUP(0, body) would produce equation="0", which was incorrectly treated as a standalone lookup sentinel and roundtripped as name(body) instead of preserving the input expression. Fix view_compat_from_proto to return None when the proto message has all default field values, preventing zero-dimension sketch elements when deserializing old protos that lack compat data. Clarify escape_mdl_quoted_ident comment explaining why backslash+n sequences from XMILE name attributes are preserved as-is.
Code ReviewI reviewed this PR thoroughly, focusing on the MDL writer changes (view splitting, LOOKUP rewriting, equation formatting), datamodel extensions (ViewElementCompat, font, y-range preservation), serialization (protobuf, serde), and the integration with existing diagram/layout/XMILE code paths. Areas Examined
VerdictCorrect. No bugs found that would break existing code or tests. The changes are well-structured with proper fallbacks (compat fields default to 🤖 Generated with Claude Code |
Pass the original MDL view title to create_sector_group instead of the normalized title. View names go through make_view_names_unique during parsing (replacing special characters, deduplicating), but Group elements should carry the original title so the MDL writer can reproduce it in the sketch section header.
Code ReviewReviewed the full diff for PR #397 — MDL roundtrip fidelity improvements including Areas examined:
FindingsNo bugs found. The implementation is thorough and handles edge cases well:
Overall Correctness VerdictCorrect. The patch is free of bugs and blocking issues. Existing code and tests will not break — all new |
The control group skip logic matched only "Control" and "control" but Vensim allows other casings (e.g. ".CONTROL"). Use eq_ignore_ascii_case to handle all variants and prevent duplicate control sections in the output.
Code ReviewI reviewed all code changes in this PR (~3500 lines of non-documentation code across datamodel, serde, protobuf, MDL parser, view conversion, and MDL writer). FindingsNo bugs found that meet the threshold for flagging. The implementation is well-structured and consistent:
Overall Correctness VerdictCorrect. The patch is free of bugs and blocking issues. Existing code and tests will not break. |
Review cycle summaryRan 5 iterations of automated code review (Codex + Claude). Addressed the following feedback across 4 fix commits: Iteration 1 identified a name escaping bug, pipe connector flag concerns, and empty view splitting issues. Fixed Iteration 2 found the Iteration 3 flagged that Group elements carried normalized view titles instead of originals. Fixed Iteration 4 identified case-sensitive control group matching. Changed to Pre-existing limitations (tracked separately): view coordinate preservation during compose/split, per-view font metadata collapse during merge, sketch shape preservation for custom display shapes, pipe connector flag semantics. |
Gate view splitting on is_mdl_view_marker flag so XMILE organizational groups are not incorrectly split into separate views during MDL export. Groups created during MDL multi-view merge set this flag; XMILE groups leave it false. Emit a minimal valid sketch section when a model has no views, preventing malformed MDL output for equation-only models. Extract the lookup sentinel "0+0" into a named constant LOOKUP_SENTINEL shared between the MDL parser and writer to reduce coupling on a magic string. Split writer_tests.rs out of writer.rs to stay under the 6000-line lint threshold.
1 participant
Summary
ViewElementCompatstruct to preserve Vensim-specific element dimensions (width/height/bits) through parse-write roundtripsAcceptance Criteria (18/18 verified)
table ( input )syntax, explicit range bounds preserved, computed bounds when no explicit rangeTest plan
cargo test -p simlin-engine)cargo test -p simlin-engine --test mdl_roundtrip)docs/test-plans/2026-03-18-mdl-roundtrip-fidelity.mdfor Vensim visual verification