fix: address PR review issues for schematic analysis tools

Eliminate repeated file parsing by extracting _extract_lib_symbols helper
that walks already-parsed sexp_data once instead of re-reading the file
per symbol via PinLocator. Support diagonal wire overlap detection using
cross-product parallelism and 1D projection. Fix wire region inclusion to
use AABB intersection for pass-through wires. Normalize view region
coordinates. Clarify tolerance docstrings across Python, TS, and schema.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

python/commands/schematic_analysis.py

@@ -150,6 +150,33 @@ def _parse_no_connects(sexp_data: list) -> Set[Tuple[float, float]]:
     return positions
 
 
+def _extract_lib_symbols(sexp_data: list) -> Dict[str, Dict[str, Dict]]:
+    """
+    Walk the lib_symbols section of already-parsed sexp_data and return
+    pin definitions for every symbol definition.
+
+    Returns:
+        Dict mapping lib_id → pin definitions (pin_number → pin_data dict).
+    """
+    lib_symbols_section = None
+    for item in sexp_data:
+        if (isinstance(item, list) and len(item) > 0
+                and item[0] == Symbol("lib_symbols")):
+            lib_symbols_section = item
+            break
+
+    if not lib_symbols_section:
+        return {}
+
+    result: Dict[str, Dict[str, Dict]] = {}
+    for item in lib_symbols_section[1:]:
+        if (isinstance(item, list) and len(item) > 1
+                and item[0] == Symbol("symbol")):
+            symbol_name = str(item[1]).strip('"')
+            result[symbol_name] = PinLocator.parse_symbol_definition(item)
+    return result
+
+
 # ---------------------------------------------------------------------------
 # Geometry helpers
 # ---------------------------------------------------------------------------
@@ -345,7 +372,7 @@ def find_unconnected_pins(schematic_path: Path) -> List[Dict[str, Any]]:
                     return True
         return False
 
-    locator = PinLocator()
+    lib_pin_defs = _extract_lib_symbols(sexp_data)
     unconnected = []
 
     for sym in symbols:
@@ -354,7 +381,7 @@ def find_unconnected_pins(schematic_path: Path) -> List[Dict[str, Any]]:
         if sym["is_power"] or ref.startswith("_TEMPLATE") or not ref:
             continue
 
-        pin_defs = locator.get_symbol_pins(schematic_path, sym["lib_id"])
+        pin_defs = lib_pin_defs.get(sym["lib_id"], {})
         if not pin_defs:
             continue
 
