mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-02-06 16:10:09 +00:00
Don't bypass subgraph contents with subgraph (#8494)
When bypassing or muting a subgraph the contents are no longer bypassed along with it. This behaviour was less than ideal because it meant that toggling the bypass of a single subgraph node twice could change the behaviour of the node. It is entirely intended that a subgraph node which is bypassed does not have it's children execute. As part of testing this behaviour, it was found that nodes inside of a bypassed subgraph are still considered for execution even if boundry links are treated as disconnected. The following example would execute even if the subgraph is muted. <img width="826" height="476" alt="image" src="https://github.com/user-attachments/assets/7b282873-e114-494d-b8f1-74c373859151" /> To resolve this, the PR does not add the contents of a subgraphNode which is muted or bypassed to the execution map. Resolves #8489 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8494-Don-t-bypass-subgraph-contents-with-subgraph-2f86d73d365081aeba8dd2990b6ba0ad) by [Unito](https://www.unito.io)
This commit is contained in:
@@ -276,7 +276,7 @@ describe('useSelectedLiteGraphItems', () => {
|
||||
expect(selectedNodes).toContainEqual(subNode2)
|
||||
})
|
||||
|
||||
it('toggleSelectedNodesMode should apply unified state to subgraph children', () => {
|
||||
it('toggleSelectedNodesMode should not apply state to subgraph children', () => {
|
||||
const { toggleSelectedNodesMode } = useSelectedLiteGraphItems()
|
||||
const subNode1 = { id: 11, mode: LGraphEventMode.ALWAYS } as LGraphNode
|
||||
const subNode2 = { id: 12, mode: LGraphEventMode.NEVER } as LGraphNode
|
||||
@@ -294,9 +294,8 @@ describe('useSelectedLiteGraphItems', () => {
|
||||
// regularNode: BYPASS -> NEVER (since BYPASS != NEVER)
|
||||
expect(regularNode.mode).toBe(LGraphEventMode.NEVER)
|
||||
|
||||
// Subgraph children get unified state (same as their parent):
|
||||
// Both children should now be NEVER, regardless of their previous states
|
||||
expect(subNode1.mode).toBe(LGraphEventMode.NEVER) // was ALWAYS, now NEVER
|
||||
// Subgraph children do not change state
|
||||
expect(subNode1.mode).toBe(LGraphEventMode.ALWAYS) // was ALWAYS, stays ALWAYS
|
||||
expect(subNode2.mode).toBe(LGraphEventMode.NEVER) // was NEVER, stays NEVER
|
||||
})
|
||||
|
||||
@@ -317,9 +316,9 @@ describe('useSelectedLiteGraphItems', () => {
|
||||
// Selected subgraph should toggle to ALWAYS (since it was already NEVER)
|
||||
expect(subgraphNode.mode).toBe(LGraphEventMode.ALWAYS)
|
||||
|
||||
// All children should also get ALWAYS (unified with parent's new state)
|
||||
// All children should be unchanged
|
||||
expect(subNode1.mode).toBe(LGraphEventMode.ALWAYS)
|
||||
expect(subNode2.mode).toBe(LGraphEventMode.ALWAYS)
|
||||
expect(subNode2.mode).toBe(LGraphEventMode.BYPASS)
|
||||
})
|
||||
})
|
||||
|
||||
|
||||
@@ -2,10 +2,7 @@ import type { LGraphNode, Positionable } from '@/lib/litegraph/src/litegraph'
|
||||
import { LGraphEventMode, Reroute } from '@/lib/litegraph/src/litegraph'
|
||||
import { useCanvasStore } from '@/renderer/core/canvas/canvasStore'
|
||||
import { app } from '@/scripts/app'
|
||||
import {
|
||||
collectFromNodes,
|
||||
traverseNodesDepthFirst
|
||||
} from '@/utils/graphTraversalUtil'
|
||||
import { collectFromNodes } from '@/utils/graphTraversalUtil'
|
||||
|
||||
/**
|
||||
* Composable for handling selected LiteGraph items filtering and operations.
|
||||
@@ -97,16 +94,10 @@ export function useSelectedLiteGraphItems() {
|
||||
}
|
||||
|
||||
/**
|
||||
* Toggle the execution mode of all selected nodes with unified subgraph behavior.
|
||||
* Toggle the execution mode of all selected nodes
|
||||
*
|
||||
* Top-level behavior (selected nodes): Standard toggle logic
|
||||
* - If the selected node is already in the specified mode → set to ALWAYS
|
||||
* - Otherwise → set to the specified mode
|
||||
*
|
||||
* Subgraph behavior (children of selected subgraph nodes): Unified state application
|
||||
* - All children inherit the same mode that their parent subgraph node was set to
|
||||
* - This creates predictable behavior: if you toggle a subgraph to "mute",
|
||||
* ALL nodes inside become muted, regardless of their previous individual states
|
||||
* - If any nodes are not already the specified node mode → all are set to specified mode
|
||||
* - Otherwise → set all nodes to ALWAYS
|
||||
*
|
||||
* @param mode - The LGraphEventMode to toggle to (e.g., NEVER for mute, BYPASS for bypass)
|
||||
*/
|
||||
@@ -124,27 +115,8 @@ export function useSelectedLiteGraphItems() {
|
||||
)
|
||||
const newModeForSelectedNode = allNodesMatch ? LGraphEventMode.ALWAYS : mode
|
||||
|
||||
// Process each selected node independently to determine its target state and apply to children
|
||||
selectedNodeArray.forEach((selectedNode) => {
|
||||
// Apply standard toggle logic to the selected node itself
|
||||
|
||||
for (const selectedNode of selectedNodeArray)
|
||||
selectedNode.mode = newModeForSelectedNode
|
||||
|
||||
// If this selected node is a subgraph, apply the same mode uniformly to all its children
|
||||
// This ensures predictable behavior: all children get the same state as their parent
|
||||
if (selectedNode.isSubgraphNode?.() && selectedNode.subgraph) {
|
||||
traverseNodesDepthFirst([selectedNode], {
|
||||
visitor: (node) => {
|
||||
// Skip the parent node since we already handled it above
|
||||
if (node === selectedNode) return undefined
|
||||
|
||||
// Apply the parent's new mode to all children uniformly
|
||||
node.mode = newModeForSelectedNode
|
||||
return undefined
|
||||
}
|
||||
})
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
return {
|
||||
|
||||
@@ -63,11 +63,18 @@ export const graphToPrompt = async (
|
||||
? new ExecutableGroupNodeDTO(node, [], nodeDtoMap)
|
||||
: new ExecutableNodeDTO(node, [], nodeDtoMap)
|
||||
|
||||
nodeDtoMap.set(dto.id, dto)
|
||||
|
||||
if (
|
||||
node.mode === LGraphEventMode.NEVER ||
|
||||
node.mode === LGraphEventMode.BYPASS
|
||||
) {
|
||||
continue
|
||||
}
|
||||
|
||||
for (const innerNode of dto.getInnerNodes()) {
|
||||
nodeDtoMap.set(innerNode.id, innerNode)
|
||||
}
|
||||
|
||||
nodeDtoMap.set(dto.id, dto)
|
||||
}
|
||||
|
||||
const output: ComfyApiWorkflow = {}
|
||||
|
||||
Reference in New Issue
Block a user