From 05587d8a1956ebb2e022c570aec0a4eab4a72df2 Mon Sep 17 00:00:00 2001 From: filtered <176114999+webfiltered@users.noreply.github.com> Date: Mon, 24 Mar 2025 06:00:55 +1100 Subject: [PATCH] Fix moving existing links can result in loopback (#838) Prevents nodes connecting links to themselves when moving existing links. If moving multiple links with reroutes, this will instead _reconnect_ any links that would become loopbacks, only without any rereoutes. --- src/LGraphCanvas.ts | 4 +- src/LGraphNode.ts | 8 ++ src/canvas/FloatingRenderLink.ts | 8 ++ src/canvas/LinkConnector.ts | 81 ++++++++---------- src/canvas/MovingRenderLink.ts | 8 ++ src/canvas/ToInputRenderLink.ts | 12 ++- src/canvas/ToOutputRenderLink.ts | 11 ++- test/LinkConnector.integration.test.ts | 109 +++++++++++++++++++++++++ 8 files changed, 187 insertions(+), 54 deletions(-) diff --git a/src/LGraphCanvas.ts b/src/LGraphCanvas.ts index d3d1efe34..d6702b9e0 100644 --- a/src/LGraphCanvas.ts +++ b/src/LGraphCanvas.ts @@ -2597,8 +2597,8 @@ export class LGraphCanvas implements ConnectionColorContext { let highlightPos: Point | undefined let highlightInput: INodeInputSlot | undefined - if (!firstLink || firstLink.node === node) { - // No link / node loopback + if (!firstLink || !linkConnector.isNodeValidDrop(node)) { + // No link, or none of the dragged links may be dropped here } else if (linkConnector.state.connectingTo === "input") { if (inputId === -1 && outputId === -1) { // Allow support for linking to widgets, handled externally to LiteGraph diff --git a/src/LGraphNode.ts b/src/LGraphNode.ts index 203c128d8..72f10cfab 100644 --- a/src/LGraphNode.ts +++ b/src/LGraphNode.ts @@ -2330,6 +2330,14 @@ export class LGraphNode implements Positionable, IPinnable, IColorable { return null } + canConnectTo( + node: LGraphNode, + toSlot: INodeInputSlot, + fromSlot: INodeOutputSlot, + ) { + return this.id !== node.id && LiteGraph.isValidConnection(fromSlot.type, toSlot.type) + } + /** * Connect an output of this node to an input of another node * @param slot (could be the number of the slot or the string with the name of the slot) diff --git a/src/canvas/FloatingRenderLink.ts b/src/canvas/FloatingRenderLink.ts index 780a2db37..dfb745c40 100644 --- a/src/canvas/FloatingRenderLink.ts +++ b/src/canvas/FloatingRenderLink.ts @@ -96,6 +96,14 @@ export class FloatingRenderLink implements RenderLink { this.fromPos = fromReroute.pos } + canConnectToInput(): true { + return true + } + + canConnectToOutput(): true { + return true + } + canConnectToReroute(reroute: Reroute): boolean { if (this.toType === "input") { if (reroute.origin_id === this.inputNode?.id) return false diff --git a/src/canvas/LinkConnector.ts b/src/canvas/LinkConnector.ts index fdb1fcbdd..6daa2820a 100644 --- a/src/canvas/LinkConnector.ts +++ b/src/canvas/LinkConnector.ts @@ -1,5 +1,5 @@ import type { RenderLink } from "./RenderLink" -import type { ConnectingLink, ISlotType, ItemLocator, LinkNetwork, LinkSegment } from "@/interfaces" +import type { ConnectingLink, ItemLocator, LinkNetwork, LinkSegment } from "@/interfaces" import type { INodeInputSlot, INodeOutputSlot } from "@/interfaces" import type { LGraphNode } from "@/LGraphNode" import type { Reroute } from "@/Reroute" @@ -7,7 +7,6 @@ import type { CanvasPointerEvent } from "@/types/events" import type { IWidget } from "@/types/widgets" import { LinkConnectorEventMap, LinkConnectorEventTarget } from "@/infrastructure/LinkConnectorEventTarget" -import { LiteGraph } from "@/litegraph" import { LLink } from "@/LLink" import { LinkDirection } from "@/types/globalEnums" @@ -338,6 +337,9 @@ export class LinkConnector { const { connectingTo } = state const { canvasX, canvasY } = event + // Do nothing if every connection would loop back + if (renderLinks.every(link => link.node === node)) return + // To output if (connectingTo === "output") { const output = node.getOutputOnPos([canvasX, canvasY]) @@ -437,7 +439,7 @@ export class LinkConnector { if (!result) continue const { node, output } = result - if (!isValidConnectionToOutput(link, node, output)) continue + if (!link.canConnectToOutput(node, output)) continue link.connectToRerouteOutput(reroute, node, output, this.events) } @@ -468,7 +470,7 @@ export class LinkConnector { // Assume all links are the same type, disallow loopback const firstLink = this.renderLinks[0] - if (!firstLink || firstLink.node === node) return + if (!firstLink) return // Use a single type check before looping; ensures all dropped links go to the same slot if (connectingTo === "output") { @@ -479,9 +481,7 @@ export class LinkConnector { return } - for (const link of this.renderLinks) { - link.connectToOutput(node, output, this.events) - } + this.#dropOnOutput(node, output) } else if (connectingTo === "input") { // Dropping new input link const input = node.findInputByType(firstLink.fromSlot.type)?.slot @@ -490,15 +490,13 @@ export class LinkConnector { return } - for (const link of this.renderLinks) { - link.connectToInput(node, input, this.events) - } + this.#dropOnInput(node, input) } } #dropOnInput(node: LGraphNode, input: INodeInputSlot): void { for (const link of this.renderLinks) { - if (link.toType !== "input") continue + if (!link.canConnectToInput(node, input)) continue link.connectToInput(node, input, this.events) } @@ -506,12 +504,24 @@ export class LinkConnector { #dropOnOutput(node: LGraphNode, output: INodeOutputSlot): void { for (const link of this.renderLinks) { - if (link.toType !== "output") continue + if (!link.canConnectToOutput(node, output)) { + if (link instanceof MovingRenderLink && link.link.parentId !== undefined) { + // Reconnect link without reroutes + link.outputNode.connectSlots(link.outputSlot, link.inputNode, link.inputSlot, undefined!) + } + continue + } link.connectToOutput(node, output, this.events) } } + isNodeValidDrop(node: LGraphNode): boolean { + return this.state.connectingTo === "output" + ? node.outputs.some(output => this.renderLinks.some(link => link.canConnectToOutput(node, output))) + : node.inputs.some(input => this.renderLinks.some(link => link.canConnectToInput(node, input))) + } + /** * Checks if a reroute is a valid drop target for any of the links being connected. * @param reroute The reroute that would be dropped on. @@ -537,7 +547,7 @@ export class LinkConnector { for (const renderLink of this.renderLinks) { if (renderLink.toType !== "output") continue if (!renderLink.canConnectToReroute(reroute)) continue - if (isValidConnectionToOutput(renderLink, node, output)) return true + if (renderLink.canConnectToOutput(node, output)) return true } } @@ -622,36 +632,21 @@ export class LinkConnector { } } -function isValidConnectionToOutput(link: ToOutputRenderLink | MovingRenderLink | FloatingRenderLink, outputNode: LGraphNode, output: INodeOutputSlot): boolean { - const { node: fromNode } = link - - // Node cannot connect to itself - if (fromNode === outputNode) return false - - if (link instanceof MovingRenderLink) { - const { inputSlot: { type } } = link - - // Link is already connected here / type mismatch - if (!LiteGraph.isValidConnection(type, output.type)) { - return false - } - } else { - const { fromSlot: { type } } = link - if (!LiteGraph.isValidConnection(type, output.type)) return false - } - return true -} - /** Validates that a single {@link RenderLink} can be dropped on the specified reroute. */ -function canConnectInputLinkToReroute(link: ToInputRenderLink | MovingRenderLink | FloatingRenderLink, inputNode: LGraphNode, input: INodeInputSlot, reroute: Reroute): boolean { - const { node: fromNode, fromSlot, fromReroute } = link +function canConnectInputLinkToReroute( + link: ToInputRenderLink | MovingRenderLink | FloatingRenderLink, + inputNode: LGraphNode, + input: INodeInputSlot, + reroute: Reroute, +): boolean { + const { fromReroute } = link if ( - // Node cannot connect to itself - fromNode === inputNode || + !link.canConnectToInput(inputNode, input) || // Would result in no change fromReroute?.id === reroute.id || - isInvalid(fromSlot.type, reroute, fromReroute) + // Cannot connect from child to parent reroute + fromReroute?.getReroutes()?.includes(reroute) ) { return false } @@ -666,14 +661,4 @@ function canConnectInputLinkToReroute(link: ToInputRenderLink | MovingRenderLink } } return true - - /** Checks connection type & rejects infinite loops. */ - function isInvalid(type: ISlotType, reroute: Reroute, fromReroute?: Reroute): boolean { - return Boolean( - // Type mismatch - !LiteGraph.isValidConnection(type, input.type) || - // Cannot connect from child to parent reroute - fromReroute?.getReroutes()?.includes(reroute), - ) - } } diff --git a/src/canvas/MovingRenderLink.ts b/src/canvas/MovingRenderLink.ts index 7fe0fdd5f..ef722df7e 100644 --- a/src/canvas/MovingRenderLink.ts +++ b/src/canvas/MovingRenderLink.ts @@ -86,6 +86,14 @@ export class MovingRenderLink implements RenderLink { this.fromSlotIndex = this.toType === "input" ? outputIndex : inputIndex } + canConnectToInput(inputNode: LGraphNode, input: INodeInputSlot): this is this { + return this.node.canConnectTo(inputNode, input, this.outputSlot) + } + + canConnectToOutput(outputNode: LGraphNode, output: INodeOutputSlot): this is this { + return outputNode.canConnectTo(this.node, this.inputSlot, output) + } + canConnectToReroute(reroute: Reroute): boolean { if (this.toType === "input") { if (reroute.origin_id === this.inputNode.id) return false diff --git a/src/canvas/ToInputRenderLink.ts b/src/canvas/ToInputRenderLink.ts index de75e045f..aa62db17a 100644 --- a/src/canvas/ToInputRenderLink.ts +++ b/src/canvas/ToInputRenderLink.ts @@ -1,8 +1,8 @@ import type { RenderLink } from "./RenderLink" import type { LinkConnectorEventTarget } from "@/infrastructure/LinkConnectorEventTarget" -import type { LinkNetwork, Point } from "@/interfaces" +import type { INodeInputSlot, INodeOutputSlot, LinkNetwork, Point } from "@/interfaces" import type { LGraphNode } from "@/LGraphNode" -import type { INodeInputSlot, INodeOutputSlot, LLink } from "@/litegraph" +import type { LLink } from "@/LLink" import type { Reroute } from "@/Reroute" import { LinkDirection } from "@/types/globalEnums" @@ -31,6 +31,14 @@ export class ToInputRenderLink implements RenderLink { : this.node.getOutputPos(outputIndex) } + canConnectToInput(inputNode: LGraphNode, input: INodeInputSlot): this is this { + return this.node.canConnectTo(inputNode, input, this.fromSlot) + } + + canConnectToOutput(): false { + return false + } + connectToInput(node: LGraphNode, input: INodeInputSlot, events: LinkConnectorEventTarget) { const { node: outputNode, fromSlot, fromReroute } = this if (node === outputNode) return diff --git a/src/canvas/ToOutputRenderLink.ts b/src/canvas/ToOutputRenderLink.ts index 585156aec..5d821a639 100644 --- a/src/canvas/ToOutputRenderLink.ts +++ b/src/canvas/ToOutputRenderLink.ts @@ -1,8 +1,7 @@ import type { RenderLink } from "./RenderLink" import type { LinkConnectorEventTarget } from "@/infrastructure/LinkConnectorEventTarget" -import type { LinkNetwork, Point } from "@/interfaces" +import type { INodeInputSlot, INodeOutputSlot, LinkNetwork, Point } from "@/interfaces" import type { LGraphNode } from "@/LGraphNode" -import type { INodeInputSlot, INodeOutputSlot } from "@/litegraph" import type { Reroute } from "@/Reroute" import { LinkDirection } from "@/types/globalEnums" @@ -31,6 +30,14 @@ export class ToOutputRenderLink implements RenderLink { : this.node.getInputPos(inputIndex) } + canConnectToInput(): false { + return false + } + + canConnectToOutput(outputNode: LGraphNode, output: INodeOutputSlot): this is this { + return this.node.canConnectTo(outputNode, this.fromSlot, output) + } + canConnectToReroute(reroute: Reroute): boolean { if (reroute.origin_id === this.node.id) return false return true diff --git a/test/LinkConnector.integration.test.ts b/test/LinkConnector.integration.test.ts index bbda41c70..8cefcd509 100644 --- a/test/LinkConnector.integration.test.ts +++ b/test/LinkConnector.integration.test.ts @@ -1,3 +1,5 @@ +import type { CanvasPointerEvent } from "@/types/events" + import { afterEach, describe, expect, vi } from "vitest" import { LinkConnector } from "@/canvas/LinkConnector" @@ -120,6 +122,29 @@ const test = baseTest.extend({ }, }) +function mockedNodeTitleDropEvent(node: LGraphNode): CanvasPointerEvent { + return { + canvasX: node.pos[0] + node.size[0] / 2, + canvasY: node.pos[1] + 16, + } as any +} + +function mockedInputDropEvent(node: LGraphNode, slot: number): CanvasPointerEvent { + const pos = node.getInputPos(slot) + return { + canvasX: pos[0], + canvasY: pos[1], + } as any +} + +function mockedOutputDropEvent(node: LGraphNode, slot: number): CanvasPointerEvent { + const pos = node.getOutputPos(slot) + return { + canvasX: pos[0], + canvasY: pos[1], + } as any +} + describe("LinkConnector Integration", () => { afterEach(({ validateLinkIntegrity }) => { validateLinkIntegrity() @@ -223,6 +248,44 @@ describe("LinkConnector Integration", () => { expect(output._floatingLinks?.size).toBeOneOf([0, undefined]) } }) + + test("Should prevent node loopback when dropping on node", ({ graph, connector }) => { + const hasOutputNode = graph.getNodeById(1)! + const hasInputNode = graph.getNodeById(2)! + const hasInputNode2 = graph.getNodeById(3)! + + const reroutesBefore = LLink.getReroutes(graph, graph.links.get(hasInputNode.inputs[0].link!)!) + + const atOutputNodeEvent = mockedNodeTitleDropEvent(hasOutputNode) + + connector.moveInputLink(graph, hasInputNode.inputs[0]) + connector.dropLinks(graph, atOutputNodeEvent) + + const outputNodes = hasOutputNode.getOutputNodes(0) + expect(outputNodes).toEqual([hasInputNode, hasInputNode2]) + + const reroutesAfter = LLink.getReroutes(graph, graph.links.get(hasInputNode.inputs[0].link!)!) + expect(reroutesAfter).toEqual(reroutesBefore) + }) + + test("Should prevent node loopback when dropping on input", ({ graph, connector }) => { + const hasOutputNode = graph.getNodeById(1)! + const hasInputNode = graph.getNodeById(2)! + + const originalOutputNodes = hasOutputNode.getOutputNodes(0) + const reroutesBefore = LLink.getReroutes(graph, graph.links.get(hasInputNode.inputs[0].link!)!) + + const atHasOutputNode = mockedInputDropEvent(hasOutputNode, 0) + + connector.moveInputLink(graph, hasInputNode.inputs[0]) + connector.dropLinks(graph, atHasOutputNode) + + const outputNodes = hasOutputNode.getOutputNodes(0) + expect(outputNodes).toEqual(originalOutputNodes) + + const reroutesAfter = LLink.getReroutes(graph, graph.links.get(hasInputNode.inputs[0].link!)!) + expect(reroutesAfter).toEqual(reroutesBefore) + }) }) describe("Moving output links", () => { @@ -392,6 +455,52 @@ describe("LinkConnector Integration", () => { expect(graph.floatingLinks.size).toBe(1) expect(floatingReroute.linkIds.size).toBe(0) }) + + test("Should prevent node loopback when dropping on node", ({ graph, connector }) => { + const hasOutputNode = graph.getNodeById(1)! + const hasInputNode = graph.getNodeById(2)! + + const reroutesBefore = LLink.getReroutes(graph, graph.links.get(hasOutputNode.outputs[0].links![0])!) + + const atInputNodeEvent = mockedNodeTitleDropEvent(hasInputNode) + + connector.moveOutputLink(graph, hasOutputNode.outputs[0]) + connector.dropLinks(graph, atInputNodeEvent) + + expect(hasOutputNode.getOutputNodes(0)).toEqual([hasInputNode]) + expect(hasInputNode.getOutputNodes(0)).toEqual([graph.getNodeById(3)]) + + // Moved link should have the same reroutes + const reroutesAfter = LLink.getReroutes(graph, graph.links.get(hasInputNode.outputs[0].links![0])!) + expect(reroutesAfter).toEqual(reroutesBefore) + + // Link recreated to avoid loopback should have no reroutes + const reroutesAfter2 = LLink.getReroutes(graph, graph.links.get(hasOutputNode.outputs[0].links![0])!) + expect(reroutesAfter2).toEqual([]) + }) + + test("Should prevent node loopback when dropping on output", ({ graph, connector }) => { + const hasOutputNode = graph.getNodeById(1)! + const hasInputNode = graph.getNodeById(2)! + + const reroutesBefore = LLink.getReroutes(graph, graph.links.get(hasOutputNode.outputs[0].links![0])!) + + const atInputNodeOutSlot = mockedOutputDropEvent(hasInputNode, 0) + + connector.moveOutputLink(graph, hasOutputNode.outputs[0]) + connector.dropLinks(graph, atInputNodeOutSlot) + + expect(hasOutputNode.getOutputNodes(0)).toEqual([hasInputNode]) + expect(hasInputNode.getOutputNodes(0)).toEqual([graph.getNodeById(3)]) + + // Moved link should have the same reroutes + const reroutesAfter = LLink.getReroutes(graph, graph.links.get(hasInputNode.outputs[0].links![0])!) + expect(reroutesAfter).toEqual(reroutesBefore) + + // Link recreated to avoid loopback should have no reroutes + const reroutesAfter2 = LLink.getReroutes(graph, graph.links.get(hasOutputNode.outputs[0].links![0])!) + expect(reroutesAfter2).toEqual([]) + }) }) describe("Floating links", () => {