Merge pull request #1565 from seqeralabs/fix/symmetric-diamond-join-centre

fix(layout): close a port-fed symmetric diamond on its entry centreline

Author: pinin4fjords

src/nf_metro/layout/engine.py

@@ -1802,9 +1802,9 @@ def _place_pass_c_content(
             y_spacing,
         )
         # Stage 6.7b: carry each symmetric fork's dead-end continuation onto its
-        # branch track.  Stage 6.7c/6.7d then pin the section and its entry port
-        # to the incoming bundle -- both must follow the recenter that fixes the
-        # branch Ys they align to.
+        # branch track.  Stage 6.7c/6.7d then pin the section, its entry port and
+        # the reconvergence spine that port feeds to the incoming bundle -- all
+        # must follow the recenter that fixes the branch Ys they align to.
         _carry_symmetric_branch_continuations(graph, section_y_padding)
         _align_symfan_section_to_row_feeder(graph)
         _center_lr_entry_ports_on_fork(graph, y_spacing)

src/nf_metro/layout/phases/fan_bundles.py

@@ -1139,24 +1139,56 @@ def _align_symfan_section_to_row_feeder(graph: MetroGraph) -> None:
         shift_section(graph, section, dy=delta)
 
 
+def _entry_fork_join(
+    graph: MetroGraph, section: Section, branches: list[str]
+) -> Station | None:
+    """The station a port-fed two-way fork cleanly reconverges at, or None.
+
+    The port-fork counterpart of :func:`_iter_fork_join_diamonds`, which admits
+    only station forks.  Requiring the two branches to be the join's *only*
+    feeders keeps it the point where the whole bundle reunites, so no third
+    feeder has a competing claim on its Y.
+    """
+    if len(branches) != 2:
+        return None
+    succs = [_in_section_ontrack_successors(graph, section, b) for b in branches]
+    if succs[0] != succs[1] or len(succs[0]) != 1:
+        return None
+    join_id = succs[0][0]
+    feeders = {
+        e.source
+        for e in graph.edges_to(join_id)
+        if _is_in_section_on_track(graph.station_for_edge_source(e), section.id)
+    }
+    if feeders != set(branches):
+        return None
+    return graph.stations[join_id]
+
+
 def _center_lr_entry_ports_on_fork(graph: MetroGraph, y_spacing: float) -> None:
-    """Centre an LR entry port on the two-way fork it fans into.
+    """Centre an LR entry port, and the join it reconverges at, on its two-way fork.
 
     Under ``diamond_style: symmetric`` a section's LR entry port that fans into
     branches at exactly two distinct Ys should sit at their midpoint, so the
     fork reads symmetric about the incoming bundle and the run from the feeding
     section arrives straight.  Otherwise the port stays pinned to whichever
     branch the section layout seated it on (e.g. the top branch of a
-    reconverging diamond), leaving the inter-section run kinked.  Only the port
-    moves; the branch stations keep their places.
+    reconverging diamond), leaving the inter-section run kinked.
+
+    When the fork reconverges at a clean in-section join, that join and its
+    linear trunk continuation ride the same midpoint centreline, so the diamond
+    closes symmetrically and the trunk leaves it straight.  A station fork gets
+    this for free -- the join inherits the fork's own trunk track -- but a port
+    fork has no station on the centreline to inherit from, so the join would
+    otherwise stay on the track of whichever branch fed it first, turning the
+    diamond into a lopsided fan.  The branch stations themselves keep their
+    places either way.
 
     An off-grid midpoint (branches an odd number of slots apart) is only a
