compat: add XMILE straight-line link compatibility

[bpowers]

Summary

• When exporting a LinkShape::Straight link, calculate the straight-line angle from element positions and set it as the angle attribute (for compatibility with Stella and other SD software)
• When importing, detect if an angle exactly matches the calculated straight-line angle and convert to LinkShape::Straight (using a tight 0.01 degree epsilon for roundtrip fidelity)
• Maintains byte-for-byte identical XMILE output after roundtrip

Test plan

• Existing roundtrip tests pass with byte-for-byte comparison
• New unit tests for export angle calculation
• New unit tests for import straight-line detection
• Full test suite passes

[claude]

Review of PR #203: XMILE Straight-Line Link Compatibility

Thank you for this PR! The implementation addresses an important compatibility issue with other SD software. Overall, the code is well-structured and thoroughly tested.

Strengths

1. Excellent test coverage - comprehensive unit tests for export, import, roundtrip fidelity, and edge cases
2. Well-documented approach - clear comments explaining the tight epsilon (0.01 degrees) for roundtrip fidelity
3. Backwards compatible - gracefully handles missing position data with fallback behavior
4. Clean architecture - building a position map before converting ViewObjects avoids multiple passes

Code Quality Observations

Potential Issue: Angle Wraparound Edge Case

In is_straight_line_angle (line 1279-1291), the wraparound logic may need additional testing. Rust's atan2 returns values in [-180, 180] after to_degrees(), but XMILE angles appear to be in canvas coordinates (positive values). Consider adding a test for links near ±180 degrees.

Code Duplication

Two functions get element positions:

• get_element_position (line 1250) works with datamodel::StockFlow
• ViewObject::position (line 2682) works with ViewObject

Consider whether this duplication is necessary or if one could be refactored.

Minor Nit

Line 1250: get_element_position could use .find_map() for more idiomatic Rust instead of a manual loop.

Testing Suggestions

1. Add test for angle wraparound near ±180 degrees
2. Test missing positions (when uid not in position map) to verify fallback to is_straight: true
3. Add integration test that verifies byte-for-byte XMILE equality after roundtrip

Summary

Solid implementation that solves a real compatibility problem. Main suggestion is additional test coverage for edge cases. Code follows project conventions and best practices.

Recommendation: Approve with minor suggestions for additional test coverage.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 662a73da4f

ℹ️ 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".

[chatgpt-codex-connector]

Convert XMILE angle before straight-line comparison

Here the straight-line detection compares the raw angle attribute from the XMILE file against a canvas-coordinate angle from calculate_straight_line_angle. XMILE connector angles are in the XMILE coordinate system (see the existing conversion helpers used elsewhere in this module), so values like 270 for a vertical-down link won’t match the computed canvas 90 and will be incorrectly imported as Arc instead of Straight. This breaks the new compatibility behavior for XMILE files that use standard 0–360° angles, because straight connectors will remain curved on import unless is_straight is explicitly set.

Useful? React with 👍 / 👎.

[codecov]

Codecov Report

❌ Patch coverage is 93.10345% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.99%. Comparing base (68a4c1d) to head (5f96933).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/simlin-compat/src/xmile.rs	93.10%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #203      +/-   ##
==========================================
+ Coverage   75.98%   75.99%   +0.01%     
==========================================
  Files          48       48              
  Lines       15320    15402      +82     
==========================================
+ Hits        11641    11705      +64     
- Misses       3679     3697      +18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[bpowers]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a4f8f57603

ℹ️ 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".

[chatgpt-codex-connector]

Convert link angles to XMILE CCW coordinates

XMILE specifies connector angles as “0 is 3 o’clock and angles increase counter‑clockwise” (doc/xmile-v1.0.html lines 7928–7930). Internal view coordinates use a canvas system with +Y downward, so calculate_straight_line_angle/normalize_angle produce clockwise angles. Writing LinkShape::Arc angles directly (angle passthrough here) means exported XMILE arcs are mirrored vertically and any non‑horizontal connectors will render with the wrong curvature in external tools; similarly, re‑imported arcs/straight detection will be compared in the wrong coordinate system. This regression shows up as soon as an arc or straight line is not exactly horizontal.

Useful? React with 👍 / 👎.

[claude]

Code Review: XMILE Straight-Line Link Compatibility

