Skip to content

Fix loopback rendering (issue #439)#496

Open
eisbaw wants to merge 1 commit into
wireviz:masterfrom
eisbaw:loopback_issue439
Open

Fix loopback rendering (issue #439)#496
eisbaw wants to merge 1 commit into
wireviz:masterfrom
eisbaw:loopback_issue439

Conversation

@eisbaw
Copy link
Copy Markdown

@eisbaw eisbaw commented Apr 24, 2026

Closes #439.

Three root causes fixed in the loop-handling path:

  1. Loop-only connectors silently dropped. A connector declared with loops: but not referenced in any connection set was treated as an "unused template" and never instantiated. Such connectors are now auto-instantiated with an Info: message noting the promotion.

  2. All loops forced onto a single side. The previous create_graph picked one side for the whole connector from ports_left/ports_right and emitted all loop edges there. On two-sided connectors this produced stray dangling arcs on the wrong side. The dead-end raise Exception("No side for loops") could also trigger on connectors whose only activations came from loops.

    Now each pin records its activated sides in a new Connector.pin_sides: Dict[Pin, Set[Side]], and Connector.resolve_loops() picks per-loop endpoints by intersecting the two pins' known sides. Disjoint sides produce a cross-connector loop; loop-only pin pairs fall back to an existing port column; the no-information default is RIGHT.

  3. Loop edge ports used pin numbers instead of positions. Pin ports in the connector node are emitted as p{pinindex+1}{l|r} (1-based positions). Cable- and mate-edge emission correctly translate pin number to position via pins.index(pin) + 1. Loop edge emission did not -- it built port IDs directly from the pin number in loops: [[a, b]]. When pin numbers happened to equal their positions (default pincount=4 yielding [1,2,3,4]) the two agreed by accident. On connectors with an explicit non-sequential pins: list (e.g. a D-Sub wired for differential pairs), the loop edges referenced nonexistent port IDs and GraphViz silently drew them at the node centre, producing weird arcs routed over the top of the connector. This bug was pre-existing but only surfaced once loop rendering was exercised more broadly.

Files touched

  • src/wireviz/DataClasses.py -- Connector.pin_sides, resolve_loops()
  • src/wireviz/Harness.py -- call resolve_loops() before node emission, per-loop (side_a, side_b) edges, pin-position lookup on the loop endpoints
  • src/wireviz/wireviz.py -- auto-instantiate loop-only connectors with an Info log
  • examples/loopback_bug.yml -- driving example: loop-only connector, loops on a two-sided connector, loops on a single-side connector
  • examples/loopback_return.yml -- three-connector return-path illustration; documents that cross-connector "loopbacks" are modelled as cables, not loops:
  • examples/loopback_nonseq.yml -- regression witness for the pin-number-vs-pin-position bug

Test plan

  • All 14 examples/ex*.yml and 8 tutorial/tutorial*.yml still render without errors or warnings.
  • New examples/loopback_bug.yml renders with X1 (loop-only) instantiated, X2's loop on its right side next to the cable it shares pins with, X3's loop on its right side.
  • New examples/loopback_nonseq.yml renders with loop edges referencing pin-table positions 1-2 and 3-4 (for pin numbers [1, 14] and [3, 16]), visible as coils between adjacent pin rows.
  • Reviewer to verify BOM/HTML outputs are unaffected (this change touches only graph emission for the loop edges and auto-instantiation).

Known follow-ups (not in this PR)

  • style: simple + loops: emits port pXr unrecognized warnings in GraphViz because simple-style nodes don't render a pin table; this is a pre-existing issue and would be cleanest to reject at Connector.__post_init__.
  • loops: referencing pinlabels: instead of pin numbers is still rejected by post-init with a clear error (TODO in the pre-existing code).
  • ports_left/ports_right are now strictly derivable from pin_sides and could be made @property in a follow-up; left as-is to keep this change minimally invasive.

Three root causes addressed, all in the loop-handling path:

1. Loop-only connectors were silently dropped as "unused templates"
   because they were not referenced in any connection set. Such
   connectors are still legitimate harness components and are now
   auto-instantiated when declared with `loops:`, with an "Info:"
   message noting the promotion.

2. All loops on a connector were forced onto a single side, chosen
   globally from `ports_left`/`ports_right`. When a connector had
   cables on both sides, loops ended up on whichever side was
   picked first, producing stray edges dangling away from the pins
   they belonged to. The dead-end `raise Exception("No side for
   loops")` could also trigger on connectors whose only
   activations came from loops.

   Fixed by tracking per-pin activated sides in Connector.pin_sides
   and adding Connector.resolve_loops(), which picks each loop's
   two endpoints by intersecting the two pins' known sides.
   Disjoint sides produce a cross-connector loop (LEFT-RIGHT span);
   loop-only pin pairs fall back to an existing port column; the
   no-information default is RIGHT.

3. Loop edge emission referenced port IDs built from the raw pin
   NUMBER in `loops: [[a, b]]`. Pin ports in the connector node
   are emitted with 1-based POSITIONS (`p{pinindex+1}{l|r}`), and
   cable- and mate-edge emission correctly translate pin number
   to position via `pins.index(pin) + 1`. When pin numbers happen
   to equal their 1-based positions (e.g., default pincount=4
   yielding pins [1,2,3,4]) the two agreed by accident. When they
   diverged -- e.g., a D-Sub with an explicit non-sequential
   `pins:` list wired for differential pairs -- the loop edge
   referenced ports that did not exist and GraphViz silently drew
   those edges at the node centre, producing loop arcs routed
   over the top of the connector rather than between the actual
   pin rows. This bug was latent before the loop-side fix above
   but was only uncovered by widening test coverage.

Added example files exercising all three cases:
- examples/loopback_bug.yml (loop-only connector; loops on a
  two-sided connector; loops on a single-side connector)
- examples/loopback_return.yml (three-connector return-path
  illustration; documents that cross-connector loops are modelled
  as cables, not `loops:`)
- examples/loopback_nonseq.yml (regression witness for the
  pin-number-vs-pin-position bug)

Fixes wireviz#439.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@eisbaw eisbaw force-pushed the loopback_issue439 branch from f73dca0 to dd0e0a7 Compare April 24, 2026 20:33
SomethingNew71 added a commit to ClassicMiniDIY/WireViz that referenced this pull request May 4, 2026
…Designator

The auto-instantiation pass added in 222e671 (port of upstream PR wireviz#496)
guarded against re-instantiating a template by checking
`template_name in harness.connectors`, but that only catches the case
where the template was used as a literal designator. When a template is
used via the `Template.Designator` syntax (e.g. `DSub.X1`), the harness
holds the *instance* (X1), not the template name (DSub) — so the guard
missed the case and the template would get a phantom floating connector
named after the template carrying duplicate loop edges.

Also skip when `template_name` appears in `designators_and_templates.values()`,
which is the canonical record of templates that have been bound via the
T.X syntax during connection-set processing.

Verified against examples/loopback_bug.yml (loop-only connector still
auto-instantiates correctly) and a regression case where a template with
loops is referenced as `DSub.X1` / `DSub.X2` (no phantom DSub connector
generated, no Info: auto-instantiating log).

Addresses gemini-code-assist review feedback on
#1

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@kvid
Copy link
Copy Markdown
Collaborator

kvid commented May 7, 2026

@eisbaw - Thank you very much for this contribution. I look forward to test this, but have not had the time yet.

About your root cause number one (Loop-only connectors silently dropped) - I suspect this is as intended. Please read #328 where I reported something similar as a bug, then Daniel (the owner) argued why it is intended, and convinced me that it is a good design choice. The autogeneration advantages are greater than the minor disadvantage when needing an unconnected connector. He has described it in the syntax documentation, and to inform the user, all unconnected entries are listed in a run-time warning: "Warning: The following components are not referenced in any connection set:"

@kvid
Copy link
Copy Markdown
Collaborator

kvid commented May 14, 2026

Please don't regard this as a hostile attack on your suggested changes. I just want to understand your use case and what is a better functionality for that.

@eisbaw wrote:

Closes #439.

Are you sure this is the correct issue number? #439 is about wires between two different connectors, but your PR seems only to suggest changes about loops between pins on the same connector.

However, the reported issues #432 and #465 seem related to your root cause 3.

  1. Loop-only connectors silently dropped.

See my previous comment above and #328.

  1. All loops forced onto a single side. The previous create_graph picked one side for the whole connector from ports_left/ports_right and emitted all loop edges there.

I understand you feel this is wrong. Please explain why. What reasons in your use case drive you to prefer a loop from pin X to be on the same side as another connection from the same pin?

Remember that connections on both sides are just a drawing technique to reduce crossing lines in the diagram - not representing a physical two-sided connector

On two-sided connectors this produced stray dangling arcs on the wrong side.

Please define "wrong side" in your use case.

The dead-end raise Exception("No side for loops") could also trigger on connectors whose only activations came from loops.

Please provide an example YAML input that triggers this exception with the currently latest release.

Now each pin records its activated sides in a new Connector.pin_sides: Dict[Pin, Set[Side]], and Connector.resolve_loops() picks per-loop endpoints by intersecting the two pins' known sides. Disjoint sides produce a cross-connector loop;

Please describe what you mean by a "cross-connector loop" in this case? Do you suggest a spline wire between a left pin and a right pin on the same connector?

loop-only pin pairs fall back to an existing port column; the no-information default is RIGHT.

AFAIK, the existing default is the LEFT side. Is RIGHT a better default in your use case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a way to add a loopback wire, add example of it

2 participants