-    valid seat when the fork reconverges at an in-section join: that join
-    already sits at the midpoint, so the port matches it and the exit fork it
-    mirrors.  A non-reconverging dead-end fan instead keeps its port on the
-    feeder trunk with the branches straddling half a slot either side, so
-    seating the port on the off-grid midpoint there would drag the row's trunk
-    off the inter-section run.
+    valid seat when the fork reconverges: a non-reconverging dead-end fan
+    instead keeps its port on the feeder trunk with the branches straddling
+    half a slot either side, so seating the port on the off-grid midpoint there
+    would drag the row's trunk off the inter-section run.
     """
     if graph.diamond_style != "symmetric":
         return
@@ -1181,10 +1213,17 @@ def _center_lr_entry_ports_on_fork(graph: MetroGraph, y_spacing: float) -> None:
             midpoint = (branch_ys[0] + branch_ys[1]) / 2.0
             if abs(graph.stations[pid].y - midpoint) >= 1.0:
                 _set_port_y(graph, pid, midpoint)
+            join = _entry_fork_join(graph, section, branches)
+            if join is not None and abs(join.y - midpoint) >= 1.0:
+                join.y = midpoint
+                _pull_continuation_onto(graph, section, join)
             if off_grid:
-                # The port (and the diamond spine it feeds) rides the off-grid
-                # midpoint centreline, like the join it reconverges at, so the
-                # grid snap must treat it as half-grid, not re-seat it on a row.
+                # The port rides the off-grid midpoint centreline, so the grid
+                # snap must treat it as half-grid, not re-seat it on a row.  The
+                # spine it feeds is deliberately left out: the snap runs earlier
+                # in the pipeline, so entries here would reach it only on a
+                # subsequent layout pass, where a whole spine of them would
+                # outvote the branch rows for the group's grid origin.
                 graph.half_grid_station_ids.add(pid)
 
 

tests/test_layout_invariants.py

@@ -9806,6 +9806,23 @@ def _direct_fork_children(
     return triples
 
 
+def _shared_successors(graph: MetroGraph, a: Station, b: Station) -> list[Station]:
+    """The real, on-track stations both *a* and *b* feed directly."""
+    targets = {
+        sid: [
+            graph.station_for_edge_target(e).id
+            for e in graph.edges_from(sid)
+            if not (t := graph.station_for_edge_target(e)).is_port
+            and not t.is_hidden
+            and not t.off_track
+        ]
+        for sid in (a.id, b.id)
+    }
+    return [
+        graph.stations[sid] for sid in sorted(set(targets[a.id]) & set(targets[b.id]))
+    ]
+
+
 _SYMMETRIC_DEADEND_FANOUT_FIXTURES = [
     "topologies/symmetric_deadend_fanout.mmd",
     "topologies/symmetric_deadend_fanout_relay.mmd",
@@ -9949,21 +9966,27 @@ def test_symmetric_fork_entry_port_stays_on_feeder_trunk(fixture):
 
 _SYMMETRIC_RECONVERGING_DIAMOND_FIXTURES = [
     "topologies/symmetric_diamond_odd_slot_entry.mmd",
+    "topologies/paired_input_fan_branch_tree.mmd",
+    "topologies/rowmate_tb_side_entry_top_align.mmd",
 ]
 
 
 @pytest.mark.parametrize("fixture", _SYMMETRIC_RECONVERGING_DIAMOND_FIXTURES)
 def test_symmetric_reconverging_diamond_entry_port_centres_on_fork(fixture):
-    """A reconverging symmetric diamond centres its LR entry port on the fork.
+    """A reconverging symmetric diamond closes on its LR entry port's centreline.
 
     When a ``diamond_style: symmetric`` fork reconverges at an in-section join,
-    the join sits at the branch midpoint, so the entry port fanning into that
-    fork must sit there too -- even when the branches are an odd number of grid
-    slots apart and the midpoint lands off-grid.  Otherwise the entry fork reads
-    lopsided (the run lands level with one branch, the other kinks off it) while
-    the exit fork about the join is symmetric.  The off-grid-midpoint skip
-    applies only to a non-reconverging dead-end fan, whose port stays on its
-    feeder trunk.
+    the branches straddle the entry port's Y and the join returns to it, so the
+    diamond reads as a bubble on the incoming bundle -- even when the branches
+    are an odd number of grid slots apart and the centreline lands off-grid.
+    Otherwise the diamond reads lopsided: the run lands level with one branch
+    and the other kinks off it, the two legs differing in length.  The
+    off-grid-midpoint skip applies only to a non-reconverging dead-end fan,
+    whose port stays on its feeder trunk.
+
+    A station fork closes on its centreline for free, the join inheriting the
+    fork's own trunk track.  A port fork has no station on the centreline to
+    inherit from, so its join has to be seated there explicitly.
     """
     graph = _layout_diamond(fixture, "symmetric")
     checked = 0
