Bundled subgraph fixes (#4964)

### Group support for subgraph unpacking
The unpacking code would silently delete groups (the cosmetic colored
rectangles). They are now correctly transferred.
### Fix subgraph node position on conversion to subgraph
Converting to subgraph will no longer cause nodes to inch upwards

![subgraph-conversion-positioning](https://github.com/user-attachments/assets/e120c3f9-5602-4dba-9075-c1eadb534f9a)
### Make unpacking use same positioning calcs as conversion
Non trivial, but unpacking is now a proper inverse for conversion.

![subgraph-conversion-inverse](https://github.com/user-attachments/assets/4fcaffca-1c97-4d71-93f7-1af569b1c941)
### Clean up old output links when unpacking
Unpacked nodes were left with dangling outputs. This would cause
cascading issues later, such as when consecutively unpacking nested
subgraphs.
### Minor refactoring for code clarity

┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-4964-Bundled-subgraph-fixes-24e6d73d365081d3a043ef1531d9d38a)
by [Unito](https://www.unito.io)

src/lib/litegraph/src/LGraph.ts

@@ -20,6 +20,7 @@ import type { SubgraphEventMap } from './infrastructure/SubgraphEventMap'
 import type {
   DefaultConnectionColors,
   Dictionary,
+  HasBoundingRect,
   IContextMenuValue,
   INodeInputSlot,
   INodeOutputSlot,
@@ -28,7 +29,8 @@ import type {
   MethodNames,
   OptionalProps,
   Point,
-  Positionable
+  Positionable,
+  Size
 } from './interfaces'
 import { LiteGraph, SubgraphNode } from './litegraph'
 import {
@@ -1569,6 +1571,9 @@ export class LGraph
       boundingRect
     )
 
+    //Correct for title height. It's included in bounding box, but not _posSize
+    subgraphNode.pos[1] += LiteGraph.NODE_TITLE_HEIGHT / 2
+
     // Add the subgraph node to the graph
     this.add(subgraphNode)
 
@@ -1668,14 +1673,21 @@ export class LGraph
     if (!(subgraphNode instanceof SubgraphNode))
       throw new Error('Can only unpack Subgraph Nodes')
     this.beforeChange()
-    const center = [0, 0]
+    //NOTE: Create bounds can not be called on positionables directly as the subgraph is not being displayed and boundingRect is not initialized.
-    for (const node of subgraphNode.subgraph.nodes) {
+    //NOTE: NODE_TITLE_HEIGHT is explicitly excluded here
-      center[0] += node.pos[0] + node.size[0] / 2
+    const positionables = [
-      center[1] += node.pos[1] + node.size[1] / 2
+      ...subgraphNode.subgraph.nodes,
-    }
+      ...subgraphNode.subgraph.reroutes.values(),
-    center[0] /= subgraphNode.subgraph.nodes.length
+      ...subgraphNode.subgraph.groups
-    center[1] /= subgraphNode.subgraph.nodes.length
+    ].map((p: { pos: Point; size?: Size }): HasBoundingRect => {
+      return {
+        boundingRect: [p.pos[0], p.pos[1], p.size?.[0] ?? 0, p.size?.[1] ?? 0]
+      }
+    })
+    const bounds = createBounds(positionables) ?? [0, 0, 0, 0]
+    const center = [bounds[0] + bounds[2] / 2, bounds[1] + bounds[3] / 2]
 
+    const toSelect: Positionable[] = []
     const offsetX = subgraphNode.pos[0] - center[0] + subgraphNode.size[0] / 2
     const offsetY = subgraphNode.pos[1] - center[1] + subgraphNode.size[1] / 2
     const movedNodes = multiClone(subgraphNode.subgraph.nodes)
@@ -1697,6 +1709,21 @@ export class LGraph
       for (const input of node.inputs) {
         input.link = null
       }
+      for (const output of node.outputs) {
+        output.links = []
+      }
+      toSelect.push(node)
+    }
+    const groups = structuredClone(
+      [...subgraphNode.subgraph.groups].map((g) => g.serialize())
+    )
+    for (const g_info of groups) {
+      const group = new LGraphGroup(g_info.title, g_info.id)
+      this.add(group, true)
+      group.configure(g_info)
+      group.pos[0] += offsetX
+      group.pos[1] += offsetY
+      toSelect.push(group)
     }
     //cleanup reoute.linkIds now, but leave link.parentIds dangling
     for (const islot of subgraphNode.inputs) {
@@ -1722,17 +1749,16 @@ export class LGraph
         }
       }
     }
-
+    const newLinks: {
-    const newLinks: [
+      oid: NodeId
-      NodeId,
+      oslot: number
-      number,
+      tid: NodeId
-      NodeId,
+      tslot: number
-      number,
+      id: LinkId
-      LinkId,
+      iparent?: RerouteId
-      RerouteId | undefined,
+      eparent?: RerouteId
-      RerouteId | undefined,
+      externalFirst: boolean
-      boolean
+    }[] = []
-    ][] = []
     for (const [, link] of subgraphNode.subgraph._links) {
       let externalParentId: RerouteId | undefined
       if (link.origin_id === SUBGRAPH_INPUT_ID) {
@@ -1757,16 +1783,16 @@ export class LGraph
         for (const linkId of subgraphNode.outputs[link.target_slot].links ??
           []) {
           const sublink = this.links[linkId]
-          newLinks.push([
+          newLinks.push({
-            link.origin_id,
+            oid: link.origin_id,
-            link.origin_slot,
+            oslot: link.origin_slot,
-            sublink.target_id,
+            tid: sublink.target_id,
-            sublink.target_slot,
+            tslot: sublink.target_slot,
-            link.id,
+            id: link.id,
-            link.parentId,
+            iparent: link.parentId,
-            sublink.parentId,
+            eparent: sublink.parentId,
-            true
+            externalFirst: true
-          ])
+          })
           sublink.parentId = undefined
         }
         continue
@@ -1778,47 +1804,47 @@ export class LGraph
         }
         link.target_id = target_id
       }
-      newLinks.push([
+      newLinks.push({
-        link.origin_id,
+        oid: link.origin_id,
-        link.origin_slot,
+        oslot: link.origin_slot,
-        link.target_id,
+        tid: link.target_id,
-        link.target_slot,
+        tslot: link.target_slot,
-        link.id,
+        id: link.id,
-        link.parentId,
+        iparent: link.parentId,
-        externalParentId,
+        eparent: externalParentId,
-        false
+        externalFirst: false
-      ])
+      })
     }
     this.remove(subgraphNode)
     this.subgraphs.delete(subgraphNode.subgraph.id)
     const linkIdMap = new Map<LinkId, LinkId[]>()
     for (const newLink of newLinks) {
       let created: LLink | null | undefined
-      if (newLink[0] == SUBGRAPH_INPUT_ID) {
+      if (newLink.oid == SUBGRAPH_INPUT_ID) {
         if (!(this instanceof Subgraph)) {
           console.error('Ignoring link to subgraph outside subgraph')
           continue
         }
-        const tnode = this._nodes_by_id[newLink[2]]
+        const tnode = this._nodes_by_id[newLink.tid]
-        created = this.inputNode.slots[newLink[1]].connect(
+        created = this.inputNode.slots[newLink.oslot].connect(
-          tnode.inputs[newLink[3]],
+          tnode.inputs[newLink.tslot],
           tnode
         )
-      } else if (newLink[2] == SUBGRAPH_OUTPUT_ID) {
+      } else if (newLink.tid == SUBGRAPH_OUTPUT_ID) {
         if (!(this instanceof Subgraph)) {
           console.error('Ignoring link to subgraph outside subgraph')
           continue
         }
-        const tnode = this._nodes_by_id[newLink[0]]
+        const tnode = this._nodes_by_id[newLink.oid]
-        created = this.outputNode.slots[newLink[3]].connect(
+        created = this.outputNode.slots[newLink.tslot].connect(
-          tnode.outputs[newLink[1]],
+          tnode.outputs[newLink.oslot],
           tnode
         )
       } else {
-        created = this._nodes_by_id[newLink[0]].connect(
+        created = this._nodes_by_id[newLink.oid].connect(
-          newLink[1],
+          newLink.oslot,
-          this._nodes_by_id[newLink[2]],
+          this._nodes_by_id[newLink.tid],
-          newLink[3]
+          newLink.tslot
         )
       }
       if (!created) {
@@ -1826,12 +1852,12 @@ export class LGraph
         continue
       }
       //This is a little unwieldy since Map.has isn't a type guard
-      const linkIds = linkIdMap.get(newLink[4]) ?? []
+      const linkIds = linkIdMap.get(newLink.id) ?? []
       linkIds.push(created.id)
-      if (!linkIdMap.has(newLink[4])) {
+      if (!linkIdMap.has(newLink.id)) {
-        linkIdMap.set(newLink[4], linkIds)
+        linkIdMap.set(newLink.id, linkIds)
       }
-      newLink[4] = created.id
+      newLink.id = created.id
     }
     const rerouteIdMap = new Map<RerouteId, RerouteId>()
     for (const reroute of subgraphNode.subgraph.reroutes.values()) {
@@ -1847,17 +1873,18 @@ export class LGraph
       ])
       rerouteIdMap.set(reroute.id, migratedReroute.id)
       this.reroutes.set(migratedReroute.id, migratedReroute)
+      toSelect.push(migratedReroute)
     }
     //iterate over newly created links to update reroute parentIds
     for (const newLink of newLinks) {
-      const linkInstance = this.links.get(newLink[4])
+      const linkInstance = this.links.get(newLink.id)
       if (!linkInstance) {
         continue
       }
       let instance: Reroute | LLink | undefined = linkInstance
-      let parentId: RerouteId | undefined = newLink[6]
+      let parentId: RerouteId | undefined = undefined
-      if (newLink[7]) {
+      if (newLink.externalFirst) {
-        parentId = newLink[6]
+        parentId = newLink.eparent
         //TODO: recursion check/helper method? Probably exists, but wouldn't mesh with the reference tracking used by this implementation
         while (parentId) {
           instance.parentId = parentId
@@ -1869,7 +1896,7 @@ export class LGraph
           parentId = instance.parentId
         }
       }
-      parentId = newLink[5]
+      parentId = newLink.iparent
       while (parentId) {
         const migratedId = rerouteIdMap.get(parentId)
         if (!migratedId) throw new Error('Broken Id link when unpacking')
@@ -1883,8 +1910,8 @@ export class LGraph
         if (!oldReroute) throw new Error('Broken Id link when unpacking')
         parentId = oldReroute.parentId
       }
-      if (!newLink[7]) {
+      if (!newLink.externalFirst) {
-        parentId = newLink[6]
+        parentId = newLink.eparent
         while (parentId) {
           instance.parentId = parentId
           instance = this.reroutes.get(parentId)
@@ -1897,18 +1924,13 @@ export class LGraph
       }
     }
 
-    const nodes: LGraphNode[] = []
     for (const nodeId of nodeIdMap.values()) {
       const node = this._nodes_by_id[nodeId]
-      nodes.push(node)
       node._setConcreteSlots()
       node.arrange()
     }
-    const reroutes = [...rerouteIdMap.values()]
-      .map((i) => this.reroutes.get(i))
-      .filter((x): x is Reroute => !!x)
 
-    this.canvasAction((c) => c.selectItems([...nodes, ...reroutes]))
+    this.canvasAction((c) => c.selectItems(toSelect))
     this.afterChange()
   }
 

src/lib/litegraph/test/subgraph/SubgraphConversion.test.ts

@@ -3,6 +3,7 @@ import { assert, describe, expect, it } from 'vitest'
 import {
   ISlotType,
   LGraph,
+  LGraphGroup,
   LGraphNode,
   LiteGraph
 } from '@/lib/litegraph/src/litegraph'
@@ -80,7 +81,7 @@ describe('SubgraphConversion', () => {
       expect(graph.nodes.length).toBe(4)
       expect(graph.links.size).toBe(2)
     })
-    it('Should keep reroutes', () => {
+    it('Should keep reroutes and groups', () => {
       const subgraph = createTestSubgraph({
         outputs: [{ name: 'value', type: 'number' }]
       })
@@ -98,6 +99,7 @@ describe('SubgraphConversion', () => {
       const outer = createNode(graph, ['number'])
       const outerLink = subgraphNode.connect(0, outer, 0)
       assert(outerLink)
+      subgraph.add(new LGraphGroup())
 
       subgraph.createReroute([10, 10], innerLink)
       graph.createReroute([10, 10], outerLink)
@@ -105,6 +107,7 @@ describe('SubgraphConversion', () => {
       graph.unpackSubgraph(subgraphNode)
 
       expect(graph.reroutes.size).toBe(2)
+      expect(graph.groups.length).toBe(1)
     })
     it('Should map reroutes onto split outputs', () => {
       const subgraph = createTestSubgraph({