@@ -394,7 +421,7 @@ def find_overlapping_elements(
 
     Args:
         schematic_path: Path to .kicad_sch file
-        tolerance: Distance in mm below which elements are considered overlapping
+        tolerance: Distance threshold in mm for label proximity and wire collinearity checks. Symbol overlap uses bounding-box intersection.
 
     Returns dict: {overlappingSymbols, overlappingLabels, overlappingWires, totalOverlaps}
     """
@@ -407,7 +434,7 @@ def find_overlapping_elements(
     overlapping_labels = []
     overlapping_wires = []
 
-    locator = PinLocator()
+    lib_pin_defs = _extract_lib_symbols(sexp_data)
 
     # --- Symbol-symbol overlap using bounding-box intersection (O(n²)) ---
     non_template_symbols = [s for s in symbols if not s["reference"].startswith("_TEMPLATE") and s["reference"]]
@@ -415,7 +442,7 @@ def find_overlapping_elements(
     # Pre-compute bounding boxes for all non-template symbols
     symbol_bboxes = []
     for sym in non_template_symbols:
-        pin_defs = locator.get_symbol_pins(schematic_path, sym["lib_id"])
+        pin_defs = lib_pin_defs.get(sym["lib_id"], {})
         bbox = None
         if pin_defs:
             bbox = _compute_symbol_bbox_direct(sym, pin_defs)
@@ -490,30 +517,43 @@ def _check_wire_overlap(
     """
     Check if two wire segments are collinear and overlapping.
 
+    Works for horizontal, vertical, and diagonal wires. Uses direction
+    vectors, cross-product parallelism, point-to-line distance for
+    collinearity, and 1D projection overlap.
+
     Returns overlap info dict or None.
     """
     s1, e1 = w1["start"], w1["end"]
     s2, e2 = w2["start"], w2["end"]
 
-    # Check horizontal collinearity
-    if abs(s1[1] - e1[1]) < tolerance and abs(s2[1] - e2[1]) < tolerance:
-        if abs(s1[1] - s2[1]) < tolerance:
-            # Both horizontal, same Y
-            min1, max1 = min(s1[0], e1[0]), max(s1[0], e1[0])
-            min2, max2 = min(s2[0], e2[0]), max(s2[0], e2[0])
-            if min1 < max2 and min2 < max1:
-                return {
-                    "wire1": {"start": {"x": s1[0], "y": s1[1]}, "end": {"x": e1[0], "y": e1[1]}},
-                    "wire2": {"start": {"x": s2[0], "y": s2[1]}, "end": {"x": e2[0], "y": e2[1]}},
-                    "type": "collinear_overlap",
-                }
+    d1 = (e1[0] - s1[0], e1[1] - s1[1])
+    d2 = (e2[0] - s2[0], e2[1] - s2[1])
 
-    # Check vertical collinearity
-    if abs(s1[0] - e1[0]) < tolerance and abs(s2[0] - e2[0]) < tolerance:
-        if abs(s1[0] - s2[0]) < tolerance:
-            # Both vertical, same X
-            min1, max1 = min(s1[1], e1[1]), max(s1[1], e1[1])
-            min2, max2 = min(s2[1], e2[1]), max(s2[1], e2[1])
+    len1 = math.sqrt(d1[0] ** 2 + d1[1] ** 2)
+    len2 = math.sqrt(d2[0] ** 2 + d2[1] ** 2)
+    if len1 < 1e-12 or len2 < 1e-12:
+        return None  # degenerate zero-length segment
+
+    # Cross product to check parallel
+    cross = d1[0] * d2[1] - d1[1] * d2[0]
+    if abs(cross) > tolerance * max(len1, len2):
+        return None  # not parallel
+
+    # Point-to-line distance: s2 relative to line through s1 along d1
+    ds = (s2[0] - s1[0], s2[1] - s1[1])
+    perp_dist = abs(ds[0] * d1[1] - ds[1] * d1[0]) / len1
+    if perp_dist > tolerance:
+        return None  # parallel but offset
+
+    # Project onto d1 direction for 1D overlap check
+    u1 = (d1[0] / len1, d1[1] / len1)
+    proj_s1 = s1[0] * u1[0] + s1[1] * u1[1]
+    proj_e1 = e1[0] * u1[0] + e1[1] * u1[1]
+    proj_s2 = s2[0] * u1[0] + s2[1] * u1[1]
+    proj_e2 = e2[0] * u1[0] + e2[1] * u1[1]
+
+    min1, max1 = min(proj_s1, proj_e1), max(proj_s1, proj_e1)
+    min2, max2 = min(proj_s2, proj_e2), max(proj_s2, proj_e2)
     if min1 < max2 and min2 < max1:
         return {
             "wire1": {"start": {"x": s1[0], "y": s1[1]}, "end": {"x": e1[0], "y": e1[1]}},
@@ -549,7 +589,7 @@ def get_elements_in_region(
     wires = _parse_wires(sexp_data)
     labels = _parse_labels(sexp_data)
 
-    locator = PinLocator()
+    lib_pin_defs = _extract_lib_symbols(sexp_data)
 
     # Symbols: include if position is within bounds
     region_symbols = []
@@ -564,7 +604,7 @@ def get_elements_in_region(
                 "isPower": sym["is_power"],
             }
             # Include pin positions (compute directly to handle unannotated duplicates)
-            pin_defs = locator.get_symbol_pins(schematic_path, sym["lib_id"])
+            pin_defs = lib_pin_defs.get(sym["lib_id"], {})
             if pin_defs:
                 pin_positions = _compute_pin_positions_direct(sym, pin_defs)
                 if pin_positions:
@@ -574,12 +614,13 @@ def get_elements_in_region(
                     }
             region_symbols.append(entry)
 
-    # Wires: include if ANY endpoint is within bounds
+    # Wires: include if any part of the wire intersects the region
     region_wires = []
     for w in wires:
         s, e = w["start"], w["end"]
         if (_point_in_rect(s[0], s[1], min_x, min_y, max_x, max_y) or
-                _point_in_rect(e[0], e[1], min_x, min_y, max_x, max_y)):
+                _point_in_rect(e[0], e[1], min_x, min_y, max_x, max_y) or
+                _line_segment_intersects_aabb(s[0], s[1], e[0], e[1], min_x, min_y, max_x, max_y)):
             region_wires.append({
                 "start": {"x": s[0], "y": s[1]},
                 "end": {"x": e[0], "y": e[1]},
@@ -669,7 +710,7 @@ def find_wires_crossing_symbols(schematic_path: Path) -> List[Dict[str, Any]]:
     symbols = _parse_symbols(sexp_data)
     wires = _parse_wires(sexp_data)
 
-    locator = PinLocator()
+    lib_pin_defs = _extract_lib_symbols(sexp_data)
     margin = 0.5  # mm margin to shrink bbox (avoids false positives at pin tips)
     pin_tolerance = 0.05  # mm
 
@@ -682,7 +723,7 @@ def find_wires_crossing_symbols(schematic_path: Path) -> List[Dict[str, Any]]:
         if sym["is_power"] or ref.startswith("_TEMPLATE") or not ref:
             continue
 
-        pin_defs = locator.get_symbol_pins(schematic_path, sym["lib_id"])
+        pin_defs = lib_pin_defs.get(sym["lib_id"], {})
         if not pin_defs:
             continue
 

python/kicad_interface.py

@@ -2584,6 +2584,8 @@ class KiCADInterface:
             y1 = float(params.get("y1", 0))
             x2 = float(params.get("x2", 297))
             y2 = float(params.get("y2", 210))
+            x1, x2 = min(x1, x2), max(x1, x2)
+            y1, y2 = min(y1, y2), max(y1, y2)
             out_format = params.get("format", "png")
             width = int(params.get("width", 800))
             height = int(params.get("height", 600))

python/schemas/tool_schemas.py

@@ -1754,7 +1754,7 @@ SCHEMATIC_TOOLS = [
                 },
                 "tolerance": {
                     "type": "number",
-                    "description": "Distance in mm below which elements are considered overlapping (default: 0.5)"
+                    "description": "Distance threshold in mm for label proximity and wire collinearity checks. Symbol overlap uses bounding-box intersection. (default: 0.5)"
                 }
             },
             "required": ["schematicPath"]

python/tests/test_schematic_analysis.py

@@ -25,10 +25,12 @@ from commands.schematic_analysis import (
     _parse_symbols,
     _parse_no_connects,
     _load_sexp,
+    _extract_lib_symbols,
     _line_segment_intersects_aabb,
     _point_in_rect,
     _distance,
     _aabb_overlap,
+    _check_wire_overlap,
     _compute_symbol_bbox_direct,
     compute_symbol_bbox,
     find_unconnected_pins,
@@ -529,3 +531,167 @@ class TestIntegrationGetElementsInRegion:
         sym = result["symbols"][0]
         assert "pins" in sym
         assert len(sym["pins"]) == 2  # Resistor has 2 pins
+
+    def test_wire_passing_through_region_included(self):
+        """A wire that passes through a region (no endpoints inside) should be included."""
+        extra = """
+        (wire (pts (xy 0 50) (xy 100 50))
+            (stroke (width 0) (type default))
+            (uuid "w-through"))
+        """
+        tmp = _make_temp_schematic(extra)
+        result = get_elements_in_region(tmp, 40, 40, 60, 60)
+        assert result["counts"]["wires"] == 1
+
+    def test_wire_outside_region_excluded(self):
+        """A wire entirely outside a region should not be included."""
+        extra = """
+        (wire (pts (xy 0 0) (xy 10 0))
+            (stroke (width 0) (type default))
+            (uuid "w-outside"))
+        """
+        tmp = _make_temp_schematic(extra)
+        result = get_elements_in_region(tmp, 40, 40, 60, 60)
+        assert result["counts"]["wires"] == 0
+
+
+# ===================================================================
+# Unit tests — _check_wire_overlap
+# ===================================================================
+
+class TestCheckWireOverlap:
+    """Test wire overlap detection for horizontal, vertical, and diagonal cases."""
+
+    def test_horizontal_overlap(self):
+        w1 = {"start": (10, 50), "end": (30, 50)}
+        w2 = {"start": (20, 50), "end": (40, 50)}
+        result = _check_wire_overlap(w1, w2, 0.5)
+        assert result is not None
+        assert result["type"] == "collinear_overlap"
+
+    def test_vertical_overlap(self):
+        w1 = {"start": (50, 10), "end": (50, 30)}
+        w2 = {"start": (50, 20), "end": (50, 40)}
+        result = _check_wire_overlap(w1, w2, 0.5)
+        assert result is not None
+        assert result["type"] == "collinear_overlap"
+
+    def test_diagonal_overlap(self):
+        w1 = {"start": (0, 0), "end": (20, 20)}
+        w2 = {"start": (10, 10), "end": (30, 30)}
+        result = _check_wire_overlap(w1, w2, 0.5)
+        assert result is not None
+        assert result["type"] == "collinear_overlap"
+
+    def test_horizontal_no_overlap(self):
+        w1 = {"start": (10, 50), "end": (20, 50)}
+        w2 = {"start": (30, 50), "end": (40, 50)}
+        result = _check_wire_overlap(w1, w2, 0.5)
+        assert result is None
+
+    def test_parallel_offset_no_overlap(self):
+        """Two parallel wires offset perpendicularly should not overlap."""
+        w1 = {"start": (0, 0), "end": (20, 20)}
+        w2 = {"start": (0, 5), "end": (20, 25)}
+        result = _check_wire_overlap(w1, w2, 0.5)
+        assert result is None
+
+    def test_non_parallel_no_overlap(self):
+        """Two wires at different angles should not overlap."""
+        w1 = {"start": (0, 0), "end": (10, 10)}
+        w2 = {"start": (0, 0), "end": (10, 0)}
+        result = _check_wire_overlap(w1, w2, 0.5)
+        assert result is None
+
+    def test_zero_length_segment(self):
+        w1 = {"start": (10, 10), "end": (10, 10)}
+        w2 = {"start": (10, 10), "end": (20, 20)}
+        result = _check_wire_overlap(w1, w2, 0.5)
+        assert result is None
+
+
+@pytest.mark.integration
+class TestIntegrationDiagonalWireOverlap:
+    """Integration tests for diagonal collinear wire overlap detection."""
+
+    def test_diagonal_collinear_wire_overlap(self):
+        """Two 45-degree wires that overlap should be detected."""
+        extra = """
+        (wire (pts (xy 0 0) (xy 20 20))
+            (stroke (width 0) (type default))
+            (uuid "w-diag1"))
+        (wire (pts (xy 10 10) (xy 30 30))
+            (stroke (width 0) (type default))
+            (uuid "w-diag2"))
+        """
+        tmp = _make_temp_schematic(extra)
+        result = find_overlapping_elements(tmp, tolerance=0.5)
+        assert len(result["overlappingWires"]) >= 1
+
+    def test_diagonal_parallel_no_overlap(self):
+        """Two parallel 45-degree wires that are offset should not overlap."""
+        extra = """
+        (wire (pts (xy 0 0) (xy 20 20))
+            (stroke (width 0) (type default))
+            (uuid "w-diag1"))
+        (wire (pts (xy 0 5) (xy 20 25))
+            (stroke (width 0) (type default))
+            (uuid "w-diag2"))
+        """
+        tmp = _make_temp_schematic(extra)
+        result = find_overlapping_elements(tmp, tolerance=0.5)
+        assert len(result["overlappingWires"]) == 0
+
+    def test_diagonal_non_collinear_no_overlap(self):
+        """Two wires at different angles crossing should not be flagged as collinear overlap."""
+        extra = """
+        (wire (pts (xy 0 0) (xy 20 20))
+            (stroke (width 0) (type default))
+            (uuid "w-diag1"))
+        (wire (pts (xy 0 20) (xy 20 0))
+            (stroke (width 0) (type default))
+            (uuid "w-diag2"))
+        """
+        tmp = _make_temp_schematic(extra)
+        result = find_overlapping_elements(tmp, tolerance=0.5)
+        assert len(result["overlappingWires"]) == 0
+
+
+# ===================================================================
+# Unit tests — _extract_lib_symbols
+# ===================================================================
+
+class TestExtractLibSymbols:
+    """Test _extract_lib_symbols helper."""
+
+    def test_extracts_pins_from_lib_symbols(self):
+        sexp = sexpdata.loads("""(kicad_sch
+            (lib_symbols
+                (symbol "Device:R"
+                    (symbol "Device:R_0_1"
+                        (pin passive (at 0 3.81 270) (length 1.27)
+                            (name "~" (effects (font (size 1.27 1.27))))
+                            (number "1" (effects (font (size 1.27 1.27)))))
+                        (pin passive (at 0 -3.81 90) (length 1.27)
+                            (name "~" (effects (font (size 1.27 1.27))))
+                            (number "2" (effects (font (size 1.27 1.27)))))))
+            )
+        )""")
+        result = _extract_lib_symbols(sexp)
+        assert "Device:R" in result
+        pins = result["Device:R"]
+        assert "1" in pins
+        assert "2" in pins
+        assert pins["1"]["y"] == pytest.approx(3.81)
+
+    def test_empty_schematic_returns_empty(self):
+        sexp = sexpdata.loads("(kicad_sch)")
+        result = _extract_lib_symbols(sexp)
+        assert result == {}
+
+    def test_no_lib_symbols_section(self):
+        sexp = sexpdata.loads("""(kicad_sch
+            (wire (pts (xy 0 0) (xy 10 10)))
+        )""")
+        result = _extract_lib_symbols(sexp)
+        assert result == {}

src/tools/schematic.ts

@@ -1165,7 +1165,7 @@ Note: operates on .kicad_sch files only. To modify a PCB footprint use edit_comp
     "Detect spatially overlapping symbols, wires, and labels in the schematic. Finds duplicate power symbols at the same position, collinear overlapping wires, and labels stacked on top of each other.",
     {
       schematicPath: z.string().describe("Path to the .kicad_sch schematic file"),
-      tolerance: z.number().optional().describe("Distance in mm below which elements are considered overlapping (default: 0.5)"),
+      tolerance: z.number().optional().describe("Distance threshold in mm for label proximity and wire collinearity checks. Symbol overlap uses bounding-box intersection. (default: 0.5)"),
     },
     async (args: { schematicPath: string; tolerance?: number }) => {
       const result = await callKicadScript("find_overlapping_elements", args);