@@ -9983,6 +10006,12 @@ def test_symmetric_reconverging_diamond_entry_port_centres_on_fork(fixture):
             f"{fixture}: entry port {fork.id!r} station y={fork.y:.1f} but port "
             f"record y={None if port is None else round(port.y, 1)} -- desynced"
         )
+        for join in _shared_successors(graph, lo, hi):
+            assert abs(join.y - fork.y) < 1.0, (
+                f"{fixture}: diamond fed by entry port {fork.id!r} rejoins at "
+                f"{join.id!r} (y={join.y:.1f}), off the port's centreline "
+                f"y={fork.y:.1f} -- the diamond closes lopsided"
+            )
         checked += 1
     assert checked, f"{fixture}: no entry-port fork found to check"
 

tests/test_symmetric_merge_median.py

@@ -1,17 +1,21 @@
-"""A symmetric-mode multi-track merge anchors on the median of its feeders.
+"""A symmetric-mode multi-track merge anchors between its innermost feeders.
 
 When 3+ metro lines on distinct tracks converge on a single shared station and
 no single predecessor already carries the full bundle, the station is a genuine
-multi-track merge.  In ``diamond_style: symmetric`` the merge must sit on the
-median predecessor track so each feeder bends toward it by the least amount --
+multi-track merge.  In ``diamond_style: symmetric`` the merge must sit in the
+middle of its feeder spread so each feeder bends toward it by the least amount --
 not snap to the first-declared line's (extreme) track, which forces every other
 feeder into a longer detour (#1277).
+
+The least-detour seat is the closed band between the two innermost feeders: an
+odd count collapses it to the single median track, and across an even count's
+band the total feeder bend is constant, so anywhere in it is equally good.
 """
 
 from __future__ import annotations
 
 from pathlib import Path
-from statistics import median_low
+from statistics import median_high, median_low
 
 import networkx as nx
 import pytest
@@ -70,15 +74,16 @@ def _convergence_merges(graph) -> list[tuple[str, list[float]]]:
 
 
 @pytest.mark.parametrize("name", _SYMMETRIC_FIXTURES)
-def test_symmetric_merge_sits_on_median_feeder_track(name: str) -> None:
+def test_symmetric_merge_sits_between_its_innermost_feeders(name: str) -> None:
     graph = _layout(name)
     for sid, pred_ys in _convergence_merges(graph):
-        expected = median_low(pred_ys)
+        lo, hi = median_low(pred_ys), median_high(pred_ys)
         actual = graph.stations[sid].y
-        assert abs(actual - expected) <= SAME_COORD_TOLERANCE, (
-            f"{name}: merge {sid!r} sits at y={actual:.1f}, not on the median "
-            f"feeder track y={expected:.1f} (feeders {pred_ys}) -- snapped to an "
-            "extreme track, forcing other feeders into longer detours"
+        assert lo - SAME_COORD_TOLERANCE <= actual <= hi + SAME_COORD_TOLERANCE, (
+            f"{name}: merge {sid!r} sits at y={actual:.1f}, outside the "
+            f"[{lo:.1f}, {hi:.1f}] band between its innermost feeders "
+            f"(feeders {pred_ys}) -- it snapped toward an extreme track, "
+            "forcing other feeders into longer detours"
         )