mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-01-26 10:59:53 +00:00
Fix subgraphNode widget cloning with compressed target_slot (#7388)
## Cause When graphs are actually exported, several layers of cleanup are applied. Among these is link compression. Any widgets with inputs that aren't used do not have inputs stored in the workflow. This was implemented for backwards compatibility with the old "convert to input" system for widgets. As part of this process, the target_slots on links are rewritten such that they point to the index of the widget as if unconnected widgets did not exist. This "incorrect" state for links is only corrected AFTER a workflow has loaded because the 'fix' method needs nodes to be initialized in order to calculate the correct target_slot This becomes a problem when subgraphs are introduced. SubgraphInputs need to resolve a link to its target slot in order to construct a clone of the linked widget DURING the loading process. Since this target slot is not accurate, this can result in the cloned widget having the wrong type. For a minimal reproduction: - Create a subgraph with an Empty Latent Image with batch_size linked to the Subgraph Input - Export the workflow - On load, the batch_size has step and min attributes which incorrectly correspond to width ## Fix There's multiple possible ways to address this and input on direction is appreciated - Fix links before loading graph - Likely to break with any dynamic state - Fix links, then load graph again - Ugly, bad performance, dynamic state may require multiple passes to correctly ripple - In the Subgraph code, ignore target_slot and instead `.find()` input with matching linkId (proposed) - Promising, but means accepting that state is just wrong sometimes. Another forever footgun. - Entirely remove the input compression - Some people may complain, and old workflows still need to be supported - Only remove target_slot redirection inside subgraphs - Creates ugly logical difference between what happens inside and outside subgraphs. - Still leaves old workflows broken ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-7388-Remove-target_slot-compression-from-subgraph-exports-2c66d73d3650815d8c96c5047958ab67) by [Unito](https://www.unito.io)
This commit is contained in:
@@ -0,0 +1,150 @@
|
|||||||
|
{
|
||||||
|
"id": "e0cb1d7e-5437-4911-b574-c9603dfbeaee",
|
||||||
|
"revision": 0,
|
||||||
|
"last_node_id": 2,
|
||||||
|
"last_link_id": 0,
|
||||||
|
"nodes": [
|
||||||
|
{
|
||||||
|
"id": 2,
|
||||||
|
"type": "8bfe4227-f272-49e1-a892-0a972a86867c",
|
||||||
|
"pos": [
|
||||||
|
-317,
|
||||||
|
-336
|
||||||
|
],
|
||||||
|
"size": [
|
||||||
|
210,
|
||||||
|
58
|
||||||
|
],
|
||||||
|
"flags": {},
|
||||||
|
"order": 0,
|
||||||
|
"mode": 0,
|
||||||
|
"inputs": [],
|
||||||
|
"outputs": [],
|
||||||
|
"properties": {
|
||||||
|
"proxyWidgets": [
|
||||||
|
[
|
||||||
|
"-1",
|
||||||
|
"batch_size"
|
||||||
|
]
|
||||||
|
]
|
||||||
|
},
|
||||||
|
"widgets_values": [
|
||||||
|
1
|
||||||
|
]
|
||||||
|
}
|
||||||
|
],
|
||||||
|
"links": [],
|
||||||
|
"groups": [],
|
||||||
|
"definitions": {
|
||||||
|
"subgraphs": [
|
||||||
|
{
|
||||||
|
"id": "8bfe4227-f272-49e1-a892-0a972a86867c",
|
||||||
|
"version": 1,
|
||||||
|
"state": {
|
||||||
|
"lastGroupId": 0,
|
||||||
|
"lastNodeId": 1,
|
||||||
|
"lastLinkId": 1,
|
||||||
|
"lastRerouteId": 0
|
||||||
|
},
|
||||||
|
"revision": 0,
|
||||||
|
"config": {},
|
||||||
|
"name": "New Subgraph",
|
||||||
|
"inputNode": {
|
||||||
|
"id": -10,
|
||||||
|
"bounding": [
|
||||||
|
-562,
|
||||||
|
-358,
|
||||||
|
120,
|
||||||
|
60
|
||||||
|
]
|
||||||
|
},
|
||||||
|
"outputNode": {
|
||||||
|
"id": -20,
|
||||||
|
"bounding": [
|
||||||
|
-52,
|
||||||
|
-358,
|
||||||
|
120,
|
||||||
|
40
|
||||||
|
]
|
||||||
|
},
|
||||||
|
"inputs": [
|
||||||
|
{
|
||||||
|
"id": "b4a8bc2a-8e9f-41aa-938d-c567a11d2c00",
|
||||||
|
"name": "batch_size",
|
||||||
|
"type": "INT",
|
||||||
|
"linkIds": [
|
||||||
|
1
|
||||||
|
],
|
||||||
|
"pos": [
|
||||||
|
-462,
|
||||||
|
-338
|
||||||
|
]
|
||||||
|
}
|
||||||
|
],
|
||||||
|
"outputs": [],
|
||||||
|
"widgets": [],
|
||||||
|
"nodes": [
|
||||||
|
{
|
||||||
|
"id": 1,
|
||||||
|
"type": "EmptyLatentImage",
|
||||||
|
"pos": [
|
||||||
|
-382,
|
||||||
|
-376
|
||||||
|
],
|
||||||
|
"size": [
|
||||||
|
270,
|
||||||
|
106
|
||||||
|
],
|
||||||
|
"flags": {},
|
||||||
|
"order": 0,
|
||||||
|
"mode": 0,
|
||||||
|
"inputs": [
|
||||||
|
{
|
||||||
|
"localized_name": "batch_size",
|
||||||
|
"name": "batch_size",
|
||||||
|
"type": "INT",
|
||||||
|
"widget": {
|
||||||
|
"name": "batch_size"
|
||||||
|
},
|
||||||
|
"link": 1
|
||||||
|
}
|
||||||
|
],
|
||||||
|
"outputs": [
|
||||||
|
{
|
||||||
|
"localized_name": "LATENT",
|
||||||
|
"name": "LATENT",
|
||||||
|
"type": "LATENT",
|
||||||
|
"links": null
|
||||||
|
}
|
||||||
|
],
|
||||||
|
"properties": {
|
||||||
|
"Node name for S&R": "EmptyLatentImage"
|
||||||
|
},
|
||||||
|
"widgets_values": [
|
||||||
|
512,
|
||||||
|
512,
|
||||||
|
1
|
||||||
|
]
|
||||||
|
}
|
||||||
|
],
|
||||||
|
"groups": [],
|
||||||
|
"links": [
|
||||||
|
{
|
||||||
|
"id": 1,
|
||||||
|
"origin_id": -10,
|
||||||
|
"origin_slot": 0,
|
||||||
|
"target_id": 1,
|
||||||
|
"target_slot": 0,
|
||||||
|
"type": "INT"
|
||||||
|
}
|
||||||
|
],
|
||||||
|
"extra": {}
|
||||||
|
}
|
||||||
|
]
|
||||||
|
},
|
||||||
|
"config": {},
|
||||||
|
"extra": {
|
||||||
|
"frontendVersion": "1.35.1"
|
||||||
|
},
|
||||||
|
"version": 0.4
|
||||||
|
}
|
||||||
@@ -329,6 +329,15 @@ test.describe('Subgraph Operations', () => {
|
|||||||
expect(newInputName).toBe(labelClickRenamedName)
|
expect(newInputName).toBe(labelClickRenamedName)
|
||||||
expect(newInputName).not.toBe(initialInputLabel)
|
expect(newInputName).not.toBe(initialInputLabel)
|
||||||
})
|
})
|
||||||
|
test('Can create widget from link with compressed target_slot', async ({
|
||||||
|
comfyPage
|
||||||
|
}) => {
|
||||||
|
await comfyPage.loadWorkflow('subgraphs/subgraph-compressed-target-slot')
|
||||||
|
const step = await comfyPage.page.evaluate(() => {
|
||||||
|
return window['app'].graph.nodes[0].widgets[0].options.step
|
||||||
|
})
|
||||||
|
expect(step).toBe(10)
|
||||||
|
})
|
||||||
})
|
})
|
||||||
|
|
||||||
test.describe('Subgraph Creation and Deletion', () => {
|
test.describe('Subgraph Creation and Deletion', () => {
|
||||||
|
|||||||
@@ -300,17 +300,24 @@ export class SubgraphNode extends LGraphNode implements BaseLGraph {
|
|||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
|
|
||||||
const resolved = link.resolve(this.subgraph)
|
const { inputNode } = link.resolve(this.subgraph)
|
||||||
if (!resolved.input || !resolved.inputNode) {
|
if (!inputNode) {
|
||||||
console.warn('Invalid resolved link', resolved, this)
|
console.warn('Failed to resolve inputNode', link, this)
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
|
||||||
|
//Manually find input since target_slot can't be trusted
|
||||||
|
const targetInput = inputNode.inputs.find((inp) => inp.link === linkId)
|
||||||
|
if (!targetInput) {
|
||||||
|
console.warn('Failed to find corresponding input', link, inputNode)
|
||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
|
|
||||||
// No widget - ignore this link
|
// No widget - ignore this link
|
||||||
const widget = resolved.inputNode.getWidgetFromSlot(resolved.input)
|
const widget = inputNode.getWidgetFromSlot(targetInput)
|
||||||
if (!widget) continue
|
if (!widget) continue
|
||||||
|
|
||||||
this.#setWidget(subgraphInput, input, widget, resolved.input.widget)
|
this.#setWidget(subgraphInput, input, widget, targetInput.widget)
|
||||||
break
|
break
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user