[Major] Split layout and draw of LGraphNode (Part1) #510

huchenlei · 2025-02-10T04:14:57Z

Splits layout and draw of LGraphNode.

This PR splits the layout and draw of LGraphNode so we can perform layout operations easier. Also this would help us better handle which element should interact with the cursor.

huchenlei · 2025-02-10T17:08:48Z

Seems like when doing layout of a node with multiple DOM widgets, the location of DOM widget is not correctly captured.

The root issue is that DOMWidget is performing layout during drawing. https://github.com/Comfy-Org/ComfyUI_frontend/blob/aaca5191ab4451ae9edf41a71b363cb482161b56/src/scripts/domWidget.ts#L241-L251

webfiltered

Shiny! The more we split measure / arrange / draw, the easier our lives will be. Code is looking good - haven't tested yet though. Let me know if you want a hand w/that.

webfiltered · 2025-02-10T19:37:32Z

src/LGraphNode.ts

-
-      slot.draw(ctx, {
-        pos,
+      slot.data.draw(ctx, {


Had to go hunting for the meaning of data.draw here. From the way the interface is written, I assume there is a larger plan to use this in many other ways.

Not really requesting anything here, just wondering if there is friendlier or more explicit way to do this?

webfiltered · 2025-02-10T19:40:00Z

src/types/layout.ts

+export interface LayoutPoint {
+  x: number
+  y: number
+}
+
+export interface LayoutSize {
+  width: number
+  height: number
+}
+
+export interface LayoutRect extends LayoutPoint, LayoutSize { }


🤔 Is this a new design for litegraph structs? I do think we should stick to the same data types everywhere.

If it is, we should probably actually have a quick look at real world perf - size/pos/rect easily get to millions of ops per second, and we're only going to be increasing this as we implement more layout containers. We're GPU-constrained on most systems atm, but that will likely change at some point.

webfiltered · 2025-02-10T19:44:22Z

src/utils/layout.ts

+import type { LayoutRect } from "../types/layout"
+
+export function computeBounds(elements: LayoutRect[]): LayoutRect {


measure.ts turned into a layout-function dumping ground. Some of it can probably move in with this guy?

webfiltered · 2025-02-10T20:00:18Z

src/utils/layout.ts

+  const minX = Math.min(...elements.map(e => e.x))
+  const minY = Math.min(...elements.map(e => e.y))
+  const maxX = Math.max(...elements.map(e => e.x + e.width))
+  const maxY = Math.max(...elements.map(e => e.y + e.height))


Suggested change

const minX = Math.min(...elements.map(e => e.x))

const minY = Math.min(...elements.map(e => e.y))

const maxX = Math.max(...elements.map(e => e.x + e.width))

const maxY = Math.max(...elements.map(e => e.y + e.height))

let minX = Infinity

let minY = Infinity

let maxX = -Infinity

let maxY = -Infinity

for (const e of elements) {

if (e.x < minX) minX = e.x

if (e.y < minY) minY = e.y

if (e.x + e.width > maxX) maxX = e.x + e.width

if (e.y + e.height > maxY) maxY = e.y + e.height

}

Prefer single loop to uhm, 12 haha. 😛

n.b. in an error state (e.g. no elements have a valid "width"), this will have +/- Infinity instead of NaN.

webfiltered · 2025-02-10T20:02:38Z

src/LGraphNode.ts

@@ -3242,36 +3238,117 @@ export class LGraphNode implements Positionable, IPinnable {
        highlight,
      })
    }
+  }


Suggested change

}

}

#measureSlots<T extends INodeInputSlot | INodeOutputSlot, C extends NodeInputSlot | NodeOutputSlot>(

layoutSlots: ElementGroupLayout<C>,

slots: T[],

slotClass: new (plain: T) => C,

connectingLink: ConnectingLink | null,

) {

if (!slots?.length) return

for (const [index, input] of slots.entries()) {

const slot = toClass(slotClass, input)

const pos = this.getConnectionPos(true, index)

const isValid = slot.isValidTarget(connectingLink)

const highlight = isValid && this.mouseOver?.inputId === index

layoutSlots.elements.push({

type: "slot",

id: index,

x: pos[0] - this.pos[0],

y: pos[1] - this.pos[1],

width: LiteGraph.NODE_SLOT_HEIGHT * 0.5,

height: LiteGraph.NODE_SLOT_HEIGHT * 0.5,

data: slot,

highlight,

invalid: !isValid,

})

}

}

I couldn't stop myself!

(replaces forEach below)

webfiltered · 2025-02-10T20:02:49Z

src/LGraphNode.ts

+    this.inputs?.forEach((input, index) => {
+      const slot = toClass(NodeInputSlot, input)
+      const pos = this.getConnectionPos(true, index)
+      const isValid = slot.isValidTarget(connectingLink)
+      const highlight = isValid && this.mouseOver?.inputId === index
+
+      layout.inputSlots.elements.push({
+        type: "slot",
+        id: index,
+        x: pos[0] - this.pos[0],
+        y: pos[1] - this.pos[1],
+        width: LiteGraph.NODE_SLOT_HEIGHT * 0.5,
+        height: LiteGraph.NODE_SLOT_HEIGHT * 0.5,
+        data: slot,
+        highlight,
+        invalid: !isValid,
+      })
+    })


Suggested change