Summary

This PR adds bidirectional conversion between Simlin's internal LinkShape::Straight representation and XMILE angle attributes for compatibility with other SD software like Stella. The implementation is generally solid with good test coverage, but there are a few important issues to address.

---

Critical Issues

1. Zero-Length Vector Handling (High Priority)

Location: src/simlin-compat/src/xmile.rs:1252-1256

Issue: When from and to positions are identical (dx=0, dy=0), atan2(0.0, 0.0) returns 0.0, which is mathematically undefined behavior. While Rust's f64::atan2 handles this case without panicking, it's semantically incorrect to calculate an angle for a zero-length link.

Impact: Links with coincident endpoints will silently export with angle=0, which could cause roundtrip issues or unexpected behavior in other software.

Recommendation: Add an early return or assertion to handle this edge case by making the function return Option and returning None when dx and dy are both near zero.

---

2. Missing Test for Negative Angles and Wraparound

Issue: While test_normalize_angle() tests angle normalization, there's no test verifying that the wraparound logic in is_straight_line_angle() (lines 1275-1276) actually works correctly for angles near 0°/360°.

Recommendation: Add a test case that verifies -0.005 degrees normalizes and matches a calculated angle near 0.

---

Code Quality Issues

3. Inconsistent Error Handling Philosophy

Location: src/simlin-compat/src/xmile.rs:2053-2055

Issue: Per CLAUDE.md guidelines: Strongly prefer .unwrap() over .unwrap_or_default() and it is valuable to understand when our assumptions are wrong. The fallback to is_straight=true silently hides a data integrity issue.

Impact: If element positions are missing (possibly due to data corruption or parsing errors), the export will succeed but produce incorrect XMILE that might not match the visual representation.

Recommendation: Consider using .unwrap() with a descriptive panic message instead of the silent fallback.

---

4. Performance: Redundant HashMap Allocation in Tests

Location: src/simlin-compat/src/xmile.rs:2029-2037

Issue: The link_from_xmile_with_view helper rebuilds the position HashMap on every call. While this is test-only code, it's creating technical debt.

Impact: Negligible for current tests, but could slow down if more link conversion tests are added.

---

Positive Aspects

1. Excellent commit messages - Both commits follow the project's commit message style perfectly with clear why explanations in the body.

2. Comprehensive test coverage - The new tests cover:

   • Export angle calculation (horizontal and diagonal)
   • Import straight-line detection
   • Curved link preservation
   • Roundtrip fidelity with tight epsilon

3. Well-documented code - Doc comments clearly explain the purpose and behavior of each function, including edge cases like the 1°/359° wraparound.

4. Smart epsilon choice - The 0.01-degree epsilon strikes a good balance between roundtrip fidelity and floating-point tolerance.

5. Simplified angle handling - The second commit's refactoring to use a single normalize_angle function instead of bidirectional conversion functions is a clear improvement in maintainability.

6. Proper separation of concerns - The position lookup is cleanly separated from the angle calculation logic.

---

Minor Suggestions

5. Documentation: Clarify Coordinate System

The comments mention canvas coordinates but it might be clearer to explicitly state that:

• X-axis points right
• Y-axis points down (standard screen coordinates)
• Angles are measured clockwise from the positive X-axis

This would help future maintainers understand why atan2(dy, dx) is correct.

6. Consider Adding Integration Test

While unit tests are comprehensive, consider adding an integration test that:

1. Creates a model with a straight link
2. Exports to XMILE
3. Re-imports
4. Verifies the link remains LinkShape::Straight

This would validate the full roundtrip behavior mentioned in the PR description.

---

Performance Considerations

The changes add computational overhead during XMILE import/export:

• Export: O(n) position lookup for each straight link (acceptable)
• Import: O(1) HashMap lookup and angle calculation per link (excellent)

The position HashMap is built once during view conversion, which is optimal.

---

Security Concerns

None identified. The code:

• Handles NaN/infinity from atan2 safely (normalized to valid angles)
• Validates UIDs through Option types
• No unsafe operations or external input parsing

---

Recommendation

Approve with changes requested:

