fix: check_wire_collisions reports false positives for unannotated components
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 <noreply@anthropic.com>
This commit is contained in:
@@ -85,7 +85,7 @@ def _parse_symbols(sexp_data: list) -> List[Dict[str, Any]]:
|
|||||||
"""
|
"""
|
||||||
Parse all placed symbol instances from the schematic S-expression.
|
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 = []
|
symbols = []
|
||||||
for item in sexp_data:
|
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
|
x, y, rotation = 0.0, 0.0, 0.0
|
||||||
reference = ""
|
reference = ""
|
||||||
is_power = False
|
is_power = False
|
||||||
|
mirror_x = False
|
||||||
|
mirror_y = False
|
||||||
|
|
||||||
for sub in item:
|
for sub in item:
|
||||||
if isinstance(sub, list) and len(sub) >= 2:
|
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])
|
y = float(sub[2])
|
||||||
if len(sub) >= 4:
|
if len(sub) >= 4:
|
||||||
rotation = float(sub[3])
|
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:
|
elif sub[0] == Symbol("property") and len(sub) >= 3:
|
||||||
prop_name = str(sub[1]).strip('"')
|
prop_name = str(sub[1]).strip('"')
|
||||||
if prop_name == "Reference":
|
if prop_name == "Reference":
|
||||||
@@ -120,6 +128,8 @@ def _parse_symbols(sexp_data: list) -> List[Dict[str, Any]]:
|
|||||||
"x": x,
|
"x": x,
|
||||||
"y": y,
|
"y": y,
|
||||||
"rotation": rotation,
|
"rotation": rotation,
|
||||||
|
"mirror_x": mirror_x,
|
||||||
|
"mirror_y": mirror_y,
|
||||||
"is_power": is_power,
|
"is_power": is_power,
|
||||||
})
|
})
|
||||||
return symbols
|
return symbols
|
||||||
@@ -518,6 +528,44 @@ def get_elements_in_region(
|
|||||||
# Tool 5: check_wire_collisions
|
# 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]]:
|
def check_wire_collisions(schematic_path: Path) -> List[Dict[str, Any]]:
|
||||||
"""
|
"""
|
||||||
Detect wires passing through component bodies without connecting to their pins.
|
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:
|
if sym["is_power"] or ref.startswith("_TEMPLATE") or not ref:
|
||||||
continue
|
continue
|
||||||
|
|
||||||
bbox = compute_symbol_bbox(schematic_path, ref, locator)
|
# Get pin definitions by lib_id (works regardless of reference designator,
|
||||||
if bbox is None:
|
# 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
|
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
|
# Expand degenerate dimensions (pins in a line) to approximate body size
|
||||||
min_body = 1.5 # mm minimum half-extent for component body
|
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:
|
if max_x <= min_x or max_y <= min_y:
|
||||||
continue
|
continue
|
||||||
|
|
||||||
pin_positions = locator.get_all_symbol_pins(schematic_path, ref)
|
|
||||||
pin_set = set()
|
pin_set = set()
|
||||||
for pos in pin_positions.values():
|
for pos in pin_positions.values():
|
||||||
pin_set.add((pos[0], pos[1]))
|
pin_set.add((pos[0], pos[1]))
|
||||||
|
|||||||
@@ -382,6 +382,39 @@ class TestIntegrationCheckWireCollisions:
|
|||||||
d1_collisions = [c for c in result if c["component"]["reference"] == "D1"]
|
d1_collisions = [c for c in result if c["component"]["reference"] == "D1"]
|
||||||
assert len(d1_collisions) >= 1
|
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
|
@pytest.mark.integration
|
||||||
class TestIntegrationGetElementsInRegion:
|
class TestIntegrationGetElementsInRegion:
|
||||||
|
|||||||
Reference in New Issue
Block a user