this.inputs?.forEach((input, index) => {

const slot = toClass(NodeInputSlot, input)

const pos = this.getConnectionPos(true, index)

const isValid = slot.isValidTarget(connectingLink)

const highlight = isValid && this.mouseOver?.inputId === index

layout.inputSlots.elements.push({

type: "slot",

id: index,

x: pos[0] - this.pos[0],

y: pos[1] - this.pos[1],

width: LiteGraph.NODE_SLOT_HEIGHT * 0.5,

height: LiteGraph.NODE_SLOT_HEIGHT * 0.5,

data: slot,

highlight,

invalid: !isValid,

})

})

this.#measureSlots(layout.inputSlots, this.inputs, NodeInputSlot, connectingLink)

webfiltered · 2025-02-10T20:02:55Z

src/LGraphNode.ts

+    this.outputs?.forEach((output, index) => {
+      const slot = toClass(NodeOutputSlot, output)
+      const pos = this.getConnectionPos(false, index)


Suggested change

this.outputs?.forEach((output, index) => {

const slot = toClass(NodeOutputSlot, output)

const pos = this.getConnectionPos(false, index)

outputs 1/3

webfiltered · 2025-02-10T20:02:59Z

src/LGraphNode.ts

-      // change opacity of incompatible slots when dragging a connection
+    this.outputs?.forEach((output, index) => {
+      const slot = toClass(NodeOutputSlot, output)
+      const pos = this.getConnectionPos(false, index)
      const isValid = slot.isValidTarget(connectingLink)


Suggested change

const isValid = slot.isValidTarget(connectingLink)

outputs 2/3

webfiltered · 2025-02-10T20:03:06Z

src/LGraphNode.ts

+      const highlight = isValid && this.mouseOver?.outputId === index
+
+      layout.outputSlots.elements.push({
+        type: "slot",
+        id: index,
+        x: pos[0] - this.pos[0],
+        y: pos[1] - this.pos[1],
+        width: LiteGraph.NODE_SLOT_HEIGHT * 0.5,
+        height: LiteGraph.NODE_SLOT_HEIGHT * 0.5,
+        data: slot,
+        highlight,
+        invalid: !isValid,
+      })
+    })


Suggested change

const highlight = isValid && this.mouseOver?.outputId === index

layout.outputSlots.elements.push({

type: "slot",

id: index,

x: pos[0] - this.pos[0],

y: pos[1] - this.pos[1],

width: LiteGraph.NODE_SLOT_HEIGHT * 0.5,

height: LiteGraph.NODE_SLOT_HEIGHT * 0.5,

data: slot,

highlight,

invalid: !isValid,

})

})

this.#measureSlots(layout.outputSlots, this.outputs, NodeOutputSlot, connectingLink)

outputs 3/3

webfiltered · 2025-02-10T20:03:12Z

src/LGraphNode.ts

+      let posY = this.widgets_up ? 2 : (this.widgets_start_y ?? widgetsStartY + 2)
+      if (this.horizontal) {
+        posY = 2
+      }


Suggested change

let posY = this.widgets_up ? 2 : (this.widgets_start_y ?? widgetsStartY + 2)

if (this.horizontal) {

posY = 2

}

let posY = this.widgets_up || this.horizontal ? 2 : (this.widgets_start_y ?? widgetsStartY + 2)

nit

[Major] Split layout and draw

4cee8ca

huchenlei requested a review from webfiltered February 10, 2025 04:14

webfiltered reviewed Feb 10, 2025

View reviewed changes

huchenlei mentioned this pull request Feb 13, 2025

Split layout and draw for slots #524

Merged

huchenlei closed this Feb 14, 2025

huchenlei deleted the layout_node branch February 14, 2025 01:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Major] Split layout and draw of LGraphNode (Part1) #510

[Major] Split layout and draw of LGraphNode (Part1) #510

huchenlei commented Feb 10, 2025

huchenlei commented Feb 10, 2025

webfiltered left a comment

webfiltered Feb 10, 2025

webfiltered Feb 10, 2025

webfiltered Feb 10, 2025

webfiltered Feb 10, 2025

webfiltered Feb 10, 2025

webfiltered Feb 10, 2025

webfiltered Feb 10, 2025

webfiltered Feb 10, 2025 •

edited

Loading

webfiltered Feb 10, 2025 •

edited

Loading

webfiltered Feb 10, 2025 •

edited

Loading

webfiltered Feb 10, 2025

		import type { LayoutRect } from "../types/layout"

		export function computeBounds(elements: LayoutRect[]): LayoutRect {

-  const minX = Math.min(...elements.map(e => e.x))
-  const minY = Math.min(...elements.map(e => e.y))
-  const maxX = Math.max(...elements.map(e => e.x + e.width))
-  const maxY = Math.max(...elements.map(e => e.y + e.height))
+  let minX = Infinity
+  let minY = Infinity
+  let maxX = -Infinity
+  let maxY = -Infinity
+  for (const e of elements) {
+    if (e.x < minX) minX = e.x
+    if (e.y < minY) minY = e.y
+    if (e.x + e.width > maxX) maxX = e.x + e.width
+    if (e.y + e.height > maxY) maxY = e.y + e.height
+  }

	this.outputs?.forEach((output, index) => {
	const slot = toClass(NodeOutputSlot, output)
	const pos = this.getConnectionPos(false, index)

[Major] Split layout and draw of LGraphNode (Part1) #510

[Major] Split layout and draw of LGraphNode (Part1) #510

Conversation

huchenlei commented Feb 10, 2025

huchenlei commented Feb 10, 2025

webfiltered left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webfiltered Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

webfiltered Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

webfiltered Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webfiltered Feb 10, 2025 •

edited

Loading

webfiltered Feb 10, 2025 •

edited

Loading

webfiltered Feb 10, 2025 •

edited

Loading