1. Must fix: Handle zero-length vectors in calculate_straight_line_angle (edit variable names #1)
2. Should fix: Add wraparound test (editor overlay #2)
3. Consider: Replace silent fallback with explicit error handling (server-side support #3)

The core implementation is sound and the test coverage is good. The issues identified are edge cases that should be addressed before merge.

[claude]

Code Review

Summary

This PR adds XMILE compatibility for straight-line links by calculating angles on export and detecting straight lines on import. The implementation is well-designed with good test coverage.

Positive Aspects

Strong Design Decisions:

• ✅ Tight epsilon (0.01°) for roundtrip fidelity is excellent - ensures byte-for-byte XMILE compatibility while still detecting true straight lines
• ✅ Handling angle wraparound (e.g., -179° vs 179°) in is_straight_line_angle shows attention to edge cases
• ✅ Clear separation of coordinate systems with consistent normalization at XMILE boundaries
• ✅ Comprehensive test coverage including export, import, roundtrip, and edge cases

Code Quality:

• ✅ Well-structured helper functions with clear single responsibilities
• ✅ Good documentation explaining the "why" (compatibility with Stella) and not just the "what"
• ✅ Thorough unit tests covering multiple scenarios

Issues Found

1. Critical Logic Error in link_from_xmile (src/simlin-compat/src/xmile.rs:1988-1997)

The original link_from_xmile function has an incorrect angle conversion:

// Line 1988-1990: This is WRONG
let xmile_angle = normalize_angle(v.angle.unwrap_or(0.0));
datamodel::view_element::LinkShape::Arc(convert_angle_from_xmile_to_canvas(
    xmile_angle,
))

Problem: The code normalizes the angle FIRST, then converts from XMILE to canvas. But the angle in the XMILE file is already in XMILE format [0, 360). The conversion function signature says it takes XMILE format as input. So this should be:

// Normalize first (in case file has angles outside [0, 360))
let xmile_angle = normalize_angle(v.angle.unwrap_or(0.0));
// Then convert to canvas format for internal use
let canvas_angle = convert_angle_from_xmile_to_canvas(xmile_angle);
datamodel::view_element::LinkShape::Arc(canvas_angle)

Wait - looking more carefully at the diff, I see this WAS actually changed correctly. The old code had:

convert_angle_from_canvas_to_xmile(v.angle.unwrap_or(0.0))

And the new code has:

let xmile_angle = normalize_angle(v.angle.unwrap_or(0.0));
convert_angle_from_xmile_to_canvas(xmile_angle)

This is correct! The old code was treating incoming XMILE angles as canvas angles. The new code correctly treats them as XMILE angles. ✅

2. Inconsistent Variable Naming (src/simlin-compat/src/xmile.rs:1269-1270)

In test_convert_angles, the variable names changed from input/output to xmile/canvas:

for (xmile, canvas) in cases {
    assert_eq\!(*canvas, convert_angle_from_xmile_to_canvas(*xmile));
    assert_eq\!(*xmile, convert_angle_from_canvas_to_xmile(*canvas));
}

This is actually an improvement! The new names are much clearer about what coordinate system each value represents. ✅

3. Potential Issue: Missing Position Validation

In get_element_position (lines 1256-1269), the function returns None if the element isn't found. This is good defensive programming. However, in Link::from (lines 2018-2021), there's a fallback to is_straight = true when positions aren't found:

if let (Some((from_x, from_y)), Some((to_x, to_y))) = (...) {
    // Calculate angle
} else {
    // Fallback if positions aren't found
    (Some(true), None, None)
}

Concern: This fallback uses is_straight = true, which the PR description says other software like Stella doesn't support. If positions can't be found (which could happen with malformed data), this will generate incompatible XMILE.

Recommendation: Either:

• Log a warning when this happens, or
• Use a default angle (e.g., 0°) instead of is_straight = true, or
• Document why this fallback is acceptable

4. Test Brittleness in test_link_roundtrip (src/simlin-compat/src/xmile.rs:2142-2144)

The test case changed from:

shape: LinkShape::Arc(351.3),

to:

shape: LinkShape::Arc(-45.0), // canvas format

This is a meaningful change - the test now uses canvas format consistently. But 351.3° and -45° are different angles (351.3° ≈ -8.7°, not -45°).

Question: Was this intentional? If so, the commit message should explain why the test case value changed. If not, this could indicate a conversion error.

Looking at the conversion:

• Canvas -45° → XMILE: 360° - ((-45 + 360) % 360) = 360° - 315° = 45°
• XMILE 45° → Canvas: -45° ✅

So the new test case is internally consistent. The old test case (351.3°) was probably in the wrong format.

5. Documentation: Angle Coordinate Systems

The code has good inline comments, but consider adding a module-level doc comment explaining the two coordinate systems:

• Canvas format: [-180, 180], Y-down, used internally throughout simlin
• XMILE format: [0, 360), counter-clockwise, Y-up, used in XMILE files

This would help future developers understand the conversions.

Performance Considerations

Position Map Building (src/simlin-compat/src/xmile.rs:3067-3076):
When converting a View to datamodel::View, the code builds a position HashMap from all ViewObjects:

let positions: std::collections::HashMap<i32, (f64, f64)> = v
    .objects
    .iter()
    .filter_map(|obj| {
        let uid = obj.uid()?;
        let pos = obj.position()?;
        Some((uid, pos))
    })
    .collect();

This is O(n) which is fine. However, for very large models with thousands of elements, this creates a complete position map even though only Links need it.

Impact: Low - this is only during import/export, not simulation. The O(n) cost is unavoidable since we need to iterate through objects anyway.

Recommendation: No change needed, but worth documenting that the position map is built once per view conversion.

Security Considerations

No security issues identified. The code:

• Uses unwrap_or with sensible defaults instead of bare unwrap()
• Handles edge cases (missing positions, wraparound angles)
• Normalizes input angles to prevent unexpected behavior

Test Coverage Assessment

Excellent test coverage:

• ✅ test_normalize_angle: Tests normalization edge cases
• ✅ test_convert_angles: Tests bidirectional conversion
• ✅ test_straight_link_export_calculates_angle: Tests export angle calculation
• ✅ test_straight_link_export_diagonal: Tests non-axis-aligned angles
• ✅ test_straight_link_import_detects_straight: Tests import detection
• ✅ test_curved_link_import_stays_curved: Tests Arc preservation
• ✅ test_straight_link_import_roundtrip_fidelity: Tests epsilon behavior
• ✅ test_link_roundtrip: Tests full roundtrip

Suggested Additional Test:
Consider adding a test for the fallback case where element positions aren't found:

#[test]
fn test_straight_link_export_missing_positions() {
    let view = StockFlow {
        elements: vec\![], // No elements, positions can't be found
        view_box: Default::default(),
        zoom: 1.0,
    };
    
    let link = datamodel::view_element::Link {
        uid: 10,
        from_uid: 999, // Non-existent
        to_uid: 888,   // Non-existent
        shape: LinkShape::Straight,
    };
    
    let xmile_link = Link::from(link, &view);
    // Verify the fallback behavior
}

Minor Nitpicks

1. Line 1275: The function signature comment could be more precise about the epsilon handling
2. Line 3067: Consider extracting the position map building into a separate function for testability

Verdict

Approve with Minor Suggestions

The PR is well-implemented with strong test coverage and thoughtful design decisions. The main suggestion is to address the fallback behavior when element positions aren't found (Issue #3). The tight epsilon for roundtrip fidelity is particularly well-reasoned.

The code follows Rust best practices and the repository's conventions. The commit messages clearly explain the "why" behind the changes.

Action Items

• Review fallback behavior when element positions not found (lines 2018-2021)
• Verify test case change from 351.3° to -45° was intentional (line 2144)
• Consider adding module-level documentation about coordinate systems
• Consider adding test for missing position fallback case

Great work overall! The implementation is solid and the testing is thorough.

[bpowers]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5f969332a5

ℹ️ 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".

[chatgpt-codex-connector]

Fall back to names when UIDs are missing

Straight-line detection only consults from_uid/to_uid to look up positions, so any XMILE file that omits those attributes (common in the repo, e.g. test/test-models/samples/teacup/teacup_w_diagram.xmile uses only <from>/<to> names) will never satisfy this lookup and will still be imported as Arc. That means the new “angle-matches-straight” path won’t run for typical XMILE inputs, and straight connectors will continue to render curved. Consider falling back to resolving positions by LinkEnd::Named (or alias) when UIDs are absent so the feature works with name-only XMILE files.

Useful? React with 👍 / 👎.