From 53564cbc58dd54f43dbb4a0f5af744c33d1576ff Mon Sep 17 00:00:00 2001 From: Eugene Mikhantyev Date: Sun, 15 Mar 2026 10:55:11 +0000 Subject: [PATCH] fix: check_wire_collisions reports false positives for unannotated components MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PinLocator.get_all_symbol_pins resolves symbols by reference designator, so when multiple components share the same unannotated reference (e.g. "Q?"), it always returned the first match's pin positions. Every duplicate then got an identical bounding box, causing a single wire to be flagged against all N instances instead of only the ones it actually crosses. Fix: add _compute_pin_positions_direct() that computes absolute pin positions directly from each symbol's own (at x y rotation) and (mirror ...) data plus pin definitions fetched by lib_id — no reference-name lookup involved. Also extend _parse_symbols to capture mirror_x/mirror_y flags. Add regression test: two "R?" at different positions, wire crossing only one → must produce 0 collisions against the far-away component. Co-Authored-By: Claude Sonnet 4.6 --- python/commands/schematic_analysis.py | 68 +++++++++++++++++++++++-- python/tests/test_schematic_analysis.py | 33 ++++++++++++ 2 files changed, 96 insertions(+), 5 deletions(-) diff --git a/python/commands/schematic_analysis.py b/python/commands/schematic_analysis.py index 8fa58e5..9d91730 100644 --- a/python/commands/schematic_analysis.py +++ b/python/commands/schematic_analysis.py @@ -85,7 +85,7 @@ def _parse_symbols(sexp_data: list) -> List[Dict[str, Any]]: """ Parse all placed symbol instances from the schematic S-expression. - Returns list of dicts: {reference, lib_id, x, y, rotation, is_power} + Returns list of dicts: {reference, lib_id, x, y, rotation, mirror_x, mirror_y, is_power} """ symbols = [] for item in sexp_data: @@ -98,6 +98,8 @@ def _parse_symbols(sexp_data: list) -> List[Dict[str, Any]]: x, y, rotation = 0.0, 0.0, 0.0 reference = "" is_power = False + mirror_x = False + mirror_y = False for sub in item: if isinstance(sub, list) and len(sub) >= 2: @@ -108,6 +110,12 @@ def _parse_symbols(sexp_data: list) -> List[Dict[str, Any]]: y = float(sub[2]) if len(sub) >= 4: rotation = float(sub[3]) + elif sub[0] == Symbol("mirror"): + m = str(sub[1]) + if m == "x": + mirror_x = True + elif m == "y": + mirror_y = True elif sub[0] == Symbol("property") and len(sub) >= 3: prop_name = str(sub[1]).strip('"') if prop_name == "Reference": @@ -120,6 +128,8 @@ def _parse_symbols(sexp_data: list) -> List[Dict[str, Any]]: "x": x, "y": y, "rotation": rotation, + "mirror_x": mirror_x, + "mirror_y": mirror_y, "is_power": is_power, }) return symbols @@ -518,6 +528,44 @@ def get_elements_in_region( # Tool 5: check_wire_collisions # --------------------------------------------------------------------------- +def _compute_pin_positions_direct( + sym: Dict[str, Any], pin_defs: Dict[str, Dict] +) -> Dict[str, List[float]]: + """ + Compute absolute schematic pin positions for a symbol instance directly from + its parsed position/rotation/mirror data and pin definitions in local coords. + + Unlike PinLocator.get_all_symbol_pins, this does NOT do a reference-name + lookup in the schematic, so it works correctly when multiple symbols share + the same reference designator (e.g. unannotated "Q?"). + + KiCad transform order: mirror (in local coords) → rotate → translate. + """ + sym_x = sym["x"] + sym_y = sym["y"] + rotation = sym["rotation"] + mirror_x = sym.get("mirror_x", False) + mirror_y = sym.get("mirror_y", False) + + result: Dict[str, List[float]] = {} + for pin_num, pin_data in pin_defs.items(): + rel_x = float(pin_data["x"]) + rel_y = float(pin_data["y"]) + + # Apply mirroring in local symbol coordinates + if mirror_x: + rel_y = -rel_y + if mirror_y: + rel_x = -rel_x + + # Apply symbol rotation + if rotation != 0: + rel_x, rel_y = PinLocator.rotate_point(rel_x, rel_y, rotation) + + result[pin_num] = [sym_x + rel_x, sym_y + rel_y] + return result + + def check_wire_collisions(schematic_path: Path) -> List[Dict[str, Any]]: """ Detect wires passing through component bodies without connecting to their pins. @@ -546,11 +594,22 @@ def check_wire_collisions(schematic_path: Path) -> List[Dict[str, Any]]: if sym["is_power"] or ref.startswith("_TEMPLATE") or not ref: continue - bbox = compute_symbol_bbox(schematic_path, ref, locator) - if bbox is None: + # Get pin definitions by lib_id (works regardless of reference designator, + # so unannotated components with duplicate "Q?" references are handled correctly). + pin_defs = locator.get_symbol_pins(schematic_path, sym["lib_id"]) + if not pin_defs: continue - min_x, min_y, max_x, max_y = bbox + # Compute absolute pin positions directly from this symbol's own position/rotation, + # bypassing the reference-name lookup in PinLocator (which always finds the first + # symbol with a given reference, breaking for unannotated duplicates like "Q?"). + pin_positions = _compute_pin_positions_direct(sym, pin_defs) + if not pin_positions: + continue + + xs = [p[0] for p in pin_positions.values()] + ys = [p[1] for p in pin_positions.values()] + min_x, min_y, max_x, max_y = min(xs), min(ys), max(xs), max(ys) # Expand degenerate dimensions (pins in a line) to approximate body size min_body = 1.5 # mm minimum half-extent for component body @@ -573,7 +632,6 @@ def check_wire_collisions(schematic_path: Path) -> List[Dict[str, Any]]: if max_x <= min_x or max_y <= min_y: continue - pin_positions = locator.get_all_symbol_pins(schematic_path, ref) pin_set = set() for pos in pin_positions.values(): pin_set.add((pos[0], pos[1])) diff --git a/python/tests/test_schematic_analysis.py b/python/tests/test_schematic_analysis.py index 95d89e7..35b6d1e 100644 --- a/python/tests/test_schematic_analysis.py +++ b/python/tests/test_schematic_analysis.py @@ -382,6 +382,39 @@ class TestIntegrationCheckWireCollisions: d1_collisions = [c for c in result if c["component"]["reference"] == "D1"] assert len(d1_collisions) >= 1 + def test_unannotated_duplicates_not_over_reported(self): + """ + Regression: two components with the same unannotated reference ("R?") at + different positions should each produce independent bounding boxes. + A wire crossing only one of them must produce exactly 1 collision, not 2. + + Before the fix, PinLocator.get_all_symbol_pins always resolved "R?" to + the first match, so both symbols got identical bboxes and the same wire + was counted against both. + """ + # R? at (100, 100): Device:R pins are at (100, 96.19) and (100, 103.81). + # Effective bbox (after expansion + margin) ≈ x=[99,101], y=[96.69,103.31]. + # R? at (200, 100): identical type but far away → no intersection with wire. + r_at_100 = _make_resistor_sexp("R?", 100, 100) + r_at_200 = _make_resistor_sexp("R?", 200, 100) + # Horizontal wire crossing the body of the first R? only + wire = """ + (wire (pts (xy 95 100) (xy 105 100)) + (stroke (width 0) (type default)) + (uuid "w-collision")) + """ + tmp = _make_temp_schematic(r_at_100 + r_at_200 + wire) + result = check_wire_collisions(tmp) + # The wire must not be reported against the far-away R? at (200, 100) + collisions_at_200 = [ + c for c in result + if abs(c["component"]["position"]["x"] - 200) < 0.5 + ] + assert len(collisions_at_200) == 0, ( + "Wire at x≈100 must not be flagged against the R? at x=200; " + "likely caused by reference-lookup always returning the first 'R?'" + ) + @pytest.mark.integration class TestIntegrationGetElementsInRegion: