-
Notifications
You must be signed in to change notification settings - Fork 512
Fix vue node slot position calculation #7877
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
479d33f
da5ad4f
230d5f5
1a80229
96a708e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,7 @@ import { test as base, expect } from '@playwright/test' | |
| import dotenv from 'dotenv' | ||
| import * as fs from 'fs' | ||
|
|
||
| import type { LGraphNode } from '../../src/lib/litegraph/src/litegraph' | ||
| import type { LGraphNode, LGraph } from '../../src/lib/litegraph/src/litegraph' | ||
| import type { NodeId } from '../../src/platform/workflow/validation/schemas/workflowSchema' | ||
| import type { KeyCombo } from '../../src/schemas/keyBindingSchema' | ||
| import type { useWorkspaceStore } from '../../src/stores/workspaceStore' | ||
|
|
@@ -1591,14 +1591,29 @@ export class ComfyPage { | |
| return window['app'].graph.nodes | ||
| }) | ||
| } | ||
| async getNodeRefsByType(type: string): Promise<NodeReference[]> { | ||
| async waitForGraphNodes(count: number) { | ||
| await this.page.waitForFunction((count) => { | ||
| return window['app']?.canvas.graph?.nodes?.length === count | ||
| }, count) | ||
| } | ||
| async getNodeRefsByType( | ||
| type: string, | ||
| includeSubgraph: boolean = false | ||
| ): Promise<NodeReference[]> { | ||
| return Promise.all( | ||
| ( | ||
| await this.page.evaluate((type) => { | ||
| return window['app'].graph.nodes | ||
| .filter((n: LGraphNode) => n.type === type) | ||
| .map((n: LGraphNode) => n.id) | ||
| }, type) | ||
| await this.page.evaluate( | ||
| ({ type, includeSubgraph }) => { | ||
| const graph = ( | ||
| includeSubgraph ? window['app'].canvas.graph : window['app'].graph | ||
| ) as LGraph | ||
| const nodes = graph.nodes | ||
| return nodes | ||
| .filter((n: LGraphNode) => n.type === type) | ||
| .map((n: LGraphNode) => n.id) | ||
| }, | ||
| { type, includeSubgraph } | ||
| ) | ||
|
Comment on lines
+1599
to
+1616
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Guard against missing graph when If |
||
| ).map((id: NodeId) => this.getNodeRefById(id)) | ||
| ) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -163,4 +163,14 @@ export class VueNodeHelpers { | |
| incrementButton: widget.getByTestId('increment') | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Enter the subgraph of a node. | ||
| * @param nodeId - The ID of the node to enter the subgraph of. If not provided, the first matched subgraph will be entered. | ||
| */ | ||
| async enterSubgraph(nodeId?: string): Promise<void> { | ||
| const locator = nodeId ? this.getNodeLocator(nodeId) : this.page | ||
| const editButton = locator.getByTestId('subgraph-enter-button') | ||
| await editButton.click() | ||
| } | ||
|
Comment on lines
+167
to
+175
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
As written, Proposed change async enterSubgraph(nodeId?: string): Promise<void> {
const locator = nodeId ? this.getNodeLocator(nodeId) : this.page
- const editButton = locator.getByTestId('subgraph-enter-button')
- await editButton.click()
+ const editButton = locator.getByTestId('subgraph-enter-button').first()
+ await editButton.waitFor({ state: 'visible' })
+ await editButton.click()
}🤖 Prompt for AI Agents |
||
| } | ||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -102,7 +102,7 @@ test.describe('Vue Node Link Interaction', () => { | |||
| test.beforeEach(async ({ comfyPage }) => { | ||||
| await comfyPage.setSetting('Comfy.UseNewMenu', 'Top') | ||||
| await comfyPage.setSetting('Comfy.VueNodes.Enabled', true) | ||||
| await comfyPage.setup() | ||||
| // await comfyPage.setup() | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Remove commented-out code instead of leaving it commented. Dead code should be removed rather than commented out. If this setup call is truly no longer needed (perhaps because Proposed fix- // await comfyPage.setup()📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||
| await comfyPage.loadWorkflow('vueNodes/simple-triple') | ||||
| await comfyPage.vueNodes.waitForNodes() | ||||
| await fitToViewInstant(comfyPage) | ||||
|
|
@@ -993,4 +993,51 @@ test.describe('Vue Node Link Interaction', () => { | |||
| expect(linked).toBe(true) | ||||
| }) | ||||
| }) | ||||
|
|
||||
| test('Dragging from subgraph input connects to correct slot', async ({ | ||||
| comfyPage, | ||||
| comfyMouse | ||||
| }) => { | ||||
| // Setup workflow with a KSampler node | ||||
| await comfyPage.executeCommand('Comfy.NewBlankWorkflow') | ||||
| await comfyPage.waitForGraphNodes(0) | ||||
| await comfyPage.executeCommand('Workspace.SearchBox.Toggle') | ||||
| await comfyPage.nextFrame() | ||||
| await comfyPage.searchBox.fillAndSelectFirstNode('KSampler') | ||||
| await comfyPage.waitForGraphNodes(1) | ||||
|
|
||||
| // Convert the KSampler node to a subgraph | ||||
| let ksamplerNode = (await comfyPage.getNodeRefsByType('KSampler'))?.[0] | ||||
| await comfyPage.vueNodes.selectNode(String(ksamplerNode.id)) | ||||
| await comfyPage.executeCommand('Comfy.Graph.ConvertToSubgraph') | ||||
|
|
||||
| // Enter the subgraph | ||||
| await comfyPage.vueNodes.enterSubgraph() | ||||
| await fitToViewInstant(comfyPage) | ||||
|
|
||||
| // Get the KSampler node inside the subgraph | ||||
| ksamplerNode = (await comfyPage.getNodeRefsByType('KSampler', true))?.[0] | ||||
| const positiveInput = await ksamplerNode.getInput(1) | ||||
| const negativeInput = await ksamplerNode.getInput(2) | ||||
|
|
||||
| const positiveInputPos = await getSlotCenter( | ||||
| comfyPage.page, | ||||
| ksamplerNode.id, | ||||
| 1, | ||||
| true | ||||
| ) | ||||
|
|
||||
| const sourceSlot = await comfyPage.getSubgraphInputSlot() | ||||
| const calculatedSourcePos = await sourceSlot.getOpenSlotPosition() | ||||
|
|
||||
| await comfyMouse.move(calculatedSourcePos) | ||||
| await comfyMouse.drag(positiveInputPos) | ||||
| await comfyMouse.drop() | ||||
|
|
||||
| // Verify connection went to the correct slot | ||||
| const positiveLinks = await positiveInput.getLinkCount() | ||||
| const negativeLinks = await negativeInput.getLinkCount() | ||||
| expect(positiveLinks).toBe(1) | ||||
| expect(negativeLinks).toBe(0) | ||||
| }) | ||||
|
Comment on lines
+997
to
+1042
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Subgraph regression test: add sync + avoid “first subgraph” ambiguity
Proposed change let ksamplerNode = (await comfyPage.getNodeRefsByType('KSampler'))?.[0]
+ expect(ksamplerNode).toBeTruthy()
await comfyPage.vueNodes.selectNode(String(ksamplerNode.id))
await comfyPage.executeCommand('Comfy.Graph.ConvertToSubgraph')
// Enter the subgraph
- await comfyPage.vueNodes.enterSubgraph()
+ await comfyPage.vueNodes.enterSubgraph(String(ksamplerNode.id))
await fitToViewInstant(comfyPage)
+ await comfyPage.vueNodes.waitForNodes()
// Get the KSampler node inside the subgraph
ksamplerNode = (await comfyPage.getNodeRefsByType('KSampler', true))?.[0]
+ expect(ksamplerNode).toBeTruthy()🤖 Prompt for AI Agents |
||||
| }) | ||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,6 @@ | ||
| import { LGraphNodeProperties } from '@/lib/litegraph/src/LGraphNodeProperties' | ||
| import { | ||
| calculateInputSlotPos, | ||
| calculateInputSlotPosFromSlot, | ||
| calculateOutputSlotPos, | ||
| getSlotPosition | ||
| } from '@/renderer/core/canvas/litegraph/slotCalculations' | ||
| import type { SlotPositionContext } from '@/renderer/core/canvas/litegraph/slotCalculations' | ||
|
|
@@ -3349,7 +3347,7 @@ export class LGraphNode | |
| * @returns Position of the input slot | ||
| */ | ||
| getInputPos(slot: number): Point { | ||
| return calculateInputSlotPos(this.#getSlotPositionContext(), slot) | ||
| return getSlotPosition(this, slot, true) | ||
| } | ||
|
Comment on lines
3349
to
3351
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Avoid ambiguous With Proposed change import {
calculateInputSlotPosFromSlot,
- getSlotPosition
+ getSlotPosition as getSlotPositionFromLayout
} from '@/renderer/core/canvas/litegraph/slotCalculations'
getInputPos(slot: number): Point {
- return getSlotPosition(this, slot, true)
+ return getSlotPositionFromLayout(this, slot, true)
}
getOutputPos(outputSlotIndex: number): Point {
- return getSlotPosition(this, outputSlotIndex, false)
+ return getSlotPositionFromLayout(this, outputSlotIndex, false)
}
getSlotPosition(slotIndex: number, isInput: boolean): Point {
- return getSlotPosition(this, slotIndex, isInput)
+ return getSlotPositionFromLayout(this, slotIndex, isInput)
}Also applies to: 3369-3371 |
||
|
|
||
| /** | ||
|
|
@@ -3369,10 +3367,7 @@ export class LGraphNode | |
| * @returns Position of the output slot | ||
| */ | ||
| getOutputPos(outputSlotIndex: number): Point { | ||
| return calculateOutputSlotPos( | ||
| this.#getSlotPositionContext(), | ||
| outputSlotIndex | ||
| ) | ||
| return getSlotPosition(this, outputSlotIndex, false) | ||
| } | ||
|
|
||
| /** | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
waitForGraphNodes()strict equality is likely to be flakyWaiting for
nodes.length === countis brittle if the app ever adds implicit nodes (or if async graph init briefly overshoots and settles). Prefer>=(or make “exact” vs “at least” explicit via an option).Proposed change
🤖 Prompt for AI Agents